Bug 64772

Summary: [chromium] Defer managed texture creation and destruction until updateCompositorResources
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: enne, jamesr, kbr, senorblanco, vangelis, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 64771    
Bug Blocks: 58902    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description James Robinson 2011-07-18 18:27:09 PDT
[chromium] Defer managed texture creation and destruction until updateCompositorResources
Comment 1 James Robinson 2011-07-18 18:31:28 PDT
Created attachment 101255 [details]
Patch
Comment 2 James Robinson 2011-07-25 17:22:47 PDT
Created attachment 101947 [details]
Patch
Comment 3 Vangelis Kokkevis 2011-07-28 18:16:39 PDT
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)?
Comment 4 James Robinson 2011-07-28 19:22:51 PDT
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.
Comment 5 James Robinson 2011-07-28 19:23:03 PDT
(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
Comment 6 Adrienne Walker 2011-07-29 17:20:08 PDT
(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.
Comment 7 James Robinson 2011-08-10 16:08:57 PDT
Created attachment 103550 [details]
Patch
Comment 8 James Robinson 2011-08-10 16:14:20 PDT
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.
Comment 9 Adrienne Walker 2011-08-10 16:31:01 PDT
(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.
Comment 10 James Robinson 2011-08-11 17:48:01 PDT
Can I get an official review+?
Comment 11 Kenneth Russell 2011-08-11 18:00:00 PDT
Comment on attachment 103550 [details]
Patch

Looks good to me.
Comment 12 WebKit Review Bot 2011-08-11 19:07:38 PDT
Comment on attachment 103550 [details]
Patch

Clearing flags on attachment: 103550

Committed r92900: <http://trac.webkit.org/changeset/92900>
Comment 13 WebKit Review Bot 2011-08-11 19:07:43 PDT
All reviewed patches have been landed.  Closing bug.