This seems suspicious to me. Why can't we use those values in glyphmetrics if GetGlyphOutlineW returns 0? Does Windows provide bad values in this case, or is our GetGlyphOutlineW broken? When does the call to GetGlyphOutlineW in the second loop fail, and why ignore those failures?
BTW, we should probably quit early if max_glyphsize is 0 at the end of the first loop, rather than fail with OutOfMemory.
On Sun, 15 Sep 2013 16:32:25 -0500, Vincent Povirk wrote:
This seems suspicious to me. Why can't we use those values in glyphmetrics if GetGlyphOutlineW returns 0? Does Windows provide bad values in this case, or is our GetGlyphOutlineW broken?
Thanks for you comments. The answer is yes, as you guessed. Typically, native GetGlyphOutlineW(hdc, ' ', GGO_BITMAP, &gm, 0, NULL, ...) returns gm.gmBlackBoxX = 1 instead of 0. Unfortunately, Wine's didn't, and existing source codes (winex11, gdiplus, gdi32) expect 0 in this case.
In patch 98817, I'll update this GGO behavior to native compatible. Thus, I need to change those code before to avoid regressions.
When does the call to GetGlyphOutlineW in the second loop fail, and why ignore those failures?
Native GGO returns GDI_ERROR in GetGlyphOutlineW(hdc, ' ', GGO_BITMAP, &gm, bufsize, buf, ...) case. Please note that bufsize and buf are specified and guess they aren't null.
The second loop is for creating mask bitmaps. We don't need to update bitmaps if the character is empty. However, we also need to update the bitmap mask after that character. So, I just ignore the error and continue at that point.
Akihiro Sagawa
Thanks for you comments. The answer is yes, as you guessed. Typically, native GetGlyphOutlineW(hdc, ' ', GGO_BITMAP, &gm, 0, NULL, ...) returns gm.gmBlackBoxX = 1 instead of 0. Unfortunately, Wine's didn't, and existing source codes (winex11, gdiplus, gdi32) expect 0 in this case.
I don't think the existing code for finding the mask size would have a problem with gmBlackBoxX being 0. But it would be best to skip updating min_x, min_y, max_x, and max_y in this case, since we don't really need the mask to include the position of an empty glyph.
Does Wine's GGO return a non-zero value currently? That would mean a regression in gdiplus drawing only empty glyphs when you fix GGO.
Native GGO returns GDI_ERROR in GetGlyphOutlineW(hdc, ' ', GGO_BITMAP, &gm, bufsize, buf, ...) case. Please note that bufsize and buf are specified and guess they aren't null.
The second loop is for creating mask bitmaps. We don't need to update bitmaps if the character is empty. However, we also need to update the bitmap mask after that character. So, I just ignore the error and continue at that point.
I think a comment saying just /* empty glyph */ inside the if blocks for these new cases would make the code much clearer.
On Mon, 16 Sep 2013 11:33:34 -0500, Vincent Povirk wrote:
Typically, native GetGlyphOutlineW(hdc, ' ', GGO_BITMAP, &gm, 0, NULL, ...) returns gm.gmBlackBoxX = 1 instead of 0. Unfortunately, Wine's didn't, and existing source codes (winex11, gdiplus, gdi32) expect 0 in this case.
I don't think the existing code for finding the mask size would have a problem with gmBlackBoxX being 0. But it would be best to skip updating min_x, min_y, max_x, and max_y in this case, since we don't really need the mask to include the position of an empty glyph. Does Wine's GGO return a non-zero value currently? That would mean a regression in gdiplus drawing only empty glyphs when you fix GGO.
I didn't mean that. Native GGO returns a non-zero gmBlackBoxX/Y value. Wine's GGO currently returns a zero gmBlackBoxX/Y value for empty glyph. I ran some tests [1] before submitting. Please read that, for details. [1] http://newtestbot.winehq.org/JobDetails.pl?Key=2170
Skipping updating min-max variables might be a good idea, but I feel this is the off topic issue. The patch intends to avoid regressions after fixing GGO. In other words, the aim is creating the same mask before and after the commit.
The second loop is for creating mask bitmaps. We don't need to update bitmaps if the character is empty. However, we also need to update the bitmap mask after that character. So, I just ignore the error and continue at that point.
I think a comment saying just /* empty glyph */ inside the if blocks for these new cases would make the code much clearer.
Indeed. I'll add some comments in the next try.
Akihiro Sagawa
Typically, native GetGlyphOutlineW(hdc, ' ', GGO_BITMAP, &gm, 0, NULL, ...) returns gm.gmBlackBoxX = 1 instead of 0. Unfortunately, Wine's didn't, and existing source codes (winex11, gdiplus, gdi32) expect 0 in this case.
I don't think the existing code for finding the mask size would have a problem with gmBlackBoxX being 0. But it would be best to skip updating min_x, min_y, max_x, and max_y in this case, since we don't really need the mask to include the position of an empty glyph. Does Wine's GGO return a non-zero value currently? That would mean a regression in gdiplus drawing only empty glyphs when you fix GGO.
I didn't mean that. Native GGO returns a non-zero gmBlackBoxX/Y value. Wine's GGO currently returns a zero gmBlackBoxX/Y value for empty glyph. I ran some tests [1] before submitting. Please read that, for details. [1] http://newtestbot.winehq.org/JobDetails.pl?Key=2170
Skipping updating min-max variables might be a good idea, but I feel this is the off topic issue. The patch intends to avoid regressions after fixing GGO. In other words, the aim is creating the same mask before and after the commit.
The loop isn't going to have a problem with 1 either. As long as the calculated mask size includes the whole area where glyphs will need to be drawn, there won't be a problem. Adding some extra pixels to it isn't going to do anything take a little bit of extra time.
The logic you're adding still potentially makes the mask larger than necessary, and it doesn't appear to do this better than the existing logic in any meaningful way. And since the correct solution would remove the new code entirely, I don't see the point of adding it.
-
Akihiro Sagawa -
Vincent Povirk