674909 - Synthetic bolding uses too large pixel offset when zoom is applied to the cairo surface

Status: VERIFIED FIXED

(Core :: Graphics, defect, P2)

Description

[Thomas Arend [:tarend]]

Attachment: Irregular bolding

Bug 634997 introduced bolding on the Galaxy Tab and Galaxy S, but it doesn't look good - see attached screenshot. Bold text looks irregular, some characters seem bold, others seem regular.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

moving to graphics since it won't get seen by the appropriate people here

Comment 3

[Andreas Gal :gal]

Looks like we aren't getting any traction here. The offending patch has to be backed out. We can't carry this kind of regression on trunk for weeks.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Andreas Gal :gal from comment #3)
> Looks like we aren't getting any traction here. The offending patch has to
> be backed out. We can't carry this kind of regression on trunk for weeks.

what is the regression?

Comment 5

[John Daggett (:jtd)]

Attachment: some simple bolding tests with subpixel offsets

Comment 6

[John Daggett (:jtd)]

In reply to John Daggett (:jtd) from comment #5)
> Created attachment 553332 [details]
> some simple bolding tests with subpixel offsets

Thomas, could you attach a screenshot of the testcase above?  If possible, a PNG would be best to avoid JPEG color distortions at the pixel level.

Comment 7

[Andreas Gal :gal]

Attachment: Samsung Galaxy S 2 (renders properly)

Comment 8

[Andreas Gal :gal]

Attachment: Galaxy Tab 10" (renders crappy)

Comment 9

[Andreas Gal :gal]

Attachment: real-world example of bad bold fonts (Galaxy Tab 10")

Comment 10

[Andreas Gal :gal]

jtd, does this help? I have a device I can reproduce this with. Also, I am more than happy to mail you a device if that helps.

Comment 11

[Andreas Gal :gal]

ping

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You'll be wanting John or Paul I guess.

Comment 13

[Paul Adenot (:padenot)]

I tested all the Samsung devices I found in the office, and I can't reproduce the bug (Samsung Galaxy S2, Samsung Galaxy). I may be able to work on this, but I'm leaving the office today (my internship ends), and I don't personally have any device that can run fennec.

Comment 14

[Andreas Gal :gal]

Paul please file an IT bug with your address and cc me. We will send you a device.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

It looks like this could be related to glyph offset rounding. i.e. perhaps sometimes we round to far. It also looks like the offset is sometimes more than one pixel.

Comment 16

[Andreas Gal :gal]

#15 is definitely correct. I have seen half-glyph-size offsets in some cases. I will try to get more screen shots.

Comment 17

[John Daggett (:jtd)]

(In reply to Andreas Gal :gal from comment #9)
> Created attachment 553495 [details]
> real-world example of bad bold fonts (Galaxy Tab 10")

This definitely shows that there some sort of zoom factor involved in cairo code below gfxFont::Draw.  Synthetic bolding is done by drawing the same text twice with a 1px offset, effectively "smearing" the text.  But the text here is clearly being offset more than one pixel, which is why you see the ugly ghosting effect.  You can see this clearly in the Galaxy Tab 10" example, the serif "z" is drawn twice with a 4px offset and the "real-world example" is drawn with a 2px offset (look at the capital G in "Government Bond").

I don't know the low-level android cairo code but I imagine there's some sort low-level zoom factor.  The solution is instead of offsetting the second draw by 1px, offset it by 1/zoom instead (which will end up being 1px at the device pixel level).

Jeff, any ideas about this?

Comment 18

[John Daggett (:jtd)]

Based on a discussion with Bas, I fiddled around canvas text API's and I can now reproduce on the underlying problem using transforms applied to a canvas:

http://people.mozilla.org/~jdaggett/tests/synthetic-bold-transform.html

This shows the same problem as on the Galaxy Tab devices, the underlying transform applied to the gfxContext has a scale applied, so that the 1px shift turns into a multi-pixel shift in device pixels.  We need to invert the transform applied to the current context to determine what the appropriate shift in the inline direction should be when rendering the double-stamped text.

Comment 19

[John Daggett (:jtd)]

The testcase in the comment above applies on OSX but not Windows GDI, where the OS handles fake bolding for us.

Comment 20

[John Daggett (:jtd)]

One other thing to note is that for the case of canvas text using synthetic bolding with a transform applied, this bug has existed going back to Firefox 3 under OSX.  Windows/Linux is unaffected.

Comment 21

[Andreas Gal :gal]

Jeff?

Comment 22

[Jeff Muizelaar [:jrmuizel]]

(In reply to Andreas Gal :gal from comment #21)
> Jeff?

I have no idea what happens differently in Fennec.

Comment 23

[John Daggett (:jtd)]

Jeff suggests using gfxContext::DeviceToUser(const gfxSize& size) const with size(x:1, y:0) to determine the offset.  But I'm thinking the offset used needs to be:

1. offdev = gfxContext::UserToDevice({x:1, y:0})
2. offset = 1 / magnitude(offdev)

This is because we need to know the length of 1 device px in the user-space x-direction.

But a key issue here is whether the transform could change between the time glyph advances are laid out and calculated and the time they are drawn.  For example, if glyph advances are laid out and *cached*, followed by a scale tranform, the cached advances for the fake bold case will be wrong.  Maybe I worry too much...

Comment 24

[Andreas Gal :gal]

John, I think its worth a try. If the fix isn't right, we can still improve upon it. It will definitely improve things in most cases. Can you whip up a patch?

Comment 25

[John Daggett (:jtd)]

Yes, I'm going to test a patch using the testcase in comment 18, then ask Jeff to test on android with a galaxy whatever device.

Comment 26

[Andreas Gal :gal]

If you fix the mac bug and push to try, I can try on a device.

Comment 27

[John Daggett (:jtd)]

Attachment: patch, adjust fake bold offset based on context transform

Comment 28

[John Daggett (:jtd)]

Attachment: testcase with and without patch

Using testcase from comment 18.

Andreas, I need to run out for a bit now, I'll do a tryserver build tonight.

Comment 29

[Andreas Gal :gal]

Awesome. I will pick up the tryserver build and test it.

Comment 30

[Andreas Gal :gal]

the patch contains a bunch of trailing white space

Comment 31

[Andreas Gal :gal]

http://hg.mozilla.org/try/rev/60beb9d68739

Comment 32

[Andreas Gal :gal]

The try build with the patch applied fixes the font issue on my Galaxy Tab 10'. Great work John.

Comment 33

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 556752 [details] [diff] [review]
patch, adjust fake bold offset based on context transform

Review of attachment 556752 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +1124,5 @@
> +    t = aContext->UserToDevice(xdir);
> +    offset = 1.0 / sqrt(t.width * t.width + t.height * t.height);
> +    return offset;
> +}
> +

Please add some comments explaining the computation going on here. Also is this performance critical enough to justify special casing the identity transform? Other than that this seems fine.

Comment 34

[John Daggett (:jtd)]

Attachment: patch, adjust fake bold offset based on context transform

revised based on review comments but no major change in logic.

Comment 35

[John Daggett (:jtd)]

Attachment: patch, reftest

Comment 36

[John Daggett (:jtd)]

Comment on attachment 557092 [details] [diff] [review]
patch, adjust fake bold offset based on context transform

carry forward the r+ on the original patch, comment 33

Comment 37

[Andreas Gal :gal]

John, thanks again for the great work here.

On a related note, at large zoom factors its hard to tell that fonts are bold at all because 1 device pixel doesn't make a visual difference. We can't make the offset larger without risking messing up rendering again, so we would have to multi-strike instead of double strike based on total font width in device pixels (e.g. multi-strike until its 5% of font-width wider or something). Do you think thats worth a follow-up? Would multi-striking slow down font rendering a lot? Or is it ok since we cache glyphs. If you want to file this please do so. If its not worth it, this fix is already a huge improvement for synthetic bolding.

Comment 38

[Mark Finkle (:mfinkle) (use needinfo?)]

Did the patch land yet?

Comment 39

[Jonathan Kew [:jfkthame]]

(In reply to Mark Finkle (:mfinkle) from comment #38)
> Did the patch land yet?

Doesn't look like it - I think this was resolved in anticipation! Re-opening.

Comment 40

[Jonathan Kew [:jfkthame]]

(In reply to Andreas Gal :gal from comment #37)
> On a related note, at large zoom factors its hard to tell that fonts are
> bold at all because 1 device pixel doesn't make a visual difference.

This is a general problem with synthetic-bolding of large fonts - the same applies to big text on desktop Firefox OS X when using a font that doesn't have a real bold face. Compare:

data:text/html,<div style="font-family:Geneva;font-size:12px">This is a test for <b>synthetic bold</b>
data:text/html,<div style="font-family:Geneva;font-size:72px">This is a test for <b>synthetic bold</b>

At 12px, the boldness is very clear; at 72px, it's almost imperceptible.

Comment 41

[Jonathan Kew [:jfkthame]]

Isn't the patch here problematic for fennec because it presumably causes layout to become dependent on the zoom level? I thought fennec relied on being able to zoom without altering layout - which is why it can't use resolution-specific hinting/gridfitting.

Comment 42

[Andreas Gal :gal]

As I understand the way our synthetic bolding works it doesn't actually influence font measurement. It just double strikes and uses the regular font metrics. So layout shouldn't be affected.

mfinkle, I liked a try server build above if you want to try it. I didn't see any font artifacts zooming around in content with synthetic bolding applied.

Comment 43

[Jonathan Kew [:jfkthame]]

(In reply to Andreas Gal :gal from comment #42)
> As I understand the way our synthetic bolding works it doesn't actually
> influence font measurement. It just double strikes and uses the regular font
> metrics. So layout shouldn't be affected.

On desktop OS X, at least, synthetic bold *does* affect measurement. Simple demonstration:

data:text/html,<div style="font-family:Geneva;font-size:12px">This is a test for <b>synthetic bold</b><br><b>This is a test for</b> synthetic bold

From the code in gfxFT2Fonts.cpp, however, it looks as though synthetic bold is not actually handled correctly, with the (unexpected) result that metrics don't get adjusted. This is because bug 634997 inserted the call to AdjustAdvancesForSyntheticBold in the wrong place:

http://hg.mozilla.org/mozilla-central/annotate/922f27baed98/gfx/thebes/gfxFT2Fonts.cpp#l700

This should have been placed at the end of the function, right before the return statement. The result of this is that synthetic-bold text at small sizes will be difficult to read because the glyphs will crowd together too much.

Comment 44

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Jonathan Kew from comment #41)
> Isn't the patch here problematic for fennec because it presumably causes
> layout to become dependent on the zoom level? I thought fennec relied on
> being able to zoom without altering layout - which is why it can't use
> resolution-specific hinting/gridfitting.

Fennec does like to zoom without affecting layout, but that is becoming harder to do with the affect on readability.

I think we'd be willing to fix this bolding issue now, and then file followup bugs on any layout issues. Improving the readability, as this patch does, is a high priority for mobile.

Comment 45

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Jonathan Kew from comment #43)

> From the code in gfxFT2Fonts.cpp, however, it looks as though synthetic bold
> is not actually handled correctly, with the (unexpected) result that metrics
> don't get adjusted. This is because bug 634997 inserted the call to
> AdjustAdvancesForSyntheticBold in the wrong place:
> 
> http://hg.mozilla.org/mozilla-central/annotate/922f27baed98/gfx/thebes/
> gfxFT2Fonts.cpp#l700
> 
> This should have been placed at the end of the function, right before the
> return statement. The result of this is that synthetic-bold text at small
> sizes will be difficult to read because the glyphs will crowd together too
> much.

Worth a followup bug? Bug 627842 is focused on making small text readable by adjust the size, and yes, altering the layout a bit.

Comment 46

[Andreas Gal :gal]

Interesting. In related news, I would really see resolution-specific hinting/gridfitting used on Android if we can pull it off to improve readability.

Comment 47

[Jonathan Kew [:jfkthame]]

(In reply to Mark Finkle (:mfinkle) from comment #45)
> Worth a followup bug?

Yes - I just wanted to do a few tests first. Now filed as bug 683618.

Comment 48

[John Daggett (:jtd)]

Comment on attachment 557093 [details] [diff] [review]
patch, reftest

Reftest fails on XP and Fedora, need to rework it.

Comment 49

[Jonathan Kew [:jfkthame]]

I think what we really ought to be doing here is something a bit different from the current patch. Rather than double-striking with an offset of one device pixel (which has at least two shortcomings - imperceptible "boldness" at large sizes, and will cause zoom-dependent layout changes once bug 683618 lands), we should be increasing the glyph advances by a fixed proportion (maybe around 1/16) of the em-size, and "multi-striking" (at one-device-pixel offsets) as many times as we can fit within that added advance.

Comment 50

[John Daggett (:jtd)]

Jonathan, why don't you take a stab at creating a patch for that.

Comment 51

[Jonathan Kew [:jfkthame]]

Attachment: patch, make synthetic bolding proportionate to font size and zoom

With this patch, synthetic bold on desktop (OS X) works much better at large font sizes or zoom factors. Simple test: visit http://people.mozilla.com/~jkew/synbold.html and zoom in/out to see extremes.

Waiting for an Android build, to check behavior there as well.

Comment 52

[Jonathan Kew [:jfkthame]]

Comment on attachment 557464 [details] [diff] [review]
patch, make synthetic bolding proportionate to font size and zoom

Hmm, this didn't quite work out on Android - it doesn't seem to come up with the proper bold-offset. Trying to get enough of a debugging environment set up to figure out what's going on..... cancelling r? for now until I get this debugged - or someone points out the problem for me. :)

Comment 53

[Jonathan Kew [:jfkthame]]

Attachment: patch, make synthetic bolding proportionate to font size and zoom

OK, this version seems to work on Android as well. Pushing to tryserver, so there should be a build available for testing tomorrow.

Comment 54

[Jonathan Kew [:jfkthame]]

Tryserver builds:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ef8afd5c7766/

Andreas, could you please check how this behaves on your Galaxy device?

Comment 55

[John Daggett (:jtd)]

Comment on attachment 557635 [details] [diff] [review]
patch, make synthetic bolding proportionate to font size and zoom

Looks good!

Comment 56

[John Daggett (:jtd)]

Comment on attachment 557093 [details] [diff] [review]
patch, reftest

I was trying to set up a reftest by trying to make use of the extra offset that's used when a scale factor is applied but this turns out to be rather flakey across platforms (e.g. Fedora, winxp).  Obsoleting for now, will try to think up something better next week.

Comment 57

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 557635 [details] [diff] [review]
patch, make synthetic bolding proportionate to font size and zoom

>--- a/gfx/thebes/gfxFont.cpp
>+++ b/gfx/thebes/gfxFont.cpp
>+static double
>+CalcXScale(gfxContext *aContext)
>+{
>+    double offset = 1.0, m;
>+    gfxSize t, xdir(1.0, 0.0);

Please declare closer to first use. Also, I don't think you need offset and xdir.

>+
>+    // determine magnitude of a 1px x offset in device space
>+    t = aContext->UserToDevice(xdir);
>+    if (t.width == 1.0 && t.height == 0.0) {
>+        // short-circuit the most common case to avoid sqrt() and division
>+        return 1.0;
>+    }
>+
>+    m = sqrt(t.width * t.width + t.height * t.height);
>+
>+    NS_ASSERTION(m != 0.0, "degenerate transform while synthetic bolding");
>+    if (m == 0.0) {
>+        return 0.0; // effectively disables offset
>+    }
>+
>+    // scale factor so that offsets are 1px in device pixels
>+    offset = 1.0 / m;
>+    return offset;
>+}

Comment 58

[Andreas Gal :gal]

I tested the build on a Galaxy S Tab 10" and it looks good at various zoom factors.

Comment 59

[Jonathan Kew [:jfkthame]]

Pushed to inbound, with nits tidied up as per comment 57:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5b34c14d9f84

Comment 60

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/5b34c14d9f84

Comment 61

[Andreea Pod]

Verified fixed on build: Mozilla /5.0 (Android;Linux armv7l;rv:9.0a2) Gecko/20111018 Firefox/9.0a2 Fennec/9.0a2
Device: LG Optimus 2X