The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 9

Status

()

Core
Graphics
P2
major
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: tarend, Assigned: jfkthame)

Tracking

(Depends on: 1 bug)

Trunk
mozilla9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox9 fixed, fennec9+)

Details

Attachments

(8 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 549116 [details]
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.
moving to graphics since it won't get seen by the appropriate people here
Component: General → Graphics
Product: Fennec → Core
QA Contact: general → thebes
Duplicate of this bug: 678874

Comment 3

6 years ago
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.
(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

6 years ago
Created attachment 553332 [details]
some simple bolding tests with subpixel offsets

Comment 6

6 years ago
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

6 years ago
Created attachment 553493 [details]
Samsung Galaxy S 2 (renders properly)

Comment 8

6 years ago
Created attachment 553494 [details]
Galaxy Tab 10" (renders crappy)

Comment 9

6 years ago
Created attachment 553495 [details]
real-world example of bad bold fonts (Galaxy Tab 10")

Comment 10

6 years ago
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

6 years ago
ping
You'll be wanting John or Paul I guess.
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

6 years ago
Paul please file an IT bug with your address and cc me. We will send you a device.
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

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

Comment 17

6 years ago
(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

6 years ago
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

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

Comment 20

6 years ago
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

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

I have no idea what happens differently in Fennec.

Comment 23

6 years ago
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

6 years ago
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

6 years ago
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

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

Comment 27

6 years ago
Created attachment 556752 [details] [diff] [review]
patch, adjust fake bold offset based on context transform

Comment 28

6 years ago
Created attachment 556754 [details]
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

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

Comment 30

6 years ago
the patch contains a bunch of trailing white space

Comment 31

6 years ago
http://hg.mozilla.org/try/rev/60beb9d68739

Comment 32

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

Updated

6 years ago
Attachment #556752 - Flags: review?(jmuizelaar)

Updated

6 years ago
tracking-fennec: --- → ?

Updated

6 years ago
Summary: Bolding looks irregular (on Galaxy Tab, Galaxy S, etc.) → Synthetic bolding uses too large pixel offset when zoom is applied to the cairo surface

Updated

6 years ago
OS: Android → All
Hardware: ARM → All
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.
Attachment #556752 - Flags: review?(jmuizelaar) → review+

Comment 34

6 years ago
Created attachment 557092 [details] [diff] [review]
patch, adjust fake bold offset based on context transform

revised based on review comments but no major change in logic.
Attachment #556752 - Attachment is obsolete: true
Attachment #557092 - Flags: review?(jmuizelaar)

Comment 35

6 years ago
Created attachment 557093 [details] [diff] [review]
patch, reftest
Attachment #557093 - Flags: review?(jmuizelaar)

Comment 36

6 years ago
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
Attachment #557092 - Flags: review?(jmuizelaar) → review+

Comment 37

6 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Did the patch land yet?
(Assignee)

Comment 39

6 years ago
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 40

6 years ago
(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.
(Assignee)

Comment 41

6 years ago
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

6 years ago
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.
(Assignee)

Comment 43

6 years ago
(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.
(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.
(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

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

Comment 47

6 years ago
(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

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

Reftest fails on XP and Fedora, need to rework it.
Attachment #557093 - Flags: review?(jmuizelaar)
(Assignee)

Comment 49

6 years ago
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

6 years ago
Jonathan, why don't you take a stab at creating a patch for that.
(Assignee)

Comment 51

6 years ago
Created attachment 557464 [details] [diff] [review]
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.
Assignee: nobody → jfkthame
Status: REOPENED → ASSIGNED
Attachment #557464 - Flags: review?(jdaggett)
(Assignee)

Updated

6 years ago
Depends on: 683618
tracking-fennec: ? → 9+
(Assignee)

Comment 52

6 years ago
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. :)
Attachment #557464 - Flags: review?(jdaggett)
(Assignee)

Comment 53

6 years ago
Created attachment 557635 [details] [diff] [review]
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.
Attachment #557464 - Attachment is obsolete: true
Attachment #557635 - Flags: review?(jdaggett)
(Assignee)

Comment 54

6 years ago
Tryserver builds:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jkew@mozilla.com-ef8afd5c7766/

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

Comment 55

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

Looks good!
Attachment #557635 - Flags: review?(jdaggett) → review+

Comment 56

6 years ago
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.
Attachment #557093 - Attachment is obsolete: true
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

6 years ago
I tested the build on a Galaxy S Tab 10" and it looks good at various zoom factors.
(Assignee)

Comment 59

6 years ago
Pushed to inbound, with nits tidied up as per comment 57:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5b34c14d9f84
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/5b34c14d9f84
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
(Assignee)

Updated

6 years ago
Depends on: 686190
status-firefox9: --- → fixed

Comment 61

6 years ago
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
Status: RESOLVED → VERIFIED

Updated

5 years ago
Blocks: 728436

Updated

3 years ago
Depends on: 1036650
You need to log in before you can comment on or make changes to this bug.