1031525 - test_bug449653 crashes with canvas preference set to skia

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

Set gfx.canvas.azure.backends to skia.
Crash trying to run ./mach mochitest-plain layout/generic/test/test_bug449653.html

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

BasicLayerManager::PaintSelfOrChildren calls aGroupTarget->GetDrawTarget() which gives back DrawTargetCG and we eventually get trouble as we have SourceSurfaceSkia.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Create canvas specific draw target

Comment 3

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8447437 [details] [diff] [review]
Create canvas specific draw target

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

Looks fine to me. We may want to add support for SourceSurfaceSkia objects in DrawTargetCG's DrawSurface as well at some point?

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to George Wright (:gw280) from comment #3)
> Comment on attachment 8447437 [details] [diff] [review]
> Create canvas specific draw target
> 
> Review of attachment 8447437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine to me. We may want to add support for SourceSurfaceSkia objects
> in DrawTargetCG's DrawSurface as well at some point?

Any performance implications with DrawTargetCG+SourceSurfaceSkia vs. DrawTargetSkia+SourceSurfaceSkia?

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to George Wright (:gw280) from comment #3)
> Comment on attachment 8447437 [details] [diff] [review]
> Create canvas specific draw target
> 
> Review of attachment 8447437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine to me. We may want to add support for SourceSurfaceSkia objects
> in DrawTargetCG's DrawSurface as well at some point?

You're right, bug 1031866 hits the scenario where that is the only thing that will work.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8447437 [details] [diff] [review]
Create canvas specific draw target

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

Actually, I don't think we should do this. We've hit this branch because we decided that we weren't happy with the mTarget's backend (skia) being used for content drawn, going ahead and creating a new skia DT to replace it isn't useful.

We either need to fix the interop between SourceSurfaceCG and DrawTargetSkia, or make SupportsAzureContentForDrawTarget return true for skia on OSX.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks Matt, had a feeling it was something like that.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: DrawTargetCG + SourceSurfaceSkia support

Is this the direction of what you guys had in mind?

Comment 9

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8454697 [details] [diff] [review]
DrawTargetCG + SourceSurfaceSkia support

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

Yeah that looks reasonable to me.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8454697 [details] [diff] [review]
DrawTargetCG + SourceSurfaceSkia support

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

Looks fine, but I wonder if we need to differentiate between SurfaceType::DATA, SurfaceType::SKIA, and SurfaceType::ALLTHEOTHERS. Both DrawTargetD2D and DrawTargetCairo just call GetDataSurface (which should work for all types) if it's not a native D2D/CAIRO typed SourceSurface.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Yes, that would be a clean change. What I'd like to do on top is "tag" that "else" as a "slower path" somehow, so that we can notice when things like this happen.  Separate story.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: DrawTargetCG::DrawWindow to support non-CG DataSourceSurface types.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

https://tbpl.mozilla.org/?tree=Try&rev=8993dcdb85d6

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e47c703b96e4

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e47c703b96e4