1309913 - Not getting accelerated canvas on OS X with E10S

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

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

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.

Comment 1

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

(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.

Comment 2

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

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.

Comment 3

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

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.)

Comment 4

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

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.

Comment 5

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

I think I lied - we don't need 51 uplift, don't think this problem got exposed until bug 1294812.

Comment 6

[Nicolas Silva [:nical]]

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.

Comment 7

[Nicolas Silva [:nical]]

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.

Comment 8

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

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?

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

Comment 10

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

Attachment: Bug 1309913: Pass the compositor type to canvas on creation.

Review commit: https://reviewboard.mozilla.org/r/88754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88754/

Comment 11

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

Review ping.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 13

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

(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.

Comment 14

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

Comment on attachment 8804946 [details]
Bug 1309913: Pass the compositor type to canvas on creation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88754/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e71aa9bcb92c
Pass the compositor type to canvas on creation. r=dvander

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6027752&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/19c01532ecbd460395d33db94a365daaf9661142

Comment 17

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

Comment on attachment 8804946 [details]
Bug 1309913: Pass the compositor type to canvas on creation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88754/diff/2-3/

Comment 18

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

Comment on attachment 8804946 [details]
Bug 1309913: Pass the compositor type to canvas on creation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88754/diff/3-4/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa134dc8e0c7
Pass the compositor type to canvas on creation. r=dvander

Comment 20

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

Comment on attachment 8804946 [details]
Bug 1309913: Pass the compositor type to canvas on creation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88754/diff/4-5/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f44f18708f9
Pass the compositor type to canvas on creation. r=dvander

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aa134dc8e0c7
https://hg.mozilla.org/mozilla-central/rev/6f44f18708f9