SDL_DrawCursor() shouldn't make assumptions about screen's dirty rectangles based on its surface type

[mikrosk]

While doing some experiments with the xbios video driver I have noticed that in the cursor is not drawn in some video modes. The reason is that the draw function explicitly checks whether the screen surface is a hardware one and if so, skips notifying the underlaying driver: https://github.com/libsdl-org/SDL-1.2/blob/d0a9e90b21f1312b87533d96324cc24247ba6d99/src/video/SDL_cursor.c#L658-L670 At the first sight it looks logical, we are drawing directly the the screen so why bother, right? However some (at least xbios ;)) drivers offer hardware surfaces, yes, but in the background they still need to do some conversions before displaying the buffer.

Both https://github.com/libsdl-org/SDL-1.2/blob/d0a9e90b21f1312b87533d96324cc24247ba6d99/src/video/SDL_video.c#L1046 and https://github.com/libsdl-org/SDL-1.2/blob/d0a9e90b21f1312b87533d96324cc24247ba6d99/src/video/SDL_video.c#L1114 always end up calling video->UpdateRects even if the screen surface is a hardware one. SoI'd say the cursor drawing function shouldn't behave differently here.

Any objections against removal of the ((screen->flags & SDL_HWSURFACE) != SDL_HWSURFACE) condition?

P.S. I am aware that the root cause is actually the xbios driver, i.e. it shouldn't lie and mark its buffer as software surface but doing so would require some refactoring (the difference between being a true hardware surface and a fake one is really minimal, sometimes it's bpp for given surface, sometimes hardware it runs on) so the quick (and hopefully harmless) fix is currently just align the behaviour between SDL_DrawCursor and SDL_UpdateRects.

EDIT: I see that SDL_EraseCursor would also need the same treatment.

[slouken]

Yes, this is fine.

[mikrosk]

Btw, @slouken now I have you here on this topic :) maybe you could clarify one thing for me (yes, doing some digging again).

While my arguments about SDL_DrawCursor() are solid when looking into code, I wonder whether this independence of SDL_Flip and SDL_UpdateRects on SDL_HWSURFACE was always the case? In this very old thread you mentioned regarding the usage of hardware surfaces:

In this case, SDL_UpdateRects() and SDL_Flip() are inexpensive noops, as you are writing to memory automatically being displayed.

But clearly, this is not the case. SDL_Flip() calls SDL_UpdateRect(whole screen) and that one doesn't differentiate between software and hardware surfaces and calls backend's UpdateRects in every case, as we have learned earlier.

As your post was made approximately a year before the initial SDL_video.c commit (which I believe was the the release of SDL 1.2), do you remember whether there was some change of this philosophy?

EDIT: Found the 1.0 archive from 1999, about four months before your post. Even that one doesn't differ very much from the application logic above: there isn't any trace of an early exit in neither SDL_Flip nor SDL_UpdateRects if a plain hardware surface is supplied.