Move everything GraphicsFilter-related out of GLContext

RESOLVED FIXED in mozilla29

Status

()

Core
Graphics
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bjacob, Assigned: kamidphish)

Tracking

Trunk
mozilla29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 6 obsolete attachments)

This part of GLContext.h:

    /**
     * Applies aFilter to the texture currently bound to GL_TEXTURE_2D.
     */
    void ApplyFilterToBoundTexture(GraphicsFilter aFilter);

    /**
     * Applies aFilter to the texture currently bound to aTarget.
     */
    void ApplyFilterToBoundTexture(GLuint aTarget,
                                   GraphicsFilter aFilter);

Comment 1

4 years ago
I think this should wait until we have deleted the old layers code and then this can go directly into CompositorOGL.cpp
(Reporter)

Comment 2

4 years ago
OK, that's reasonable given that there is no urgency here. If you know the bug number for deleting old-layers code, please make it block this.
(Assignee)

Comment 3

4 years ago
Created attachment 8342155 [details] [diff] [review]
Remove ApplyFilterToBoundTexture from GLContext. Move it to CompositorOGL.

Extracted ApplyFilterToBoundTexture from GLContext. As :gal suggested, moved
the implementation into CompositorOGL.cpp.

As a bonus, refactored the old users for ApplyFilterToBoundTetxure. This
resulted in the removal of:
 * TextureImage::ApplyFilter
 * TextureImage::ScopedBindTextureAndApplyFilter
 * TextureImage::ScopedBindTexure
 * TextureImage::BindTextureAndApplyFilter
 * TextureImage::ReleaseTexture, and
 * TextureSourceOGL::UnbindTexture

AutoBindTexture was refactored to use code from GLScopedHelper.

Bonus: Cleaned up whitespace in files that were modified.
Attachment #8342155 - Flags: review?(bjacob)
(Assignee)

Updated

4 years ago
Assignee: nobody → dglastonbury
Status: NEW → ASSIGNED
(Assignee)

Comment 4

4 years ago
I'm copying the tryserver flags from :bjacob's other bugs. I'm not certain which ones I need.

https://tbpl.mozilla.org/?tree=Try&rev=a60d4c3530cb
(Reporter)

Comment 5

4 years ago
Comment on attachment 8342155 [details] [diff] [review]
Remove ApplyFilterToBoundTexture from GLContext. Move it to CompositorOGL.

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

I have some questions. I'll flag r- as a means of keep track of which patches I've already reviewed. Nothing bad here though.

When pushing this to try, make sure to get 'reftest' runs.

::: gfx/gl/GLBlitTextureImageHelper.cpp
@@ +143,5 @@
>              }
>  
> +            ScopedBindTextureUnit autoTexUnit(mGL, LOCAL_GL_TEXTURE0);
> +            ScopedBindTexture autoTex(mGL, 0);
> +            aSrc->BindTexture(LOCAL_GL_TEXTURE0);

Hm, that is a little bit convoluted. I wonder if you could instead do

ScopedBindTexture autoTex(mGL, aSrc->GetTextureID());

?

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +990,5 @@
> +  GLenum mOldTexUnit;
> +  GLuint mOldTexId;
> +
> +private:
> +  void SaveState(GLenum aTexUnit)

This function does not just save state, it also changes the active texture unit. Could you give it a more descriptive name, of take the state-change part out of it?
Attachment #8342155 - Flags: review?(bjacob) → review-
(Assignee)

Comment 6

4 years ago
(In reply to Benoit Jacob [:bjacob] from comment #5)
> Comment on attachment 8342155 [details] [diff] [review]
> Remove ApplyFilterToBoundTexture from GLContext. Move it to CompositorOGL.
> 
> Review of attachment 8342155 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> When pushing this to try, make sure to get 'reftest' runs.
> 
> ::: gfx/gl/GLBlitTextureImageHelper.cpp
> @@ +143,5 @@
> > +            ScopedBindTextureUnit autoTexUnit(mGL, LOCAL_GL_TEXTURE0);
> > +            ScopedBindTexture autoTex(mGL, 0);
> > +            aSrc->BindTexture(LOCAL_GL_TEXTURE0);
> 
> Hm, that is a little bit convoluted. I wonder if you could instead do
> 
> ScopedBindTexture autoTex(mGL, aSrc->GetTextureID());

TextureSourceOGL doesn't export GetTextureID(). (Well it certainly didn't when I was porting the LayerScope patches.)

This is because some implementations don't have an id to they own to return, eg. one grabs a temporary id from the compositor.

I was kinda being lazy. I toyed with the idea of either extending ScopedBindTexture to have a constructor that takes TextureSourceOGL or create another class, such as ScopedBindTextureSource. (I don't like to contribute to the proliferation of Poltergeist classes)

> ::: gfx/layers/opengl/CompositorOGL.cpp
> @@ +990,5 @@
> > +private:
> > +  void SaveState(GLenum aTexUnit)
> 
> This function does not just save state, it also changes the active texture
> unit. Could you give it a more descriptive name, of take the state-change
> part out of it?

This was originally called Init(), like in the ScopedGLHelpers. How about Setup?
(Assignee)

Comment 7

4 years ago
Created attachment 8342770 [details] [diff] [review]
Remove ApplyFilterToBoundTexture from GLContext. Move it to CompositorOGL.

Rebased the patch.
(Assignee)

Updated

4 years ago
Attachment #8342155 - Attachment is obsolete: true
(Assignee)

Comment 8

4 years ago
Try with reftest: https://tbpl.mozilla.org/?tree=Try&rev=3091dd5c1fd6
(Reporter)

Comment 9

4 years ago
Sorry, I didn't realize that those were TextureSourceOGL. Please ignore that comment then.

But let me ask: how could this code be made more straightforward? Maybe if we have to have these Scoped* helpers in such a convoluted way (thinking about ScopedBindTexture autoTex(mGL, 0); ) then we're better off not using them and 1) either just making GL calls or 2) making a custom Scoped helper for what we're doing here?

Not much of an opinion on naming, Init would be fine, I guess.
(Assignee)

Comment 10

4 years ago
(In reply to Benoit Jacob [:bjacob] from comment #9)
> Sorry, I didn't realize that those were TextureSourceOGL. Please ignore that
> comment then.
> 
> But let me ask: how could this code be made more straightforward? Maybe if
> we have to have these Scoped* helpers in such a convoluted way (thinking
> about ScopedBindTexture autoTex(mGL, 0); ) then we're better off not using
> them and 1) either just making GL calls or 2) making a custom Scoped helper
> for what we're doing here?
> 
> Not much of an opinion on naming, Init would be fine, I guess.

There appears that there are various little subtleties in the helpers. I was using this one to save the current texture, pushing a dummy 0, which is then overridden by the next call to ->BindTexture().  I think the "correct" way is to use a proper helper class, which is AutoBindTexture that exists in this patch. (I should stop being lazy ;-) )
(Assignee)

Comment 11

4 years ago
(In reply to Benoit Jacob [:bjacob] from comment #9)
> Sorry, I didn't realize that those were TextureSourceOGL. Please ignore that
> comment then.

I'm sorry, Benoit. I'm such an idiot. You were correct. aSrc is a TextureImage not TextureSourceOGL.
(Assignee)

Comment 12

4 years ago
Created attachment 8342841 [details] [diff] [review]
Remove ApplyFilterToBoundTexture from GLContext.Move it to CompositorOGL.

Apply Benoit's review comments.
Attachment #8342841 - Flags: review?(bjacob)
(Assignee)

Updated

4 years ago
Attachment #8342770 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=8fb27ff9fe21
Comment on attachment 8342841 [details] [diff] [review]
Remove ApplyFilterToBoundTexture from GLContext.Move it to CompositorOGL.

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

We might need another round of reviewing, if my understanding below is correct:

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +996,3 @@
>    {
> +    MOZ_ASSERT(mOldTexId == 0);
> +    mGL->GetUIntegerv(LOCAL_GL_ACTIVE_TEXTURE, &mOldTexUnit);

Here mOldTexUnit is set to be the active unit before we initialize the AutoBindTexture object...

@@ +1008,5 @@
> +
> +  AutoBindTexture(GLContext* aGL, TextureSourceOGL* aTexture,
> +                  GLenum aTexUnit = LOCAL_GL_TEXTURE0)
> +    : ScopedGLWrapper<AutoBindTexture>(aGL)
> +    , mOldTexUnit(aTexUnit)

...which overwrites the above code, which set mOldTexUnit to be the active unit to be set by the AutoBindTexture object...

@@ +1029,5 @@
>    {
> +    GLuint texUnit;
> +    mGL->GetUIntegerv(LOCAL_GL_ACTIVE_TEXTURE, &texUnit);
> +    if (texUnit != mOldTexUnit)
> +      mGL->fActiveTexture(mOldTexUnit);

...so here we are really making active again the old texture unit which was active before this AutoBindTexture was initialized, which is *not* the one that we called glBindTexture on in Bind() above...

@@ +1034,5 @@
> +
> +    GLuint texId;
> +    mGL->GetUIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &texId);
> +    if (texId != mOldTexId)
> +      mGL->fBindTexture(LOCAL_GL_TEXTURE_2D, mOldTexId);

...so here this BindTexture is taking place on a different unit (mOldTexUnit) than the BindTexture made in Bind() above (aTexUnit).

Correct?
Attachment #8342841 - Flags: review?(bjacob) → review-
(Assignee)

Comment 15

4 years ago
(In reply to Benoit Jacob [:bjacob] from comment #14)
> Correct?

Yes, I think you are. I conflated two ideas when trying to refactor this.  I think Init should look like:

  void Init(GLenum aTexUnit)
  {
    MOZ_ASSERT(mOldTexId == 0);
    mTexUnit = aTexUnit;
 
    GLenum currentTexUnit
    mGL->GetUIntegerv(LOCAL_GL_ACTIVE_TEXTURE, &currentTexUnit);
    if (currentTexUnit != mTexUnit)
      mGL->fActiveTexture(mTexUnit);
    mGL->GetUIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &mOldTexId);
  }

This begs the questions, do we need the check for the current tex unit?  It's a small gain in "efficiency". The code could be simplified to:

  void Init(GLenum aTexUnit)
  {
    MOZ_ASSERT(mOldTexId == 0);
    mTexUnit = aTexUnit;
    mGL->fActiveTexture(mTexUnit);
    mGL->GetUIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &mOldTexId);
  }

Similarly for UnwrapImpl.

Oui?
(Assignee)

Comment 16

4 years ago
Created attachment 8343454 [details] [diff] [review]
Move everything GraphicsFilter-related out of GLContext

Updated with Benoit's review comments.

New try run: https://tbpl.mozilla.org/?tree=Try&rev=b8d46936b390
Attachment #8342841 - Attachment is obsolete: true
Attachment #8343454 - Flags: review?(bjacob)
Indeed. This check was only, it seems, an attempt to avoid needlessly calling glActiveTexture if the wanted texture unit was already active --- so it was only an attempt at optimization, right?

In that case, it was probably a futile optimization, as glActiveTexture should be a cheap OpenGL call and there's no reason to expect GetUIntegerv to be cheaper than it.

So yes, the simplification that you propose seems good.

What I am unclear on is whether it matters that we leave the GL_ACTIVE_TEXTURE as we found it, when this RAII helper goes out of scope. By default, if it doesn't cost much for this RAII helper to offer the guarantee that it leaves the GL_ACTIVE_TEXTURE as it found it, then that is a nice property to have (and would make usage of this helper safe in other contexts).
(Assignee)

Comment 18

4 years ago
(Follow on from IRC conversation)

I think the goal of the AutoBindTexture is to manage the binding of textures to a specified texture unit. We're in agreement here.

The way that AutoBindTexture is used:

  {
    ...
    TextureSourceOGL* texSrc;
    AutoBindTexture autoBind(mGLContext, texSrc, LOCAL_GL_TEXTURE1);
    ...
  }
  // [1]

or

  {
    ...
    AutoBindTexture autoBind(mGLContext);
    if (maskType != MaskNone) {
      autoBind.Bind(sourceMask, LOCAL_GL_TEXTURE0);
      ...
    }
    ...
  }
  // [1]

I think the issue is: what should the active texture unit be at point [1] in both examples, ie. should Bind() and UnwrapImpl() be idempotent?

Take the following example:

  gl->fActiveTexture(GL_TEXTURE0);
  {
    AutoBindTexture autoBind(gl);
    gl->fActiveTexture(GL_TEXTURE1);
    autoBind.Bind(texSrc, GL_TEXTURE3);
    // [2]
    ...
    gl->fActiveTexture(GL_TEXTURE2);
    ...
  }
  // [3]

What should the active texture unit be at [2] and [3]? GL_TEXTURE0, GL_TEXTURE1, or GL_TEXTURE2?

My feeling is that [2] should be GL_TEXTURE1 and [2] should be GL_TEXTURE2. For my interpretation of what you said on IRC, [3] would be GL_TEXTURE0. (I don't know about [2]).
I think that comment 18 is spot on the difficulty that we have here, which is that AutoBindTexture has nontrivial enough semantics, that there is room for us to discuss/debate them. That's IMHO the problem with it. If there is room for us to discuss its semantics, that means that people reading this code late will have to think, too, about what it is actually doing, i.e. we're not making this code easier to read with this AutoBindTexture helper.

I think that RAII helpers such as AutoBindTexture are only good if they have very simple semantics that can be fully explained in a short comment.

As long as AutoBindTexture is strictly a RAII idiom, i.e. only doing anything through its constructor and destructor, it's easy enough to specify what it should be doing, in a comment like this:

/**
 * AutoBindTexture is a RAII helper.
 * Its constructor binds the specified texture on the specified texture unit,
 * saving the original texture binding on that unit.
 * Its destructor restores the original texture binding on that texture unit.
 */

But as soon as we have a public Bind method in this class, it seems much more difficult to specify semantics, because with a public Bind method we're partly back to 'imperative' GL calls instead of a pure RAII idiom.

Now, if the present use case in the compositor requires this Bind method... then maybe this code should just make plain GL calls instead of using a "modified RAII" idiom that's nontrivial to specify.
That said... it seems that I was wrong on a precise point in our IRC conversation yesterday.

I think I said (wrongly, I now realize) that the AutoBindTexture destructor should make active the texture unit that was unit before it was constructed. That's also what you said here ("[3] would be GL_TEXTURE0"):

(In reply to Dan Glastonbury :djg :kamidphish from comment #18)
> My feeling is that [2] should be GL_TEXTURE1 and [2] should be GL_TEXTURE2.
> For my interpretation of what you said on IRC, [3] would be GL_TEXTURE0. (I
> don't know about [2]).

I realize that's wrong, the AutoBindTexture should only internally call glActiveTexture to restore the original texture binding on the specified texture unit, but it should leave the active texture unit unchanged when it returns.

So yes, I agree with you that [2] should be GL_TEXTURE1 and [3] should be GL_TEXTURE2  (which I assume is what you meant --- instead of saying [2] twice).

I still amn't a fan of having a public Bind method there, though... why not have the user use separate AutoBindTexture objects, on separate scopes?
Comment on attachment 8343454 [details] [diff] [review]
Move everything GraphicsFilter-related out of GLContext

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

I would like us to fully resolve the AutoBindTexture 'debate' before proceeding with the review.
Attachment #8343454 - Flags: review?(bjacob)
(Assignee)

Comment 22

4 years ago
Created attachment 8344421 [details] [diff] [review]
Remove ApplyFilterToBoundTexture from GLContext.Move it to CompositorOGL.

Implemented changes discussed with bjacob on IRC.

https://tbpl.mozilla.org/?tree=Try&rev=89d9a255cc0a
(Assignee)

Updated

4 years ago
Attachment #8343454 - Attachment is obsolete: true
(Assignee)

Comment 23

4 years ago
(In reply to Dan Glastonbury :djg :kamidphish from comment #22)
> Created attachment 8344421 [details] [diff] [review]
> Remove ApplyFilterToBoundTexture from GLContext.Move it to CompositorOGL.
> 
> Implemented changes discussed with bjacob on IRC.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=89d9a255cc0a

Forget it. This one is buggy.
(Assignee)

Comment 24

4 years ago
Created attachment 8344451 [details] [diff] [review]
Remove ApplyFilterToBoundTexture from GLContext.Move it to CompositorOGL.

Fixed the silly errors in the previous patch. Tested against local reftests.
Attachment #8344451 - Flags: review?(bjacob)
(Assignee)

Updated

4 years ago
Attachment #8344421 - Attachment is obsolete: true
(Assignee)

Comment 25

4 years ago
Created attachment 8344452 [details] [diff] [review]
Remove Bind method from AutoBindTexture.

Refactored the places where AutoBindTexture::Bind was used. Replaced with a
simplier class that just saves the texture id bound to a texture unit. The
texture is bound directly instead of via the helper.

I didn't eliminate all occurences of AutoBindTexture because I a few places it
was cleaner than using AutoSaveTexture + TextureSourceOGL->BindTexture.

I extracted the cut'n'paste code that bound the mask texture to program into
BindMaskTextureForProgram. I dislike cut'n'paste code in multiple spots.
Attachment #8344452 - Flags: review?(bjacob)
(Assignee)

Comment 26

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=7bcb6aac2ee7
Comment on attachment 8344451 [details] [diff] [review]
Remove ApplyFilterToBoundTexture from GLContext.Move it to CompositorOGL.

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

Thanks! r=me, a couple of nits below.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1008,3 @@
>    {
> +    if (!aTexture)
> +      return;

Why the early return? The previous code didn't seem to try to avoid crashing in that case. If this should not be null, please make it a MOZ_ASSERT instead. If this can legitimately be null, please describe when and why it's happening now and didn't before.

@@ +1011,5 @@
> +
> +    MOZ_ASSERT(mOldTexId == (GLuint) -1);
> +    mTexUnit = aTexUnit;
> +
> +    ScopedBindTextureUnit savedTexUnit(mGL, aTexUnit);

'savedTexUnit' seems like a bad name for this RAII helper; I would prefer something like autoBindTexUnit.

@@ +1023,5 @@
> +  {
> +    if (mOldTexId == (GLuint) -1)
> +      return;
> +
> +    ScopedBindTextureUnit savedTexUnit(mGL, mTexUnit);

Same.
Attachment #8344451 - Flags: review?(bjacob) → review+
Comment on attachment 8344452 [details] [diff] [review]
Remove Bind method from AutoBindTexture.

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

r=me.... but please actually remove the Bind method, which, IIUC, isn't used anymore after this patch!

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1051,5 @@
> +public:
> +  AutoSaveTexture(GLContext* aGL, GLenum aTexUnit = LOCAL_GL_TEXTURE0)
> +    : ScopedGLWrapper<AutoSaveTexture>(aGL)
> +    , mTexUnit(aTexUnit)
> +    , mOldTexId((GLuint) -1)

microscopic nit: space after -
Attachment #8344452 - Flags: review?(bjacob) → review+
(Assignee)

Comment 29

4 years ago
Created attachment 8345151 [details] [diff] [review]
Remove Bind method from AutoBindTexture.Refactored the places where AutoBindTexture::Bind was used. Replaced with a

Update with review comments from bjacob.

https://tbpl.mozilla.org/?tree=Try&rev=c9efa90b34de
(Assignee)

Updated

4 years ago
Attachment #8344452 - Attachment is obsolete: true
(Assignee)

Comment 30

4 years ago
Comment on attachment 8345151 [details] [diff] [review]
Remove Bind method from AutoBindTexture.Refactored the places where AutoBindTexture::Bind was used. Replaced with a

Carry r=bjacob.
Attachment #8345151 - Flags: review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/c1b544bfad40
http://hg.mozilla.org/integration/mozilla-inbound/rev/01df0b7dcd8f
Target Milestone: --- → mozilla29
https://hg.mozilla.org/mozilla-central/rev/01df0b7dcd8f
https://hg.mozilla.org/mozilla-central/rev/c1b544bfad40
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.