944646 - Crash in reftest layout/reftests/svg/text/simple-fill-gradient.svg with skia backend

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Kevin Simons [:kevsim] (Telenor)]

Opening layout/reftests/svg/text/simple-fill-gradient.svg causes a reproducible crash when the skia backend is in use.

Comment 1

[Kevin Simons [:kevsim] (Telenor)]

Caused by GlyphBufferAzure::Flush(), which ultimately calls:

    aFont->CopyGlyphsToBuilder(buf, aThebesContext->mPathBuilder, &mat);

And CopyGlyphsToBuilder() assumes the the PathBuilder passed in is a PathBuilderCairo, which it is in not in the case where the draw target is a DrawTargetSkia.

Comment 2

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Handle non-Cairo PathBuilder in ScaledFontBase::CopyGlyphsToBuilder

Comment 3

[Kevin Simons [:kevsim] (Telenor)]

Since there is not a transform being passed to PathCairo::AppendPathToBuilder, I believe (from my quick read of that method) that this is semantically equivalent. However, it does seem to be slightly less efficient for PathBuilderCairo instances since calling StreamToSink for all of the path components is less efficient than just adding all of the points. If you'd prefer I keep the normal path for Cairo, but have an alternate path for Skia, that's no problem.

I checked and the SVG mentioned in the bug summary does now render correctly in Skia (and still renders correctly in Cairo).

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8341927 [details] [diff] [review]
Handle non-Cairo PathBuilder in ScaledFontBase::CopyGlyphsToBuilder

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

I think we want to do this using the Skia API, instead of using cairo and then streaming into skia.

For standalone builds (and maybe gecko builds at some point in the future) USE_CAIRO might not be defined, and we still want this code to work.

Comment 5

[Kevin Simons [:kevsim] (Telenor)]

Sounds very reasonable. I'll attempt to code that up tomorrow then. Thanks for the quick feedback!

Comment 6

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Part 1: Check the DrawTarget type before downcasting PathBuilders

Previously the ScaledFontBase::CopyGlyphsToBuilder methods in the
various ScaledFont subclasses static_cast-ed the passed in PathBuilder
objects without checking which sort of DrawSurface they were created by.

Comment 7

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Part 2: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder

Handle PathBuilderSkia with a separate path specifically for
BACKEND_SKIA DrawTarget instances.

Comment 8

[Kevin Simons [:kevsim] (Telenor)]

Attempted to make things work with Skia. Rendering seems to be correct based on the few SVGs I tried.

Couple of things:
1) It was a little unclear where to put the code since the heirarchy of ScaledFont is a bit confusing. ScaledFontBase does Cairo stuff, but so does ScaledFontCairo. I decided to mimic the structure of ScaledFontBase::GetPathForGlyphs. Feel free to shout if this is wrong
2) This was my first attempt at real Skia stuff, so I guess I probably messed some stuff up. I implemented things by adding each glyph one at a time to the path builder. Skia has means to do a whole strings of glyphs at a time, but I was worried that wouldn't respect the x/y position specified in the GlyphBuffer.
3) I modeled the PathSkia::AppendPathToBuilder after the similar function in PathCairo. However, to be honest, I'm not sure why this couldn't just be a method in PathBuilderSkia which takes an SkPath and appends it (rather than going through PathSkia).

Looking forward to feedback.

Comment 9

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Part 2: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder

Handle PathBuilderSkia with a separate path specifically for
BACKEND_SKIA DrawTarget instances.

Comment 10

[Kevin Simons [:kevsim] (Telenor)]

Realized the transform bit doesn't work. Revisiting.

Comment 11

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Part 2: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder

Handle PathBuilderSkia with a separate path specifically for
BACKEND_SKIA DrawTarget instances.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8342493 [details] [diff] [review]
Part 1: Check the DrawTarget type before downcasting PathBuilders

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

::: gfx/2d/ScaledFontBase.cpp
@@ +117,3 @@
>  {
>  #ifdef USE_CAIRO
> +  if (aBackendType == BACKEND_CAIRO) {

Check if we actually have mScaledFont too, or assert that we do.

@@ +145,1 @@
>  #endif

Assert that we don't get to the end of this function without finding an appropriate path type. We don't want it to silently not draw anything.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8342625 [details] [diff] [review]
Part 2: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder

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

::: gfx/2d/ScaledFontBase.cpp
@@ +120,5 @@
> +  if (aBackendType == BACKEND_SKIA) {
> +    PathBuilderSkia *builder = static_cast<PathBuilderSkia *>(aBuilder);
> +
> +    SkPaint paint;
> +    paint.setTypeface(GetSkTypeface());

Assert that GetSkTypeface() returns non-null.

@@ +128,5 @@
> +    for (uint32_t i = 0; i < aBuffer.mNumGlyphs; ++i) {
> +      SkPath path;
> +      paint.getTextPath(&aBuffer.mGlyphs[i].mIndex, 1,
> +                        aBuffer.mGlyphs[i].mPosition.x, aBuffer.mGlyphs[i].mPosition.y,
> +                        &path);

Lets do this the same way as the function above (GetPathForGlyphs), and share the common code between them (which is most of it).

Bonus points for making the cairo versions share code too.

@@ +131,5 @@
> +                        aBuffer.mGlyphs[i].mPosition.x, aBuffer.mGlyphs[i].mPosition.y,
> +                        &path);
> +
> +      RefPtr<PathSkia> skiaPath = new PathSkia(path, FILL_WINDING);
> +      skiaPath->AppendPathToBuilder(builder);

I know this matches the cairo version, but it seems really backwards to me.

How about PathBuilderSkia::AppendPath(SkPath& aPath) instead, then we can skip the construction of the PathSkia too.

Comment 14

[Kevin Simons [:kevsim] (Telenor)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> Comment on attachment 8342625 [details] [diff] [review]
> Part 2: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder
> 
> Review of attachment 8342625 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Lets do this the same way as the function above (GetPathForGlyphs), and
> share the common code between them (which is most of it).
> 

Wow, not sure how I missed this when writing in. Oh well, at least I arrived at more or less the same result ;-)

Comment 15

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Part 1: Check the DrawTarget type before downcasting PathBuilders. r=mattwoodrow

Previously the ScaledFontBase::CopyGlyphsToBuilder methods in the
various ScaledFont subclasses static_cast-ed the passed in PathBuilder
objects without checking which sort of DrawSurface they were created by.

Comment 16

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Part 2: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder

Handle PathBuilderSkia with a separate path specifically for
BACKEND_SKIA DrawTarget instances.

Comment 17

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Part 1: Check the DrawTarget type before downcasting PathBuilders. r=mattwoodrow

Previously the ScaledFontBase::CopyGlyphsToBuilder methods in the
various ScaledFont subclasses static_cast-ed the passed in PathBuilder
objects without checking which sort of DrawSurface they were created by.

Comment 18

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Part 2: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder. r=mattwoodrow

Handle PathBuilderSkia with a separate path specifically for
BACKEND_SKIA DrawTarget instances.

Comment 19

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Part 1: Check the DrawTarget type before downcasting PathBuilders. r=mattwoodrow

Previously the ScaledFontBase::CopyGlyphsToBuilder methods in the
various ScaledFont subclasses static_cast-ed the passed in PathBuilder
objects without checking which sort of DrawSurface they were created by.

Comment 20

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Part 2: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder. r=mattwoodrow

Handle PathBuilderSkia with a separate path specifically for
BACKEND_SKIA DrawTarget instances.

Comment 21

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Part 1: Check the DrawTarget type before downcasting PathBuilders. r=mattwoodrow

Previously the ScaledFontBase::CopyGlyphsToBuilder methods in the
various ScaledFont subclasses static_cast-ed the passed in PathBuilder
objects without checking which sort of DrawSurface they were created by.

Comment 22

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Part 2: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder. r=mattwoodrow

Handle PathBuilderSkia with a separate path specifically for
BACKEND_SKIA DrawTarget instances.

Comment 23

[Kevin Simons [:kevsim] (Telenor)]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=05ed47278485

Having never used try before, are these results ok? I.e. is this able to be merged or are those failures unexpected. Most of them seem to have bugs for intermittent failures.

Comment 24

[Matt Woodrow (:mattwoodrow)]

Yep, looks like you're good to go.

Comment 25

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/integration/mozilla-inbound/rev/064eb6b0604d
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f6ecbf2aac3

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/064eb6b0604d
https://hg.mozilla.org/mozilla-central/rev/1f6ecbf2aac3