Allegro.cc - Online Community

Allegro.cc Forums » Allegro Development » draw_sprite and system bitmap subbitmap failure code and images

This thread is locked; no one can reply to it. rss feed Print
 1   2 
draw_sprite and system bitmap subbitmap failure code and images
Neil Walker
Member #210
April 2000
avatar

Hello,
two faults:
1. draw_sprite failure to find correct place
2. system_bitmap with subbitmaps crashes when you destroy the subbitmaps

From testing I've found that when you draw_sprite a subbitmap of a video or system bitmap onto a different system or video bitmap, it doubles the x/y and you end up with the wrong portion showing (this is following from my thread trying to explain things).Also, I've found through testing that if you have a subbitmap of a system bitmap when you try to destroy the subbitmaps you get a crash. Video and memory bitmaps are ok.

Attached is a sample piece of code and some windows exe's to show how subbitmaps onto video or system bitmaps don't work, and how system bitmaps that have subbitmaps crash the system.

As you can see from the images, 'original.jpg' is how it works fine with memory bitmaps, but when you use video/system bitmaps you get it wrong, as in 'doublingerror.jpg'. sheet.bmp is the source bitmap.

If you look closely you can see what is happening is it is doubling the x and y position when you blit a video/memory subbitmap onto a system/video bitmap.

1. if you compile as is, it will work.
2. if you uncomment the line: //#define nonmemorybitmap you will get the error
3. if you change the line 41 to system bitmap you get the crash.

masked blit is fine. The problem, I believe, is the bit of code in the windows acceleration source file for draw_sprite that tries to determine the correct place.

Neil.
MAME Cabinet Blog / AXL LIBRARY (a games framework) / AXL Documentation and Tutorial

wii:0356-1384-6687-2022, kart:3308-4806-6002. XBOX:chucklepie

GullRaDriel
Member #3,861
September 2003
avatar

Should crash is really crashing:

1C:\Documents and Settings\Gull\Bureau\failure>gdb shouldcrash.exe
2GNU gdb 5.2.1
3Copyright 2002 Free Software Foundation, Inc.
4GDB is free software, covered by the GNU General Public License, and you are
5welcome to change it and/or distribute copies of it under certain conditions.
6Type "show copying" to see the conditions.
7There is absolutely no warranty for GDB. Type "show warranty" for details.
8This GDB was configured as "i686-pc-mingw32"...(no debugging symbols found)...
9(gdb) run
10Starting program: C:\Documents and Settings\Gull\Bureau\failure/shouldcrash.exe
11 
12 
13Program received signal SIGSEGV, Segmentation fault.
140x67a4bdea in gfx_directx_destroy_surface ()
15(gdb) bt
16#0 0x67a4bdea in gfx_directx_destroy_surface ()
17#1 0x00000000 in ?? ()

Should fail isn't:

C:\Documents and Settings\Gull\Bureau\failure>gdb shouldfail.exe
GNU gdb 5.2.1
Copyright 2002 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i686-pc-mingw32"...(no debugging symbols found)...
(gdb) run
Starting program: C:\Documents and Settings\Gull\Bureau\failure/shouldfail.exe

Program exited normally.

Should work is working:

C:\Documents and Settings\Gull\Bureau\failure>gdb shouldwork.exe
GNU gdb 5.2.1
Copyright 2002 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i686-pc-mingw32"...(no debugging symbols found)...
(gdb) run
Starting program: C:\Documents and Settings\Gull\Bureau\failure/shouldwork.exe

Program exited normally.

ALSO:
I first try with dev-cpp, -Wall -g with no optimisation:
The code given works fine when compiled on my computer.
No matter if i comment or not the #define nonmemorybitmap

Conclusion: it should be a problem with your compiler,compiling flags,level of optimisation.
It's perhaps a MSVC issue.

I'm sorry that i cannot help you more, it's working fine with me. attached are the 2 compiled from my pc.

I let you test and post to see if it is different.

LAST EDIT BEFORE LEAVING WORK:

I test with all optimisation, no optimisation, ... all i compile with dev-cpp & gcc 3.4.2 & allegro 4.0.2 is working fine. MSVC issue.

"Code is like shit - it only smells if it is not yours"
Allegro Wiki, full of examples and articles !!

CGamesPlay
Member #2,559
July 2002
avatar

$ g++ main.cpp `allegro-config --libs --static`
main.cpp:9: error: `main' must return `int'
main.cpp: In function `int main(...)':
main.cpp:84: error: return-statement with no value, in function declared with a 
   non-void return type

Fixed those.

I'm on Linux, and everything worked fine.

--
Tomasu: Every time you read this: hugging!

Ryan Patterson - <http://cgamesplay.com/>

Neil Walker
Member #210
April 2000
avatar

[edit]I've now had two other people that I know compile my code (not msvc) as I said and the same bug happens.[/edit]

GullraDriel: Firstly, the crash shouldn't be happing so that is a bug, secondly I am compiling with DevC++ and MSVC and both versions show the odd images (when I say fail I mean show the wrong images). I've had others test my devcpp exe and it fails again for them. I've also tried your 'workingbutshouldcrash' and while it doesn't crash (are you sure you enabled the system bitmap at line 41?) it does show the incorrect images - all image pairs should be the same. Did you not see this?

CGamesPlay, linux, the code I think is wrong is in the windows area so no surprises there.

Have you seen my jpegs? Is there any explanation?

Neil.
MAME Cabinet Blog / AXL LIBRARY (a games framework) / AXL Documentation and Tutorial

wii:0356-1384-6687-2022, kart:3308-4806-6002. XBOX:chucklepie

Thomas Harte
Member #33
April 2000
avatar

I've found the genuine and demonstrable Allegro bug! As Neil Walker observes (elsewhere), ddraw_draw_sprite looks like this:

1static void ddraw_draw_sprite(BITMAP * bmp, BITMAP * sprite, int x, int y)
2{
3 int sx, sy, w, h;
4 
5 if (is_video_bitmap(sprite) || is_system_bitmap(sprite)) {
6 sx = sprite->x_ofs;
7 sy = sprite->y_ofs;
8 w = sprite->w;
9 h = sprite->h;
10 
11 if (bmp->clip) {
12 if (x < bmp->cl) {
13 sx += bmp->cl - x;
14 w -= bmp->cl - x;
15 x = bmp->cl;
16 }
17 
18 if (y < bmp->ct) {
19 sy += bmp->ct - y;
20 h -= bmp->ct - y;
21 y = bmp->ct;
22 }
23 
24 if (x + w > bmp->cr)
25 w = bmp->cr - x;
26 
27 if (w <= 0)
28 return;
29 
30 if (y + h > bmp->cb)
31 h = bmp->cb - y;
32 
33 if (h <= 0)
34 return;
35 }
36 
37 ddraw_masked_blit(sprite, bmp, sx, sy, x, y, w, h);
38 }
39 else {
40 /* have to use the original software version */
41 _orig_draw_sprite(bmp, sprite, x, y);
42 }
43}

The problem relates to [x/y]_ofs, which seem to be used to hold the top left corner of the area of a bitmap that is actually in use. With ordinary bitmaps they are both set to 0. With sub bitmaps they are set somewhere else in order to only display a window within the source bitmap. The Allegro 2.x days of just fiddling with ->line pointers are obviously long behind.

The problem is that both ddraw_draw_sprite and ddraw_masked_blit apply the sub bitmap constraints. I believe (but cannot test, as still in OS X) that the following should fix the problem:

1static void ddraw_draw_sprite(BITMAP * bmp, BITMAP * sprite, int x, int y)
2{
3 int sx, sy, w, h;
4 
5 if (is_video_bitmap(sprite) || is_system_bitmap(sprite)) {
6 sx = 0;
7 sy = 0;
8 w = sprite->w;
9 h = sprite->h;
10 
11 if (bmp->clip) {
12 if (x < bmp->cl) {
13 sx += bmp->cl - x;
14 w -= bmp->cl - x;
15 x = bmp->cl;
16 }
17 
18 if (y < bmp->ct) {
19 sy += bmp->ct - y;
20 h -= bmp->ct - y;
21 y = bmp->ct;
22 }
23 
24 if (x + w > bmp->cr)
25 w = bmp->cr - x;
26 
27 if (w <= 0)
28 return;
29 
30 if (y + h > bmp->cb)
31 h = bmp->cb - y;
32 
33 if (h <= 0)
34 return;
35 }
36 
37 ddraw_masked_blit(sprite, bmp, sx, sy, x, y, w, h);
38 }
39 else {
40 /* have to use the original software version */
41 _orig_draw_sprite(bmp, sprite, x, y);
42 }
43}

Note the only change, near the top:

      sx = 0;
      sy = 0;

Which relate to lines 155 and 156 of src/win/wddaccel.c of the Allegro source.

Please could someone test this theory?

Elias
Member #358
May 2000

Someone should find the link to Matthew's patch generator..

--
"Either help out or stop whining" - Evert

Thomas Harte
Member #33
April 2000
avatar

Found it: http://www.allegro.cc/dev/make-diff.php. The diff, as shown below, is attached. Should this patch make the grade, please acknowledge both Neil Walker & myself in the authors file as I only got to ddraw_draw_sprite following a message by him on the Retrospec mailing list.

Quote:

--- src/win/wddaccel.c Thu Oct 27 19:57:01 2005 UTC
+++ src/win/wddaccel.c Mon Jan 09 22:55:04 2006 UTC
@@ -152,8 +152,8 @@ static void ddraw_draw_sprite(BITMAP * b
int sx, sy, w, h;

if (is_video_bitmap(sprite) || is_system_bitmap(sprite)) {
- sx = sprite->x_ofs;
- sy = sprite->y_ofs;
+ sx = 0; /* sprite->x_ofs & sprite->y_ofs will be accounted for in ddraw_masked_blit */
+ sy = 0;
w = sprite->w;
h = sprite->h;

@@ -508,4 +508,3 @@ void gfx_directx_enable_triple_buffering
gfx_capabilities |= GFX_CAN_TRIPLE_BUFFER;
}
}
-

Neil Walker
Member #210
April 2000
avatar

Well done Thomas. What you're saying is x and y offsets are being called both by draw_sprite and masked_blit, thus my 'doubling' effect. This makes sense as only draw_sprite was not working.

So two more questions:

1. given draw_sprite eventually calls masked_blit for system/video bitmaps at the end of the function why do we need any of the clipping checks in draw_sprite? why not just call masked_blit with the know parameters?

2. there is still the demonstrative code of system bitmaps crashing when you delete any sub-bitmap of it, which is the second part of my tests.

Neil.
MAME Cabinet Blog / AXL LIBRARY (a games framework) / AXL Documentation and Tutorial

wii:0356-1384-6687-2022, kart:3308-4806-6002. XBOX:chucklepie

Peter Wang
Member #23
April 2000

It's only two lines :-) As soon as someone confirms it works I'll commit it. I'm sure it's correct anyway. Thanks to all involved.

Neil Walker
Member #210
April 2000
avatar

If no one else volunteers, I'll recompile and see what happens. I have a poorly baby lying beside me, so who knows when I'll get stopped ;)

However, what about my comment regarding the need for all the code in draw_sprite?

Neil.
MAME Cabinet Blog / AXL LIBRARY (a games framework) / AXL Documentation and Tutorial

wii:0356-1384-6687-2022, kart:3308-4806-6002. XBOX:chucklepie

Thomas Harte
Member #33
April 2000
avatar

Quote:

there is still the demonstrative code of system bitmaps crashing when you delete any sub-bitmap of it, which is the second part of my tests.

Sorry, yes, I've found the cause of that but am otherwise lost as I haven't traced create_sub_bitmap yet.

destroy_bitmap is at line 1418 of src/graphics.c. It has a special branch for anything that passes is_video_bitmap, which includes sub bitmaps. Specifically, starting on line 1423 of that same file:

1 if (is_video_bitmap(bitmap)) {
2 /* special case for getting rid of video memory bitmaps */
3 ASSERT(!_dispsw_status);
4 
5 prev = NULL;
6 pos = vram_bitmap_list;
7 
8 while (pos) {
9 if (pos->bmp == bitmap) {
10 if (prev)
11 prev->next_y = pos->next_y;
12 else
13 vram_bitmap_list = pos->next_y;
14 
15 if (pos->x < 0) {
16 /* the driver is in charge of this object */
17 gfx_driver->destroy_video_bitmap(bitmap);
18 free(pos);
19 return;
20 }

On DirectX, gfx_driver->destroy_video_bitmap directly deletes the bitmap no questions asked. So the memory for the parent bitmap is destroyed and both it and all sub bitmaps of it become invalid. Obviously an is_sub_bitmap needs to be checked somewhere, but I have no idea whether that is meant to be the responsibility of the platform neutral code or the driver specific.

Quote:

However, what about my comment regarding the need for all the code in draw_sprite?

I have to admit that I couldn't see a purpose for it. Perhaps the change to making that function sit on top of masked_blit was a late addition to the code?

Neil Walker
Member #210
April 2000
avatar

Thanks thomas. Video bitmaps are fine, it is only 'system' bitmaps that cause the crash when you destroy a sub-bitmap. Video and memory work fine.

This code is at line 1466.

Neil.
MAME Cabinet Blog / AXL LIBRARY (a games framework) / AXL Documentation and Tutorial

wii:0356-1384-6687-2022, kart:3308-4806-6002. XBOX:chucklepie

Thomas Harte
Member #33
April 2000
avatar

Hmmm. Then I've misunderstood the code a little. But line 1471 of src/graphics.c jumps to line 656 of src/win/wddbmp.c, then on to line 299 which is an IDirectDrawSurface2_Release, freeing the memory of the parent bitmap.

Peter Wang
Member #23
April 2000

Thomas, I came to the same conclusion regarding destroy_bitmap(). I think that if a subbitmap is created by platform-specific code, then they should be destroyed by the platform-specific code. If created by the fallback case (platform neutral) then they should be destroyed by the platform neutral code. This possibly requires a new BMP_ID_* flag to remember how the subbitmap was created.

edit: I think the pos<0 prevents the problem with video bitmaps.

Neil Walker
Member #210
April 2000
avatar

The code change works, three cheers for Thomas ;)

Attached is an executable that used to fail, and now it works. I've included the source code, the bitmap, the .lib and the new dll if anyone wants to try or use a new .lib/dll file.

To see it failing simply rename the .DLL so that the exe will use the original dll and watch it show the wrong graphics!

btw, before the code is checked in, does someone want to see if all the clipping code is necessary? if you call masked_blit directly it doesn't do any of that.

This example code can also be used to see that when you use a SYSTEM bitmap (follow the comments) it crashes when you have subbitmaps and try to delete a sub-bitmap.

ps, to celebrate I've upped my AXL_FRAMS project from 70% to 90% :)

Neil.
MAME Cabinet Blog / AXL LIBRARY (a games framework) / AXL Documentation and Tutorial

wii:0356-1384-6687-2022, kart:3308-4806-6002. XBOX:chucklepie

Evert
Member #794
November 2000
avatar

Quote:

Thomas, I came to the same conclusion regarding destroy_bitmap(). I think that if a subbitmap is created by platform-specific code, then they should be destroyed by the platform-specific code. If created by the fallback case (platform neutral) then they should be destroyed by the platform neutral code. This possibly requires a new BMP_ID_* flag to remember how the subbitmap was created.

Is it not enough to know the type of bitmap and if the vtable includes functions for creating and destroying them? It seems to me that if it does, they are used and if it does not, the platform-neutral code is used. Am I missing something?

Quote:

The code change works, three cheers for Thomas ;)

Hip hip hurray!
This must be a problem that has been in Allegro since... well, forever.

Quote:

btw, before the code is checked in, does someone want to see if all the clipping code is necessary?

Doesn't really matter, since it can be done in succesive commits anyway. In fact, I think fixing the bug and removing redundant code should be done in seperate commits anyway, since you may want to roll back one but not the other if it turns out that the code was not as redundant as you thought it would be. :)

Peter Wang
Member #23
April 2000

Quote:

Is it not enough to know the type of bitmap and if the vtable includes functions for creating and destroying them? It seems to me that if it does, they are used and if it does not, the platform-neutral code is used. Am I missing something?

Hmm, yes, but a bit ugly. What do you think of this patch? (untested)
All this is a bit theoretical (not the bug Neil pointed out), since no gfx or system drivers have non-null create_sub_bitmap methods anyway.

1--- src/graphics.c (revision 5698)
2+++ src/graphics.c (local)
3@@ -1430,6 +1430,38 @@ void destroy_bitmap(BITMAP *bitmap)
4 VRAM_BITMAP *prev, *pos;
5 
6 if (bitmap) {
7+ if (is_sub_bitmap(bitmap)) {
8+ /* the order of these checks must follow that of create_sub_bitmap */
9+ if (bitmap->vtable->create_sub_bitmap) {
10+ ASSERT(FALSE);
11+ /* XXX: what do we do here? GFX_VTABLE has no
12+ destroy_bitmap method. I think we should just ``remove'' the
13+ GFX_VTABLE create_sub_bitmap method. Nobody uses it at the moment,
14+ as far as I can tell.
15+
16+ ASSERT(bitmap->vtable->destroy_bitmap);
17+ bitmap->vtable->destroy_bitmap(bitmap);
18+ */
19+ return;
20+ }
21+ else if (system_driver->create_sub_bitmap) {
22+ ASSERT(system_driver->destroy_bitmap);
23+ system_driver->destroy_bitmap(bitmap);
24+ return;
25+ }
26+ else {
27+ /* system_driver->create_bitmap is not used to create sub-bitmaps
28+ * (see create_sub_bitmap), so if system_driver->create_sub_bitmap
29+ * is NULL then this sub-bitmap must have been created by graphics.c,
30+ * so we should NOT try using system_driver->destroy_bitmap first.
31+ */
32+ ASSERT(bitmap->dat == NULL);
33+ _AL_FREE(bitmap);
34+ }
35+ return;
36+ }
37+
38+ /* non-subbitmaps */
39 if (is_video_bitmap(bitmap)) {
40 /* special case for getting rid of video memory bitmaps */
41 ASSERT(!_dispsw_status);
42@@ -1483,7 +1515,7 @@ void destroy_bitmap(BITMAP *bitmap)
43 }
44 }
45 
46- /* normal memory or sub-bitmap destruction */
47+ /* normal memory bitmap destruction */
48 if (system_driver->destroy_bitmap) {
49 if (system_driver->destroy_bitmap(bitmap))
50 return;

Quote:

Should this patch make the grade, please acknowledge both Neil Walker & myself in the authors file

Patch committed, cheers.

Neil Walker
Member #210
April 2000
avatar

Well, when draw_sprite decides it is system/video it calls masked_blit. The same masked_blit you can call directly. masked_blit does not do any clipping tests. Therefore either draw_sprite is doing extra work it shouldn't or masked_blit is not doing the checks it should.

As for the sub-bitmap fault in windows, here is some minimal code that recreates the crash on any windows system:

1#include <allegro.h>
2 
3int main(void)
4{
5 BITMAP* page1;
6 BITMAP* sub1;
7
8 allegro_init();
9 install_keyboard();
10 set_color_depth(32);
11 set_gfx_mode(GFX_AUTODETECT_WINDOWED,640,480,0,0);
12
13 page1=create_system_bitmap(640,480);
14 if(!is_system_bitmap(page1))
15 {
16 textout_ex(screen,font,"no system bitmap",0,0,makecol(255,255,255),-1);
17 readkey();
18 }
19 else
20 {
21 sub1=create_sub_bitmap(page1,100,100,100,100);
22 textout_ex(screen,font,"press any key for a crash",0,0,makecol(255,255,255),-1);
23 readkey();
24 destroy_bitmap(sub1);
25 }
26 destroy_bitmap(page1);
27 return 0;
28}
29END_OF_MAIN()

Neil.
MAME Cabinet Blog / AXL LIBRARY (a games framework) / AXL Documentation and Tutorial

wii:0356-1384-6687-2022, kart:3308-4806-6002. XBOX:chucklepie

Evert
Member #794
November 2000
avatar

Quote:

Well, when draw_sprite decides it is system/video it calls masked_blit. The same masked_blit you can call directly. masked_blit does not do any clipping tests. Therefore either draw_sprite is doing extra work it shouldn't or masked_blit is not doing the checks it should.

For a moment I was thinking you were talking about draw_sprite() and masked_blit() in general, but it's the Windows-specific hardware accelerated implementation, right?
In general, you cannot always use masked_blit() where you can use draw_sprite().

Regarding the tests, they do look a bit redundant (or missing in the case of ddraw_masked_blit). What happens if you remove them altogether? Is clipping still performed as it should? What about maksed_blit? Does that do proper clipping?

Neil Walker
Member #210
April 2000
avatar

I was referring to the ddraw code, and as far as I can see masked_blit it does no clipping checks. Similarly as far as "In general, you cannot always use masked_blit() where you can use draw_sprite()." goes, as far as I can see for video/system bitmaps (don't know about the software _original function) draw_sprite is now just a wrapper for masked_blit.

Neil.
MAME Cabinet Blog / AXL LIBRARY (a games framework) / AXL Documentation and Tutorial

wii:0356-1384-6687-2022, kart:3308-4806-6002. XBOX:chucklepie

Thomas Harte
Member #33
April 2000
avatar

Quote:

Is clipping still performed as it should? What about maksed_blit? Does that do proper clipping?

Although DirectDraw supersets the Allegro clipping functionality and the DirectDraw driver creates a clipper it appears that there is no DirectDraw version of set_clip[_rect]. So it seems to me that ddraw_masked_blit should clip "incorrectly" based on a reading of the current manual but if it were "correct" then there would be no need for the clipping in ddraw_draw_sprite.

Can anyone verify that?

Evert
Member #794
November 2000
avatar

Quote:

and as far as I can see masked_blit it does no clipping checks.

Yes, as far as I can see, I agree. However, I'd like to know what the code does in practice and if ddraw_masked_blit() actually works properly now (since it doesn't seem to do the clipping) and if the tests in ddraw_draw_sprite() are redundant or not. That will have to be tested by someone. Could you test these things?

Quote:

Similarly as far as "In general, you cannot always use masked_blit() where you can use draw_sprite()." goes, as far as I can see for video/system bitmaps (don't know about the software _original function) draw_sprite is now just a wrapper for masked_blit.

In general it certainly isn't. draw_sprite() can be used to draw 8bpp sprites onto any colourdepth target bitmap, while masked_blit() only works between equal depth bitmaps. Since video bitmaps all have the same depth as the screen anyway, there is no 8->any special case there and the two should be similar (as they indeed are now in the DX driver). I don't actually know about system bitmaps (they don't really exist in *nix), but I think they're similar to video bitmaps where their colourdepth is concerned.

Quote:

So it seems to me that ddraw_masked_blit should clip "incorrectly"

Yes, that was the impression I got.

Neil Walker
Member #210
April 2000
avatar

What do you want testing exactly, just use masked_blit to draw a bitmap onto another but make the co-ordinates outside the target bitmap?

Neil.
MAME Cabinet Blog / AXL LIBRARY (a games framework) / AXL Documentation and Tutorial

wii:0356-1384-6687-2022, kart:3308-4806-6002. XBOX:chucklepie

Thomas Harte
Member #33
April 2000
avatar

Quote:

What do you want testing exactly?

Use set_clip_rect to reduce the clipping rectangle to only a small portion of the target bitmap, then see whether draw_sprite and/or masked_blit with video/system bitmaps obeys that reduced rectangle. I may get a go on a Windows PC later, so I may have a go myself...

Neil Walker
Member #210
April 2000
avatar

Don't worry, I'll test it later as I can easily check all permutations of video/system/memory bitmaps and sub-bitmaps in all resolutions, colour depths and windowed modes with the power of AXL ;)

Neil.
MAME Cabinet Blog / AXL LIBRARY (a games framework) / AXL Documentation and Tutorial

wii:0356-1384-6687-2022, kart:3308-4806-6002. XBOX:chucklepie

 1   2 


Go to: