939276 - [SkiaGL] [Feature] Support multiple Skia DrawTargets backed by a single GLContext

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

At the moment we have to have a new GLContext for each DrawTargetSkia we want to use. This is very wasteful and doesn't allow us to have nice things like a unified texture cache shared amongst all DrawTargetSkia objects.

Comment 1

[Andreas Gal :gal]

We had a little discussion about this on IRC and were wondering whether we should have a single GLContext per thread in general.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Andreas Gal :gal from comment #1)
> We had a little discussion about this on IRC and were wondering whether we
> should have a single GLContext per thread in general.

We will for all Skia stuff, at least. Doing it for things like WebGL is more challenging because of the various state management involved. For that we really need "virtualized" GLContext, which is a bit more work.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Use a GLContextSkia class as a platfrom through which to bind GLContext and GrContext together, instead of DrawTargetSkia.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Use a single GLContext for all SkiaGL canvases

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Try run here: https://tbpl.mozilla.org/?tree=Try&rev=e45c948e8688

Comment 6

[Andreas Gal :gal]

Comment on attachment 8369263 [details] [diff] [review]
Use a GLContextSkia class as a platfrom through which to bind GLContext and GrContext together, instead of DrawTargetSkia.

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

This looks pretty good. I think I understand this and you got my r+ but I will leave bjacob up in case you want him to look as well.

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +882,5 @@
>                                                           caps);
>          }
>  
>          if (glContext) {
> +          GLContextSkia* glContextSkia = new GLContextSkia(glContext);

I think you could abstract this nicer with a base class that you insert between GenericRefCounted and GLContextSkia.

::: gfx/2d/2D.h
@@ +967,5 @@
>    }
>  
>  #ifdef USE_SKIA_GPU
> +  virtual void InitWithGLContextSkia(GenericRefCountedBase* aGLContextSkia,
> +                                        GrContext* aGrContext,

Alignment.

@@ +1075,5 @@
>  
>  #ifdef USE_SKIA_GPU
>    static TemporaryRef<DrawTarget>
> +    CreateDrawTargetSkiaWithGLContextSkia(GenericRefCountedBase* aGLContext,
> +                                             GrContext* aGrContext,

Dito.

::: gfx/2d/DrawTargetSkia.cpp
@@ +768,5 @@
>  
>  #ifdef USE_SKIA_GPU
>  void
> +DrawTargetSkia::InitWithGLContextSkia(GenericRefCountedBase* aGLContextSkia,
> +                                         GrContext* aGrContext,

Dito.

::: gfx/2d/DrawTargetSkia.h
@@ +105,5 @@
>  
>  #ifdef USE_SKIA_GPU
> +  virtual GenericRefCountedBase* GetGLContextSkia() const MOZ_OVERRIDE { return mGLContextSkia; }
> +  void InitWithGLContextSkia(GenericRefCountedBase* aGLContextSkia,
> +                                GrContext* aGrContext,

Dito.

::: gfx/2d/Factory.cpp
@@ +585,5 @@
>  
>  #ifdef USE_SKIA_GPU
>  TemporaryRef<DrawTarget>
> +Factory::CreateDrawTargetSkiaWithGLContextSkia(GenericRefCountedBase* aGLContextSkia,
> +                                                  GrContext* aGrContext,

Dito.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Andreas Gal :gal from comment #6)
> Comment on attachment 8369263 [details] [diff] [review]
> Use a GLContextSkia class as a platfrom through which to bind GLContext and
> GrContext together, instead of DrawTargetSkia.
> 
> Review of attachment 8369263 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty good. I think I understand this and you got my r+ but I
> will leave bjacob up in case you want him to look as well.

Thanks. The more reviews, the better.

> 
> ::: content/canvas/src/CanvasRenderingContext2D.cpp
> @@ +882,5 @@
> >                                                           caps);
> >          }
> >  
> >          if (glContext) {
> > +          GLContextSkia* glContextSkia = new GLContextSkia(glContext);
> 
> I think you could abstract this nicer with a base class that you insert
> between GenericRefCounted and GLContextSkia.

The base class would hold the GLContext, GrContext, and GLContextSkia? I think that would indeed be better.

> 
> ::: gfx/2d/2D.h
> @@ +967,5 @@
> >    }
> >  
> >  #ifdef USE_SKIA_GPU
> > +  virtual void InitWithGLContextSkia(GenericRefCountedBase* aGLContextSkia,
> > +                                        GrContext* aGrContext,
> 
> Alignment.

Thanks. Fixed, along with the others.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Actually, I don't think we even need the ref counting business anymore really. The GLContext and friends now live forever once they are initially created (though maybe that shouldn't be the case eventually)

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8369263 [details] [diff] [review]
Use a GLContextSkia class as a platfrom through which to bind GLContext and GrContext together, instead of DrawTargetSkia.

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

::: gfx/2d/DrawTargetSkia.h
@@ -137,5 @@
> -   * These members have inter-dependencies, but do not keep each other alive, so
> -   * destruction order is very important here: mGrContext uses mGrGLInterface, and
> -   * through it, uses mGLContext, so it is important that they be declared in the
> -   * present order.
> -   */

Don't remove that comment, as it seems still as relevant as before (the GrContext still uses the GLContext).

::: gfx/gl/GLContextSkia.h
@@ +11,5 @@
> +
> +namespace mozilla {
> +namespace gl {
> +
> +class GLContextSkia : public GenericAtomicRefCounted

The name GLContextSkia was already confusing; now that it's a class, it's even more confusing --- this is *not* a GLContext specialization. It would be useful to find a better name and use it for this class and to rename the existing files.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8369264 [details] [diff] [review]
Use a single GLContext for all SkiaGL canvases

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

::: gfx/gl/SharedSurfaceGL.cpp
@@ +304,4 @@
>      mGL->MakeCurrent();
>  
> +    ScopedFramebufferForTexture autoTex(mGL, mTex);
> +    ScopedBindFramebuffer autoFB(mGL, autoTex.FB());

It would probably be better to just maintain an FB with this texture attached to it, and then read out of it without having to rebind it.

@@ +337,5 @@
> +    bool ownsTex = false;
> +
> +    if (!tex) {
> +      tex = CreateTextureForOffscreen(prodGL, formats, size);
> +      ownsTex = true;

Leave a comment about this functionality next to the function decl.

::: gfx/gl/SurfaceStream.cpp
@@ +65,5 @@
> +    }
> +
> +    MOZ_ASSERT(mProducer);
> +
> +    mProducer->LockProdImpl();

Don't call this directly. Call LockProd. You shouldn't be able to call LockProdImpl directly, even.

@@ +210,5 @@
> +        Scrap(mStaging);
> +    }
> +
> +    New(factory, size, mProducer);
> +    New(factory, size, mStaging);

I'm guessing this is because we actually want something more similar to TripleBufferWithCopy, but without the stream owning the mProducer source. That is, we want a SwapProducer() to take our separate surface, and copy it into a ShSurf that we promote to staging. In this case, all we need is a function to do a resize of mProd. Functionally, if this is followed by a normal SwapProducers, the blank frame we overwrite mStaging with is immediately scrapped again, unless a race condition comes between these calls, during which the compositor will pick up this new blank mStaging as a 'complete' frame, and promote it to mConsumer.

Just resize mProd here.

::: gfx/layers/client/ClientCanvasLayer.cpp
@@ +43,5 @@
>      GLScreenBuffer* screen = mGLContext->Screen();
> +
> +    SurfaceCaps caps = screen->Caps();
> +    if (mStream)
> +      caps = SurfaceCaps::ForRGBA();

Why? Leave a comment.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jeff Gilbert [:jgilbert] from comment #10)
> Comment on attachment 8369264 [details] [diff] [review]
> Use a single GLContext for all SkiaGL canvases
> 
> Review of attachment 8369264 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/SharedSurfaceGL.cpp
> @@ +304,4 @@
> >      mGL->MakeCurrent();
> >  
> > +    ScopedFramebufferForTexture autoTex(mGL, mTex);
> > +    ScopedBindFramebuffer autoFB(mGL, autoTex.FB());
> 
> It would probably be better to just maintain an FB with this texture
> attached to it, and then read out of it without having to rebind it.

Yup, done.

> 
> @@ +337,5 @@
> > +    bool ownsTex = false;
> > +
> > +    if (!tex) {
> > +      tex = CreateTextureForOffscreen(prodGL, formats, size);
> > +      ownsTex = true;
> 
> Leave a comment about this functionality next to the function decl.

Done.

> 
> ::: gfx/gl/SurfaceStream.cpp
> @@ +65,5 @@
> > +    }
> > +
> > +    MOZ_ASSERT(mProducer);
> > +
> > +    mProducer->LockProdImpl();
> 
> Don't call this directly. Call LockProd. You shouldn't be able to call
> LockProdImpl directly, even.

So I'm looking into this again, but I think the problem was that the surface was somehow already locked. Looking at it again now.

> 
> @@ +210,5 @@
> > +        Scrap(mStaging);
> > +    }
> > +
> > +    New(factory, size, mProducer);
> > +    New(factory, size, mStaging);
> 
> I'm guessing this is because we actually want something more similar to
> TripleBufferWithCopy, but without the stream owning the mProducer source.
> That is, we want a SwapProducer() to take our separate surface, and copy it
> into a ShSurf that we promote to staging. In this case, all we need is a
> function to do a resize of mProd. Functionally, if this is followed by a
> normal SwapProducers, the blank frame we overwrite mStaging with is
> immediately scrapped again, unless a race condition comes between these
> calls, during which the compositor will pick up this new blank mStaging as a
> 'complete' frame, and promote it to mConsumer.
> 
> Just resize mProd here.

Oops, we don't even use this. It's from an old version of the patch. Removed.

> 
> ::: gfx/layers/client/ClientCanvasLayer.cpp
> @@ +43,5 @@
> >      GLScreenBuffer* screen = mGLContext->Screen();
> > +
> > +    SurfaceCaps caps = screen->Caps();
> > +    if (mStream)
> > +      caps = SurfaceCaps::ForRGBA();
> 
> Why? Leave a comment.

Well the screen caps are irrelevant, but yeah this is not right. It should be ForRGB() if we are opaque, and ForRGBA() otherwise.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8369264 [details] [diff] [review]
Use a single GLContext for all SkiaGL canvases

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

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +630,5 @@
>    // sErrorTarget.
>    mozilla::RefPtr<mozilla::gfx::DrawTarget> mTarget;
>  
> +#ifdef USE_SKIA_GPU
> +  gfx::SurfaceStream* mStream;

This is bad: we go from managing a GLContext by refcounting to managing a SurfaceStream imperatively without refcounting. This implies race conditions e.g. during shutdown, where the compositor thread would crash if it accessed this stream after it's been imperatively destroyed by the CanvasRenderingContext2D on the content thread.

As we discussed offline, it seems that the logical way to fix this is to make SurfaceStream reference-counted and make this hold a strong reference to mStream.

This will mean that a SurfaceStream is potentially destroyed on a different thread than it was created, which I understand is dangerous as currently that implies that some GL resources will be destroyed on a different thread than they were created, which is driver-unfriendly. However, this mimics our current behavior, with our GLContext managed by reference-counting, i.e. this is an issue that we already have. Better land the present patches while keeping the issue that we already have, and then resolve that issue in a separate bug.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Don't use refcounting here. Describe the lifetimes and figure out the right way to do it, instead of just throwing refcounting at it. This is unlikely to be a bug which gets fixed quickly, unless it starts really causing obvious trouble. Why not just do the right thing now?

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Can you describe how a correct implementation without refcounting would proceed?

In my understanding the compositing of SurfaceStream's still relies on passing SurfaceStream* pointers in IPC messages, at least in the same-process IPC case, right? "Doing the right thing" would surely start by avoiding doing that...!

Comment 15

[Andreas Gal :gal]

Why not remember the thread and send the surfacestream there for destruction?

Comment 16

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Sorry, I have a question about sharing GLContext.
If we share the GLContext for 2 skia gl canvas A and B, does A's render state affect B's gl render state(ex. z-test enable)?

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jerry Shih[:jerry] from comment #16)
> Sorry, I have a question about sharing GLContext.
> If we share the GLContext for 2 skia gl canvas A and B, does A's render
> state affect B's gl render state(ex. z-test enable)?

No, the GrContext keeps track of that stuff and resets things when necessary. Skia knows how to play nicely with itself. If we were modifying state in this context *outside* of Skia, things would be more interesting.

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Use a SkiaGLGlue class to hold GLContext and GrContext, instead of DrawTargetSkia.

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Use a single GLContext for all SkiaGL canvases

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

Try run with the new patches here: https://tbpl.mozilla.org/?tree=Try&rev=83ba093df370

The new single context patch does a little more to clean things up in our new world. The SkiaGLGlue instance (and therefore the GLContext, GrContext) now live in gfxPlatform instead of CRC2D. This makes it easy for other consumers to use a SkiaGL DT, and just getting it out of CRC2D is nice. Also gone is all of the cache purging and management stuff from the DrawTargetSkia, since we can do it in gfxPlatform now.

Comment 21

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8371488 [details] [diff] [review]
Use a SkiaGLGlue class to hold GLContext and GrContext, instead of DrawTargetSkia.

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

r+ but please pay attention to the SkiaGLGlue.cpp comment below.

::: gfx/2d/DrawTargetSkia.h
@@ +132,5 @@
>  
>    SkRect SkRectCoveringWholeSurface() const;
>  
>  #ifdef USE_SKIA_GPU
> +  RefPtr<GenericRefCountedBase> mGlue;

I suggest capitalizing this as 'mGLue' to underline that it's Skia*GL*.

Nah, just kidding.

::: gfx/gl/GLContextSkia.h
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "skia/GrGLInterface.h"
>  
> +GrGLInterface* CreateGrGLInterfaceFromGLContext(mozilla::gl::GLContext* context);
\ No newline at end of file

Please, merge that into SkiaGLGlue.h and rename GLContextSkia.cpp -> SkiaGLGlue.cpp.

Having a SkiaGLGlue.cpp will also allow you to implement SkiaGLGlue methods (like the ctor) out-of-line, and only include Skia headers in SkiaGLGlue.cpp, not SkiaGLGlue.h, which will prevent having skia headers included in a lot of non-skia-specific code.

Comment 22

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8371490 [details] [diff] [review]
Use a single GLContext for all SkiaGL canvases

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

I don't have the ability to r+ or r- this patch until i get a better understanding of lifetime management here. Some parts of this patch manage a SurfaceStream by reference counting, other hold raw pointers to a SurfaceStream. Please explain.

::: gfx/gl/SkiaGLGlue.h
@@ +5,5 @@
>  
>  #include "mozilla/RefPtr.h"
>  #include "skia/GrGLInterface.h"
>  #include "skia/GrContext.h"
> +#include "GLContext.h"

You don't need to include the expensive GLContext.h header in this header. Only include it in .cpp files that need it.

::: gfx/thebes/gfxPlatform.cpp
@@ +248,5 @@
>      NS_ASSERTION(strcmp(aTopic, "memory-pressure") == 0, "unexpected event topic");
>      Factory::PurgeAllCaches();
> +
> +#if USE_SKIA_GPU
> +    gfxPlatform::GetPlatform()->PurgeSkiaCache();

I wish that you could keep separate things in separate patches.

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8371490 [details] [diff] [review]
Use a single GLContext for all SkiaGL canvases

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

...so I'm going to cancel my review request for now. What would help me the most to make me able to review this patch, is a good lecture on SurfaceStream lifetimes, what that should be, what that was before this patch, what this becomes with this patch.

Comment 24

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

I'm ok with this.

Only real comment from me is whether we want to continue having all the "maximum number of contexts" code in CRC2D? Now it's hard coded to 64 in all cases and probably isn't worth keeping around.

Comment 25

[Kelsey Gilbert [:jgilbert]]

The short of pre-this-bug SurfaceStream lifetimes is that they're owned by GLContext, and so are bound by the lifetime of the content's GLContext. (They're actually owned by GLScreenBuffer, which has comments establishing owns/owned-by relations in the header)

Comment 26

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8371490 [details] [diff] [review]
Use a single GLContext for all SkiaGL canvases

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

Why is everything behind ifdef USE_SKIA_GPU?

Let's talk lifetimes. I thought we could just attach the lifetime of SurfaceStream to something on the content side. Why can't we do this? Can't we staple it to Canvas2D somehow?

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +630,5 @@
>    // sErrorTarget.
>    mozilla::RefPtr<mozilla::gfx::DrawTarget> mTarget;
>  
> +#ifdef USE_SKIA_GPU
> +  RefPtr<gfx::SurfaceStream> mStream;

I thought we found this doesn't need to be refcounted?

::: gfx/2d/2D.h
@@ +970,5 @@
>      MOZ_CRASH();
>    }
> +
> +  virtual uint32_t GetTextureID() const
> +  {

`{` on same line of single-line decls.

::: gfx/gl/GLScreenBuffer.h
@@ +155,5 @@
>  protected:
>      GLContext* const mGL;         // Owns us.
>      SurfaceCaps mCaps;
>      SurfaceFactory_GL* mFactory;  // Owned by us.
> +    RefPtr<SurfaceStream> mStream;

Why does this now need to be refcounted?

::: gfx/gl/SharedSurfaceGL.cpp
@@ +350,5 @@
>          return;
>  
> +    if (mOwnsTex) {
> +        GLuint tex = mTex;
> +        mGL->fDeleteTextures(1, &tex);

(I'm going to be happy when we can stop doing this constness dance after bug 969632)

::: gfx/layers/CopyableCanvasLayer.h
@@ +59,5 @@
>    nsRefPtr<gfxASurface> mSurface;
>    nsRefPtr<mozilla::gl::GLContext> mGLContext;
>    mozilla::RefPtr<mozilla::gfx::DrawTarget> mDrawTarget;
>  
> +#ifdef USE_SKIA_GPU

Again, why the ifdef?

::: gfx/layers/Layers.h
@@ +1784,5 @@
>      Data()
>        : mSurface(nullptr)
>        , mDrawTarget(nullptr)
>        , mGLContext(nullptr)
> +      , mStream(nullptr)

Surely this doesn't compile ifndef USE_SKIA_GPU?

@@ +1797,5 @@
>  
>      // Or this, for GL.
>      mozilla::gl::GLContext* mGLContext;
>  
> +#ifdef USE_SKIA_GPU

Why are we locking this behind an ifdef? Just have it take reasonable values when we're not ifdef USE_SKIA_GPU. (nullptr and 0, below)

::: gfx/layers/client/CanvasClient.cpp
@@ +113,5 @@
>    GLScreenBuffer* screen = aLayer->mGLContext->Screen();
> +  SurfaceStream* stream = nullptr;
> +
> +#ifdef USE_SKIA_GPU
> +  if (aLayer->mStream) {

Why don't we just have aLayer->mStream be null ifndef USE_SKIA_GPU? Then we wouldn't need these ifdef guards.

Comment 27

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

We have USE_SKIA_GPU because there are some platforms that aren't supported by Skia GPU, so they need to be turned off at compile time. Like big endian machines.

mStream definitely doesn't compile, I found out today after my r+. Please run a build test with USE_SKIA_GPU undefined and make sure it builds before landing please, or I will redirect the angry hordes towards you snorp :D

(Just kidding: there are no angry hordes)

Comment 28

[u480271]

Comment on attachment 8371490 [details] [diff] [review]
Use a single GLContext for all SkiaGL canvases

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

::: gfx/layers/CopyableCanvasLayer.h
@@ +62,5 @@
>  
> +#ifdef USE_SKIA_GPU
> +  gfx::SurfaceStream* mStream;
> +  gfx::SharedSurface* mTextureSurface;
> +  gfx::SurfaceFactory* mFactory;

Do mTextureSurface and mFactory need to be here? From what I can see these members are only used in ClientCanvasLayer.

Comment 29

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Benoit Jacob [:bjacob] from comment #21)
> 
> Please, merge that into SkiaGLGlue.h and rename GLContextSkia.cpp ->
> SkiaGLGlue.cpp.

Yeah, good idea. Fixed.

> 
> Having a SkiaGLGlue.cpp will also allow you to implement SkiaGLGlue methods
> (like the ctor) out-of-line, and only include Skia headers in
> SkiaGLGlue.cpp, not SkiaGLGlue.h, which will prevent having skia headers
> included in a lot of non-skia-specific code.

Yup, fixed that too.

Comment 30

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jeff Gilbert [:jgilbert] from comment #26)
> Comment on attachment 8371490 [details] [diff] [review]
> Use a single GLContext for all SkiaGL canvases
> 
> Review of attachment 8371490 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why is everything behind ifdef USE_SKIA_GPU?
> 
> Let's talk lifetimes. I thought we could just attach the lifetime of
> SurfaceStream to something on the content side. Why can't we do this? Can't
> we staple it to Canvas2D somehow?

Well, it's attached to the lifetime of the DrawTarget in CRC2D. The problem here is that if we have to throw away that DT (usually due to CRC2D::Reset()), we now have the possibility where we could be compositing a SurfaceStream that we just killed. Hence the refcounting.

> 
> ::: content/canvas/src/CanvasRenderingContext2D.h
> @@ +630,5 @@
> >    // sErrorTarget.
> >    mozilla::RefPtr<mozilla::gfx::DrawTarget> mTarget;
> >  
> > +#ifdef USE_SKIA_GPU
> > +  RefPtr<gfx::SurfaceStream> mStream;
> 
> I thought we found this doesn't need to be refcounted?

It does, see above. If we just free the SurfaceStream in CRC2D, we could be compositing a frame still (or about to composite one).

> 
> ::: gfx/2d/2D.h
> @@ +970,5 @@
> >      MOZ_CRASH();
> >    }
> > +
> > +  virtual uint32_t GetTextureID() const
> > +  {
> 
> `{` on same line of single-line decls.

Fixed.

> 
> ::: gfx/gl/GLScreenBuffer.h
> @@ +155,5 @@
> >  protected:
> >      GLContext* const mGL;         // Owns us.
> >      SurfaceCaps mCaps;
> >      SurfaceFactory_GL* mFactory;  // Owned by us.
> > +    RefPtr<SurfaceStream> mStream;
> 
> Why does this now need to be refcounted?

GLScreenBuffer doesn't need it specifically, but I think it's strange to have non-refcounted usage in a refcounted class.

> 
> ::: gfx/gl/SharedSurfaceGL.cpp
> @@ +350,5 @@
> >          return;
> >  
> > +    if (mOwnsTex) {
> > +        GLuint tex = mTex;
> > +        mGL->fDeleteTextures(1, &tex);
> 
> (I'm going to be happy when we can stop doing this constness dance after bug
> 969632)

Srsly.

> 
> ::: gfx/layers/CopyableCanvasLayer.h
> @@ +59,5 @@
> >    nsRefPtr<gfxASurface> mSurface;
> >    nsRefPtr<mozilla::gl::GLContext> mGLContext;
> >    mozilla::RefPtr<mozilla::gfx::DrawTarget> mDrawTarget;
> >  
> > +#ifdef USE_SKIA_GPU
> 
> Again, why the ifdef?

Yeah, I can remove those. SkiaGL doesn't build on some platforms, so we've been ifdefing stuff that is only used there, but no reason to really.

> 
> ::: gfx/layers/Layers.h
> @@ +1784,5 @@
> >      Data()
> >        : mSurface(nullptr)
> >        , mDrawTarget(nullptr)
> >        , mGLContext(nullptr)
> > +      , mStream(nullptr)
> 
> Surely this doesn't compile ifndef USE_SKIA_GPU?

Fixed by above comment

> 
> @@ +1797,5 @@
> >  
> >      // Or this, for GL.
> >      mozilla::gl::GLContext* mGLContext;
> >  
> > +#ifdef USE_SKIA_GPU
> 
> Why are we locking this behind an ifdef? Just have it take reasonable values
> when we're not ifdef USE_SKIA_GPU. (nullptr and 0, below)
> 

Agreed

> ::: gfx/layers/client/CanvasClient.cpp
> @@ +113,5 @@
> >    GLScreenBuffer* screen = aLayer->mGLContext->Screen();
> > +  SurfaceStream* stream = nullptr;
> > +
> > +#ifdef USE_SKIA_GPU
> > +  if (aLayer->mStream) {
> 
> Why don't we just have aLayer->mStream be null ifndef USE_SKIA_GPU? Then we
> wouldn't need these ifdef guards.

Still agree :)

Comment 31

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Dan Glastonbury :djg :kamidphish from comment #28)
> Comment on attachment 8371490 [details] [diff] [review]
> Use a single GLContext for all SkiaGL canvases
> 
> Review of attachment 8371490 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/CopyableCanvasLayer.h
> @@ +62,5 @@
> >  
> > +#ifdef USE_SKIA_GPU
> > +  gfx::SurfaceStream* mStream;
> > +  gfx::SharedSurface* mTextureSurface;
> > +  gfx::SurfaceFactory* mFactory;
> 
> Do mTextureSurface and mFactory need to be here? From what I can see these
> members are only used in ClientCanvasLayer.

Indeed. Moved them there.

Comment 32

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Use a single GLContext for all SkiaGL canvases

Addressed review comments, and folded the two patches together. SkiaGLGlue just didn't make enough sense on its own.

Comment 33

[James Willcox (:snorp) ([e-mail]) (he/him)]

Greenish try run here: https://tbpl.mozilla.org/?tree=Try&rev=b3bd5d07a5ae

Comment 34

[u480271]

Comment on attachment 8373565 [details] [diff] [review]
Use a single GLContext for all SkiaGL canvases

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

I can't really add too much from my previous comments as I'm not au fait with the interaction of GLContext, Skia GL and shared surface. I'll defer to Benoit and Jeff.

::: gfx/layers/client/ClientCanvasLayer.cpp
@@ +36,4 @@
>  ClientCanvasLayer::Initialize(const Data& aData)
>  {
>    CopyableCanvasLayer::Initialize(aData);
>   

Ewww. Trailing whitespace.

::: gfx/layers/opengl/TextureClientOGL.cpp
@@ +129,5 @@
>  StreamTextureClientOGL::InitWith(gfx::SurfaceStream* aStream)
>  {
>    MOZ_ASSERT(!IsAllocated());
>    mStream = aStream;
> +  mGL = mStream->GLContext();

Why do we store the GLContext from the stream? Now there are two pieces of state to keep in sync. Why not use mStream->GLContext() where necessary?

Comment 35

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Dan Glastonbury :djg :kamidphish from comment #34)
> Comment on attachment 8373565 [details] [diff] [review]
> Use a single GLContext for all SkiaGL canvases
> 
> Review of attachment 8373565 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can't really add too much from my previous comments as I'm not au fait
> with the interaction of GLContext, Skia GL and shared surface. I'll defer to
> Benoit and Jeff.
> 
> ::: gfx/layers/client/ClientCanvasLayer.cpp
> @@ +36,4 @@
> >  ClientCanvasLayer::Initialize(const Data& aData)
> >  {
> >    CopyableCanvasLayer::Initialize(aData);
> >   
> 
> Ewww. Trailing whitespace.
> 

Thanks, fixed.

> ::: gfx/layers/opengl/TextureClientOGL.cpp
> @@ +129,5 @@
> >  StreamTextureClientOGL::InitWith(gfx::SurfaceStream* aStream)
> >  {
> >    MOZ_ASSERT(!IsAllocated());
> >    mStream = aStream;
> > +  mGL = mStream->GLContext();
> 
> Why do we store the GLContext from the stream? Now there are two pieces of
> state to keep in sync. Why not use mStream->GLContext() where necessary?

We store the GLContext here so it is refcounted. The stream depends on it staying alive, but does not hold a ref itself (because the conventional ownership model is GLContext -> GLScreenBuffer -> SurfaceStream), so we need to hold one elsewhere.

Comment 36

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8373565 [details] [diff] [review]
Use a single GLContext for all SkiaGL canvases

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

I for one welcome our new refcounted SurfaceStream. At least it gives me with a mental model that fits in my small skull about why we might not be crashy when sharing a SurfaceStream between Child and Parent sides in the same-process-IPC case.

Comment 37

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #36)
> Comment on attachment 8373565 [details] [diff] [review]
> Use a single GLContext for all SkiaGL canvases
> 
> Review of attachment 8373565 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I for one welcome our new refcounted SurfaceStream. At least it gives me
> with a mental model that fits in my small skull about why we might not be
> crashy when sharing a SurfaceStream between Child and Parent sides in the
> same-process-IPC case.

We might be less likely to deref free'd memory, but we're much more likely to have issues with it being torn down on the wrong thread, which is also bad.

Comment 38

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jeff Gilbert [:jgilbert] from comment #37)
> (In reply to Benoit Jacob [:bjacob] from comment #36)
> > I for one welcome our new refcounted SurfaceStream. At least it gives me
> > with a mental model that fits in my small skull about why we might not be
> > crashy when sharing a SurfaceStream between Child and Parent sides in the
> > same-process-IPC case.
> 
> We might be less likely to deref free'd memory, but we're much more likely
> to have issues with it being torn down on the wrong thread, which is also
> bad.

Ah, right, I was losing sight of that other problem. But that one, we have a decent, already-used-elsewhere, way of solving: when the refcount hits zero, if that happens on the wrong thread, we can then send a message to the other thread to let it destroy things properly. If needed, te can even do that right when the refcount hits zero, before we call the destructor: see the AtomicRefCountedWithFinalize base class. This is how TextureClient/TextureHost do their destruction-on-the-right-thread dance.

Comment 39

[Benoit Jacob [:bjacob] (mostly away)]

... though of course, it's not going to be possible to do the destruction on the right thread if that thread is gone. It seems that sharing OpenGL resources between threads is a losing game anyhow. It also is a performance pitfall to have two threads access the same GL resources, anyway. We should definitely change that to just sharing underlying 'images' e.g. TextureClient's, and that would provide a much better solution to that problem, right?

Comment 40

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61cef3b533ba

Comment 41

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

\o/

Comment 42

[James Willcox (:snorp) ([e-mail]) (he/him)]

Follow up for build bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/1cedc759f877

Comment 43

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8373565 [details] [diff] [review]
Use a single GLContext for all SkiaGL canvases

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

You sort of already pushed this, but there isn't anything deadly-bad in here, that I saw. (Should just be potential rendering errors) This needs immediate follow-up fixes, though.

::: gfx/gl/SurfaceStream.cpp
@@ +63,5 @@
> +    if (!mProducer) {
> +        New(factory, src->Size(), mProducer);
> +    }
> +
> +    MOZ_ASSERT(mProducer);

mProducer might not be valid. It prooobably will be, but we should handle this.

You also probably want to assert that src->Size() == mProd->Size(), since that would catch a rendering issue I mentioned in one of the callers to this function.

This function will need to return bool success.

@@ +65,5 @@
> +    }
> +
> +    MOZ_ASSERT(mProducer);
> +
> +    mProducer->LockProd();

This lock/unlock isn't necessary. ShSurf::Copy does this for us.

::: gfx/layers/client/CanvasClient.cpp
@@ +115,5 @@
> +
> +  if (aLayer->mStream) {
> +    stream = aLayer->mStream;
> +    stream->CopySurfaceToProducer(aLayer->mTextureSurface, aLayer->mFactory);
> +    stream->SwapProducer(aLayer->mFactory, gfx::IntSize(aSize.width, aSize.height));

Oh, I think this might be a problem.
SwapProducer leaves a new mProd surface of the given size.
CopySurfaceToProducer copies from `src` to `mProd`

If a one context calls SwapProducer with sizeA, then another calls CopySurfaceToProducer with a different sizeB, we'll get a copy into a wrong-sized buffer. (And hopefully an assert in the Copy func in debug builds)

@@ +265,5 @@
> +    stream->CopySurfaceToProducer(aLayer->mTextureSurface, aLayer->mFactory);
> +    stream->SwapProducer(aLayer->mFactory, gfx::IntSize(aSize.width, aSize.height));
> +  } else {
> +    stream = screen->Stream();
> +  }

Can we merge this copy-pasta into a helper function?

::: gfx/layers/client/ClientCanvasLayer.cpp
@@ +94,5 @@
> +        if (!producer) {
> +          // Fallback to basic factory
> +          delete mFactory;
> +          mFactory = new SurfaceFactory_Basic(mGLContext, caps);
> +          producer = mStream->SwapProducer(mFactory, size);

SwapProducer can fail, generally just if size is too big. (but also randomly, potentially!) What's the failure plan here?

Comment 44

[(no longer active)]

Backed out because of build bustages: https://hg.mozilla.org/integration/mozilla-inbound/rev/91dea745dd4a

Comment 45

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8373565 [details] [diff] [review]
Use a single GLContext for all SkiaGL canvases

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

Looks good, but a few things to fix.  Overall, I'd like to see the removal of as many USE_SKIA_GPU as we can; see comment later.  Some tweaks to GetTextureID and comments should finish this off.

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +842,4 @@
>          DemoteOldestContextIfNecessary();
>  
> +#ifdef USE_SKIA_GPU
> +        SkiaGLGlue* glue = gfxPlatform::GetPlatform()->GetSkiaGLGlue();

GetSkiaGLGlueForCanvas(), so that we can have a separate one ForContent if we want to (or they can return the same thing, but let's decide that in gfxPlatform).

Also, this is a good place for USE_SKIA_GPU.  It would be even better if we could always compile in the SkiaGLGlue type, but GetSkiaGLGlue would return nullptr when we don't have USE_SKIA_GPU (then it can just go away here)

::: gfx/2d/2D.h
@@ +971,5 @@
>    }
> +
> +  virtual uint32_t GetTextureID() const {
> +    return 0;
> +  }

Don't add this.  We already have a GetNativeSurface, add a NATIVE_SURFACE_GL_TEXTURE_ID enum and use that.

::: gfx/2d/DrawTargetSkia.h
@@ +106,5 @@
>  #ifdef USE_SKIA_GPU
> +  void InitWithGrContext(GrContext* aGrContext,
> +                         const IntSize &aSize,
> +                         SurfaceFormat aFormat) MOZ_OVERRIDE;
> +  uint32_t GetTextureID() const { return mTexture; }

Same as 2D.h -- use GetNativeSurface instead.

::: gfx/gl/SharedSurfaceGL.cpp
@@ +283,5 @@
>                         gl,
>                         size,
>                         hasAlpha)
>      , mTex(tex)
> +    , mFB(gl, tex)

ScopedFramebufferForTexture doesn't do MakeCurrent(), and there's no promise that you're creating this object with the right context current.  I'm not really comfortable with a "Scoped*" object as a class member, really.  You may want to either write the code out long-form, or modify ScopedFramebufferForTexture to have an Init() function or something.

::: gfx/gl/SkiaGLGlue.h
@@ +23,5 @@
> +     * These members have inter-dependencies, but do not keep each other alive, so
> +     * destruction order is very important here: mGrContext uses mGrGLInterface, and
> +     * through it, uses mGLContext, so it is important that they be declared in the
> +     * present order.
> +     */

Maybe codify this in a destructor where you explicitly assign nullptr to things in the right order?

::: gfx/gl/SurfaceStream.h
@@ +123,5 @@
>                                          const gfx::IntSize& size) = 0;
>  
>      virtual SharedSurface* Resize(SurfaceFactory* factory, const gfx::IntSize& size);
>  
> +    virtual void CopySurfaceToProducer(SharedSurface* src, SurfaceFactory* factory) { MOZ_ASSERT(0); }

Uh, document what this method is supposed to do.  Not at all obvious!

::: gfx/layers/CopyableCanvasLayer.h
@@ +59,5 @@
>    nsRefPtr<gfxASurface> mSurface;
>    nsRefPtr<mozilla::gl::GLContext> mGLContext;
>    mozilla::RefPtr<mozilla::gfx::DrawTarget> mDrawTarget;
>  
> +  gfx::SurfaceStream* mStream;

Didn't you make this refcounted?  Is this explicitly non-ref-counted here?  Is that safe?

::: gfx/layers/Layers.h
@@ +1797,5 @@
>  
>      // Or this, for GL.
>      mozilla::gl::GLContext* mGLContext;
>  
> +#ifdef USE_SKIA_GPU

No reason for this to be USE_SKIA_GPU.

::: gfx/layers/client/CanvasClient.cpp
@@ +115,5 @@
> +
> +  if (aLayer->mStream) {
> +    stream = aLayer->mStream;
> +    stream->CopySurfaceToProducer(aLayer->mTextureSurface, aLayer->mFactory);
> +    stream->SwapProducer(aLayer->mFactory, gfx::IntSize(aSize.width, aSize.height));

this is a weird sequence of ops, but sure.  Some docs?

::: gfx/thebes/gfxPlatform.cpp
@@ +248,5 @@
>      NS_ASSERTION(strcmp(aTopic, "memory-pressure") == 0, "unexpected event topic");
>      Factory::PurgeAllCaches();
> +
> +#if USE_SKIA_GPU
> +    gfxPlatform::GetPlatform()->PurgeSkiaCache();

No USE_SKIA_GPU ifdef.  Make it a noop inside PurgeSkiaCache.

@@ +319,5 @@
>      mLayersUseDeprecated = false;
>  #endif
>  
> +#ifdef USE_SKIA_GPU
> +    mSkiaGlue = nullptr;

No USE_SKIA_GPU -- we can still have the glue type even if we have no GPU backend.

@@ +923,3 @@
>  {
>  #ifdef USE_SKIA_GPU
> +  bool usingDynamicCache = Preferences::GetBool("gfx.canvas.skiagl.dynamic-cache", false);

no need for USE_SKIA_GPU again

@@ +949,4 @@
>  #endif
>  }
>  
> +#ifdef USE_SKIA_GPU

USE_SKIA_GPU should go inside these functions, and they should always be defined.

::: gfx/thebes/gfxPlatform.h
@@ +45,5 @@
>  namespace gl {
>  class GLContext;
> +
> +#if USE_SKIA_GPU
> +class SkiaGLGlue;

Can we nuke as many of these USE_SKIA_GPU bits here and elsewhere?  If we need to define a dummy class or two in SkiaGLGlue.h, let's do that, but we should remove the sprinkling of these all over the place.  USE_SKIA_GPU is where we want to head to; we should rename the #define as WITHOUT_SKIA_GPU, and platforms that don't have it can #define that and put those guards around places where things are constructed (they can return null instead etc.).

I'm happy for this to happen in a followup patch (remove USE_SKIA_GPU define), but please make it happen.

@@ +634,5 @@
>      bool PreferMemoryOverShmem() const;
>      bool UseDeprecatedTextures() const { return mLayersUseDeprecated; }
>  
> +#ifdef USE_SKIA_GPU
> +    mozilla::gl::SkiaGLGlue* GetSkiaGLGlue();

GetSkiaGLGlueForCanvas(), so that we can have a separate one ForContent if we want to.

Comment 46

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Uh, thanks bugzilla -- I didn't mean to clear out anyone else's r+'s or r-'s on the previous patch.

Comment 47

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #44)
> Backed out because of build bustages:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/91dea745dd4a

Thanks, sorry about that.

Comment 48

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #45)
> Comment on attachment 8373565 [details] [diff] [review]
> Use a single GLContext for all SkiaGL canvases
> 
> Review of attachment 8373565 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but a few things to fix.  Overall, I'd like to see the removal
> of as many USE_SKIA_GPU as we can; see comment later.  Some tweaks to
> GetTextureID and comments should finish this off.
> 
> ::: content/canvas/src/CanvasRenderingContext2D.cpp
> @@ +842,4 @@
> >          DemoteOldestContextIfNecessary();
> >  
> > +#ifdef USE_SKIA_GPU
> > +        SkiaGLGlue* glue = gfxPlatform::GetPlatform()->GetSkiaGLGlue();
> 
> GetSkiaGLGlueForCanvas(), so that we can have a separate one ForContent if
> we want to (or they can return the same thing, but let's decide that in
> gfxPlatform).

I can't think of any reason we would need to have separate ones. The only change I think we'll want here is to return a different one for each thread. Content won't need anything different, I don't think.

> 
> Also, this is a good place for USE_SKIA_GPU.  It would be even better if we
> could always compile in the SkiaGLGlue type, but GetSkiaGLGlue would return
> nullptr when we don't have USE_SKIA_GPU (then it can just go away here)

Yeah...

> 
> ::: gfx/2d/2D.h
> @@ +971,5 @@
> >    }
> > +
> > +  virtual uint32_t GetTextureID() const {
> > +    return 0;
> > +  }
> 
> Don't add this.  We already have a GetNativeSurface, add a
> NATIVE_SURFACE_GL_TEXTURE_ID enum and use that.

Oh, didn't notice that, thanks.

> 
> ::: gfx/2d/DrawTargetSkia.h
> @@ +106,5 @@
> >  #ifdef USE_SKIA_GPU
> > +  void InitWithGrContext(GrContext* aGrContext,
> > +                         const IntSize &aSize,
> > +                         SurfaceFormat aFormat) MOZ_OVERRIDE;
> > +  uint32_t GetTextureID() const { return mTexture; }
> 
> Same as 2D.h -- use GetNativeSurface instead.

Yup

> 
> ::: gfx/gl/SharedSurfaceGL.cpp
> @@ +283,5 @@
> >                         gl,
> >                         size,
> >                         hasAlpha)
> >      , mTex(tex)
> > +    , mFB(gl, tex)
> 
> ScopedFramebufferForTexture doesn't do MakeCurrent(), and there's no promise
> that you're creating this object with the right context current.

Indeed, fixed.

>  I'm not
> really comfortable with a "Scoped*" object as a class member, really.  You
> may want to either write the code out long-form, or modify
> ScopedFramebufferForTexture to have an Init() function or something.

Why?

> 
> ::: gfx/gl/SkiaGLGlue.h
> @@ +23,5 @@
> > +     * These members have inter-dependencies, but do not keep each other alive, so
> > +     * destruction order is very important here: mGrContext uses mGrGLInterface, and
> > +     * through it, uses mGLContext, so it is important that they be declared in the
> > +     * present order.
> > +     */
> 
> Maybe codify this in a destructor where you explicitly assign nullptr to
> things in the right order?

That makes sense.

> 
> ::: gfx/gl/SurfaceStream.h
> @@ +123,5 @@
> >                                          const gfx::IntSize& size) = 0;
> >  
> >      virtual SharedSurface* Resize(SurfaceFactory* factory, const gfx::IntSize& size);
> >  
> > +    virtual void CopySurfaceToProducer(SharedSurface* src, SurfaceFactory* factory) { MOZ_ASSERT(0); }
> 
> Uh, document what this method is supposed to do.  Not at all obvious!

It copies the passed-in surface to the producer? How is that not obvious? But more docs can't be bad...

> 
> ::: gfx/layers/CopyableCanvasLayer.h
> @@ +59,5 @@
> >    nsRefPtr<gfxASurface> mSurface;
> >    nsRefPtr<mozilla::gl::GLContext> mGLContext;
> >    mozilla::RefPtr<mozilla::gfx::DrawTarget> mDrawTarget;
> >  
> > +  gfx::SurfaceStream* mStream;
> 
> Didn't you make this refcounted?  Is this explicitly non-ref-counted here? 
> Is that safe?

I did, and that should be refcounted.

> 
> ::: gfx/layers/Layers.h
> @@ +1797,5 @@
> >  
> >      // Or this, for GL.
> >      mozilla::gl::GLContext* mGLContext;
> >  
> > +#ifdef USE_SKIA_GPU
> 
> No reason for this to be USE_SKIA_GPU.

Indeed

> 
> ::: gfx/layers/client/CanvasClient.cpp
> @@ +115,5 @@
> > +
> > +  if (aLayer->mStream) {
> > +    stream = aLayer->mStream;
> > +    stream->CopySurfaceToProducer(aLayer->mTextureSurface, aLayer->mFactory);
> > +    stream->SwapProducer(aLayer->mFactory, gfx::IntSize(aSize.width, aSize.height));
> 
> this is a weird sequence of ops, but sure.  Some docs?

What's weird about it? We copy our current output to the producer and then swap in a new one for the next "frame".

> 
> ::: gfx/thebes/gfxPlatform.cpp
> @@ +248,5 @@
> >      NS_ASSERTION(strcmp(aTopic, "memory-pressure") == 0, "unexpected event topic");
> >      Factory::PurgeAllCaches();
> > +
> > +#if USE_SKIA_GPU
> > +    gfxPlatform::GetPlatform()->PurgeSkiaCache();
> 
> No USE_SKIA_GPU ifdef.  Make it a noop inside PurgeSkiaCache.

Yeah, good

> 
> @@ +319,5 @@
> >      mLayersUseDeprecated = false;
> >  #endif
> >  
> > +#ifdef USE_SKIA_GPU
> > +    mSkiaGlue = nullptr;
> 
> No USE_SKIA_GPU -- we can still have the glue type even if we have no GPU
> backend.

Sure

> 
> @@ +923,3 @@
> >  {
> >  #ifdef USE_SKIA_GPU
> > +  bool usingDynamicCache = Preferences::GetBool("gfx.canvas.skiagl.dynamic-cache", false);
> 
> no need for USE_SKIA_GPU again
> 
> @@ +949,4 @@
> >  #endif
> >  }
> >  
> > +#ifdef USE_SKIA_GPU
> 
> USE_SKIA_GPU should go inside these functions, and they should always be
> defined.
> 
> ::: gfx/thebes/gfxPlatform.h
> @@ +45,5 @@
> >  namespace gl {
> >  class GLContext;
> > +
> > +#if USE_SKIA_GPU
> > +class SkiaGLGlue;
> 
> Can we nuke as many of these USE_SKIA_GPU bits here and elsewhere?  If we
> need to define a dummy class or two in SkiaGLGlue.h, let's do that, but we
> should remove the sprinkling of these all over the place.  USE_SKIA_GPU is
> where we want to head to; we should rename the #define as WITHOUT_SKIA_GPU,
> and platforms that don't have it can #define that and put those guards
> around places where things are constructed (they can return null instead
> etc.).
> 
> I'm happy for this to happen in a followup patch (remove USE_SKIA_GPU
> define), but please make it happen.

Yeah, let's try to clean that up in a followup.

> 
> @@ +634,5 @@
> >      bool PreferMemoryOverShmem() const;
> >      bool UseDeprecatedTextures() const { return mLayersUseDeprecated; }
> >  
> > +#ifdef USE_SKIA_GPU
> > +    mozilla::gl::SkiaGLGlue* GetSkiaGLGlue();
> 
> GetSkiaGLGlueForCanvas(), so that we can have a separate one ForContent if
> we want to.

Again, I just don't see the need for this. What do you foresee here? A different GLContext for some reason?

Comment 49

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Use a single GLContext for all SkiaGL canvases

I think I got most of the requested changes. I filed bug 974040 to take care of any remaining ifdef hell.

Comment 50

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8377742 [details] [diff] [review]
Use a single GLContext for all SkiaGL canvases

Please address my review comments. I believe I found a bug in your previous patch that is still present here.

Comment 51

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jeff Gilbert [:jgilbert] from comment #43)
> Comment on attachment 8373565 [details] [diff] [review]
> Use a single GLContext for all SkiaGL canvases
> 
> Review of attachment 8373565 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You sort of already pushed this, but there isn't anything deadly-bad in
> here, that I saw. (Should just be potential rendering errors) This needs
> immediate follow-up fixes, though.

Sorry, I sort of forgot about this review!

> 
> ::: gfx/gl/SurfaceStream.cpp
> @@ +63,5 @@
> > +    if (!mProducer) {
> > +        New(factory, src->Size(), mProducer);
> > +    }
> > +
> > +    MOZ_ASSERT(mProducer);
> 
> mProducer might not be valid. It prooobably will be, but we should handle
> this.
> 
> You also probably want to assert that src->Size() == mProd->Size(), since
> that would catch a rendering issue I mentioned in one of the callers to this
> function.

Ok, added the assert for the size and returned false if don't have a producer.

> 
> This function will need to return bool success.
> 
> @@ +65,5 @@
> > +    }
> > +
> > +    MOZ_ASSERT(mProducer);
> > +
> > +    mProducer->LockProd();
> 
> This lock/unlock isn't necessary. ShSurf::Copy does this for us.

I ran into a problem early on where it was not, and I don't see where it does this for a texture-to-texture copy.

> 
> ::: gfx/layers/client/CanvasClient.cpp
> @@ +115,5 @@
> > +
> > +  if (aLayer->mStream) {
> > +    stream = aLayer->mStream;
> > +    stream->CopySurfaceToProducer(aLayer->mTextureSurface, aLayer->mFactory);
> > +    stream->SwapProducer(aLayer->mFactory, gfx::IntSize(aSize.width, aSize.height));
> 
> Oh, I think this might be a problem.
> SwapProducer leaves a new mProd surface of the given size.
> CopySurfaceToProducer copies from `src` to `mProd`
> 
> If a one context calls SwapProducer with sizeA, then another calls
> CopySurfaceToProducer with a different sizeB, we'll get a copy into a
> wrong-sized buffer. (And hopefully an assert in the Copy func in debug
> builds)

This does seem bad. The thing to do here might be to let CopySurfaceToProducer take care of calling SwapProducer, so we can be assured the sizes match. In practice with canvas this isn't going to matter, though, because we throw away the stream and DT if the size changes.

> 
> @@ +265,5 @@
> > +    stream->CopySurfaceToProducer(aLayer->mTextureSurface, aLayer->mFactory);
> > +    stream->SwapProducer(aLayer->mFactory, gfx::IntSize(aSize.width, aSize.height));
> > +  } else {
> > +    stream = screen->Stream();
> > +  }
> 
> Can we merge this copy-pasta into a helper function?

I see this twice, but the other one is just for the deprecated version...doesn't really seem worth adding a helper for that.

> 
> ::: gfx/layers/client/ClientCanvasLayer.cpp
> @@ +94,5 @@
> > +        if (!producer) {
> > +          // Fallback to basic factory
> > +          delete mFactory;
> > +          mFactory = new SurfaceFactory_Basic(mGLContext, caps);
> > +          producer = mStream->SwapProducer(mFactory, size);
> 
> SwapProducer can fail, generally just if size is too big. (but also
> randomly, potentially!) What's the failure plan here?

Well I think we want it to assert in debug builds (as it does here), but with the other changes you suggested it won't crash or anything. There just won't be anything on the screen.

Comment 52

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #51)
> > ::: gfx/layers/client/ClientCanvasLayer.cpp
> > @@ +94,5 @@
> > > +        if (!producer) {
> > > +          // Fallback to basic factory
> > > +          delete mFactory;
> > > +          mFactory = new SurfaceFactory_Basic(mGLContext, caps);
> > > +          producer = mStream->SwapProducer(mFactory, size);
> > 
> > SwapProducer can fail, generally just if size is too big. (but also
> > randomly, potentially!) What's the failure plan here?
> 
> Well I think we want it to assert in debug builds (as it does here), but
> with the other changes you suggested it won't crash or anything. There just
> won't be anything on the screen.

Actually I think we probably want to fallback to a software canvas if possible. There isn't any machinery to do that right now, I think that would need appropriate for a followup patch.

Comment 53

[Kelsey Gilbert [:jgilbert]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #51)
> > 
> > @@ +65,5 @@
> > > +    }
> > > +
> > > +    MOZ_ASSERT(mProducer);
> > > +
> > > +    mProducer->LockProd();
> > 
> > This lock/unlock isn't necessary. ShSurf::Copy does this for us.
> 
> I ran into a problem early on where it was not, and I don't see where it
> does this for a texture-to-texture copy.

We should fix this then. We should try to have these functions handle Lock/Unlock for us.

Comment 54

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Use a single GLContext for all SkiaGL canvases

Comment 55

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8377901 [details] [diff] [review]
Use a single GLContext for all SkiaGL canvases

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

::: gfx/gl/SurfaceStream.cpp
@@ +66,5 @@
> +            return false;
> +        }
> +    }
> +
> +    MOZ_ASSERT(src->Size() == mProducer->Size(), "Size mismatch");

Let's see a try run with this, before we r+ it.
It should also not assert when running a demo with two canvases of different sizes, which both produce each rAF.

Comment 56

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Use a single GLContext for all SkiaGL canvases

Alright, I think I have all the remaining issues worked out now.

Comment 57

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8378761 [details] [diff] [review]
Use a single GLContext for all SkiaGL canvases

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

Woot!

::: gfx/2d/DrawTargetSkia.cpp
@@ +756,5 @@
> +  if (aType != NativeSurfaceType::OPENGL_TEXTURE) {
> +    return nullptr;
> +  }
> +
> +  return (void*)((uint64_t)mTexture);

do me a favor and write this the opposite way?  that is,

  if (aType == OPENGL_TEXTURE) {
    return (void*)((uintptr_t) mTexture);
  }

  return nullptr;

to make it easier to add any other bits if needed, and to make it more obvious what's being returned for what.  Also note the usage of uintptr_t; I'm pretty sure we'll have that type, and it's a more valid cast.

Comment 58

[James Willcox (:snorp) ([e-mail]) (he/him)]

Landed with Vlad's changes and the prior r+s

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

I think there is still something wonky about the SharedSurface locking, but it looks ok on Try right now.

Comment 59

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/bde06c79829f for crashing webgl reftests on at least OSX 10.6 and 10.8.

Comment 60

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

(In reply to Wes Kocher (:KWierso) from comment #59)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/bde06c79829f for
> crashing webgl reftests on at least OSX 10.6 and 10.8.

I'm also going to say that this mochitest-1 failure is this patch's fault: https://tbpl.mozilla.org/php/getParsedLog.php?id=35001368&tree=Mozilla-Inbound

Comment 61

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

And browser-chrome bustage for good measure: https://tbpl.mozilla.org/php/getParsedLog.php?id=35003254&tree=Mozilla-Inbound

Comment 62

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Use a single GLContext for all SkiaGL canvases

A few more final tweaks. Try run here: https://tbpl.mozilla.org/?tree=Try&rev=46a40b900c24

Comment 63

[James Willcox (:snorp) ([e-mail]) (he/him)]

This cleaned up some stuff in ClientCanvasLayer::Initialize and fixed the SharedSurface usage in light of bug 956993.

Comment 64

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8385028 [details] [diff] [review]
Use a single GLContext for all SkiaGL canvases

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

R+, unless this assert will not give us coverage. I'm still pretty sure we should be hitting this assert on any page with multiple different-sized canvases.

::: gfx/gl/SurfaceStream.cpp
@@ +63,5 @@
> +            return false;
> +        }
> +    }
> +
> +    MOZ_ASSERT(src->Size() == mProducer->Size(), "Size mismatch");

[r+ contingent] Desktop paths are going to hit this assert, right? Not just mobile? Because right now asserts that are android/b2g-only basically are ignored by our integration tests.

Comment 65

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jeff Gilbert [:jgilbert] from comment #64)
> Comment on attachment 8385028 [details] [diff] [review]
> Use a single GLContext for all SkiaGL canvases
> 
> Review of attachment 8385028 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> R+, unless this assert will not give us coverage. I'm still pretty sure we
> should be hitting this assert on any page with multiple different-sized
> canvases.

I don't understand why you think this should assert. Each canvas has its own SurfaceStream, and we recreate the stream (and DrawTarget, etc) whenever the canvas is resized.

> 
> ::: gfx/gl/SurfaceStream.cpp
> @@ +63,5 @@
> > +            return false;
> > +        }
> > +    }
> > +
> > +    MOZ_ASSERT(src->Size() == mProducer->Size(), "Size mismatch");
> 
> [r+ contingent] Desktop paths are going to hit this assert, right? Not just
> mobile? Because right now asserts that are android/b2g-only basically are
> ignored by our integration tests.

We have asserts that are somehow mobile-only? I didn't know that. This should be hit everywhere.

Comment 66

[Kelsey Gilbert [:jgilbert]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #65)
> (In reply to Jeff Gilbert [:jgilbert] from comment #64)
> > Comment on attachment 8385028 [details] [diff] [review]
> > Use a single GLContext for all SkiaGL canvases
> > 
> > Review of attachment 8385028 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > R+, unless this assert will not give us coverage. I'm still pretty sure we
> > should be hitting this assert on any page with multiple different-sized
> > canvases.
> 
> I don't understand why you think this should assert. Each canvas has its own
> SurfaceStream, and we recreate the stream (and DrawTarget, etc) whenever the
> canvas is resized.
Right, right, I was still thinking in terms of one SurfStream per GLContext.
> 
> > 
> > ::: gfx/gl/SurfaceStream.cpp
> > @@ +63,5 @@
> > > +            return false;
> > > +        }
> > > +    }
> > > +
> > > +    MOZ_ASSERT(src->Size() == mProducer->Size(), "Size mismatch");
> > 
> > [r+ contingent] Desktop paths are going to hit this assert, right? Not just
> > mobile? Because right now asserts that are android/b2g-only basically are
> > ignored by our integration tests.
> 
> We have asserts that are somehow mobile-only? I didn't know that. This
> should be hit everywhere.
Not on purpose, but we basically have no DEBUG android/b2g test coverage for a lot of tests. If we don't run tests that hit these asserts on DEBUG builds, then we won't know if they're actually busted.

Do we know which TBPL jobs run canvas tests?

I'm working on reviving some tests from Ceder, but it's slow going since I can't fix some of the things that are wrong myself.

Comment 67

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jeff Gilbert [:jgilbert] from comment #66)
> > We have asserts that are somehow mobile-only? I didn't know that. This
> > should be hit everywhere.
> Not on purpose, but we basically have no DEBUG android/b2g test coverage for
> a lot of tests. If we don't run tests that hit these asserts on DEBUG
> builds, then we won't know if they're actually busted.

Ah, yeah. It's not a good situation.

> 
> Do we know which TBPL jobs run canvas tests?

The canvas mochitests and reftests are the main ones, but a lot of other tests use canvas as a utility.

> 
> I'm working on reviving some tests from Ceder, but it's slow going since I
> can't fix some of the things that are wrong myself.

Cool. I think Ateam was maybe working on turning on debug tests for one of the Android platforms, but I could be wrong.

Comment 68

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2dda16c0a398

Comment 69

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2dda16c0a398