624310 - harfbuzz shaper doesn't compensate for dwrite synthetic bolding

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[John Daggett (:jtd)]

Attachment: screenshot showing harfbuzz spacing vs. dwrite shaper spacing

The harfbuzz shaping code uses the same horizontal advances for both a regular face and a synthetic bold face.  The dwrite shaping API's compensate by adding small amount to the advance for each glyph.

Steps:

1. Open URL

Result: harfbuzz shaping places regular and synthetic bold glyphs in exactly the same positions.

Compare with the dwrite shaping case:

1. In about:config, set 'gfx.font_rendering.harfbuzz.level' to 0
2. Open URL

Note how the dwrite shaping code compensates for the synthetic bolding by adding to the advance metrics.

Fix is probably to add some generic synthetic bold advance relative to the font size, then set it in the dwrite font code in accordance with roughly what the dwrite shaping code is doing.

Comment 1

[Jonathan Kew [:jfkthame]]

A better fix might be to get glyph advances from DW when synthetic bold is in use, rather than from the font tables. We could do that similarly to how we get metrics when ClearType is turned off.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: patch v1 - get widths via directwrite rather than font tables when using simulated-bold

When we're using a font with DW synthetic bold, we need to call DW to get the glyph widths rather than using direct access to the metrics table, so that the widths we use will allow for the emboldening.

I've renamed the gfxFont::ProvidesHintedWidths method to ProvidesGlyphWidths here, as this is no longer used just for hinting, but for any situation where the platform font subclass wants to provide a custom glyph-widths method rather than relying on harfbuzz reading the font tables.

Comment 3

[Bas Schouten (:bas.schouten)]

Comment on attachment 504670 [details] [diff] [review]
patch v1 - get widths via directwrite rather than font tables when using simulated-bold

There's tabs all over this patch, that should be fixed :).

>diff --git a/gfx/thebes/gfxDWriteFonts.cpp b/gfx/thebes/gfxDWriteFonts.cpp
>--- a/gfx/thebes/gfxDWriteFonts.cpp
>+++ b/gfx/thebes/gfxDWriteFonts.cpp
>-gfxDWriteFont::GetHintedGlyphWidth(gfxContext *aCtx, PRUint16 aGID)
>+gfxDWriteFont::GetGlyphWidth(gfxContext *aCtx, PRUint16 aGID)
>+        if (NS_SUCCEEDED(hr)) {

Please use SUCCEEDED and FAILED macros on HRESULT :) I'm not expecting us or microsoft to change our < 0 == failure policy but it's the 'right' thing to do.

>-    return -1;
>+    mGlyphWidths.Put(aGID, width);
>+    return width;

Do you think we should still handle the FAILED case here?

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to comment #3)
> There's tabs all over this patch, that should be fixed :).

Oops. Fixed in my queue.

> Please use SUCCEEDED and FAILED macros on HRESULT :)

Ditto.

> >-    return -1;
> >+    mGlyphWidths.Put(aGID, width);
> >+    return width;
> 
> Do you think we should still handle the FAILED case here?

It's handled (in the sense of maintaining the existing behavior) by initializing the width variable to -1. If the metrics call fails for some reason, this is returned unchanged.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: patch, v1.1 - updated as per review comments

(Carrying forward r=bas.)

Requesting approval-2.0; this is basically a "missing piece" of the DirectWrite/harfbuzz integration, and manifests as a regression in the rendering of simulated-bold fonts.

Comment 6

[Jonathan Kew [:jfkthame]]

BTW, we already have test coverage for this, but the test (synthetic-bold-metrics-01.html) is marked random on windows (see bug 597300). The patch here removes that annotation, as the test should now pass.

Comment 7

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/mozilla-central/rev/02b92b61f5cd