| Summary: | SDL_BlitCopyOverlap() assumes memcpy() operates in order | ||
|---|---|---|---|
| Product: | SDL | Reporter: | Petr Pisar <ppisar> |
| Component: | video | Assignee: | Sam Lantinga <slouken> |
| Status: | RESOLVED FIXED | QA Contact: | Sam Lantinga <slouken> |
| Severity: | normal | ||
| Priority: | P2 | CC: | tolga.dalman |
| Version: | HG 2.0 | ||
| Hardware: | x86_64 | ||
| OS: | Linux | ||
| Attachments: |
Test case
replace SDL_memcpy() by SDL_memmove() patch Patch for the 1.3 branch Complete test case |
||
|
Description
Petr Pisar
2011-01-18 06:59:12 UTC
Created attachment 573 [details]
Test case
This is simple test blitting the horizontally backward overlapping rectangles.
Complete test would need to test overlapping in vertical direction and in forward direction to exclude any regressions.
Also this test does not check system memcpy(3) is affected thus when running on unaffected system, the test case does not prove anything.
If you are interested, I can improve the test to cover all the ideas.
Yes, that would be great. Thanks! (In reply to comment #0) > One would attempt to replace the SDL_memcpy() call by SDL_memmove(). I suggest that SDL_memcpy() in SDL_BlitCopyOverlap() is replaced by SDL_memmove() in any case. > That would > be good except SDL_memmove() defined in include/SDL_stdinc.h can be implemented > manually (on some platforms) and the implementation is wrong again as it relies > on SDL_memcpy() and thus system memcpy(3) again. Can you state, which platforms are affected - i.e., where memmove() is really unavailable ? Does memcpy() on overlapping areas also break there ? > You can see this bug in action in Wesnoth in Fedora 14 distribution while > scrolling the board to right direction. This bug has been originally reported > to Fedora's bug tracking system > <https://bugzilla.redhat.com/show_bug.cgi?id=669844> Yes. Please see for the Gentoo bug report: http://bugs.gentoo.org/show_bug.cgi?id=354175. Thanks! Created attachment 574 [details]
replace SDL_memcpy() by SDL_memmove() patch
As Petr suggested, here's the trivial patch.
This is fixed in SDL 1.3, thanks! http://hg.libsdl.org/SDL/rev/b9c224e16859 Thanks, Sam! How about applying this patch to the 1.2 branch also ? I don't think this is a regression of any kind ... I don't have time right now, but if you post a patch I'll apply it. Thanks! Created attachment 577 [details]
Patch for the 1.3 branch
The previous patch had been against 1.2.
The 1.2 fix is in, thanks! http://hg.libsdl.org/SDL/rev/d898ee5431f5 Created attachment 579 [details]
Complete test case
This augmented test checks for rectangles overlapping horizontally, vertically, and in both directions (destination before source and vice versa). I addition it checks if memcpy(3) provided by standard system is affected.
I must admit I could not have found reliable way who to make affected memcpy() to fail. It usually fails on my system, sometimes it doesn't. OTOH, the SDL blitter fails always. Also memory layout and all the offsets and lengths are important. If you change them you can get different results.
Despite all of that I believe the test is helpful as it fails on SDL-1.2.14 release and it passes on SDL-1.2 tip (at least on my system).
(In reply to comment #3) > (In reply to comment #0) > > That would > > be good except SDL_memmove() defined in include/SDL_stdinc.h can be > > implemented manually (on some platforms) and the implementation is wrong > > again as it relies on SDL_memcpy() and thus system memcpy(3) again. > > Can you state, which platforms are affected - i.e., where memmove() is really > unavailable ? Does memcpy() on overlapping areas also break there ? > I have no real system that offers affected memcpu() and does not provide memmove(). Just while reading the code and playing with some ifdefs I found and verified that system memcpy() could be called as implementation of SDL's memmove(). I believe such crazy system does nobody use, but I just wanted to be precise and to point there could be a problem. Maybe my hg-fu is low today, talking about 1.2 branch, but changeset d898ee5431f5 has different parrent than previous changeset and squashes everything after 6bb01861c4c0, including b9c224e16859 (The MacOSX memcpy tunes). Is it Ok? Yep, they're in different branches, so it's fine: http://hg.libsdl.org/SDL/graph |