Closed Bug 991572 Opened 7 years ago Closed 7 years ago

Stop creating a Thebes backed gfxContext in CanvasRenderingContext2D.cpp, and kill off nsICanvasRenderingContextInternal::GetThebesSurface

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file, 1 obsolete file)

We should stop creating a Thebes backed gfxContext in CanvasRenderingContext2D.cpp, and kill off nsICanvasRenderingContextInternal::GetThebesSurface.
Attached patch patch (obsolete) — Splinter Review
Attachment #8401190 - Flags: review?(matt.woodrow)
Attachment #8401190 - Flags: review?(matt.woodrow) → review+
Backed out for Crashtest orange (assertion failure):

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd167d38965c
The issue is that the gfxPattern |pat| in CanvasRenderingContext2D::Render can't be used with the gfxContext that is passed in because the gfxContext is gfxASurface backed. In debug builds we hit the MOZ_ASSERT(!pattern->IsAzure()) in gfxContext::SetPattern; in opt builds we fail painting.

The gfxContext is gfxASurface backed because nsLayoutUtils::SurfaceFromElement(HTMLCanvasElement* aElement, ...) creates such a context if SFE_NO_PREMULTIPLY_ALPHA is passed in. This all seems a bit weird to me since as I understand it both Thebes and Moz2D surfaces are pre-multiplied. Even weirder is that the gfxImageSurface that is used to create the gfxContext is wrapping a DataSourceSurface's data, and it's the DataSourceSurface that is used after the Render(). So are we abusing SourceSurface and making it non-pre-multiplied here?
It seems like a cleaner way to handle SFE_NO_PREMULTIPLY_ALPHA would be to work with pre-multiplied SourceSurfaces and only un-pre-multiply further up the call stack when the SourceSurface data is read and used to create whatever it is that is passed out to WebGL code.
Try push showing failures:

https://tbpl.mozilla.org/?tree=Try&rev=53cdc6acdec5

Multiple failures on Linux, OS X and Windows in:

content/canvas/test/webgl-conformance/test_webgl_conformance_test_suite.html
browser/devtools/shared/test/browser_telemetry_button_tilt.js
content/html/content/crashtests/798802-1.html
content/html/content/crashtests/798802-1.html

The mochitest-plain-1 failures are debug and opt failures where pixel data is wrong. The "bc" and "C" failures being are debug only due to failing the assertion mentioned in comment 4.
(In reply to Jonathan Watt [:jwatt] from comment #4)
> So are we abusing
> SourceSurface and making it non-pre-multiplied here?

Yes we are.


(In reply to Jonathan Watt [:jwatt] from comment #5)
> It seems like a cleaner way to handle SFE_NO_PREMULTIPLY_ALPHA would be to
> work with pre-multiplied SourceSurfaces and only un-pre-multiply further up
> the call stack when the SourceSurface data is read and used to create
> whatever it is that is passed out to WebGL code.

The problem case here is when drawing WebGL -> WebGL with SFE_NO_PREMULTIPLY_ALPHA. The source data is probably already non-premultiplied, so no conversions are necessary.

With your propsal we'd end up converting twice.
Okay, well I'd have to spend some time on this to get enough of a handle on things to come up with a solution to getting this patch landed. Due to upcoming PTO I'm unlikely to work on this again until May though (hence the dump of info above). If anyone else wants to pursue this, feel free.
From IRC:

mattwoodrow	jwatt: Looking at the non-premultiplied stuff
mattwoodrow	looks like there’s only two callers, so we can just change the API a bit
mattwoodrow	how about making it return a surface instead of drawing to a context
mattwoodrow	with an input flag ‘i’m ok with getting back a non-premultiplied result’
mattwoodrow	and an output saying whether that actually happened or not
mattwoodrow	then the only caller that would pass that flag (webgl) can post-process if it needs to
jgilbert		mattwoodrow, if it's only a hint, pleaseplease make this obvious from the API
mattwoodrow	Yes of course
I don't think that works - see my comment in bug 995409 comment 4.
Blocks: 987190
Depends on: 997014
(In reply to Jonathan Watt [:jwatt] [offline 17-28 April] from comment #9)
> From IRC:
> 
> mattwoodrow	jwatt: Looking at the non-premultiplied stuff
> mattwoodrow	looks like there’s only two callers, so we can just change the
> API a bit
> mattwoodrow	how about making it return a surface instead of drawing to a
> context
> mattwoodrow	with an input flag ‘i’m ok with getting back a non-premultiplied
> result’
> mattwoodrow	and an output saying whether that actually happened or not
> mattwoodrow	then the only caller that would pass that flag (webgl) can
> post-process if it needs to
> jgilbert		mattwoodrow, if it's only a hint, pleaseplease make this obvious
> from the API
> mattwoodrow	Yes of course

I filed bug 997014 for this idea.
Attached patch RebasedSplinter Review
Rebased patch on top of 997014.

Carrying r=mattwoodrow
Attachment #8401190 - Attachment is obsolete: true
Attachment #8408000 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/5e815640f63d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.