On 05/06/2013 03:05 PM, Max TenEyck Woodbury wrote:
Just to make this clear, the most recent version of this patch is such
a graceful handling, right?
I haven't worked on gdi32/freetype.c much, so I wouldn't be the one to
say for sure (you should probably talk to Alexandre Julliard, Dmitry
Timoshkov, or Huw Davies), but I'll give you my thoughts anyway:
On 05/05/2013 12:48 PM, m...@mtew.isa-geek.net wrote:
+ /* HACKHACK: If a font has tmHeight=0, let us know. */
+ if (!font->potm->otmTextMetrics.tmHeight) {
+ ERR("Font named '%s' has tmHeight=0, aveWidth=%d!\n",
font->ft_face->family_name, font->aveWidth);
+ }
+
Since we've learned that tmHeight=0 is not actually an error, it doesn't
make sense to log it using ERR(...). I would suggest a TRACE(...) or
(preferably) nothing at all. Also, when I wrote the ERR message patch
for you, it was intended only to help you locate the problematic font
and wasn't meant to be included in upstream Wine. (Which is why I marked
it with HACKHACK and didn't bother to use debugstr_a.)
As for the actual check, it makes sense to consider what the math is
trying to do: If aveWidth is extremely large (relative to tmHeight),
then assume aveWidth is wrong and set it to 0. If tmHeight is 0, then it
seems reasonable to assume that aveWidth isn't valid either.
I'm not sure why the sanity check cares to do this:
(aveWidth+tmHeight-1) / tmHeight > 100
When it can just do this:
(aveWidth+tmHeight-1) / tmHeight > 100
= (aveWidth-1)/tmHeight + 1 > 100
= (aveWidth-1)/tmHeight > 99
= (aveWidth-1) > tmHeight*99
= aveWidth > tmHeight*99 + 1
≈ aveWidth > tmHeight*100
This allows us to write out tmHeight just once, and is also much easier
to understand at a glance.
So, I would just simplify the whole sanity check to something like:
/* Make sure that the font has sane width/height ratio */
if (font->aveWidth > font->potm->otmTextMetrics.tmHeight * 100)
{
WARN("Ignoring too large font->aveWidth %d\n", font->aveWidth);
font->aveWidth = 0;
}
But even then, the sanity check itself sounds like a workaround for a
much deeper bug. Maybe add a "FIXME: Investigate why this is necessary"
comment while you're at it?
Best,
Sam
