Closed Bug 1031525 Opened 6 years ago Closed 6 years ago

test_bug449653 crashes with canvas preference set to skia

Categories

(Core :: Canvas: 2D, defect)

32 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: milan, Assigned: milan)

References

Details

Attachments

(1 file, 2 obsolete files)

Set gfx.canvas.azure.backends to skia.
Crash trying to run ./mach mochitest-plain layout/generic/test/test_bug449653.html
BasicLayerManager::PaintSelfOrChildren calls aGroupTarget->GetDrawTarget() which gives back DrawTargetCG and we eventually get trouble as we have SourceSurfaceSkia.
Blocks: 932958
Attachment #8447437 - Flags: review?(matt.woodrow)
Attachment #8447437 - Flags: review?(gwright)
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?
Attachment #8447437 - Flags: review?(gwright) → review+
(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?
(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.
See Also: → 1031866
Attachment #8447437 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8447437 - Flags: review+ → review-
Thanks Matt, had a feeling it was something like that.
Is this the direction of what you guys had in mind?
Attachment #8447437 - Attachment is obsolete: true
Attachment #8454697 - Flags: feedback?(matt.woodrow)
Attachment #8454697 - Flags: feedback?(gwright)
Comment on attachment 8454697 [details] [diff] [review]
DrawTargetCG + SourceSurfaceSkia support

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

Yeah that looks reasonable to me.
Attachment #8454697 - Flags: feedback?(gwright) → feedback+
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.
Attachment #8454697 - Flags: feedback?(matt.woodrow) → feedback+
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.
Attachment #8454697 - Attachment is obsolete: true
Attachment #8455450 - Flags: review?(matt.woodrow)
Attachment #8455450 - Flags: review?(gwright)
Attachment #8455450 - Flags: review?(matt.woodrow) → review+
Attachment #8455450 - Flags: review?(gwright)
Assignee: nobody → milan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e47c703b96e4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Blocks: 1150944
You need to log in before you can comment on or make changes to this bug.