SDL
SDL copied to clipboard
libsdl-org
SDL_MUSTLOCK produces wrong results
This issue is described by #3837, however the results of SDL_MUSTLOCK are still incorrect as of SDL 2.0.22. The documentation for SDL_LockSurface specifically references the SDL_MUSTLOCK macro as being the correct way to check whether an SDL_Surface requires locking.
SDL_MUSTLOCK is defined in SDL_surface.h as:
/**
* Evaluates to true if the surface needs to be locked before access.
*/
#define SDL_MUSTLOCK(S) (((S)->flags & SDL_RLEACCEL) != 0)
The flags field of SDL_Surface is only checked against or set with SDL_RLEACCEL in the following methods:
- SDL_SetColorKey
- SDL_LockSurface
- SDL_UnlockSurface
- SDL_ConvertSurface
- SDL_FreeSurface
- Blit functions (as reported by #3837)
As previously described in #3837, SDL_SetSurfaceRLE does not set the SDL_RLEACCEL flag. This means that SDL_MUSTLOCK almost always returns the incorrect results (Edit: until a blit is performed).
Proposed fix: SDL_MUSTLOCK should be redefined as:
/**
* Evaluates to true if the surface needs to be locked before access.
*/
#define SDL_MUSTLOCK(S) (SDL_HasSurfaceRLE((S)) == SDL_TRUE)
Correction: I did not look at the source for the blit functions. #3837 does report that performing a blit does set the SDL_RLEACCEL flag.
I'll take a look at this more closely, but fixing the function to set the flag is the right approach, not changing the macro, I think.
I didn't know if there was some reason that "fixing the function" was not done previously when SDL_HasSurfaceRLE was added for SDL 2.0.14. So long as the results of SDL_MUSTLOCK are correct then the implementation details don't matter so much. Thanks for looking into it!
fixing the function to set the flag is the right approach
I looked at this a few months ago and this statement was a misunderstanding on my part of how this works. If I recall, @monkey0506 actually had the right approach, but I need to look this over again before I make changes.
Okay, so the only concern here is that if we change that macro to use SDL_HasSurfaceRLE, then something built with older headers will not work (as it doesn't work now), but something built with newer headers will fail to run on a system with an SDL older than 2.0.14.
We can't change the macro to look for the same flag as SDL_HasSurfaceRLE, because the flag is buried in an opaque struct that the public headers don't have access to.
So the best plan is to add a new internal flag that it uses instead of SDL_RLEACCEL (SDL_RLEACCEL_READY or something), and set the public SDL_RLEACCEL flag when we are setting SDL_COPY_RLE_DESIRED. This makes any version of the headers work with any build of SDL, and means you don't have to upgrade headers to get the bug fix.
Alternately, we leave this broken until SDL3, since this is going to add some confusion. In SDL3, we can just make SDL_MUSTLOCK a function instead of a macro, and/or we can just add a public "must lock this surface" flag instead of having code that reads tea leaves. :)
@slouken, your call; either approach is fine with me (or both, where we patch it up now and rework it in SDL3).
I don't think there is bug here.
SDL_MUSTLOCK tells if you need to call lock/unlock before accessing surface->pixels. It works. For a given surface, sometime you may need to lock/unlock and sometime not. That has to be checked at run-time each time before accessing surface->pixels.
SDL_SetSurfaceRLE is meant to have the surface to be RLE encoded for the next blit (eg SDL_COPY_RLE_DESIRED ). It doesn't encode the surface immediately. SDL_HasSurfaceRLE tells the user want it to be RLE encoded for the next blit. It doesn't tell you if the surface is actually RLE encoded.
In fact, SDL_MUSTLOCK is somehow the function that tells you if the surface is RLE encoded. And so that you should lock/unlock before accessing the pixels.
... Then, I believe most people don't use RLE encoding, but you have to theoretically call MUST_LOCK. So it may be more efficient to leave this a as macro.
