Bug 64772 – [chromium] Defer managed texture creation and destruction until updateCompositorResources

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Defer managed texture creation and destruction until updateCompositorResources

[James Robinson]

Created attachment 101255 [details]
Patch

[James Robinson]

Created attachment 101947 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 101947 [details]
Patch

I like the split but it worries me a bit that we have to pass a pointer to a GraphicsContext3D every time we need to do the real GL operation.  This allows for inconsistent context usage, like for example creating a texture with one context and deleting it with another. 

Since we know that every manager is dealing with only one context, how about we keep the context as a member of the manager (doesn't have to be set by the constructor, just needs to be there before any actual GL operation takes place)?

[James Robinson]

The reason I didn't do that was that I want to start being more careful about when we use the compositor context so that it'll be easier to enforce the threading constraints on the context.  The way I've been thinking to enforce this is to limit access to the GraphicsContext3D pointer itself and guard places that are passed the pointer with ASSERT()s, since I can't put ASSERT()s in the GraphicsContext3D interface itself.  This does depend on the TextureManager always being passed in the same GraphicsContext3D*, but I'm not totally sure what to do about that.

It could be that I'm being overly paranoid here about people misusing GraphicsContext3Ds that are member functions.

[James Robinson]

(In reply to comment #4)
> It could be that I'm being overly paranoid here about people misusing GraphicsContext3Ds that are member functions.

member variables that is

[Adrienne Walker]

(In reply to comment #3)
> (From update of attachment 101947 [details])
> I like the split but it worries me a bit that we have to pass a pointer to a GraphicsContext3D every time we need to do the real GL operation.  This allows for inconsistent context usage, like for example creating a texture with one context and deleting it with another. 

On the other hand, in the previous world, we could be binding with the same GraphicsContext3D that we created with, but it would be the current one to draw with because some LayerTexture hadn't been updated to use a new one.  I think the consistency issues aren't increased here, they've just been shifted around.

We could always keep around the GraphicsContext3D pointer in debug builds and assert that we are always using a consistent one for each LayerTexture.  That's kind of messy, but it would catch errors like this.

[James Robinson]

Created attachment 103550 [details]
Patch

[James Robinson]

This adds a GraphicsContext3D* in debug only so we can ASSERT() that we aren't mixing up our contexts.  I've also renamed LayerTexture since there are layer and non-layer users of it now (RenderSurfaces and the HUD use the class as well).

Another thought I had as a potential follow-up was to delegate the creation/destruction logic out to a client interface so I don't have to make TextureManager aware of how the actual resources are made and destroyed.  Then TextureManager would just be responsible for keeping track of resource use and deciding who gets evicted when under pressure.  That would also make it easier to write some C++ unit tests for the class, which is something I should have done ages ago.

[Adrienne Walker]

(In reply to comment #8)
> This adds a GraphicsContext3D* in debug only so we can ASSERT() that we aren't mixing up our contexts.  I've also renamed LayerTexture since there are layer and non-layer users of it now (RenderSurfaces and the HUD use the class as well).

Awesome.  This patch looks like a great refactoring.

> Another thought I had as a potential follow-up was to delegate the creation/destruction logic out to a client interface so I don't have to make TextureManager aware of how the actual resources are made and destroyed.  Then TextureManager would just be responsible for keeping track of resource use and deciding who gets evicted when under pressure.  That would also make it easier to write some C++ unit tests for the class, which is something I should have done ages ago.

This also sounds like a great idea.

[James Robinson]

Can I get an official review+?

[Kenneth Russell]

Comment on attachment 103550 [details]
Patch

Looks good to me.

[WebKit Review Bot]

Comment on attachment 103550 [details]
Patch

Clearing flags on attachment: 103550

Committed r92900: <http://trac.webkit.org/changeset/92900>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.