634997 - no synthetic bolding on Android

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Thomas Arend [:tarend]]

See attached screenshot

(1) go to m.spiegel.de (and other sites like GNews, etc.) on a Samsung Galaxy Tab and compare it to other mobile devices and our desktop build

Result:
Font rendering is different, not as smooth, bold face is missing 
Expected: rendering should be the same

Also: comparing Fennec with stock browser shows that our image (and font) rendering is sub-par: we show far more artifacts.

Comment 1

[Thomas Arend [:tarend]]

Attachment: Font rendering on Galaxy Tab vs. Nexus 1

Comment 2

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

other than the lack of bold, the rendering differences aren't immediately obvious to me with these screenshots. Perhaps some better shots would be useful?

http://blog.lassey.us/2010/09/13/android-screen-shots-without-rooting-your-phone/

Comment 3

[Thomas Arend [:tarend]]

Font type and kerning seem slightly different, the visual appearance of the pages are definitely different. I will *try* to capture better screenshots. Or you can try it on any Galaxy Tab.

Comment 4

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

If different fonts are being used, than some differences are to be expected. Let's see if we can get the fonts list. We might send that to logcat.

Comment 5

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

(In reply to comment #3)
> Font type and kerning seem slightly different, the visual appearance of the
> pages are definitely different. I will *try* to capture better screenshots. Or
> you can try it on any Galaxy Tab.

I don't have a Galaxy Tab, nor do any of the "font people" at mozilla as far as I know. If you follow the instructions I linked to, you'll get more useful screen shots.

Also, as Mark said, if the two devices has a different set of fonts, this is to be expected. Also, differences between the fonts the stock browser uses and the fonts we use may be explained by bug 633109, but that's pure conjecture.

Comment 6

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

This may be relevant: In the about config, doing a search under font.name.* , I found that it lists Droid Sans for some languages on my droid 2.  Perhaps we can confirm that there are different fonts being used on the two devices by knowing what font names are being used in the fennec preferences?

Comment 7

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

On start up we echo what font files we load to the logcat, as Mark said if you can post that, we'll know what fonts are being loaded

Comment 8

[Matt Brubeck (:mbrubeck)]

Here's is the log from my Verizon Galaxy Tab (Android 2.2, not rooted or modified):

I/Gecko   ( 3449): Font: /system/fonts/DroidSerif-Bold.ttf
I/Gecko   ( 3449): font family: Droid Serif
I/Gecko   ( 3449): Font: /system/fonts/DroidSansFallback.ttf
I/Gecko   ( 3449): font family: Droid Sans Fallback
I/Gecko   ( 3449): Font: /system/fonts/Clockopia.ttf
I/Gecko   ( 3449): font family: Clockopia
I/Gecko   ( 3449): Font: /system/fonts/DroidSans-Bold.ttf
I/Gecko   ( 3449): font family: Droid Sans
I/Gecko   ( 3449): Font: /system/fonts/Georgia.ttf
I/Gecko   ( 3449): font family: Georgia
I/Gecko   ( 3449): Font: /system/fonts/DroidSansMono.ttf
I/Gecko   ( 3449): font family: Droid Sans Mono
I/Gecko   ( 3449): Font: /system/fonts/DroidSans_Subset.ttf
I/Gecko   ( 3449): font family: Droid Sans
I/Gecko   ( 3449): Font: /system/fonts/DroidSansHebrew.ttf
I/Gecko   ( 3449): font family: Droid Sans Hebrew
I/Gecko   ( 3449): Font: /system/fonts/DroidSansArabic.ttf
I/Gecko   ( 3449): font family: Droid Sans Arabic
I/Gecko   ( 3449): Font: /system/fonts/Verdana.ttf
I/Gecko   ( 3449): font family: Verdana
I/Gecko   ( 3449): Font: /system/fonts/Comic.ttf
I/Gecko   ( 3449): font family: Comic Sans MS
I/Gecko   ( 3449): Font: /system/fonts/DroidSerif-Italic.ttf
I/Gecko   ( 3449): font family: Droid Serif
I/Gecko   ( 3449): Font: /system/fonts/DroidSerif-Regular.ttf
I/Gecko   ( 3449): font family: Droid Serif
I/Gecko   ( 3449): Font: /system/fonts/DroidSansThai.ttf
I/Gecko   ( 3449): font family: Droid Sans Thai
I/Gecko   ( 3449): Font: /system/fonts/DroidSans.ttf
I/Gecko   ( 3449): font family: Droid Sans
I/Gecko   ( 3449): Font: /system/fonts/DroidSerif-BoldItalic.ttf
I/Gecko   ( 3449): font family: Droid Serif
I/Gecko   ( 3449): Font: /system/fonts/DroidSansMono_EBook.ttf
I/Gecko   ( 3449): font family: Droid Sans Mono

Comment 9

[Aaron Train [:aaronmt]]

Nexus One (Android 2.3.3) , raw output from logcat

I/GeckoFonts( 8254): got: /system/fonts/DroidSansMono.ttf;droid sans mono,;1266978337;117072;0,;/system/fonts/DroidSerif-Regular.ttf;droid serif,;1266978337;172532;0,;/system/fonts/DroidSerif-Bold.ttf;droid serif,;1266978337;184836;0,;/system/fonts/DroidSansHebrew.ttf;droid sans hebrew,;1217592000;23076;0,;/system/fonts/DroidSerif-BoldItalic.ttf;droid serif,;1266978337;189916;0,;/system/fonts/DroidSansThai.ttf;droid sans thai,;1217592000;36028;0,;/system/fonts/DroidSansArabic.ttf;droid sans arabic,;1217592000;35908;0,;/system/fonts/DroidSerif-Italic.ttf;droid serif,;1266978337;177176;0,;/system/fonts/DroidSans.ttf;droid sans,;1266978337;190044;0,;/system/fonts/DroidSans-Bold.ttf;droid sans,;1266978337;191032;0,;/system/fonts/Clockopia.ttf;clockopia,;1266978337;6880;0,;/system/fonts/DroidSansFallback.ttf;droid sans fallback,;1279142112;3640264;0,; from the cache

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: test case

Comment 11

[Matt Brubeck (:mbrubeck)]

It looks like the difference is that the Galaxy Tab includes a version of the Verdana font family, but Fennec fails to render Verdana in bold or italic styles.

On the Nexus One, there are no Verdana fonts, so Fennec correctly uses a fallback font instead.

Opera Mobile appears to render Verdana correctly (including bold and italic), while the stock browser on the Galaxy Tab appears to fall back and use Droid Sans instead of Verdana.

Comment 12

[Matt Brubeck (:mbrubeck)]

It seems the problem is that the Galaxy Tab ships Verdana, but no Verdana Bold.  (The Nexus One and most other Android phones do not ship Verdana at all.)

Can we detect this situation, and refuse to use the font if there is no bold variant?  If not, should we just blacklist Verdana on Android?  (The default Droid Sans which ships on all Android devices is a reasonable substitute for Verdana.)

Comment 13

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

as far as I know we should double strike the normal font for bold

Comment 14

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

jdaggett, any idea why we're not double striking these fonts?

Comment 15

[Daniel Veditz [:dveditz]]

Not blocking Macaw

Comment 16

[Doug Turner (:dougt)]

jdaggett?

Comment 17

[John Daggett (:jtd)]

(In reply to comment #14)
> jdaggett, any idea why we're not double striking these fonts?

Argh, more Samsung font wackiness.  What's in the water they're drinking? ;)

The double-striking behavior for bolding isn't automatic, you need to explicitly enable it in the gfxFont subclass.  We only do this on OSX currently, with GDI/DirectWrite it's done natively.  So the FT2 font subclass of gfxFont needs to mimic the gfxMacFont code:

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxMacFont.cpp#61

Looks like you need to pass in the 'aNeedsBold' boolean in the constructor for that to work.  We should also verify that synthetic italics is also working (again, use the Mac font code as a guide).

As Matt notes in comment 11, this problem is not specific to either Verdana or the Tab per se, it's simply that we're seeing this problem because Samsung (unwisely) chose to ship font families that lack bold/italic faces (Georgia will probably have the same problem).

Comment 18

[John Daggett (:jtd)]

One thing I don't understand is why the reftest below passes on Android (reftests/font-face):

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

This test should catch the problem this bug details.

Comment 19

[Paul Adenot (:padenot)]

Attachment: jdaggett test case on Motorola XOOM

Here is a screenshot of jdaggett's test case, on a Motorola XOOM running Fennec Nightlies. It doesn't show any bold. If this is related, I guess I could work on this, having a device that reproduce the bug.

Comment 20

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

Paul - It would be great if you could start to work on this bug.

Comment 21

[Paul Adenot (:padenot)]

Attachment: Patch v0 -- Adds synthetic bolding support on Android

Here is a patch, which show a correct behavior with mbrubeck attached test case, as well as the test case provided by jdaggett in comment 18. I've tested m.spiegel.de too, and it seems to work.

I've used a Samsung Galaxy S (stock, not rooted nor modified) to test, and it was failings all the aforementioned test cases.

The patch basically ports the behavior of the Mac version, as mentioned in comment 17.

Comment 22

[John Daggett (:jtd)]

Comment on attachment 546543 [details] [diff] [review]
Patch v0 -- Adds synthetic bolding support on Android

Looks good, the only problem I see is that your patch has a bunch of tabs in it.  Please remove all tabs and verify the spacing before landing.

Comment 23

[Paul Adenot (:padenot)]

Attachment: Patch v1 -- Replaced tabs by spaces

Here is the patch with spacing fixed.

I took the liberty to remove an XXX comment saying that this bug should be fixed.

Comment 24

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

I tried to apply this patch and got:
applying attachment.cgi?id=548422
patching file gfx/thebes/gfxAndroidPlatform.cpp
Hunk #1 FAILED at 636
1 out of 1 hunks FAILED -- saving rejects to file gfx/thebes/gfxAndroidPlatform.cpp.rej
patching file gfx/thebes/gfxFT2Fonts.cpp
Hunk #1 FAILED at 105
Hunk #2 FAILED at 691
Hunk #3 FAILED at 797
Hunk #4 FAILED at 871
4 out of 4 hunks FAILED -- saving rejects to file gfx/thebes/gfxFT2Fonts.cpp.rej
patching file gfx/thebes/gfxFT2Fonts.h
Hunk #1 FAILED at 120
1 out of 1 hunks FAILED -- saving rejects to file gfx/thebes/gfxFT2Fonts.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh attachment.cgi?id=548422

Comment 25

[Paul Adenot (:padenot)]

Blassey, this patch applies fine on my clean, updated tree.

Comment 26

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/17835995c2d2

Comment 27

[Thomas Arend [:tarend]]

I see bold font now, but it doesn't render consistently - I filed new Bug 634997 for the issue.

Comment 28

[Andreas Gal :gal]

This patch broke a tier 1 platform. Why was it not backed out? Please do so.

Comment 29

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

(In reply to Andreas Gal :gal from comment #28)
> This patch broke a tier 1 platform. Why was it not backed out? Please do so.

what did it break?

Comment 30

[Andreas Gal :gal]

"I see bold font now, but it doesn't render consistently - I filed new Bug 634997 for the issue."

Comment 31

[Andreas Gal :gal]

Actually the wrong bug number is quoted there. Its 674909. Bold fonts look completely messed up on Samsung devices and I think italic fonts are broken too.

Comment 32

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

(In reply to Andreas Gal :gal from comment #30)
> "I see bold font now, but it doesn't render consistently - I filed new Bug
> 634997 for the issue."

that's not a regression. Before this landed there were no bold fonts for these devices. Now there are. You're seeing the limits on how good synthetic bold can be, or at least the limits of our implementation of it. If there are things we can do to improve that, we should look at them, but that's a different bug.

Comment 33

[Andreas Gal :gal]

We went from a visual glitch to a barely readable state. I strongly disagree with taking this patch as is, but I am not driving here so your call.

Comment 34

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

That being said, I wonder if the synthetic bolding is required anymore since bug 636042 landed. Was this just one bad font that was causing the issue on these devices?

Can someone with an effected device do a build with this backed out?

Comment 35

[Brendan Eich [:brendan]]

It doesn't matter if a change exposed latent bugs that then bit hard, or directly regressed something -- regression is as regression does. I'm with Andreas: if we are serious about tier 1 status, nightly usability bugs like this should lead to a back-out.

Backing out is done all the time on tier-1 plaform regression. Hg makes it easy enough. It's the right thing.

/be

Comment 36

[Andreas Gal :gal]

I will try to test #34 and report back.

Comment 37

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

Brendan, from what I've been told until today the current behavior of synthetically bolding the fonts is preferable to not bolding them at all. Andreas seems to disagree, we should figure out what we can do.

Comment 38

[John Daggett (:jtd)]

Due to the problems noted on bug 674909, I think we should back this one out and rework the patch to solve the underlying problem, namely that the current double-stamping approach to fake bolding doesn't correctly account for transforms applied to the current gfxContext.  This is a problem that exists anywhere we use the double-stamping logic (e.g. OSX) for synthetic bolding.  See bug 674909, comment 18 for more details.

The reason this only shows up on some devices is that fonts are usually packaged in regular-bold-italic-bold-italic combinations where synthetic bolding isn't required.  Samsung, for whatever reason, decided to ship just the regular face of Verdana with no bold or italic faces, that's why the problem shows up on these devices.

Bug 636042 is unrelated to this issue.

Comment 39

[Jonathan Kew [:jfkthame]]

Comment on attachment 548422 [details] [diff] [review]
Patch v1 -- Replaced tabs by spaces

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

::: gfx/thebes/gfxFT2Fonts.cpp
@@ +696,5 @@
>                                            aRunStart, aRunLength, aRunScript);
>      }
>  
>      if (!ok) {
> +        aTextRun->AdjustAdvancesForSyntheticBold(aRunStart, aRunLength);

This is the wrong place - we need to call AdjustAdvancesForSyntheticBold *after* the glyphs for the run have been set up with their normal metrics. And we need to apply it in the harfbuzz-shaped case (which is the usual codepath) as well. So it should go at the end of the function, right before return PR_TRUE.

Filed bug 683618 as a followup to fix this.

Comment 40

[Jonathan Kew [:jfkthame]]

Closing this, as we didn't end up backing it out, and the followup issues (bug 674909, bug 683618) have now been fixed as well.