Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11638028:
Mailbox support for texture layers. (Closed)
|
Created:
8 years ago by alexst (slow to review) Modified:
7 years, 11 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMailbox support for texture layers.
A callback object is passed along with the mailbox name to the layer. This callback is triggered with a sync point to signal when the mailbox is no longer in use by the compositor.
TextureLayerImpl packages a mailbox up as a resource on willDraw
and releases it when a new mailbox is ready for consumption or when the layer is destroyed.
BUG=123444
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175529
Patch Set 1 : #
Total comments: 23
Patch Set 2 : Feedback #Patch Set 3 : Formatting #
Total comments: 20
Patch Set 4 : Feedback, callback runs on main. #
Total comments: 3
Patch Set 5 : Nits #Patch Set 6 : Tests #
Total comments: 28
Patch Set 7 : Comments #
Total comments: 4
Patch Set 8 : More tests, cleanup #
Total comments: 6
Patch Set 9 : Amended tests #Patch Set 10 : Rebase #
Total comments: 8
Patch Set 11 : Comments #
Messages
Total messages: 38 (0 generated)
https://codereview.chromium.org/11638028/diff/2001/cc/mailbox_release_client.h File cc/mailbox_release_client.h (right): https://codereview.chromium.org/11638028/diff/2001/cc/mailbox_release_client.... cc/mailbox_release_client.h:17: virtual void mailboxReleased(const std::string& name, unsigned syncPoint) = 0; nit: chrome style (naming, 80 cols) https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:242: const int8* name = reinterpret_cast<const int8*>(mailbox.c_str()); nit: .data() instead of c_str() to avoid forcing an extra \0. https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:251: GLC(context3d, context3d->texParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE)); nit: we shouldn't have to set the tex parameters, since they should already have been set by the producer. https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:256: Resource resource(textureId, gfx::Size(), 0, GL_LINEAR); We probably want to pass all the info so that these can have the proper values. https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:298: if (!resource->mailbox.isZero()) { This is not quite right, since we will have a mailbox set for compositor-originated resources we sent to the parent, but we don't want to produce before sending in that case. https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer_impl.cc File cc/texture_layer_impl.cc (right): https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer_impl.cc#n... cc/texture_layer_impl.cc:35: { You probably need to do more here, for the case where the TextureLayer was removed from the tree, and so this gets deleted during the commit. You won't get any other willDraw, so you can't properly return the texture to the client. https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer_impl.cc#n... cc/texture_layer_impl.cc:130: m_mailboxClient->mailboxReleased(m_lastMailbox, context->insertSyncPoint()); We need to be clear about which thread the client is called on. Right now it would be called on the main thread in some cases and on the impl thread in other cases, and that probably isn't viable.
I think we need to decouple the layer from the mailbox a bit. Their lifetimes can differ, and we need to be able to notify the owner of the mailbox when we're done with it, even if they destroyed the layer. Unless, we want it tied to the layer, and we own the resource and delete it if the layer goes away. But I liked the simplicity from our end of always requiring the provider of the mailbox to clean it up. Antoine: What if we put the map<mailbox, callbackobject> on ResourceProvider, and had: - non uber: TextureLayerImpl::willDraw() tell ResourceProvider when it's done with something - uber: DelegatingRendererLayer::receiveAck() tell RP when it's done with it. ? https://codereview.chromium.org/11638028/diff/2001/cc/mailbox_release_client.h File cc/mailbox_release_client.h (right): https://codereview.chromium.org/11638028/diff/2001/cc/mailbox_release_client.... cc/mailbox_release_client.h:12: class CC_EXPORT MailboxReleaseClient { This file should be in chromium style. And it needs to be added to cc.gyp https://codereview.chromium.org/11638028/diff/2001/cc/mailbox_release_client.... cc/mailbox_release_client.h:20: } } // namespace cc https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:242: const int8* name = reinterpret_cast<const int8*>(mailbox.c_str()); how come you need reinterpret_cast to do this? what if you use .data() instead? https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:253: GLC(context3d, context3d->flush()); how come the flush is needed? https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer.h File cc/texture_layer.h (right): https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer.h#newcode31 cc/texture_layer.h:31: static scoped_refptr<TextureLayer> createWithMailboxClient(MailboxReleaseClient*); Instead of using a client, can we use a base::bind() callback on the setTextureMailbox() method? Then in the the webkit_bindings layer, we can hide the bind() stuff, if we need to expose setTextureMailbox on the webkit api. Because otherwise if the layer is deleted, how are we going to send the notification? We could, during commit, store the callback object (with the mailbox as a key) on the impl thread somewhere. Then when we are done with it, we'll always have the callback object available. (even the impl layer may be gone by the time we get the ack from the parent compositor, so it shouldn't stay on a layerImpl either, I think) https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer_impl.cc File cc/texture_layer_impl.cc (right): https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer_impl.cc#n... cc/texture_layer_impl.cc:14: #include <public/WebGraphicsContext3D.h> use "third_party/WebKit/..."
On Wed, Dec 19, 2012 at 6:16 PM, <danakj@chromium.org> wrote: > I think we need to decouple the layer from the mailbox a bit. Their > lifetimes > can differ, and we need to be able to notify the owner of the mailbox when > we're > done with it, even if they destroyed the layer. Unless, we want it tied to > the > layer, and we own the resource and delete it if the layer goes away. > > But I liked the simplicity from our end of always requiring the provider > of the > mailbox to clean it up. > > Antoine: What if we put the map<mailbox, callbackobject> on > ResourceProvider, > and had: > - non uber: TextureLayerImpl::willDraw() tell ResourceProvider when it's > done > with something > - uber: DelegatingRendererLayer::**receiveAck() tell RP when it's done > with it. > ? I would push towards a solution where the über vs non-über notion doesn't have to leak into the layer types. A similar logic will have to exist for DelegatedRendererLayer, and possibly VideoLayer. Unfortunately I haven't thought hard about the problem (I suspect the renderers will have to be involved), so I don't have a good answer right now. Involving the RP seems fair though, since it's the one that maintains the isInUseByConsumer notion, which is really what matters here. Antoine > > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > mailbox_release_client.h<https://codereview.chromium.org/11638028/diff/2001/cc/mailbox_release_client.h> > File cc/mailbox_release_client.h (right): > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > mailbox_release_client.h#**newcode12<https://codereview.chromium.org/11638028/diff/2001/cc/mailbox_release_client.h#newcode12> > cc/mailbox_release_client.h:**12: class CC_EXPORT MailboxReleaseClient { > This file should be in chromium style. And it needs to be added to > cc.gyp > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > mailbox_release_client.h#**newcode20<https://codereview.chromium.org/11638028/diff/2001/cc/mailbox_release_client.h#newcode20> > cc/mailbox_release_client.h:**20: } > } // namespace cc > > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > resource_provider.cc<https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc> > File cc/resource_provider.cc (right): > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > resource_provider.cc#**newcode242<https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#newcode242> > cc/resource_provider.cc:242: const int8* name = reinterpret_cast<const > int8*>(mailbox.c_str()); > how come you need reinterpret_cast to do this? what if you use .data() > instead? > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > resource_provider.cc#**newcode253<https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#newcode253> > cc/resource_provider.cc:253: GLC(context3d, context3d->flush()); > how come the flush is needed? > > https://codereview.chromium.**org/11638028/diff/2001/cc/**texture_layer.h<htt... > File cc/texture_layer.h (right): > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > texture_layer.h#newcode31<https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer.h#newcode31> > cc/texture_layer.h:31: static scoped_refptr<TextureLayer> > createWithMailboxClient(**MailboxReleaseClient*); > Instead of using a client, can we use a base::bind() callback on the > setTextureMailbox() method? > > Then in the the webkit_bindings layer, we can hide the bind() stuff, if > we need to expose setTextureMailbox on the webkit api. > > Because otherwise if the layer is deleted, how are we going to send the > notification? > > We could, during commit, store the callback object (with the mailbox as > a key) on the impl thread somewhere. Then when we are done with it, > we'll always have the callback object available. > > (even the impl layer may be gone by the time we get the ack from the > parent compositor, so it shouldn't stay on a layerImpl either, I think) > > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > texture_layer_impl.cc<https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer_impl.cc> > File cc/texture_layer_impl.cc (right): > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > texture_layer_impl.cc#**newcode14<https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer_impl.cc#newcode14> > cc/texture_layer_impl.cc:14: #include <public/WebGraphicsContext3D.**h> > use "third_party/WebKit/..." > > https://codereview.chromium.**org/11638028/<https://codereview.chromium.org/1... > > -- > > >
I'll have a new patch up shortly. General question about code style. It seems like all the new changes in CC files follow chromium style guide, as in, both chromium and webkit styles already appear in texture_layer.cc. I also remember hearing that we should stay consistent with what's there. What's the official protocol, do you know? https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:242: const int8* name = reinterpret_cast<const int8*>(mailbox.c_str()); On 2012/12/20 02:14:21, piman wrote: > nit: .data() instead of c_str() to avoid forcing an extra \0. Done. https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:242: const int8* name = reinterpret_cast<const int8*>(mailbox.c_str()); consumeTextureCHROMIUM expects signed data both, c_str() and data() are unsigned, hence the cast. On 2012/12/20 02:16:19, danakj wrote: > how come you need reinterpret_cast to do this? what if you use .data() instead? https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:251: GLC(context3d, context3d->texParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE)); On 2012/12/20 02:14:21, piman wrote: > nit: we shouldn't have to set the tex parameters, since they should already have > been set by the producer. Done. https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:253: GLC(context3d, context3d->flush()); Backing texture for the mailbox is in a different context, we need to make sure it latches to the newly generated texture id before we start using it. On 2012/12/20 02:16:19, danakj wrote: > how come the flush is needed? https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:256: Resource resource(textureId, gfx::Size(), 0, GL_LINEAR); I only know the size but not the format from swapBuffers. It's easy to plumb it through, but will it be used anywhere? External texture id's follow the same pattern and since mailboxes are an alternate way to represent them, I left this part the same. On 2012/12/20 02:14:21, piman wrote: > We probably want to pass all the info so that these can have the proper values. https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:298: if (!resource->mailbox.isZero()) { On 2012/12/20 02:14:21, piman wrote: > This is not quite right, since we will have a mailbox set for > compositor-originated resources we sent to the parent, but we don't want to > produce before sending in that case. Done. https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer.h File cc/texture_layer.h (right): https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer.h#newcode31 cc/texture_layer.h:31: static scoped_refptr<TextureLayer> createWithMailboxClient(MailboxReleaseClient*); I was thinking of trying this approach myself. New CL coming up with base::bind callbacks. I think I like it better. On 2012/12/20 02:16:19, danakj wrote: > Instead of using a client, can we use a base::bind() callback on the > setTextureMailbox() method? > > Then in the the webkit_bindings layer, we can hide the bind() stuff, if we need > to expose setTextureMailbox on the webkit api. > > Because otherwise if the layer is deleted, how are we going to send the > notification? > > We could, during commit, store the callback object (with the mailbox as a key) > on the impl thread somewhere. Then when we are done with it, we'll always have > the callback object available. > > (even the impl layer may be gone by the time we get the ack from the parent > compositor, so it shouldn't stay on a layerImpl either, I think) https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer_impl.cc File cc/texture_layer_impl.cc (right): https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer_impl.cc#n... cc/texture_layer_impl.cc:14: #include <public/WebGraphicsContext3D.h> Goes away with base::bind On 2012/12/20 02:16:19, danakj wrote: > use "third_party/WebKit/..." https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer_impl.cc#n... cc/texture_layer_impl.cc:35: { On 2012/12/20 02:14:21, piman wrote: > You probably need to do more here, for the case where the TextureLayer was > removed from the tree, and so this gets deleted during the commit. You won't get > any other willDraw, so you can't properly return the texture to the client. Done. https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer_impl.cc#n... cc/texture_layer_impl.cc:130: m_mailboxClient->mailboxReleased(m_lastMailbox, context->insertSyncPoint()); That's one of the things I wanted to discuss. Like you said, there will be cases when the callback needs to be triggered from both main and impl. For example, when a mailbox is set on main and then set again before a commit happened, in that case we'll need to release the old buffer on main, whereas commited buffers get released on impl. I could try something along the lines of wrapping the original callback to post taks on main thread inside texture_layer and pass that on to layer_impl, or we could document this as callback coming from any thread and leave it up to the user to proxy it to the desired thread. That's what I am doing now in my test. What are your thoughts on either of these? On 2012/12/20 02:14:21, piman wrote: > We need to be clear about which thread the client is called on. Right now it > would be called on the main thread in some cases and on the impl thread in other > cases, and that probably isn't viable.
Generally new files in new style. old files in oldstyle unless its been converted. But in old style ive started doing things like NULL instead of 0 since thatll just be lots of pain to find and change later. but especially formatting i'd match the style of the file. On Friday, December 21, 2012, wrote: > Reviewers: danakj, piman, > > Message: > I'll have a new patch up shortly. > > General question about code style. It seems like all the new changes in CC > files > follow chromium style guide, as in, both chromium and webkit styles already > appear in texture_layer.cc. I also remember hearing that we should stay > consistent with what's there. What's the official protocol, do you know? > > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > resource_provider.cc<https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc> > File cc/resource_provider.cc (right): > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > resource_provider.cc#**newcode242<https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#newcode242> > cc/resource_provider.cc:242: const int8* name = reinterpret_cast<const > int8*>(mailbox.c_str()); > On 2012/12/20 02:14:21, piman wrote: > >> nit: .data() instead of c_str() to avoid forcing an extra \0. >> > > Done. > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > resource_provider.cc#**newcode242<https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#newcode242> > cc/resource_provider.cc:242: const int8* name = reinterpret_cast<const > int8*>(mailbox.c_str()); > consumeTextureCHROMIUM expects signed data both, c_str() and data() are > unsigned, hence the cast. > > On 2012/12/20 02:16:19, danakj wrote: > >> how come you need reinterpret_cast to do this? what if you use .data() >> > instead? > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > resource_provider.cc#**newcode251<https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#newcode251> > cc/resource_provider.cc:251: GLC(context3d, > context3d->texParameteri(GL_**TEXTURE_2D, GL_TEXTURE_WRAP_T, > GL_CLAMP_TO_EDGE)); > On 2012/12/20 02:14:21, piman wrote: > >> nit: we shouldn't have to set the tex parameters, since they should >> > already have > >> been set by the producer. >> > > Done. > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > resource_provider.cc#**newcode253<https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#newcode253> > cc/resource_provider.cc:253: GLC(context3d, context3d->flush()); > Backing texture for the mailbox is in a different context, we need to > make sure it latches to the newly generated texture id before we start > using it. > On 2012/12/20 02:16:19, danakj wrote: > >> how come the flush is needed? >> > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > resource_provider.cc#**newcode256<https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#newcode256> > cc/resource_provider.cc:256: Resource resource(textureId, gfx::Size(), > 0, GL_LINEAR); > I only know the size but not the format from swapBuffers. > > It's easy to plumb it through, but will it be used anywhere? External > texture id's follow the same pattern and since mailboxes are an > alternate way to represent them, I left this part the same. > > On 2012/12/20 02:14:21, piman wrote: > >> We probably want to pass all the info so that these can have the >> > proper values. > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > resource_provider.cc#**newcode298<https://codereview.chromium.org/11638028/diff/2001/cc/resource_provider.cc#newcode298> > cc/resource_provider.cc:298: if (!resource->mailbox.isZero()) { > On 2012/12/20 02:14:21, piman wrote: > >> This is not quite right, since we will have a mailbox set for >> compositor-originated resources we sent to the parent, but we don't >> > want to > >> produce before sending in that case. >> > > Done. > > https://codereview.chromium.**org/11638028/diff/2001/cc/**texture_layer.h<htt... > File cc/texture_layer.h (right): > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > texture_layer.h#newcode31<https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer.h#newcode31> > cc/texture_layer.h:31: static scoped_refptr<TextureLayer> > createWithMailboxClient(**MailboxReleaseClient*); > I was thinking of trying this approach myself. New CL coming up with > base::bind callbacks. I think I like it better. > > On 2012/12/20 02:16:19, danakj wrote: > >> Instead of using a client, can we use a base::bind() callback on the >> setTextureMailbox() method? >> > > Then in the the webkit_bindings layer, we can hide the bind() stuff, >> > if we need > >> to expose setTextureMailbox on the webkit api. >> > > Because otherwise if the layer is deleted, how are we going to send >> > the > >> notification? >> > > We could, during commit, store the callback object (with the mailbox >> > as a key) > >> on the impl thread somewhere. Then when we are done with it, we'll >> > always have > >> the callback object available. >> > > (even the impl layer may be gone by the time we get the ack from the >> > parent > >> compositor, so it shouldn't stay on a layerImpl either, I think) >> > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > texture_layer_impl.cc<https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer_impl.cc> > File cc/texture_layer_impl.cc (right): > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > texture_layer_impl.cc#**newcode14<https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer_impl.cc#newcode14> > cc/texture_layer_impl.cc:14: #include <public/WebGraphicsContext3D.**h> > Goes away with base::bind > On 2012/12/20 02:16:19, danakj wrote: > >> use "third_party/WebKit/..." >> > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > texture_layer_impl.cc#**newcode35<https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer_impl.cc#newcode35> > cc/texture_layer_impl.cc:35: { > On 2012/12/20 02:14:21, piman wrote: > >> You probably need to do more here, for the case where the TextureLayer >> > was > >> removed from the tree, and so this gets deleted during the commit. You >> > won't get > >> any other willDraw, so you can't properly return the texture to the >> > client. > > Done. > > https://codereview.chromium.**org/11638028/diff/2001/cc/** > texture_layer_impl.cc#**newcode130<https://codereview.chromium.org/11638028/diff/2001/cc/texture_layer_impl.cc#newcode130> > cc/texture_layer_impl.cc:130: > m_mailboxClient->**mailboxReleased(m_lastMailbox, > context->insertSyncPoint()); > That's one of the things I wanted to discuss. > > Like you said, there will be cases when the callback needs to be > triggered from both main and impl. For example, when a mailbox is set on > main and then set again before a commit happened, in that case we'll > need to release the old buffer on main, whereas commited buffers get > released on impl. > > I could try something along the lines of wrapping the original callback > to post taks on main thread inside texture_layer and pass that on to > layer_impl, or we could document this as callback coming from any thread > and leave it up to the user to proxy it to the desired thread. That's > what I am doing now in my test. What are your thoughts on either of > these? > > On 2012/12/20 02:14:21, piman wrote: > >> We need to be clear about which thread the client is called on. Right >> > now it > >> would be called on the main thread in some cases and on the impl >> > thread in other > >> cases, and that probably isn't viable. >> > > Description: > Mailbox support for texture layers. > > This is a first draft of what it might look like. > Uploading to continue discussion and get some feedback. > > I tested with webview to make sure this actually runs. > > I create a texture layer with a MailboxReleaseClient, > which gets notified when the previous mailbox is no longer in use. > I pass a sync point along with the mailbox name. > > TextureLayerImpl packages a mailbox up as a resource on willDraw > and releases it when a new mailbox is ready for consumption. > > We could also shift the burden of release notification into the resource > manager > instead of the layer, but I wanted to run this by everyone before I write > any > more code. > > BUG= > > > Please review this at https://codereview.chromium.**org/11638028/<https://codereview.chromium.org/1... > > SVN Base: svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > Affected files: > A cc/mailbox_release_client.h > M cc/resource_provider.h > M cc/resource_provider.cc > M cc/texture_layer.h > M cc/texture_layer.cc > M cc/texture_layer_impl.h > M cc/texture_layer_impl.cc > > >
New patch
I like the delete calling the callback business. A bunch of little nits, but two larger questions: 1. Can we wrap the callback in a postTask on the main thread, so we that we can be sure that the impl thread won't block during the callback? A unit test that blocks in the release callback and ensures impl can keep going would be very nice. 2. What happens in ubercomp land when the texture layer is deleted but the browser is still using the texture? https://codereview.chromium.org/11638028/diff/12001/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11638028/diff/12001/cc/resource_provider.cc#n... cc/resource_provider.cc:295: if (!resource->mailbox.isZero() && resource->external) { Should this be just if (!mailbox.IsZero) {} and then DCHECK that resource->external? since that's the only use case here now. https://codereview.chromium.org/11638028/diff/12001/cc/resource_provider.h File cc/resource_provider.h (right): https://codereview.chromium.org/11638028/diff/12001/cc/resource_provider.h#ne... cc/resource_provider.h:23: #include <string> Stick all the <> headers above the "" headers while you're here please. https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer.cc File cc/texture_layer.cc (right): https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer.cc#newco... cc/texture_layer.cc:114: DCHECK(m_mailboxLayer); Can we DCHECK that mailboxName.isempty == callback.isnull? https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer.h File cc/texture_layer.h (right): https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer.h#newcode30 cc/texture_layer.h:30: // Used when mailbox names are specified instead of texture id's nit: "IDs" https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer.h#newcode31 cc/texture_layer.h:31: static scoped_refptr<TextureLayer> createMailboxLayer(); nit: createForMailbox? "MailboxLayer" sounds like a type https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer.h#newcode72 cc/texture_layer.h:72: TextureLayer(TextureLayerClient*, bool mailboxLayer); nit: "usesMailbox"? https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer_impl.cc File cc/texture_layer_impl.cc (right): https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer_impl.cc#... cc/texture_layer_impl.cc:34: ResourceProvider* provider = layerTreeImpl()->resource_provider(); DCHECK() that we're a mailbox layer here? https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer_impl.cc#... cc/texture_layer_impl.cc:35: DCHECK(!provider->inUseByConsumer(m_externalTextureResource)); This will work for now, but puts us in the same position overall for ubercomp. The texture in the impl layer may indeed be in use by the consumer when the layer disappears. What then? https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer_impl.cc#... cc/texture_layer_impl.cc:49: m_mailboxReleaseCallback = releaseCallback; can we prefix all these things with something? like m_pendingMailboxReleaseCallback, m_pendingMailboxName, m_havePendingMailbox? https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer_impl.cc#... cc/texture_layer_impl.cc:55: handleMailboxResources(resourceProvider); nit: prefer to keep all the logic in here as there's only one caller for handleMailboxResources. Since that's the path forward, maybe consider making the current (pretty short) code an early out if (!m_usesMailbox) {} and then you don't need to nest all the new code at all. https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer_impl.h File cc/texture_layer_impl.h (right): https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer_impl.h#n... cc/texture_layer_impl.h:8: #include <string> nit: space below <> headers https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer_impl.h#n... cc/texture_layer_impl.h:17: static scoped_ptr<TextureLayerImpl> create(LayerTreeImpl* treeImpl, int id, bool mailboxLayer = false) no default values for parameters in chromium style https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer_impl.h#n... cc/texture_layer_impl.h:42: void setTextureMailbox(const std::string& mailboxName, nit: don't line split (old style here still), or put the first parameter on a new line as well so alignment doesn't depend on the function name.
I updated the nits and made sure the callback is a postTask on main. https://codereview.chromium.org/11638028/diff/12001/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11638028/diff/12001/cc/resource_provider.cc#n... cc/resource_provider.cc:295: if (!resource->mailbox.isZero() && resource->external) { I was addressing an earlier comment below so we only produce external mailboxes. "we will have a mailbox set for compositor-originated resources we sent to the parent, but we don't want to produce before sending in that case." On 2013/01/02 16:17:22, danakj wrote: > Should this be just if (!mailbox.IsZero) {} > > and then DCHECK that resource->external? since that's the only use case here > now. https://codereview.chromium.org/11638028/diff/12001/cc/resource_provider.h File cc/resource_provider.h (right): https://codereview.chromium.org/11638028/diff/12001/cc/resource_provider.h#ne... cc/resource_provider.h:23: #include <string> On 2013/01/02 16:17:22, danakj wrote: > Stick all the <> headers above the "" headers while you're here please. Done. https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer_impl.cc File cc/texture_layer_impl.cc (right): https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer_impl.cc#... cc/texture_layer_impl.cc:35: DCHECK(!provider->inUseByConsumer(m_externalTextureResource)); Since callback is stored on the resource, it could be executed by the consumer when it's done. This would become an if statement instead once that usage case is active. On 2013/01/02 16:17:22, danakj wrote: > This will work for now, but puts us in the same position overall for ubercomp. > The texture in the impl layer may indeed be in use by the consumer when the > layer disappears. What then? https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer_impl.cc#... cc/texture_layer_impl.cc:49: m_mailboxReleaseCallback = releaseCallback; On 2013/01/02 16:17:22, danakj wrote: > can we prefix all these things with something? like > m_pendingMailboxReleaseCallback, m_pendingMailboxName, m_havePendingMailbox? Done. https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer_impl.cc#... cc/texture_layer_impl.cc:55: handleMailboxResources(resourceProvider); On 2013/01/02 16:17:22, danakj wrote: > nit: prefer to keep all the logic in here as there's only one caller for > handleMailboxResources. > > Since that's the path forward, maybe consider making the current (pretty short) > code an early out if (!m_usesMailbox) {} and then you don't need to nest all the > new code at all. Done. https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer_impl.h File cc/texture_layer_impl.h (right): https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer_impl.h#n... cc/texture_layer_impl.h:42: void setTextureMailbox(const std::string& mailboxName, On 2013/01/02 16:17:22, danakj wrote: > nit: don't line split (old style here still), or put the first parameter on a > new line as well so alignment doesn't depend on the function name. Done.
https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer_impl.cc File cc/texture_layer_impl.cc (right): https://codereview.chromium.org/11638028/diff/12001/cc/texture_layer_impl.cc#... cc/texture_layer_impl.cc:35: DCHECK(!provider->inUseByConsumer(m_externalTextureResource)); On 2013/01/02 19:31:35, alexst wrote: > Since callback is stored on the resource, it could be executed by the consumer > when it's done. This would become an if statement instead once that usage case > is active. Will that require the consumer to know that this layer is gone? Or are you thinking that all resources are freed by the browser process once it's done with them? I think that's going to be problematic because we ship contents textures to the browser as well. And, while it could differentiate, I'd rather they came back to the renderer and it freed them. (Also keeps it in the same process.) So, I think that the freeing responsibilities here should move to something else that lives on the LTHImpl and would outlive this layer. > > On 2013/01/02 16:17:22, danakj wrote: > > This will work for now, but puts us in the same position overall for ubercomp. > > The texture in the impl layer may indeed be in use by the consumer when the > > layer disappears. What then? >
https://codereview.chromium.org/11638028/diff/15003/cc/texture_layer.cc File cc/texture_layer.cc (right): https://codereview.chromium.org/11638028/diff/15003/cc/texture_layer.cc#newco... cc/texture_layer.cc:15: static void runCallback(const base::Callback<void(unsigned)>& callback, unsigned syncPoint) { This one would be runCallbackOnMainThread (it's always on the main thread). https://codereview.chromium.org/11638028/diff/15003/cc/texture_layer.cc#newco... cc/texture_layer.cc:19: static void runCallbackOnMain(Thread *mainThread, const base::Callback<void(unsigned)>& callback, unsigned syncPoint) { This would postCallbackToMainThread or something. https://codereview.chromium.org/11638028/diff/15003/cc/texture_layer_impl.cc File cc/texture_layer_impl.cc (right): https://codereview.chromium.org/11638028/diff/15003/cc/texture_layer_impl.cc#... cc/texture_layer_impl.cc:36: DCHECK(!provider->inUseByConsumer(m_externalTextureResource)); I'm okay with a TODO here for ubercomp, and we can make that happen in a follow-up. I think this needs tests and it should be good to go.
I added tests and the remaining nits should be addressed.
I don't think you are using GLC() correctly. Look at what it does in gl_renderer.h https://codereview.chromium.org/11638028/diff/25001/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11638028/diff/25001/cc/resource_provider.cc#n... cc/resource_provider.cc:245: GLC(context3d, textureId = context3d->createTexture()); just do the assignment textureID = context3D->createTexture() on the previous line. there's no point in doing a glGetError after a createTexture call since it just allocates a client side id https://codereview.chromium.org/11638028/diff/25001/cc/resource_provider.cc#n... cc/resource_provider.cc:248: GLC(context3d, context3d->flush()); similarly no reason to glGetError after a flush - if the consume call generated an error the GLC macro would catch it there why are you flushing here? for correctness? https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer.h File cc/texture_layer.h (right): https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer.h#newcode30 cc/texture_layer.h:30: // Used when mailbox names are specified instead of texture ID's IDs, not ID's https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer.h#newcode58 cc/texture_layer.h:58: void setTextureMailbox(const std::string&, const base::Callback<void(unsigned)>&); second time base::Callback<void(unsigned)> is used - use a typedef?
https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest.cc File cc/texture_layer_unittest.cc (right): https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:5: #include <string> put this between cc/texture_layer.h and the rest of the headers (with spaces) https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:7: #include "base/callback.h" move this header to the top of the next group of headers. https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:219: { the extra scoping/indent here doesn't seem useful? https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:239: // Test resetting the mailbox. How about the case when the resource from the mailbox is in use by a consumer (the browser)? Can we test that too? https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:248: Release(m_testData.m_mailbox1, _)).Times(1); Since you just cleared it, how will this happen? Without the VerifyAndClearExpectations after, is this being checked? https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:267: TEST_F(TextureLayerWithMailboxTest, syncImplAndGetCallback) I think this test name is wrong, it is not doing anything with impl in this test at all AFAICT. I think it's more like replaceMailboxOnMainThreadBeforeCommit. https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:280: Release(m_testData.m_mailbox1, _)).Times(1); Seems like this EXPECT should move to line 287? https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:317: EXPECT_EQ(proxy()->isMainThread(), true); nit: (expected value, actual value) Otherwise the output reads backward when it fails. https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:323: m_layerTreeHost->initializeRendererIfNeeded(); I don't think you should need this line? https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:335: virtual void didCommit() OVERRIDE { nit: { on new line https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:344: virtual void afterTest() OVERRIDE { nit: { on new line https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:352: MULTI_THREAD_TEST_F(TextureLayerImplWithMailbox); would this work as SINGLE_AND_MULTI_THREAD_TEST_F? If so I prefer that.
New patch https://codereview.chromium.org/11638028/diff/25001/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11638028/diff/25001/cc/resource_provider.cc#n... cc/resource_provider.cc:245: GLC(context3d, textureId = context3d->createTexture()); Copy-pasted code from above, I'll clean up. On 2013/01/03 23:40:14, jamesr wrote: > just do the assignment textureID = context3D->createTexture() on the previous > line. there's no point in doing a glGetError after a createTexture call since it > just allocates a client side id https://codereview.chromium.org/11638028/diff/25001/cc/resource_provider.cc#n... cc/resource_provider.cc:248: GLC(context3d, context3d->flush()); Backing texture for the mailbox is in a different context, we need to make sure it latches to the newly generated texture id before we start using it. On 2013/01/03 23:40:14, jamesr wrote: > similarly no reason to glGetError after a flush - if the consume call generated > an error the GLC macro would catch it there > > why are you flushing here? for correctness? https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest.cc File cc/texture_layer_unittest.cc (right): https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:219: { It's to ensure the object receiving callbacks outlives the layer since I am testing callbacks from the destructor. On 2013/01/04 15:10:35, danakj wrote: > the extra scoping/indent here doesn't seem useful? https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:248: Release(m_testData.m_mailbox1, _)).Times(1); This next expectation is for mailbox1 set below, that gets triggered on destruction. On 2013/01/04 15:10:35, danakj wrote: > Since you just cleared it, how will this happen? > > Without the VerifyAndClearExpectations after, is this being checked? https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:267: TEST_F(TextureLayerWithMailboxTest, syncImplAndGetCallback) On 2013/01/04 15:10:35, danakj wrote: > I think this test name is wrong, it is not doing anything with impl in this test > at all AFAICT. > > I think it's more like replaceMailboxOnMainThreadBeforeCommit. Done. https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:280: Release(m_testData.m_mailbox1, _)).Times(1); On 2013/01/04 15:10:35, danakj wrote: > Seems like this EXPECT should move to line 287? Done. https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:323: m_layerTreeHost->initializeRendererIfNeeded(); On 2013/01/04 15:10:35, danakj wrote: > I don't think you should need this line? Done. https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:335: virtual void didCommit() OVERRIDE { On 2013/01/04 15:10:35, danakj wrote: > nit: { on new line Done. https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:344: virtual void afterTest() OVERRIDE { On 2013/01/04 15:10:35, danakj wrote: > nit: { on new line Done. https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:352: MULTI_THREAD_TEST_F(TextureLayerImplWithMailbox); On 2013/01/04 15:10:35, danakj wrote: > would this work as SINGLE_AND_MULTI_THREAD_TEST_F? If so I prefer that. Done.
Thanks, this LGTM though I think we could still test that the callback happens at the right time when the resource is in use with the consumer. Comment below to that effect. https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest.cc File cc/texture_layer_unittest.cc (right): https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:219: { On 2013/01/04 16:20:48, alexst wrote: > It's to ensure the object receiving callbacks outlives the layer since I am > testing callbacks from the destructor. I see. Can you add a quick comment here to that effect so it doesn't get refactored into a flaky crash or something in the future? https://codereview.chromium.org/11638028/diff/25001/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:239: // Test resetting the mailbox. On 2013/01/04 15:10:35, danakj wrote: > How about the case when the resource from the mailbox is in use by a consumer > (the browser)? Can we test that too? Do you think this is something we can test here? Maybe call prepareSendToParent() with the texture, then set a new mailbox and/or delete the layer, and make sure the callback happens on receiveFromParent()? https://codereview.chromium.org/11638028/diff/15005/cc/texture_layer.cc File cc/texture_layer.cc (right): https://codereview.chromium.org/11638028/diff/15005/cc/texture_layer.cc#newco... cc/texture_layer.cc:15: static void runCallbackOnMainThread(const TextureLayer::MailboxCallback& callback, unsigned syncPoint) { nit: { on new line https://codereview.chromium.org/11638028/diff/15005/cc/texture_layer.cc#newco... cc/texture_layer.cc:19: static void postCallbackToMainThread(Thread *mainThread, const TextureLayer::MailboxCallback& callback, unsigned syncPoint) { nit: { on new line (old style files make me sad at this point) https://codereview.chromium.org/11638028/diff/15005/cc/texture_layer.h File cc/texture_layer.h (right): https://codereview.chromium.org/11638028/diff/15005/cc/texture_layer.h#newcode32 cc/texture_layer.h:32: // Used when mailbox names are specified instead of texture IDs I'ma let you finish, but this is the nittiest comment of all time. nit: period at the end of the sentence.
https://codereview.chromium.org/11638028/diff/15005/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11638028/diff/15005/cc/resource_provider.cc#n... cc/resource_provider.cc:247: context3d->flush(); I'd still like to understand what the meaning of this flush is - is it for synchronization with other command buffers, synchronization with GL contexts, or something else?
> Do you think this is something we can test here? Maybe call > prepareSendToParent() with the texture, then set a new mailbox and/or delete the > layer, and make sure the callback happens on receiveFromParent()? I forgot to comment on that, but yes. I was actually looking to see how to make it happen.
On Fri, Jan 4, 2013 at 8:20 AM, <alexst@chromium.org> wrote: > > https://codereview.chromium.**org/11638028/diff/25001/cc/** > resource_provider.cc#**newcode248<https://codereview.chromium.org/11638028/diff/25001/cc/resource_provider.cc#newcode248> > cc/resource_provider.cc:248: GLC(context3d, context3d->flush()); > Backing texture for the mailbox is in a different context, we need to > make sure > it latches to the newly generated texture id before we start using it. > Could you expand on this? What's the race condition you are attempting to avoid and how does this avoid it? In general, I expect everything mailbox-related to be tied to sync points, not flush ordering.
Actually, come to think of it, it's a remnant of my earlier prototype, where the mailbox was consumed by a different context on the same channel than the one that was doing the rendering, hence the flush to make sure the rendering context saw the changes. Since consume now happens in the resource provider, it's the same context that does both consume and rendering and the flush is not needed. Good catch, guys, thank you for paying attention. > synchronization with other command buffers, synchronization with GL contexts, or > something else?
> > How about the case when the resource from the mailbox is in use by a consumer > > (the browser)? Can we test that too? > > Do you think this is something we can test here? Maybe call > prepareSendToParent() with the texture, then set a new mailbox and/or delete the > layer, and make sure the callback happens on receiveFromParent()? Added the tests, new patch is up.
https://codereview.chromium.org/11638028/diff/27002/cc/texture_layer_unittest.cc File cc/texture_layer_unittest.cc (right): https://codereview.chromium.org/11638028/diff/27002/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:19: no newline here https://codereview.chromium.org/11638028/diff/27002/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:375: implLayer->willDraw(m_hostImpl.activeTree()->resource_provider()); if you didDraw() after this, would the test still work? if so, i'd prefer you did that, so that we keep them paired and avoid future weirdness. https://codereview.chromium.org/11638028/diff/27002/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:396: provider->deleteResource(id); can you check expectations after this delete that EXPECT_CALL Times(0). Then check after receive that Times(1) ?
https://codereview.chromium.org/11638028/diff/27002/cc/texture_layer_unittest.cc File cc/texture_layer_unittest.cc (right): https://codereview.chromium.org/11638028/diff/27002/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:19: On 2013/01/07 18:24:28, danakj wrote: > no newline here Done. https://codereview.chromium.org/11638028/diff/27002/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:375: implLayer->willDraw(m_hostImpl.activeTree()->resource_provider()); Yes, should work, added the call. On 2013/01/07 18:24:28, danakj wrote: > if you didDraw() after this, would the test still work? if so, i'd prefer you > did that, so that we keep them paired and avoid future weirdness. https://codereview.chromium.org/11638028/diff/27002/cc/texture_layer_unittest... cc/texture_layer_unittest.cc:396: provider->deleteResource(id); On 2013/01/07 18:24:28, danakj wrote: > can you check expectations after this delete that EXPECT_CALL Times(0). > > Then check after receive that Times(1) ? Done.
Thanks and LGTM
I still think a typedef for base::Callback<void(unsigned)> would make the code shorter and clearer
On 2013/01/07 21:08:51, jamesr wrote: > I still think a typedef for base::Callback<void(unsigned)> would make the code > shorter and clearer There's TextureLayer::MailboxCallback. Maybe it got missed being used somewhere..
https://codereview.chromium.org/11638028/diff/32013/cc/transferable_resource.h File cc/transferable_resource.h (right): https://codereview.chromium.org/11638028/diff/32013/cc/transferable_resource.... cc/transferable_resource.h:23: base::Callback<void(unsigned)> releaseCallback; oh I see. How about Mailbox::ReleaseCallback, and have texture layer etc use that?
https://codereview.chromium.org/11638028/diff/32013/cc/resource_provider.h File cc/resource_provider.h (right): https://codereview.chromium.org/11638028/diff/32013/cc/resource_provider.h#ne... cc/resource_provider.h:89: ResourceId createResourceFromTextureMailbox(const std::string& mailboxName, const base::Callback<void(unsigned)>& releaseCallback); this is what I was talking about. ResourceProvider shouldn't depend on TextureLayer, though - maybe this one is OK (TextureLayer can't really depend on ResourceProvider directly either)
https://codereview.chromium.org/11638028/diff/32013/cc/resource_provider.h File cc/resource_provider.h (right): https://codereview.chromium.org/11638028/diff/32013/cc/resource_provider.h#ne... cc/resource_provider.h:89: ResourceId createResourceFromTextureMailbox(const std::string& mailboxName, const base::Callback<void(unsigned)>& releaseCallback); On 2013/01/07 21:11:49, jamesr wrote: > this is what I was talking about. ResourceProvider shouldn't depend on > TextureLayer, though - maybe this one is OK (TextureLayer can't really depend on > ResourceProvider directly either) I agree it would look better. Maybe splitting the mailbox into its own header and putting the typedef there is a good way to go.
Just to follow up, do we have a consensus one way or another? I'd like to land this soon, so I can unblock my work on webview. If the lack of typedef is still bothering you, I can put up a follow on patch to clean it up.
It's not a blocking issue for me. On Mon, Jan 7, 2013 at 5:27 PM, <alexst@chromium.org> wrote: > Just to follow up, do we have a consensus one way or another? > > I'd like to land this soon, so I can unblock my work on webview. If the > lack of > typedef is still bothering you, I can put up a follow on patch to clean it > up. > > https://codereview.chromium.**org/11638028/<https://codereview.chromium.org/1... >
Great, thanks for the feedback. I'll start landing it. On 2013/01/08 01:28:27, jamesr1 wrote: > It's not a blocking issue for me. > > On Mon, Jan 7, 2013 at 5:27 PM, <mailto:alexst@chromium.org> wrote: > > > Just to follow up, do we have a consensus one way or another? > > > > I'd like to land this soon, so I can unblock my work on webview. If the > > lack of > > typedef is still bothering you, I can put up a follow on patch to clean it > > up. > > > > > https://codereview.chromium.**org/11638028/%3Chttps://codereview.chromium.org...> > >
On 2013/01/08 01:27:40, alexst wrote: > Just to follow up, do we have a consensus one way or another? > > I'd like to land this soon, so I can unblock my work on webview. If the lack of > typedef is still bothering you, I can put up a follow on patch to clean it up. I think a followup to move Mailbox to a header with a typedefed callback would be very nice. I'd be happy to review that :)
https://codereview.chromium.org/11638028/diff/32013/cc/transferable_resource.h File cc/transferable_resource.h (right): https://codereview.chromium.org/11638028/diff/32013/cc/transferable_resource.... cc/transferable_resource.h:23: base::Callback<void(unsigned)> releaseCallback; NAK here, this is intended to be a serialized type and we shouldn't serialize a callback. The callback should directly be on the ResourceProvider::Resource, there's no particular reason it needs to be on the Mailbox.
Couple of more things from my side, otherwise, modulo my other comment, this looks good. https://codereview.chromium.org/11638028/diff/32013/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11638028/diff/32013/cc/resource_provider.cc#n... cc/resource_provider.cc:246: GLC(context3d, context3d->consumeTextureCHROMIUM(GL_TEXTURE_2D, name)); As an optimization (maybe for a follow up, add a TODO?), it would be even better to delay the creation of the textureId (and the consume) until the lockForRead, so that in the ÜC case, we don't create/destroy a texture ID (which causes a flush) that we'll never use. https://codereview.chromium.org/11638028/diff/32013/cc/texture_layer_impl.cc File cc/texture_layer_impl.cc (right): https://codereview.chromium.org/11638028/diff/32013/cc/texture_layer_impl.cc#... cc/texture_layer_impl.cc:71: DCHECK(!resourceProvider->inUseByConsumer(m_externalTextureResource)); This check may not be true with ubercomp (because the resource may not be back from the parent yet), however it's also not needed, since if the resource is still in use, it'll just be marked for deletion, the deletion itself (and the callback) will happen once the texture comes back from the parent.
Moved the mailbox callback out of transferable_resource.h and addressed comments. I'll do a followup with clean up and lockForRead optimization. New patch. https://codereview.chromium.org/11638028/diff/32013/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11638028/diff/32013/cc/resource_provider.cc#n... cc/resource_provider.cc:246: GLC(context3d, context3d->consumeTextureCHROMIUM(GL_TEXTURE_2D, name)); On 2013/01/08 02:01:52, piman wrote: > As an optimization (maybe for a follow up, add a TODO?), it would be even better > to delay the creation of the textureId (and the consume) until the lockForRead, > so that in the ÜC case, we don't create/destroy a texture ID (which causes a > flush) that we'll never use. Done. https://codereview.chromium.org/11638028/diff/32013/cc/texture_layer_impl.cc File cc/texture_layer_impl.cc (right): https://codereview.chromium.org/11638028/diff/32013/cc/texture_layer_impl.cc#... cc/texture_layer_impl.cc:71: DCHECK(!resourceProvider->inUseByConsumer(m_externalTextureResource)); On 2013/01/08 02:01:52, piman wrote: > This check may not be true with ubercomp (because the resource may not be back > from the parent yet), however it's also not needed, since if the resource is > still in use, it'll just be marked for deletion, the deletion itself (and the > callback) will happen once the texture comes back from the parent. Done.
Cool, LGTM!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexst@chromium.org/11638028/41003
Message was sent while issue was closed.
Change committed as 175529 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
