942503 - Move Blit* helpers out of GLContext

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

This piece of GLContext.h:

    void BlitFramebufferToFramebuffer(GLuint srcFB, GLuint destFB,
                                      const gfxIntSize& srcSize,
                                      const gfxIntSize& destSize);
    void BlitFramebufferToFramebuffer(GLuint srcFB, GLuint destFB,
                                      const gfxIntSize& srcSize,
                                      const gfxIntSize& destSize,
                                      const GLFormats& srcFormats);
    void BlitTextureToFramebuffer(GLuint srcTex, GLuint destFB,
                                  const gfxIntSize& srcSize,
                                  const gfxIntSize& destSize,
                                  GLenum srcTarget = LOCAL_GL_TEXTURE_2D);
    void BlitFramebufferToTexture(GLuint srcFB, GLuint destTex,
                                  const gfxIntSize& srcSize,
                                  const gfxIntSize& destSize,
                                  GLenum destTarget = LOCAL_GL_TEXTURE_2D);
    void BlitTextureToTexture(GLuint srcTex, GLuint destTex,
                              const gfxIntSize& srcSize,
                              const gfxIntSize& destSize,
                              GLenum srcTarget = LOCAL_GL_TEXTURE_2D,
                              GLenum destTarget = LOCAL_GL_TEXTURE_2D);

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

Comment on attachment 8337397 [details] [diff] [review]
patch

Want to take a look? I still have to try-server this.

Comment 3

[Andreas Gal :gal]

Tree closed. Can't try for now. Ugh.

Comment 4

[Andreas Gal :gal]

https://tbpl.mozilla.org/?tree=Try&rev=8ddb0e9b9971

Comment 5

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

Comment on attachment 8337397 [details] [diff] [review]
patch

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

This is definitely going in the right direction whence Feedback+. But I'll defer the actual reviewing to Jeff Gilbert since SharedSurfaceGL is his baby and he might have more precise ideas about it than I have.

I wonder if we want these helpers to be static methods in SharedSurfaceGL, like your current patch does, or be global methods, perhaps living in their own file. No strong opinion either way.

::: gfx/gl/SharedSurfaceGL.h
@@ +60,1 @@
>      GLContext* const mGL;

You may not want to complicate the review process here by adding new nontrivial future-design comments on existing unchanged code ;-)

Comment 6

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

The latter review comment refers to this:

+    // I don't think this is safe or sane, but I would prefer to fix this via
+    // 1-context-per-thread.
     GLContext* const mGL;

Comment 7

[Andreas Gal :gal]

(In reply to Benoit Jacob [:bjacob] from comment #6)
> The latter review comment refers to this:
> 
> +    // I don't think this is safe or sane, but I would prefer to fix this
> via
> +    // 1-context-per-thread.
>      GLContext* const mGL;

Yeah happy to delete the comment. Just saying that this doesn't hold a reference and is very likely memory-unsafe in shutdown situations. I prefer not fixing this right now. I proposed how I would fix it at some point :)

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8337397 [details] [diff] [review]
patch

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

This patch cleans this code up somewhat, but I'm not convinced that we should actually remove it from GLContext. Basically, this blit code is providing helper functionality to GLContext, with a non-trivial initialization cost which is done lazily if desired functionality requires it, and then lasts for the lifetime of the context. (Since we likely will only do this never, or multiple times per GLContext)

It seems like it'd be cleanest to encapsulate this functionality in a not-strictly-necessary struct, to facilitate code cleanliness. (But still hang the lifetime off of GLContext, not a shorter-lived SharedSurface)
Basically, use a lazily-initialized GLContext::mBlitProgram.

Asude: It also seems like, ideally, we could add a callback mechanism for OnGLContextDestroy which we could use to free our objects from outside of GLContext itself. However, this is probably not useful complexity unless we identify other places that would benefit from this.

::: gfx/gl/SharedSurfaceGL.h
@@ +55,5 @@
>      typedef gfx::APITypeT APITypeT;
>      typedef gfx::AttachmentType AttachmentType;
>  
> +    // I don't think this is safe or sane, but I would prefer to fix this via
> +    // 1-context-per-thread.

Ideally we should formalize the lifetime assumptions we make with strong and weak pointers, but I haven't had a chance to do this yet. This code is not expected to function if GLContext collapses before SharedSurfaces are destroyed. SharedSurfaces are owned (indirectly) by GLContext, so it's not possible for SharedSurfaces to outlive their contexts.

@@ +80,5 @@
> +                                         const gfxIntSize& srcSize,
> +                                         const gfxIntSize& destSize,
> +                                         GLenum srcTarget);
> +
> +    BlitProgram mBlit;

I think it's better to attach this to GLContext, since it's already lazily evaluated. If we don't want to do this, we should probably warn if we do this more than once per GLContext. (Otherwise we might tickle this performance cliff where we have to recompile these programs each time we run this, more or less.

@@ +118,5 @@
>      GLContext* GL() const {
>          return mGL;
>      }
> +
> +    BlitProgram* Blit() {

Why not just pass this around by reference?

Comment 9

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

(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> Comment on attachment 8337397 [details] [diff] [review]
> patch
> 
> Review of attachment 8337397 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch cleans this code up somewhat, but I'm not convinced that we
> should actually remove it from GLContext. Basically, this blit code is
> providing helper functionality to GLContext, with a non-trivial
> initialization cost which is done lazily if desired functionality requires
> it, and then lasts for the lifetime of the context. (Since we likely will
> only do this never, or multiple times per GLContext)
> 
> It seems like it'd be cleanest to encapsulate this functionality in a
> not-strictly-necessary struct, to facilitate code cleanliness. (But still
> hang the lifetime off of GLContext, not a shorter-lived SharedSurface)
> Basically, use a lazily-initialized GLContext::mBlitProgram.
> 
> Asude: It also seems like, ideally, we could add a callback mechanism for
> OnGLContextDestroy which we could use to free our objects from outside of
> GLContext itself. However, this is probably not useful complexity unless we
> identify other places that would benefit from this.

I feel strongly in favor of making GLContext be strictly an abstraction for an OpenGL context with GL entry points, and removing this kind of helpers from GLContext, even if they are useful and even if it's convenient lifetime-wise to have them tied somehow to GLContext. My reason for feeling strongly this way is that while taken individually this kind of helper looks sane and justified, they are collectively making GLContext very complicated (see all the blockers to bug 942491). Having a single class (here GLContext) play many different roles is a big anti-pattern, and here it's very concretely preventing us from easily implementing useful features (e.g. GLContext multiplexing to name just one thing that bug 901498 aims to make easy to implement).

Now the nontrivial initialization and lifetime issue that you mention is interesting, but since this seems to be only used by one user (SharedSurfaceGL) at the moment, I think that it's fair to move it to either SharedSurfaceGL or something managed by SharedSurfaceGL, and if and when another unrelated user also wants to use this helper, it will be their job to figure how to make it work. I prefer that over the present situation which is making GLContext more complicated, which is a cost today, without certainty that it's going to be needed by more than just SharedSurfaceGL.

For these reasons, I really believe that not only this should be taken out of GLContext, but GLContext shouldn't have any reference to this helper. As soon as GLContext has any reference to this helper (like OnGLContextDestroy), GLContext is already playing a new role (as "manager" of this helper).

> 
> ::: gfx/gl/SharedSurfaceGL.h
> @@ +55,5 @@
> >      typedef gfx::APITypeT APITypeT;
> >      typedef gfx::AttachmentType AttachmentType;
> >  
> > +    // I don't think this is safe or sane, but I would prefer to fix this via
> > +    // 1-context-per-thread.
> 
> Ideally we should formalize the lifetime assumptions we make with strong and
> weak pointers, but I haven't had a chance to do this yet. This code is not
> expected to function if GLContext collapses before SharedSurfaces are
> destroyed. SharedSurfaces are owned (indirectly) by GLContext, so it's not
> possible for SharedSurfaces to outlive their contexts.
> 
> @@ +80,5 @@
> > +                                         const gfxIntSize& srcSize,
> > +                                         const gfxIntSize& destSize,
> > +                                         GLenum srcTarget);
> > +
> > +    BlitProgram mBlit;
> 
> I think it's better to attach this to GLContext,

I disagree for the same reason as above.

Comment 10

[Andreas Gal :gal]

I would say whoever grabs this gets to decide the design. Benoit, Jeff, either of you please go for this. Both arguments seem sound and I don't have a strong opinion either way.

Comment 11

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

Had a fruitful phone call with Jeff today; we now agree on an approach here and in other similar GLContext-cleanup bugs.

The difficulty is that we want to reconcile two different constraints:
 - (My complaint) We want to take as much as possible out of GLContext
 - (Jeff's complaint) But we also want to align the lifespan of certain helpers, like the present one, to that of the GLContext, so we don't recreate them too often. The present helper in particular is particularly expensive to recreate.

The short-term compromise we agreed on was to take this helper to a separate class, but still have an instance of that class managed by the GLContext instance. So the number of lines of code in GLContext incurred by that helper doesn't fall down to zero, it stays as 'a few', basically a RefPtr<BlitHelper> mBlitHelper; and the strict minimum around it to create it and get it. All nontrivial code is moved to that BlitHelper class.

In the longer term the plan outlined in bug 901498 will allow to take this BlitHelper reference out of the core GLContext class and into a 'wrapper' as described there.

The short-term solution that we're going to do here is still a good step in the right direction.

Comment 12

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

Alright, so there are two separate parts here:
 - the Blit* functions considered above; and
 - BlitTextureImage, which is a wholly separate thing and uses separate code, shaders etc. (yes, that is bad, but not necessarily worth fixing as BlitTextureImage doesn't seem to be used with new-textures, so the port to new-textures (bug 893300) will take care of it).

Let's do both here, but as separate patches, moving them to separate helper classes.

The latter depends on bug 944172.

Comment 13

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

Attachment: Move Blit* functions out of GLContext, except for BlitTextureImage

Comment 14

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

Attachment: Move BlitTextureImage out of GLContext

Comment 15

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8339608 [details] [diff] [review]
Move Blit* functions out of GLContext, except for BlitTextureImage

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

::: gfx/gl/GLBlitHelper.cpp
@@ +474,5 @@
> +    ScopedBindFramebuffer boundFB(mGL, srcFB);
> +    ScopedGLState scissor(mGL, LOCAL_GL_SCISSOR_TEST, false);
> +
> +    mGL->fCopyTexSubImage2D(destTarget, 0,
> +                       0, 0,

indent mismatches here, and elsewhere in this file.

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8339610 [details] [diff] [review]
Move BlitTextureImage out of GLContext

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

I'm going to assume that GLBlitTextureImageHelper is a straight copy/paste of the relevant code.

Comment 17

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

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

Comment 18

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

another try for windows:

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

Comment 19

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

another one for windows:

https://tbpl.mozilla.org/?tree=Try&rev=203e9079293b

Comment 20

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/843b0a943af7
http://hg.mozilla.org/integration/mozilla-inbound/rev/1b0fc4fd9a75

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/843b0a943af7
https://hg.mozilla.org/mozilla-central/rev/1b0fc4fd9a75