4.2.1 seems to be fooked with font grabbing
Neil Walker

Hello,
I was in the process of testing 4.2.1 release to see if it fixes the system bitmap problem with sub-bitmaps (http://allegro.cc/forums/thread/589858/646845#target) - and it does, however I've now come across quite a major bug.

Before I get out the debugger and fail to find the fault, just wondered if anyone else has come across this. It is a windows build, btw.

Basically, when you use grab_font_from_bitmap(f) when the bitmap is not a memory bitmap, (i.e. video or system bitmap) it takes about 15 to 20 seconds to make the call. I have 5 fonts to be grabbed so that's about two minutes just waiting for a program to start. I guess it's a locking problem? In 4.2.0 it is immediate (milliseconds) for any bitmap type.

[edit]tell a lie, with memory bitmaps grab_from_bitmap akes about 2 seconds per bitmap font, so something is definitely up[/edit]

The bitmap is fine and I can run 4.2.0 on it and all is well.

I guess the simplest question is, if it's related specifically to the grab_font code, can anyone do a diff to see what has changed?

Peter Wang

Here are the changes in fontbmp.c from 4.2.0 to 4.2.1. The _AL_MALLOC/_AL_FREE changes are nothing, so the only thing that may make a difference is the first hunk.

1--- fontbmp.c (/mirror/allegro/allegro/tags/v4-2-0/src/fontbmp.c) (revision 7758)
2+++ fontbmp.c (/mirror/allegro/allegro/tags/v4-2-1/src/fontbmp.c) (revision 7758)
3@@ -32,7 +32,10 @@ static void font_find_character(BITMAP *
4 {
5 int c;
6
7- if (bitmap_color_depth(bmp) == 8) {
8+ if (_bitmap_has_alpha(bmp)) {
9+ c = getpixel(bmp, 0, 0);
10+ }
11+ else if (bitmap_color_depth(bmp) == 8) {
12 c = 255;
13 }
14 else {
15@@ -85,7 +88,7 @@ static int import_bitmap_font_mono(BITMA
16 if(w <= 0 || h <= 0) {
17 int j;
18
19- gl<i> = _al_malloc(sizeof(FONT_GLYPH) + 8);
20+ gl<i> = _AL_MALLOC(sizeof(FONT_GLYPH) + 8);
21 gl<i>->w = 8;
22 gl<i>->h = 8;
23
24@@ -94,7 +97,7 @@ static int import_bitmap_font_mono(BITMA
25 else {
26 int sx = ((w + 7) / 8), j, k;
27
28- gl<i> = _al_malloc(sizeof(FONT_GLYPH) + sx * h);
29+ gl<i> = _AL_MALLOC(sizeof(FONT_GLYPH) + sx * h);
30 gl<i>->w = w;
31 gl<i>->h = h;
32
33@@ -222,18 +225,18 @@ FONT *grab_font_from_bitmap(BITMAP *bmp)
34 import_x = 0;
35 import_y = 0;
36
37- f = _al_malloc(sizeof *f);
38+ f = _AL_MALLOC(sizeof *f);
39 if (end == -1) end = bitmap_font_count(bmp) + begin;
40
41 if (bitmap_font_ismono(bmp)) {
42- FONT_MONO_DATA* mf = _al_malloc(sizeof(FONT_MONO_DATA));
43+ FONT_MONO_DATA* mf = _AL_MALLOC(sizeof(FONT_MONO_DATA));
44
45- mf->glyphs = _al_malloc(sizeof(FONT_GLYPH*) * (end - begin));
46+ mf->glyphs = _AL_MALLOC(sizeof(FONT_GLYPH*) * (end - begin));
47
48 if ( import_bitmap_font_mono(bmp, mf->glyphs, end - begin) ) {
49- free(mf->glyphs);
50- free(mf);
51- free(f);
52+ _AL_FREE(mf->glyphs);
53+ _AL_FREE(mf);
54+ _AL_FREE(f);
55 f = NULL;
56 }
57 else {
58@@ -247,13 +250,13 @@ FONT *grab_font_from_bitmap(BITMAP *bmp)
59 }
60 }
61 else {
62- FONT_COLOR_DATA* cf = _al_malloc(sizeof(FONT_COLOR_DATA));
63- cf->bitmaps = _al_malloc(sizeof(BITMAP*) * (end - begin));
64+ FONT_COLOR_DATA* cf = _AL_MALLOC(sizeof(FONT_COLOR_DATA));
65+ cf->bitmaps = _AL_MALLOC(sizeof(BITMAP*) * (end - begin));
66
67 if( import_bitmap_font_color(bmp, cf->bitmaps, end - begin) ) {
68- free(cf->bitmaps);
69- free(cf);
70- free(f);
71+ _AL_FREE(cf->bitmaps);
72+ _AL_FREE(cf);
73+ _AL_FREE(f);
74 f = 0;
75 }
76 else {

The first section was changed due to this:

r5721 (orig r5674):  elias | 2006-01-19 07:36:55 +1100

Patch to add support for anti-aliased bitmap fonts. It does the following:
- Allow to read in alpha fonts. If alpha values are detected in a 32bit font bitmap, the top left pixel is used as mask color instead of yel- Add a make_trans_font function. This simply changes the vtable of the passed color font to one using draw_trans_sprite for render_char.
- Since there were now 3 locations using bitmap_has_alpha, made it an internal function.

But bitmap_has_alpha is implemented very inefficiently:

1--- mirror/allegro/allegro/branches/4.2/src/gfx.c (revision 5720)
2+++ mirror/allegro/allegro/branches/4.2/src/gfx.c (revision 5721)
3@@ -163,6 +163,29 @@ void clear_bitmap(BITMAP *bitmap)
4 
5 
6 
7+/* _bitmap_has_alpha:
8+ * Checks whether this bitmap has an alpha channel.
9+ */
10+int _bitmap_has_alpha(BITMAP *bmp)
11+{
12+ int x, y, c;
13+
14+ if (bitmap_color_depth(bmp) != 32)
15+ return FALSE;
16+
17+ for (y = 0; y < bmp->h; y++) {
18+ for (x = 0; x < bmp->w; x++) {
19+ c = getpixel(bmp, x, y);
20+ if (geta32(c))
21+ return TRUE;
22+ }
23+ }
24+
25+ return FALSE;
26+}
27+
28+
29+
30 /* vsync:
31 * Waits for a retrace.
32 */

So I guess that's the problem.

Neil Walker

and the fix is?

I'm not at my dev machine at the minute, but to prove it, I'll redo the bitmaps in 8bpp and see if it does anything. But how can scanning through a bitmap take so long? and why is it using getpixel and not _getpixel? and shouldn't there be a flag in the functions to turn this new alpha/alias checking off? and why is the get alpha called for 8bpp explicitly then in the get alpha code returned immediately if 8bpp? so many questions ;)

btw, pardon my French but those if statements at the top look awefully messy.

Cheers.

Thomas Fjellstrom
Quote:

But how can scanning through a bitmap take so long?

Reading from video bitmaps is VERY VERY slow.

Neil Walker

these are system bitmaps as well that take 20 or so seconds. memory bitmaps have also increased from milliseconds to seconds.

Thomas Fjellstrom

I would assume system bitmaps are just as slow.. I'm not 100% sure though.

Neil Walker

if you take a stab at a memory bitmap taking 100ms to scan in 4.2 (it was immediate for me for 5 bitmaps) and in the new one it takes 1200ms (it took about 7 seconds in 4.2.1), then that's a 12x decrease in speed with the new code, even with memory bitmaps.

But I think Peter is right and the fault is the way the new code patch works for checking for aliased/alpha. For starters there's one check looking for 8bpp but the function it calls returns immediately if it is 8bpp, then there's the use of getpixel.

Thomas Fjellstrom

This is what I'm saying, its reading every single pixel from the bitmap, and checking if theres a non solid alpha value. That reading is comparatively slow with memorybitmaps, but much slower on non memory bitmaps (which you first started talking about).

Evert
Quote:

I'm not at my dev machine at the minute, but to prove it, I'll redo the bitmaps in 8bpp and see if it does anything.

Or you can use any bitmap that isn't 32 bpp.

Quote:

But how can scanning through a bitmap take so long?

Because it's checking the entire bitmap for the presence of an alpha channel for each character you grab from it, that's why. It breaks out of this as soon as it finds an alpha value, but the worst case is obviously going to be the case where no alpha value is set at all.
It shouldn't be very difficult to provide a fix for this that only does the check once per call of grab_font_from_bitmap(), give me half an hour or so.

Quote:

and why is it using getpixel and not _getpixel?

Because _getpixel() only works on 8 bit memory bitmaps whereas getpixel() works on any type of bitmap in any colour depth?
Beside which, you're not supposed to call grab_font_from_bitmap() in speed-critical code. In other words, the speed difference between getpixel() and _getpixel*() should not be that important.
That said, feel free to optimise it and submit a patch (EDIT: see attachment).

Quote:

and shouldn't there be a flag in the functions to turn this new alpha/alias checking off?

I don't think so. That would really make things messy.

Quote:

and why is the get alpha called for 8bpp explicitly then in the get alpha code returned immediately if 8bpp?

It isn't called for 8bpp. The code checks first if the bitmap has an alpha channel (false for all but 32 bpp, which can have one), then if it's 8bpp:

static void font_find_character(BITMAP *bmp, int *x, int *y, int *w, int *h)
{
   int c;

   if (_bitmap_has_alpha(bmp)) {
      c = getpixel(bmp, 0, 0);
   }
   else if (bitmap_color_depth(bmp) == 8) {
      c = 255;
   }
   else {
      c = makecol_depth(bitmap_color_depth(bmp), 255, 255, 0);
   }

Quote:

those if statements at the top look awefully messy.

How would you do this then?

Now, personal opinion: it makes no sense whatsoever to call grab_font_from_bitmap on a video or system bitmap, precisely because you need to read data back from teh bitmap. This should be obvious, but if you want we can add a sentence to the documentation explaining this.

EDIT: patch attached.

Neil Walker
Quote:

Beside which, you're not supposed to call grab_font_from_bitmap() in speed-critical code.

It's still a lot slower for memory bitmaps, but liveable I guess.

Quote:

it makes no sense whatsoever to call grab_font_from_bitmap on a video or system bitmap

I've always treated system bitmaps as if they were memory bitmaps, but faster if available.

Quote:

How would you do this then?

My mistake, I didn't notice the diff - :)

Quote:

EDIT: patch attached.

Excellent, thanks Evert. The obvious fixes are always the best. Hopefully it'll make it to the next version.

Evert
Quote:

Excellent, thanks Evert.

Just so there's no confusion, this means you tested it and it corrects the problem for you, right? I know the code compiles and links fine, but I didn't have an opportunity to check that it actually works. It should, but it wouldn't be the first time I write code that "should" work but doesn't. ;)

Quote:

Hopefully it'll make it to the next version.

I'll commit it as soon as someone tests it and confirms it works as intended. :)

Neil Walker

Erm, no. For 4.2.1 I was lazy and just got the pre-built version. If no-one takes you up by the end of today to recompile (I can test it) a new dll, I'll do it tomorrow, if someone can tell me the diff tool to use (windows).

tobing
Quote:

if someone can tell me the diff tool to use (windows)

WinMerge.

Evert

Reglar patch should be fine as well.

Neil Walker

I use winmerge and oddly just tried to find out how to read in diff files a few minutes ago, but all I could find was an option to create a diff from two files, not open one up.

torhu

http://gnuwin32.sourceforge.net/packages/patch.htm

To apply the patch, when in the allegro dir:

patch -p0 -i fontbmp.diff

You can always add --dry-run to see if everything is ok before you actually apply the patch.

Winmerge, TortoiseSVN and many others can create patches, or you can use the original:

http://gnuwin32.sourceforge.net/packages/diffutils.htm

Evert

bump

Neil Walker

I haven't forgotten, I've been ill most of the week.

Thread #589885. Printed from Allegro.cc