WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
64772
[chromium] Defer managed texture creation and destruction until updateCompositorResources
https://bugs.webkit.org/show_bug.cgi?id=64772
Summary
[chromium] Defer managed texture creation and destruction until updateComposi...
James Robinson
Reported
2011-07-18 18:27:09 PDT
[chromium] Defer managed texture creation and destruction until updateCompositorResources
Attachments
Patch
(23.36 KB, patch)
2011-07-18 18:31 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(27.11 KB, patch)
2011-07-25 17:22 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(50.30 KB, patch)
2011-08-10 16:08 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
James Robinson
Comment 1
2011-07-18 18:31:28 PDT
Created
attachment 101255
[details]
Patch
James Robinson
Comment 2
2011-07-25 17:22:47 PDT
Created
attachment 101947
[details]
Patch
Vangelis Kokkevis
Comment 3
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)?
James Robinson
Comment 4
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.
James Robinson
Comment 5
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
Adrienne Walker
Comment 6
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.
James Robinson
Comment 7
2011-08-10 16:08:57 PDT
Created
attachment 103550
[details]
Patch
James Robinson
Comment 8
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.
Adrienne Walker
Comment 9
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.
James Robinson
Comment 10
2011-08-11 17:48:01 PDT
Can I get an official review+?
Kenneth Russell
Comment 11
2011-08-11 18:00:00 PDT
Comment on
attachment 103550
[details]
Patch Looks good to me.
WebKit Review Bot
Comment 12
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
>
WebKit Review Bot
Comment 13
2011-08-11 19:07:43 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug