maybe a fix for destroying system bitmaps?

maybe a fix for destroying system bitmaps?

Neil Walker

Member #210

April 2000

On my travels I tried to find out why destroy_bitmap() was crashing when you destroyed a system bitmap that was also a sub-bitmap. In graphics.c in the destroy_bitmap it does the following (I've cut out the superfluous stuff):

1void destroy_bitmap(BITMAP *bitmap)
2{
 if (bitmap) {
    if (is_video_bitmap(bitmap)) {
      ... stuff here
    }
    else if (is_system_bitmap(bitmap) ) {
 if (gfx_driver->destroy_system_bitmap) {
    gfx_driver->destroy_system_bitmap(bitmap);
    return;
 }
    }
13 
    /* normal memory or sub-bitmap destruction */
    if (system_driver->destroy_bitmap) {
 if (system_driver->destroy_bitmap(bitmap))
    return;
    }
   ...stuff here
 }
21}

So it does something special for video bitmaps and the special gfx_driver destroy if it is a system bitmap, but further down there is a standard system_driver->destroy for all else. However the comment says 'normal or sub-bitmap destruction', which intrigued me.

Anyway, to cut a long story short I changed
else if (is_system_bitmap(bitmap) ) {

to
else if (is_system_bitmap(bitmap) && !is_sub_bitmap(bitmap)) {

and it now works perfectly with destroying system sub-bitmaps. Does this seem like a fix? My reasoning was someone put the comment in for destroying sub-bitmaps for a reason but maybe forgot about system sub-bitmaps which need a standard destroy not the gfx_driver destroy.

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

Quote:

and it now works perfectly with destroying system sub-bitmaps. Does this seem like a fix?

If it works then yes, it looks like a fix to me. Well done! (But check below for some background).
Personally, I would say (again) that it's up to the system driver that implements destroy_system_bitmap to take proper care of sub bitmaps and not have destroy_bitmap() care about them. So the proper (object oriented) way to fix this, IMO, is to change the Windows implementation of destroy_system_bitmap (which is in wddbmp.c).

Quote:

[...]

My reasoning was someone put the comment in for destroying sub-bitmaps for a reason but maybe forgot about system sub-bitmaps which need a standard destroy not the gfx_driver destroy.

It's an interesting line of reasoning, but unfortunately it's bogus and it's a bit lucky that the comment put you on the right track. The same comment is present in Allegro 3.1, which did not have system bitmaps. So the comment just refers to `normal bitmaps and sub-bitmaps' as opposed to `video bitmaps and sub-bitmaps'.

Neil Walker

Member #210

April 2000

Sadly, I don't know enough about the internals, but at least it should put whoever looks after the Windows code (Elias?) in the right direction, and failing that, just doing the code change I did.

I had a look at the destroy_system_bitmap but it's a small function and it does the same thing as lots of other function in calling stuff that I cannot find in the source code anywhere, e.g. gfx_directx_destroy_surface(...). Where are these gfx_directx_ functions defined/declared?

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

Quote:

at least it should put whoever looks after the Windows code

No one looks after the Windows code, that's the main problem.

Anyway, try

1/* gfx_directx_destroy_system_bitmap:
2 */
3void gfx_directx_destroy_system_bitmap(BITMAP *bmp)
4{
5   /* Special case: use normal destroy_bitmap() for subbitmaps of system bitmaps.
6    * Checked here rather than in destroy_bitmap() because that function should not
7    * make assumptions about the relation between system bitmaps and subbitmaps
8    * thereof.
9    */
10   if (is_sub_bitmap(bmp)) {
11      if (system_driver->destroy_bitmap) {
12   if (system_driver->destroy_bitmap(bmp))
13      return;
14      }
15 
16      if (bmp->dat)
17   _AL_FREE(bmp->dat);
18 
19      _AL_FREE(bmp);
20 
21      return;
22   }
23 
24   /* destroy the surface */
25   gfx_directx_destroy_surface(DDRAW_SURFACE_OF(bmp));
26 
27   _AL_FREE(bmp);
28}

I think it would also work without the if (system_driver->...) special case, since that seems to be NULL for Windows anyway, but best not to make that assumption, I think.
(EDIT: fixed code & tested it).

Quote:

gfx_directx_destroy_surface(...). Where are these gfx_directx_ functions defined/declared?

eglebbk@gandalf:~/Program/Allegro/svn/allegro/allegro/branches/4.2>grep gfx_directx_destroy_surface src/win/*.c
src/win/wddbmp.c:/* gfx_directx_destroy_surface:
src/win/wddbmp.c:void gfx_directx_destroy_surface(DDRAW_SURFACE *surf)
src/win/wddbmp.c:      gfx_directx_destroy_surface(surf);
src/win/wddbmp.c:      gfx_directx_destroy_surface(surf);
src/win/wddbmp.c:      gfx_directx_destroy_surface(surf);
src/win/wddbmp.c:   gfx_directx_destroy_surface(DDRAW_SURFACE_OF(bmp));
src/win/wddovl.c:      gfx_directx_destroy_surface(overlay_surface);
src/win/wddraw.c:      gfx_directx_destroy_surface(gfx_directx_primary_surface);
src/win/wddwin.c:      gfx_directx_destroy_surface(surf);
src/win/wddwin.c:      gfx_directx_destroy_surface(surf);
src/win/wddwin.c:      gfx_directx_destroy_surface(offscreen_surface);
eglebbk@gandalf:~/Program/Allegro/svn/allegro/allegro/branches/4.2>

grep is your friend!

EDIT: Proposed patch attached (tested on XP-64 beta).

Neil Walker

Member #210

April 2000

Hello,
Doesn't compile. Unresolved external symbol __AL_FREE. There's an al_free(), though I can't see how that's different from simply using free().

btw, I didn't realise Robin Burrows did some Allegro coding.

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

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

Elias

Member #358

May 2000

Quote:

btw, I didn't realise Robin Burrows did some Allegro coding.

He even took part in 1 or 2 of the early speedhacks. And he used to hang out in the #allegro of the early days as Oddish. Working on Mappy most of the time back then

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

Neil Walker

Member #210

April 2000

He still does Mappy. Alongside tinyXML, these are among the finest add-ons any Allegro coder can have.

btw, the code change works a treat Evert (assuming AL_FREE is fixed, I just changed it to _al_free(), you can mark that change as working

Now, move back to the VRAM-VRAM thread for more excitement...

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

Quote:

Doesn't compile. Unresolved external symbol __AL_FREE.

That's _AL_FREE, with one underscore. Anyway, the patch worked well (as posted) when I tested it.
However, you may want to check the one I posted on AD just now, since that may be a bit `nicer' in having destroy_system_bitmap() return a status code that tells destroy_bitmap() to deal with the bitmap as it would normally.

Neil Walker

Member #210

April 2000

It is one _ in my code, the compiler puts two in it for me when it returns the error. Is something wrong?

Where is _AL_FREE() anyway, my search couldn't find it, only the function _al_free(), and why are you using it, all the code I've seen on my travels have just had free(bmp).

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

Quote:

It is one _ in my code, the compiler puts two in it for me when it returns the error. Is something wrong?

I'd say your compiler is broken, though I can't see how or why. What compiler are you using? Have you tried another one? Does it complain about the same code in destroy_bitmap()?

Quote:

Where is _AL_FREE() anyway, my search couldn't find it

It's in capitals, which means it's a macro, which suggests that it's in a headerfile.
grep tells me it's include/allegro/internal/aintern.h. No, I have no idea what it's intended purpose is.

Quote:

and why are you using it

Because I C&P'ed that code from destroy_bitmap() and that was using it.

Neil Walker

Member #210

April 2000

Do we have the same code! destroy_bitmap in graphics.c only has free(bitmap) and free(bitmap->dat). Searching the entire include directory only finds the function declaration for _al_free()

But for the record, I have just tried mingw32 (I normally use VC) and that failed with the same error.

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

No, you don't have the same code. _AL_MALLOC/_AL_FREE were added since 4.2.0. They serve no purpose in the 4.2.x series other than to minimise differences between 4.2 and 4.3, to reduce conflicts when merging changes from 4.2 to 4.3. You can just replace _AL_FREE by free.