Closed Bug 1274111 Opened 4 years ago Closed 4 years ago

Rendering issues with rotate() using skia

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: truber, Assigned: lsalzman)

References

(Blocks 1 open bug)

Details

(Keywords: correctness, testcase, Whiteboard: [gfx-noted])

Attachments

(4 files)

Attached image cairovskia.png
Characters are laid out incorrectly when gfx.content.azure.backends is set to skia and text is rotated. Testcase attached showing the issue.

Found on Linux but not sure if it is specific to Linux or not.
Attached file testcase.html
Attached file about:support
Lee, any ideas?
Flags: needinfo?(lsalzman)
Keywords: correctness
Whiteboard: [gfx-noted]
This mostly is selectively adapted code from SkFontHost_FreeType in such a way that it is roughly compatible with how Cairo is generating the glyph bitmaps.

The glyph width/height/left/top values are unfortunately not exported, via cairo text extents, in the way Skia needs them to agree with how glyph bitmaps are generated in SkFontHost_FreeType_common. Since there is no way to get the right values other than directly asking freetype for them, that is hence what this patch now makes the font host do.

This also disables hinting with rotation matrices like the SkFontHost_FreeType does, as it is necessary to prevent the hinting from going haywire.

All in all, this is a hack over a hack, and one day we really want to replace this with something that doesn't route through Cairo... But not in this bug. :)
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Flags: needinfo?(lsalzman)
Attachment #8758502 - Flags: review?(gwright)
Comment on attachment 8758502 [details] [diff] [review]
use freetype directly for calculating glyph sizes in SkFontHost_cairo

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

::: gfx/skia/skia/src/ports/SkFontHost_cairo.cpp
@@ +69,5 @@
>      FT_Face fFace;
>  };
>  
> +static bool bothZero(SkScalar a, SkScalar b) {
> +    return 0 == a && 0 == b;

SkScalar is either a float or a double, so this should probably be 0.0

@@ +308,5 @@
> +    CairoLockedFTFace faceLock(fScaledFont);
> +    FT_Face face = faceLock.getFace();
> +
> +    FT_Error err = FT_Load_Glyph( face, glyph->getGlyphID(), fLoadGlyphFlags );
> +    if (err != 0) {

This probably needs a call to glyph->zeroMetrics().

@@ +311,5 @@
> +    FT_Error err = FT_Load_Glyph( face, glyph->getGlyphID(), fLoadGlyphFlags );
> +    if (err != 0) {
> +        return;
> +    }
> +

Do we know how much of a perf hit this is going to be? If I recall correctly, one of the main reasons we decided to pull extents from cairo instead of FreeType was because we already needed to calculate the extents in cairo for layout, and so us pulling them from cairo would mean hitting the cache. My memory is a little hazy but it might be worth checking. Otherwise, this looks fine to me, on the assumption these calculations are correct.
Attachment #8758502 - Flags: review?(gwright) → review+
(In reply to George Wright (:gw280) (:gwright) from comment #5)
> Do we know how much of a perf hit this is going to be? If I recall
> correctly, one of the main reasons we decided to pull extents from cairo
> instead of FreeType was because we already needed to calculate the extents
> in cairo for layout, and so us pulling them from cairo would mean hitting
> the cache. My memory is a little hazy but it might be worth checking.
> Otherwise, this looks fine to me, on the assumption these calculations are
> correct.

It will cause one extra call to FT_Load_Glyph to fetch the metrics unfortunately, but at least this will be cached in Skia's own glyph cache. This should also only just be for metrics, so we shouldn't be generating the glyph bitmap twice. Without invasive solutions to either Cairo or Skia, we can't do much better right now, though. We really just need to fix how metrics are done in thebes, but this will suffice so long as we're only really using this setup for canvas at the moment.
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0eda9a4bb6
use freetype directly for calculating glyph sizes in SkFontHost_cairo. r=gwright
https://hg.mozilla.org/mozilla-central/rev/2a0eda9a4bb6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Duplicate of this bug: 1293661
You need to log in before you can comment on or make changes to this bug.