Closed Bug 942506 Opened 11 years ago Closed 11 years ago

Move everything GraphicsFilter-related out of GLContext

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29

People

(Reporter: bjacob, Assigned: u480271)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 6 obsolete files)

Remove ApplyFilterToBoundTexture from GLContext.Move it to CompositorOGL.
(deleted), patch
bjacob
: review+
Details | Diff | Splinter Review
Remove Bind method from AutoBindTexture.Refactored the places where AutoBindTexture::Bind was used. Replaced with a
(deleted), patch
u480271
: review+
Details | Diff | Splinter Review 
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);
I think this should wait until we have deleted the old layers code and then this can go directly into CompositorOGL.cpp
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.
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: nobody → dglastonbury
Status: NEW → ASSIGNED
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
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-
(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?
Rebased the patch.
Attachment #8342155 - Attachment is obsolete: true
Try with reftest: https://tbpl.mozilla.org/?tree=Try&rev=3091dd5c1fd6
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.
(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 ;-) )
(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.
Apply Benoit's review comments.
Attachment #8342841 - Flags: review?(bjacob)
Attachment #8342770 - Attachment is obsolete: true
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-
(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?
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).
(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)
Implemented changes discussed with bjacob on IRC.

https://tbpl.mozilla.org/?tree=Try&rev=89d9a255cc0a
Attachment #8343454 - Attachment is obsolete: true
(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.
Fixed the silly errors in the previous patch. Tested against local reftests.
Attachment #8344451 - Flags: review?(bjacob)
Attachment #8344421 - Attachment is obsolete: true
Attached patch Remove Bind method from AutoBindTexture. (obsolete) (deleted) — — Splinter Review 
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)
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+
Attachment #8344452 - Attachment is obsolete: true
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+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: