549816 - synthetic variations reftest failing with dwrite enabled

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[John Daggett (:jtd)]

With DirectWrite enabled, synthetic variations reftest fails.

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a2pre) Gecko/20100228 Minefield/3.7a2pre

Steps to reproduce:

1. Enable DirectWrite (gfx.font_rendering.directwrite.enabled to true, restart)
2. Load test below:

http://people.mozilla.org/~jdaggett/font-face/synthetic-variations.html

Result: several lines up from the bottom, normal and italic appear bolded in two rows.  These should appear non-bolded, as on other lines.

This reftest passes when DirectWrite is not enabled, when GDI is used.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Fix & use cross platform synthetic bolding logic with DWrite

So, I decided to remove my DWrite synthetic bolding logic, and start listening to aNeedsBold in CreateFontInstance. However this produced some undesired results, the bottom 4 lines of the synthetic variations test would no longer be bolded. This was because nothing was turning on aNeedsBold when in a family with multiple faces a 600/700/800/900 font was matched to a lower weight one (the 300 in this case), when the weightDistance was actually 0. I added an additional check there which I think is what we want, we should look carefully at if what I did was right. All font-face reftests pass on both GDI and DWrite with this patch.

I verified GDI still works (not surprising since that ignores aNeedsBold), but Mac does use it, so I'm not sure if that will be alright.

Comment 2

[Jonathan Kew [:jfkthame]]

Comment on attachment 430127 [details] [diff] [review]
Fix & use cross platform synthetic bolding logic with DWrite

This boldness stuff confuses me every time I try to read it, but I think this is OK! It also seems to behave correctly with updated GDI code I'm working on that does make use of the aNeedsBold flag.

Comment 3

[John Daggett (:jtd)]

Comment on attachment 430127 [details] [diff] [review]
Fix & use cross platform synthetic bolding logic with DWrite

-    if (((weightDistance == 0 && baseWeight >= 6) 
-        || (weightDistance > 0)) && !fe->IsBold()) {
+    if (aNeedsBold) {

-    if (weightDistance > 0 && wghtSteps <= absDistance) {
+    if ((weightDistance > 0 && wghtSteps <= absDistance) ||
+        (baseWeight >= 6 && !matchFE->IsBold() &&
+          (wghtSteps - 1) <= weightDistance)) {

The current code only sets aNeedsBold when bolder is used, it doesn't
set it in cases where the weight is >= 600 but the font is actually <
600.  That's why the gfxMacFont constructor has the logic below:

  if (!aFontEntry->IsBold()
      && ((weightDistance == 0 && targetWeight >= 600) || (weightDistance > 0 && aNeedsBold)))
  {
      mSyntheticBoldOffset = 1;  // devunit offset when double-striking text to fake boldness
  }

The simplest solution is to tweak the original code in
gfxDWriteFont::gfxDWriteFont and leave the code in
gfxFontFamily::FindFontForStyle as is:

  if (!fe->IsBold() &&
      ((weightDistance == 0 && baseWeight >= 6) || (weightDistance > 0 && aNeedsBold)))
  {
      sims |= DWRITE_FONT_SIMULATIONS_BOLD;
  }

Comment 4

[John Daggett (:jtd)]

Hmm, looks like Jonathan's patch for GDI fonts on bug 502906 makes the same assumption you're making so maybe this logic would fit better in FindFontForStyle.  If so, then you'll also need to update the logic in gfxMacFont.

Comment 5

[Bas Schouten (:bas.schouten)]

(In reply to comment #4)
> Hmm, looks like Jonathan's patch for GDI fonts on bug 502906 makes the same
> assumption you're making so maybe this logic would fit better in
> FindFontForStyle.  If so, then you'll also need to update the logic in
> gfxMacFont.

I was assuming it wouldn't matter, since I thought it would just be redundant, but not harmful. (I couldn't check since I have no Mac to test)

Comment 6

[Jonathan Kew [:jfkthame]]

Well, it doesn't cause any unit-test failures (as I've been running with this patch in place, and pushing tryserver tests), but it can presumably be tidied up a bit if we take this approach.

Comment 7

[John Daggett (:jtd)]

Comment on attachment 430127 [details] [diff] [review]
Fix & use cross platform synthetic bolding logic with DWrite

Plusing the review and I'll add a follow-on patch to tweak things a bit.

Comment 8

[John Daggett (:jtd)]

Attachment: follow-on patch, adjust code to always use needsBold

Always use the needsBold when synthetic bolding is necessary, not just in the 'bolder' case.

Comment 9

[John Daggett (:jtd)]

Landed on trunk
http://hg.mozilla.org/mozilla-central/rev/232629d34cc7
http://hg.mozilla.org/mozilla-central/rev/fbef676a2762