Closed Bug 944646 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: kevin, Assigned: kevin)

Details

Attachments

(2 files, 11 obsolete files)

10.14 KB, patch
Details | Diff | Splinter Review
6.00 KB, patch
Details | Diff | Splinter Review
Opening layout/reftests/svg/text/simple-fill-gradient.svg causes a reproducible crash when the skia backend is in use.
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.
Attachment #8341927 - Flags: review?(matt.woodrow)
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 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.
Attachment #8341927 - Flags: review?(matt.woodrow) → review-
Sounds very reasonable. I'll attempt to code that up tomorrow then. Thanks for the quick feedback!
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.
Attachment #8341927 - Attachment is obsolete: true
Handle PathBuilderSkia with a separate path specifically for
BACKEND_SKIA DrawTarget instances.
Attachment #8342493 - Attachment description: Check the DrawTarget type before downcasting PathBuilders → Part 1: Check the DrawTarget type before downcasting PathBuilders
Attachment #8342493 - Flags: review?(matt.woodrow)
Attachment #8342494 - Attachment description: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder → Part 2: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder
Attachment #8342494 - Flags: review?(matt.woodrow)
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.
Handle PathBuilderSkia with a separate path specifically for
BACKEND_SKIA DrawTarget instances.
Attachment #8342494 - Attachment is obsolete: true
Attachment #8342494 - Flags: review?(matt.woodrow)
Attachment #8342573 - Attachment description: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder → Part 2: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder
Attachment #8342493 - Flags: review?(matt.woodrow)
Realized the transform bit doesn't work. Revisiting.
Handle PathBuilderSkia with a separate path specifically for
BACKEND_SKIA DrawTarget instances.
Attachment #8342573 - Attachment is obsolete: true
Attachment #8342625 - Attachment description: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder → Part 2: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder
Attachment #8342625 - Flags: review?(matt.woodrow)
Attachment #8342493 - Flags: review?(matt.woodrow)
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.
Attachment #8342493 - Flags: review?(matt.woodrow) → review+
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.
(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 ;-)
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.
Attachment #8342493 - Attachment is obsolete: true
Handle PathBuilderSkia with a separate path specifically for
BACKEND_SKIA DrawTarget instances.
Attachment #8342625 - Attachment is obsolete: true
Attachment #8342625 - Flags: review?(matt.woodrow)
Attachment #8345243 - Attachment description: Check the DrawTarget type before downcasting PathBuilders. r=mattwoodrow → Part 1: Check the DrawTarget type before downcasting PathBuilders. r=mattwoodrow
Attachment #8345244 - Attachment description: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder → Part 2: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder
Attachment #8345244 - Flags: review?(matt.woodrow)
Attachment #8345244 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8345243 - Attachment is obsolete: true
Handle PathBuilderSkia with a separate path specifically for
BACKEND_SKIA DrawTarget instances.
Attachment #8345244 - Attachment is obsolete: true
Attachment #8345766 - Attachment description: Check the DrawTarget type before downcasting PathBuilders. r=mattwoodrow → Part 1: Check the DrawTarget type before downcasting PathBuilders. r=mattwoodrow
Attachment #8345767 - Attachment description: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder. r=mattwoodrow → Part 2: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder. 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.
Attachment #8345766 - Attachment is obsolete: true
Handle PathBuilderSkia with a separate path specifically for
BACKEND_SKIA DrawTarget instances.
Attachment #8345767 - Attachment is obsolete: true
Attachment #8346393 - Attachment description: Check the DrawTarget type before downcasting PathBuilders. r=mattwoodrow → Part 1: Check the DrawTarget type before downcasting PathBuilders. r=mattwoodrow
Attachment #8346394 - Attachment description: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder. r=mattwoodrow → Part 2: Implement Skia path in ScaledFontBase::CopyGlyphsToBuilder. 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.
Attachment #8346393 - Attachment is obsolete: true
Handle PathBuilderSkia with a separate path specifically for
BACKEND_SKIA DrawTarget instances.
Attachment #8346394 - Attachment is obsolete: true
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.
Yep, looks like you're good to go.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.