Closed Bug 1309913 Opened 8 years ago Closed 8 years ago

Not getting accelerated canvas on OS X with E10S

Categories

(Core :: Graphics, defect, P1)

Product:
Core
Component:
Graphics
Version:
52 Branch
Platform:
Unspecified
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: milan, Assigned: milan)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(1 file)

Bug 1309913: Pass the compositor type to canvas on creation.
58 bytes, text/x-review-board-request
dvander
: review+
Details 
Even if the acceleration is reported, it appears we're not actually creating SkiaGL canvas when we should be.

Chances are a regression from bug 1294812.
Assignee: nobody → milan
Priority: -- → P1
Whiteboard: [gfx-noted]
(In reply to Milan Sreckovic [:milan] from comment #0)
> Even if the acceleration is reported, it appears we're not actually creating
> SkiaGL canvas when we should be.
> 
> Chances are a regression from bug 1294812.

Not really, though it could have gotten it worse.  Probably came from bug 1298345.
Keywords: regression
Version: unspecified → 51 Branch
It seems the root cause is that gfxPlatform::mCompositorBackend isn't set in the child process.  This is why it only fails on e10s.  Bug 1298345 broke this, bug 1294812 would have made it worse, so both need to be sorted out.
And it isn't set because NotifyCompositorCreated() only happens on the parent process.  Need to figure out if gfxPlatform::mCompositorBackend is expected to be correctly set in the child process in the first place, or if it's meant for parent process only (and thus gfxPlatform::GetCompositorBackend() would only be valid in the process that owns the compositor.)
I don't think gfxPlatform::GetCompositorBackend() ever properly returned the compositor backend, when queried in the content process, since it was introduced in bug 1179051 in 42.  We just never used it that way until bug 1298345 in 51.

Whatever we do here, we need to uplift to 51, as we haven't been accelerating canvas since early September.
I think I lied - we don't need 51 uplift, don't think this problem got exposed until bug 1294812.
No longer blocks: 1298345
See Also: 1298345 →
Version: 51 Branch → 52 Branch
I investigated a bit and gfxPlatform::AllowOpenGLCanvas is always returning false in the content process due to mCompositorBackend being LayersBackend::LAYERS_NONE (it should be GL). I assume that this is because NotifyCompositorCreated is not called, I'm looking into why.
nsBaseWidget::CreateCompositor is not called on the content process. It used to be the case (a while ago). Whatever is replacing it now need to call gfxPlaftorm::NotifyCompositorCreated.
Before we get further - David, did you envision gfxPlatform::GetCompositorBackend() returning the "compositor type" from all processes, or is this a method that should only be used from the process that owns the compositor?  I imagine we want it on all processes; it does make it more useful, and we do need it in the UI process if we want about:support to report things correctly, so once the GPU process owns the compositor, we already have the situation where we need it from the "wrong" process.  So, it feels like the content process should have the correct value as well.
That makes sense?
Flags: needinfo?(dvander)
Whoops, I forgot when reviewing bug 1294812 that this code would be running in the content process too.

AllowOpenGLCanvas should be taking in a layers backend rather than getting one from gfxPlatform. Unfortunately I don't know off the top of my head how to get there from a DOM call, but it usually goes something like: DOM element -> window -> docshell -> presshell -> root frame -> view -> widget -> ClientLayerManager -> LayersBackend. Example of getting widget from DOM is here [1]. But I don't know if that's always going to work.

If not we can stick the LayersBackend in gfxVars, then it will be synced to all processes automatically. It just won't guarantee that it matches the actual layers backend.

[1] http://searchfox.org/mozilla-central/rev/8cf1367dd89cc36ef8f025dfc6af6d5c086838a7/dom/base/nsDOMWindowUtils.cpp#380
Flags: needinfo?(dvander)
Review ping.
Flags: needinfo?(dvander)
Comment on attachment 8804946 [details]
Bug 1309913: Pass the compositor type to canvas on creation.

https://reviewboard.mozilla.org/r/88754/#review89030

::: dom/canvas/CanvasRenderingContext2D.h:75
(Diff revision 1)
>  
>    virtual JSObject* WrapObject(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) override;
>  
>    HTMLCanvasElement* GetCanvas() const
>    {
> -    if (mCanvasElement->IsInNativeAnonymousSubtree()) {
> +    if (!mCanvasElement || mCanvasElement->IsInNativeAnonymousSubtree()) {

Was this change intentional?

::: gfx/thebes/gfxPlatform.cpp:1291
(Diff revision 1)
> -  {
> +  // The callers have to do the right thing.
> +  bool correctBackend = !XRE_IsParentProcess() ||
> +    ((mCompositorBackend == LayersBackend::LAYERS_OPENGL) &&
> +     (GetContentBackendFor(mCompositorBackend) == BackendType::SKIA));
> +
> +  if (gfxPrefs::CanvasAzureAccelerated() && correctBackend) {

In an E10S world, is there a reason to have correctBackend included in this check anymore? XRE_IsParentProcess will probably always be false for canvas elements.

But all of this is covered more accurately by the LAYERS_OPENGL check you added to canvas. If you want to force callers to understand what they're doing, you can just make them pass the compositor backend to AllowOpenGLCanvas.
Attachment #8804946 - Flags: review?(dvander) → review+
(In reply to David Anderson [:dvander] from comment #12)
> Comment on attachment 8804946 [details]
> Bug 1309913: Pass the compositor type to canvas on creation.
> 
> https://reviewboard.mozilla.org/r/88754/#review89030
> 
> ::: dom/canvas/CanvasRenderingContext2D.h:75
> (Diff revision 1)
> >  
> >    virtual JSObject* WrapObject(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) override;
> >  
> >    HTMLCanvasElement* GetCanvas() const
> >    {
> > -    if (mCanvasElement->IsInNativeAnonymousSubtree()) {
> > +    if (!mCanvasElement || mCanvasElement->IsInNativeAnonymousSubtree()) {
> 
> Was this change intentional?

Drive by null pointer check.  In one of the previous versions of this patch, I was calling GetCanvas() with mCanvasElement being null and crashing.

> 
> ::: gfx/thebes/gfxPlatform.cpp:1291
> (Diff revision 1)
> > -  {
> > +  // The callers have to do the right thing.
> > +  bool correctBackend = !XRE_IsParentProcess() ||
> > +    ((mCompositorBackend == LayersBackend::LAYERS_OPENGL) &&
> > +     (GetContentBackendFor(mCompositorBackend) == BackendType::SKIA));
> > +
> > +  if (gfxPrefs::CanvasAzureAccelerated() && correctBackend) {
> 
> In an E10S world, is there a reason to have correctBackend included in this
> check anymore? XRE_IsParentProcess will probably always be false for canvas
> elements.

Right now, the only scenario I ran into where this wasn't the case, was the about:support/about:telemetry scenario.

> 
> But all of this is covered more accurately by the LAYERS_OPENGL check you
> added to canvas. If you want to force callers to understand what they're
> doing, you can just make them pass the compositor backend to
> AllowOpenGLCanvas.

Right - if there is in fact a canvas - in the about:support case, there is no canvas.  I'll still double check if there is a way to make this consistent.
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e71aa9bcb92c
Pass the compositor type to canvas on creation. r=dvander
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa134dc8e0c7
Pass the compositor type to canvas on creation. r=dvander
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f44f18708f9
Pass the compositor type to canvas on creation. r=dvander
Flags: needinfo?(milan)
https://hg.mozilla.org/mozilla-central/rev/aa134dc8e0c7
https://hg.mozilla.org/mozilla-central/rev/6f44f18708f9
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
See Also: → 1325518
Depends on: 1325518
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: