harfbuzz shaper doesn't compensate for dwrite synthetic bolding

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jtd, Assigned: jfkthame)

Tracking

Trunk
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 1 obsolete attachment)

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.
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.
Blocks: 597300
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.
Assignee: nobody → jfkthame
Attachment #504670 - Flags: review?(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?
Attachment #504670 - Flags: review?(bas.schouten) → review+
(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.
(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.
Attachment #504670 - Attachment is obsolete: true
Attachment #504854 - Flags: review+
Attachment #504854 - Flags: approval2.0?
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.
Attachment #504854 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/02b92b61f5cd
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.