Add gpu::MailboxHolder to hold state for a gpu::Mailbox

Add gpu::MailboxHolder to hold state for a gpu::Mailbox

gpu::Mailbox by itself can hold only a texture id, but in common usage it comes
with texture target and syncpoint information for cross-context sharing.  To
reduce repetition of this pattern, gpu::MailboxHolder holds:

* a gpu::Mailbox
* a GL texture target
* a syncpoint index

Refactor other classes to use a gpu::MailboxHolder instead of separate
gpu::Mailbox and associated state.

Syncpoints are created with uint32 indices; make sure all uses of syncpoints use uint32.

BUG=None
TEST=local build, unittests on CrOS snow, desktop Linux

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245730

[sheu, 2013-12-06 22:32:40]

As discussed.  danakj@: PTAL

[sheu, 2013-12-06 22:35:11]

It's probably most relevant just to look at gpu/command_buffer/common/mailbox.h
for now; the rest of the CL is mostly just one/two/three-liners and such.

[danakj, 2013-12-06 22:37:03]

https://chromiumcodereview.appspot.com/105743004/diff/200001/gpu/command_buff...
File gpu/command_buffer/common/mailbox.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/200001/gpu/command_buff...
gpu/command_buffer/common/mailbox.h:30: typedef
base::Callback<void(scoped_ptr<MailboxHolder>)> ReleaseCallback;
At least in cc, we also include a "is_lost" boolean in the release callback, and
we don't bother to pass back the target and mailbox id as the release-ee already
had that info. I think you should stick with just the simple struct for now.

https://chromiumcodereview.appspot.com/105743004/diff/200001/gpu/command_buff...
gpu/command_buffer/common/mailbox.h:38: void set_sync_point(uint32 sync_point) {
sync_point_ = sync_point; }
Can you do the set only if the |sync_point| != 0?

[sheu, 2013-12-06 22:54:11]

https://chromiumcodereview.appspot.com/105743004/diff/200001/gpu/command_buff...
File gpu/command_buffer/common/mailbox.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/200001/gpu/command_buff...
gpu/command_buffer/common/mailbox.h:30: typedef
base::Callback<void(scoped_ptr<MailboxHolder>)> ReleaseCallback;
On 2013/12/06 22:37:03, danakj wrote:
> At least in cc, we also include a "is_lost" boolean in the release callback,
and
> we don't bother to pass back the target and mailbox id as the release-ee
already
> had that info. I think you should stick with just the simple struct for now.

That's cc::ReleaseCallback (and cc::SingleReleaseCallback) you're referring to? 
They're cc::-specific, so I'm not feeling the need to let their definitions
influence how we define the callback in gpu::.

Anyways, I pass in the entire struct so we don't have to change all the callback
definitions later if more state is desired.  I'm not particularly set on this
though, so if you'd like to see a simpler callback definition I'll do that.  I'm
going to assume something like:

base::Callback<void(uint32 sync_point)>

https://chromiumcodereview.appspot.com/105743004/diff/200001/gpu/command_buff...
gpu/command_buffer/common/mailbox.h:38: void set_sync_point(uint32 sync_point) {
sync_point_ = sync_point; }
On 2013/12/06 22:37:03, danakj wrote:
> Can you do the set only if the |sync_point| != 0?

We use this also to replace cc::TextureMailbox::ResetSyncPoint(), by explicitly
setting to 0.

In any case, I'm not sure why there would be a reason for setting only if
|sync_point| != 0 ?

[piman, 2013-12-06 23:29:41]

Could you also port TransferableResource to using gpu::MailboxHolder? It also
has those 3 fields.
It's probably also useful to add a ParamTraits to be able to IPC MailboxHolder.

https://chromiumcodereview.appspot.com/105743004/diff/200001/content/browser/...
File content/browser/aura/owned_mailbox.cc (right):

https://chromiumcodereview.appspot.com/105743004/diff/200001/content/browser/...
content/browser/aura/owned_mailbox.cc:29:
mailbox_holder_.set_sync_point(sync_point);
We explicitly don't want to update the sync point if it's 0. It's for the case
where the compositor returned the mailbox without using it.

https://chromiumcodereview.appspot.com/105743004/diff/200001/gpu/command_buff...
File gpu/command_buffer/common/mailbox.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/200001/gpu/command_buff...
gpu/command_buffer/common/mailbox.h:28: class GPU_EXPORT MailboxHolder {
Should this be a struct? It's a value type, copyable and IPC-able, and all
accessors are trivial. We usually use structs for these kind, making it
explicit.

https://chromiumcodereview.appspot.com/105743004/diff/200001/gpu/command_buff...
gpu/command_buffer/common/mailbox.h:30: typedef
base::Callback<void(scoped_ptr<MailboxHolder>)> ReleaseCallback;
On 2013/12/06 22:54:12, sheu wrote:
> On 2013/12/06 22:37:03, danakj wrote:
> > At least in cc, we also include a "is_lost" boolean in the release callback,
> and
> > we don't bother to pass back the target and mailbox id as the release-ee
> already
> > had that info. I think you should stick with just the simple struct for now.
> 
> That's cc::ReleaseCallback (and cc::SingleReleaseCallback) you're referring
to? 
> They're cc::-specific, so I'm not feeling the need to let their definitions
> influence how we define the callback in gpu::.
> 
> Anyways, I pass in the entire struct so we don't have to change all the
callback
> definitions later if more state is desired.  I'm not particularly set on this
> though, so if you'd like to see a simpler callback definition I'll do that. 
I'm
> going to assume something like:
> 
> base::Callback<void(uint32 sync_point)>

I'm pretty meh about defining this here.
Among all the existing users of MailboxHolder there's only one that uses this
callback. I'm not convinced there is value in defining one true way of releasing
a mailbox (if this one-true-way doesn't apply to most existing cases), it seems
more work to try to fit all the use cases into a single callback model.

[sheu, 2013-12-07 01:03:50]

I was going to add the ParamTraits in the next CL with actual IPC.  But since
converting TransferableResource, I need it now, so it goes in here.  PTAL

https://chromiumcodereview.appspot.com/105743004/diff/200001/gpu/command_buff...
File gpu/command_buffer/common/mailbox.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/200001/gpu/command_buff...
gpu/command_buffer/common/mailbox.h:28: class GPU_EXPORT MailboxHolder {
On 2013/12/06 23:29:42, piman wrote:
> Should this be a struct? It's a value type, copyable and IPC-able, and all
> accessors are trivial. We usually use structs for these kind, making it
> explicit.

Sure.

https://chromiumcodereview.appspot.com/105743004/diff/200001/gpu/command_buff...
gpu/command_buffer/common/mailbox.h:30: typedef
base::Callback<void(scoped_ptr<MailboxHolder>)> ReleaseCallback;
On 2013/12/06 23:29:42, piman wrote:
> On 2013/12/06 22:54:12, sheu wrote:
> > On 2013/12/06 22:37:03, danakj wrote:
> > > At least in cc, we also include a "is_lost" boolean in the release
callback,
> > and
> > > we don't bother to pass back the target and mailbox id as the release-ee
> > already
> > > had that info. I think you should stick with just the simple struct for
now.
> > 
> > That's cc::ReleaseCallback (and cc::SingleReleaseCallback) you're referring
> to? 
> > They're cc::-specific, so I'm not feeling the need to let their definitions
> > influence how we define the callback in gpu::.
> > 
> > Anyways, I pass in the entire struct so we don't have to change all the
> callback
> > definitions later if more state is desired.  I'm not particularly set on
this
> > though, so if you'd like to see a simpler callback definition I'll do that. 
> I'm
> > going to assume something like:
> > 
> > base::Callback<void(uint32 sync_point)>
> 
> I'm pretty meh about defining this here.
> Among all the existing users of MailboxHolder there's only one that uses this
> callback. I'm not convinced there is value in defining one true way of
releasing
> a mailbox (if this one-true-way doesn't apply to most existing cases), it
seems
> more work to try to fit all the use cases into a single callback model.

I'm getting more users of the callback soon.  Right now, there's
media::VideoFrame; the desktop/tab capture path will use it too.  See:

https://chromiumcodereview.appspot.com/83793004/

[piman, 2013-12-07 01:32:33]

https://chromiumcodereview.appspot.com/105743004/diff/200001/gpu/command_buff...
File gpu/command_buffer/common/mailbox.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/200001/gpu/command_buff...
gpu/command_buffer/common/mailbox.h:30: typedef
base::Callback<void(scoped_ptr<MailboxHolder>)> ReleaseCallback;
On 2013/12/07 01:03:50, sheu wrote:
> On 2013/12/06 23:29:42, piman wrote:
> > On 2013/12/06 22:54:12, sheu wrote:
> > > On 2013/12/06 22:37:03, danakj wrote:
> > > > At least in cc, we also include a "is_lost" boolean in the release
> callback,
> > > and
> > > > we don't bother to pass back the target and mailbox id as the release-ee
> > > already
> > > > had that info. I think you should stick with just the simple struct for
> now.
> > > 
> > > That's cc::ReleaseCallback (and cc::SingleReleaseCallback) you're
referring
> > to? 
> > > They're cc::-specific, so I'm not feeling the need to let their
definitions
> > > influence how we define the callback in gpu::.
> > > 
> > > Anyways, I pass in the entire struct so we don't have to change all the
> > callback
> > > definitions later if more state is desired.  I'm not particularly set on
> this
> > > though, so if you'd like to see a simpler callback definition I'll do
that. 
> > I'm
> > > going to assume something like:
> > > 
> > > base::Callback<void(uint32 sync_point)>
> > 
> > I'm pretty meh about defining this here.
> > Among all the existing users of MailboxHolder there's only one that uses
this
> > callback. I'm not convinced there is value in defining one true way of
> releasing
> > a mailbox (if this one-true-way doesn't apply to most existing cases), it
> seems
> > more work to try to fit all the use cases into a single callback model.
> 
> I'm getting more users of the callback soon.  Right now, there's
> media::VideoFrame; the desktop/tab capture path will use it too.  See:
> 
> https://chromiumcodereview.appspot.com/83793004/

Right, but like I said, this doesn't fit all use case, it really depends on the
local framework. Also the callback really has nothing to do with the GPU process
protocol. It just doesn't belong here as the "one true way" of returning a
mailbox.
Truth is, VideoFrame's callback should also have the is_lost bool (right now we
drop it on the floor because we can't really do anything with it anyway). Then
it'll have to migrate off this callback. And maybe the other ones you're
planning on adding, too.

https://chromiumcodereview.appspot.com/105743004/diff/220001/gpu/ipc/gpu_comm...
File gpu/ipc/gpu_command_buffer_traits.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/220001/gpu/ipc/gpu_comm...
gpu/ipc/gpu_command_buffer_traits.h:36: struct GPU_EXPORT
ParamTraits<gpu::MailboxHolder> {
You should be able to use IPC_STRUCT_TRAITS_BEGIN/END

[sheu, 2013-12-07 01:44:14]

https://chromiumcodereview.appspot.com/105743004/diff/200001/gpu/command_buff...
File gpu/command_buffer/common/mailbox.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/200001/gpu/command_buff...
gpu/command_buffer/common/mailbox.h:30: typedef
base::Callback<void(scoped_ptr<MailboxHolder>)> ReleaseCallback;
On 2013/12/07 01:32:34, piman wrote:
> On 2013/12/07 01:03:50, sheu wrote:
> > On 2013/12/06 23:29:42, piman wrote:
> > > On 2013/12/06 22:54:12, sheu wrote:
> > > > On 2013/12/06 22:37:03, danakj wrote:
> > > > > At least in cc, we also include a "is_lost" boolean in the release
> > callback,
> > > > and
> > > > > we don't bother to pass back the target and mailbox id as the
release-ee
> > > > already
> > > > > had that info. I think you should stick with just the simple struct
for
> > now.
> > > > 
> > > > That's cc::ReleaseCallback (and cc::SingleReleaseCallback) you're
> referring
> > > to? 
> > > > They're cc::-specific, so I'm not feeling the need to let their
> definitions
> > > > influence how we define the callback in gpu::.
> > > > 
> > > > Anyways, I pass in the entire struct so we don't have to change all the
> > > callback
> > > > definitions later if more state is desired.  I'm not particularly set on
> > this
> > > > though, so if you'd like to see a simpler callback definition I'll do
> that. 
> > > I'm
> > > > going to assume something like:
> > > > 
> > > > base::Callback<void(uint32 sync_point)>
> > > 
> > > I'm pretty meh about defining this here.
> > > Among all the existing users of MailboxHolder there's only one that uses
> this
> > > callback. I'm not convinced there is value in defining one true way of
> > releasing
> > > a mailbox (if this one-true-way doesn't apply to most existing cases), it
> > seems
> > > more work to try to fit all the use cases into a single callback model.
> > 
> > I'm getting more users of the callback soon.  Right now, there's
> > media::VideoFrame; the desktop/tab capture path will use it too.  See:
> > 
> > https://chromiumcodereview.appspot.com/83793004/
> 
> Right, but like I said, this doesn't fit all use case, it really depends on
the
> local framework. Also the callback really has nothing to do with the GPU
process
> protocol. It just doesn't belong here as the "one true way" of returning a
> mailbox.
> Truth is, VideoFrame's callback should also have the is_lost bool (right now
we
> drop it on the floor because we can't really do anything with it anyway). Then
> it'll have to migrate off this callback. And maybe the other ones you're
> planning on adding, too.

This isn't really inherent to the gpu::MailboxHolder, I agree.  What I see
though is that there are already two places that have the pattern of having a
callback to be run when a Mailbox is done with:

* cc::ReleaseCallback (and cc::SingleReleaseCallback)
* media::VideoFrame::MailboxHolder::TextureNoLongerNeededCallback

I'm going to add a third one to the video capture pipeline.  And this one
shouldn't take a cc::-specific type like ReleaseCallback.  My concern then is to
avoid a third almost-identical type definition, so if you have any better ideas,
I'm game.

[piman, 2013-12-07 01:53:58]

On Fri, Dec 6, 2013 at 5:44 PM, <sheu@chromium.org> wrote:

>
> https://chromiumcodereview.appspot.com/105743004/diff/
> 200001/gpu/command_buffer/common/mailbox.h
> File gpu/command_buffer/common/mailbox.h (right):
>
> https://chromiumcodereview.appspot.com/105743004/diff/
> 200001/gpu/command_buffer/common/mailbox.h#newcode30
> gpu/command_buffer/common/mailbox.h:30: typedef
> base::Callback<void(scoped_ptr<MailboxHolder>)> ReleaseCallback;
> On 2013/12/07 01:32:34, piman wrote:
>
>> On 2013/12/07 01:03:50, sheu wrote:
>> > On 2013/12/06 23:29:42, piman wrote:
>> > > On 2013/12/06 22:54:12, sheu wrote:
>> > > > On 2013/12/06 22:37:03, danakj wrote:
>> > > > > At least in cc, we also include a "is_lost" boolean in the
>>
> release
>
>> > callback,
>> > > > and
>> > > > > we don't bother to pass back the target and mailbox id as the
>>
> release-ee
>
>> > > > already
>> > > > > had that info. I think you should stick with just the simple
>>
> struct for
>
>> > now.
>> > > >
>> > > > That's cc::ReleaseCallback (and cc::SingleReleaseCallback)
>>
> you're
>
>> referring
>> > > to?
>> > > > They're cc::-specific, so I'm not feeling the need to let their
>> definitions
>> > > > influence how we define the callback in gpu::.
>> > > >
>> > > > Anyways, I pass in the entire struct so we don't have to change
>>
> all the
>
>> > > callback
>> > > > definitions later if more state is desired.  I'm not
>>
> particularly set on
>
>> > this
>> > > > though, so if you'd like to see a simpler callback definition
>>
> I'll do
>
>> that.
>> > > I'm
>> > > > going to assume something like:
>> > > >
>> > > > base::Callback<void(uint32 sync_point)>
>> > >
>> > > I'm pretty meh about defining this here.
>> > > Among all the existing users of MailboxHolder there's only one
>>
> that uses
>
>> this
>> > > callback. I'm not convinced there is value in defining one true
>>
> way of
>
>> > releasing
>> > > a mailbox (if this one-true-way doesn't apply to most existing
>>
> cases), it
>
>> > seems
>> > > more work to try to fit all the use cases into a single callback
>>
> model.
>
>> >
>> > I'm getting more users of the callback soon.  Right now, there's
>> > media::VideoFrame; the desktop/tab capture path will use it too.
>>
> See:
>
>> >
>> > https://chromiumcodereview.appspot.com/83793004/
>>
>
>  Right, but like I said, this doesn't fit all use case, it really
>>
> depends on the
>
>> local framework. Also the callback really has nothing to do with the
>>
> GPU process
>
>> protocol. It just doesn't belong here as the "one true way" of
>>
> returning a
>
>> mailbox.
>> Truth is, VideoFrame's callback should also have the is_lost bool
>>
> (right now we
>
>> drop it on the floor because we can't really do anything with it
>>
> anyway). Then
>
>> it'll have to migrate off this callback. And maybe the other ones
>>
> you're
>
>> planning on adding, too.
>>
>
> This isn't really inherent to the gpu::MailboxHolder, I agree.  What I
> see though is that there are already two places that have the pattern of
> having a callback to be run when a Mailbox is done with:
>
>
> * cc::ReleaseCallback (and cc::SingleReleaseCallback)
> * media::VideoFrame::MailboxHolder::TextureNoLongerNeededCallback
>
> I'm going to add a third one to the video capture pipeline.  And this
> one shouldn't take a cc::-specific type like ReleaseCallback.  My
> concern then is to avoid a third almost-identical type definition, so if
> you have any better ideas, I'm game.
>

"almost" != identical.
There is cc::ReleaseCallback. There's no path where this migrates
to gpu::MailboxHolder::ReleaseCallback. It's specific to how mailboxes are
used in cc. So that one will stay in cc.
There is the VideoFrame one. It's specific to the needs of the media
pipeline. It belongs in media.
If you need one for the video capture pipeline, that is not the same as the
media pipeline, then you should add one in the video capture pipeline
framework. If it's the same as the VideoFrame one, maybe you can reuse that
one. I don't see a problem with either solution.


>
> https://chromiumcodereview.appspot.com/105743004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sheu, 2014-01-03 19:36:38]

Updated and rebased.  PTAL

[danakj, 2014-01-06 20:13:42]

LG overall

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/layers/textur...
File cc/layers/texture_layer_unittest.cc (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/layers/textur...
cc/layers/texture_layer_unittest.cc:343: mailbox1_ = TextureMailbox(m1, 1,
sync_point1_);
Can you either use GL_TEXTURE_2D here, or make some variables to give these "1"
and "2" literals names? Since we don't appear to test for them later at all,
probably the former is fine.

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/layers/textur...
cc/layers/texture_layer_unittest.cc:1835: mailbox.SetName(
Would it make sense to add a gpu::Mailbox::SetName(std::string) method (It could
DCHECK that the string is 64 bytes long), since this is getting duplicated a lot
now?

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/resources/res...
File cc/resources/resource_provider.cc (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/resources/res...
cc/resources/resource_provider.cc:748: resource->mailbox.mailbox().name));
Law of Demeter is sad about this. Can we have a name() method on TextureMailbox
that returns the mailbox().name?

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/resources/res...
File cc/resources/resource_provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/resources/res...
cc/resources/resource_provider_unittest.cc:105: GLuint InsertSyncPoint() {
return next_sync_point_++; }
I think for consistency, things that aren't part of the TestWebGraphicsContext3D
interface should be uint32, so everything except the waitSyncPoint and
insertSyncPointmethods?

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/resources/tex...
File cc/resources/texture_mailbox.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/resources/tex...
cc/resources/texture_mailbox.h:38: void set_sync_point(uint32 sync_point);
make this method inline as the name suggests it will be.

https://chromiumcodereview.appspot.com/105743004/diff/460001/content/browser/...
File content/browser/aura/owned_mailbox.cc (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/content/browser/...
content/browser/aura/owned_mailbox.cc:28: void
OwnedMailbox::set_sync_point(uint32 sync_point) {
keep the old method name, set_sync_point implies a simple inline method

https://chromiumcodereview.appspot.com/105743004/diff/460001/content/common/g...
File content/common/gpu/client/gl_helper.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/content/common/g...
content/common/gpu/client/gl_helper.h:299: // Creates a mailbox holder that is
attached to the given texture id. Returns
", with a sync point to wait on before using the mailbox." ?

https://chromiumcodereview.appspot.com/105743004/diff/460001/content/renderer...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/content/renderer...
content/renderer/media/android/webmediaplayer_android.cc:69:
scoped_ptr<gpu::MailboxHolder> mailbox_holder) {
I'm a little uncertain about returning a scoped_ptr here for a couple reasons:

1) It's not a pattern used anywhere else we've used mailboxes
2) The underlying mailbox is actually ref-counted, and the original creator of
the mailbox is already holding onto a reference to the mailbox with a texture
id. So this scoped_ptr isn't really matching any lifetime of the underlying
mailbox data.

That said I'm not an owner here so I won't block this change either if y'all
think it's appropriate.

https://chromiumcodereview.appspot.com/105743004/diff/460001/gpu/command_buff...
File gpu/command_buffer/common/mailbox.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/gpu/command_buff...
gpu/command_buffer/common/mailbox.h:28: struct GPU_EXPORT MailboxHolder {
1 class/struct per file.

https://chromiumcodereview.appspot.com/105743004/diff/460001/media/base/video...
File media/base/video_frame.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/media/base/video...
media/base/video_frame.h:264: scoped_ptr<gpu::MailboxHolder> mailbox_holder_;
There's really no reason to use a scoped_ptr here at all anymore, you could just
store a MailboxHolder directly, and pass it in by const-ref in the
WrapNativeTexture method, instead of dealing with scoped_ptrs. This would also
lead to not passing scoped ptrs in the callback anymore.

[sheu, 2014-01-06 22:44:43]

Updated.  danakj@: PTAL

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/layers/textur...
File cc/layers/texture_layer_unittest.cc (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/layers/textur...
cc/layers/texture_layer_unittest.cc:343: mailbox1_ = TextureMailbox(m1, 1,
sync_point1_);
On 2014/01/06 20:13:42, danakj wrote:
> Can you either use GL_TEXTURE_2D here, or make some variables to give these
"1"
> and "2" literals names? Since we don't appear to test for them later at all,
> probably the former is fine.

Done.

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/layers/textur...
cc/layers/texture_layer_unittest.cc:1835: mailbox.SetName(
On 2014/01/06 20:13:42, danakj wrote:
> Would it make sense to add a gpu::Mailbox::SetName(std::string) method (It
could
> DCHECK that the string is 64 bytes long), since this is getting duplicated a
lot
> now?

It's done this way only for unittests -- it's really just a test-hack way to
construct a mailbox.  I'd leave it as is.

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/resources/res...
File cc/resources/resource_provider.cc (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/resources/res...
cc/resources/resource_provider.cc:748: resource->mailbox.mailbox().name));
On 2014/01/06 20:13:42, danakj wrote:
> Law of Demeter is sad about this. Can we have a name() method on
TextureMailbox
> that returns the mailbox().name?

We need to have the mailbox() accessible for other things, through mailbox().  I
avoided making an accessor for name() here since then we could reach the mailbox
name through two paths -- mailbox().name and name(), which could be possibly
dangerous in the future (code getting out of sync, etc.)

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/resources/res...
File cc/resources/resource_provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/resources/res...
cc/resources/resource_provider_unittest.cc:105: GLuint InsertSyncPoint() {
return next_sync_point_++; }
On 2014/01/06 20:13:42, danakj wrote:
> I think for consistency, things that aren't part of the
TestWebGraphicsContext3D
> interface should be uint32, so everything except the waitSyncPoint and
> insertSyncPointmethods?

Sure.

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/resources/tex...
File cc/resources/texture_mailbox.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/resources/tex...
cc/resources/texture_mailbox.h:38: void set_sync_point(uint32 sync_point);
On 2014/01/06 20:13:42, danakj wrote:
> make this method inline as the name suggests it will be.

Done.

https://chromiumcodereview.appspot.com/105743004/diff/460001/content/browser/...
File content/browser/aura/owned_mailbox.cc (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/content/browser/...
content/browser/aura/owned_mailbox.cc:28: void
OwnedMailbox::set_sync_point(uint32 sync_point) {
On 2014/01/06 20:13:42, danakj wrote:
> keep the old method name, set_sync_point implies a simple inline method

Done.

https://chromiumcodereview.appspot.com/105743004/diff/460001/content/common/g...
File content/common/gpu/client/gl_helper.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/content/common/g...
content/common/gpu/client/gl_helper.h:299: // Creates a mailbox holder that is
attached to the given texture id. Returns
On 2014/01/06 20:13:42, danakj wrote:
> ", with a sync point to wait on before using the mailbox." ?

Done.

https://chromiumcodereview.appspot.com/105743004/diff/460001/content/renderer...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/content/renderer...
content/renderer/media/android/webmediaplayer_android.cc:69:
scoped_ptr<gpu::MailboxHolder> mailbox_holder) {
On 2014/01/06 20:13:42, danakj wrote:
> I'm a little uncertain about returning a scoped_ptr here for a couple reasons:
> 
> 1) It's not a pattern used anywhere else we've used mailboxes
> 2) The underlying mailbox is actually ref-counted, and the original creator of
> the mailbox is already holding onto a reference to the mailbox with a texture
> id. So this scoped_ptr isn't really matching any lifetime of the underlying
> mailbox data.
> 
> That said I'm not an owner here so I won't block this change either if y'all
> think it's appropriate.

Well, it's scoped_ptr-ing the MailboxHolder, not the Mailbox itself.  I think of
the Mailbox as the primitive that gets passed around/copied as necessary, and
the MailboxHolder being wrapping also some more state to go with it.  The former
doesn't need to be scoped_ptr; the latter should.

https://chromiumcodereview.appspot.com/105743004/diff/460001/gpu/command_buff...
File gpu/command_buffer/common/mailbox.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/gpu/command_buff...
gpu/command_buffer/common/mailbox.h:28: struct GPU_EXPORT MailboxHolder {
On 2014/01/06 20:13:42, danakj wrote:
> 1 class/struct per file.

Done.

https://chromiumcodereview.appspot.com/105743004/diff/460001/media/base/video...
File media/base/video_frame.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/media/base/video...
media/base/video_frame.h:264: scoped_ptr<gpu::MailboxHolder> mailbox_holder_;
On 2014/01/06 20:13:42, danakj wrote:
> There's really no reason to use a scoped_ptr here at all anymore, you could
just
> store a MailboxHolder directly, and pass it in by const-ref in the
> WrapNativeTexture method, instead of dealing with scoped_ptrs. This would also
> lead to not passing scoped ptrs in the callback anymore.

I made this a pointer so things that don't have a mailbox can have this member
be NULL.  The scoped_ptr bits just fall out of that decision -- it's
scoped_ptr-ing the MailboxHolder, not the Mailbox itself.

[danakj, 2014-01-06 23:28:32]

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/layers/textur...
File cc/layers/texture_layer_unittest.cc (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/layers/textur...
cc/layers/texture_layer_unittest.cc:1835: mailbox.SetName(
On 2014/01/06 22:44:44, sheu wrote:
> On 2014/01/06 20:13:42, danakj wrote:
> > Would it make sense to add a gpu::Mailbox::SetName(std::string) method (It
> could
> > DCHECK that the string is 64 bytes long), since this is getting duplicated a
> lot
> > now?
> 
> It's done this way only for unittests -- it's really just a test-hack way to
> construct a mailbox.  I'd leave it as is.

Then a helper method here or somewhere to produce a gpu::Mailbox from a string
(or from a char)?

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/resources/res...
File cc/resources/resource_provider.cc (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/resources/res...
cc/resources/resource_provider.cc:748: resource->mailbox.mailbox().name));
On 2014/01/06 22:44:44, sheu wrote:
> On 2014/01/06 20:13:42, danakj wrote:
> > Law of Demeter is sad about this. Can we have a name() method on
> TextureMailbox
> > that returns the mailbox().name?
> 
> We need to have the mailbox() accessible for other things, through mailbox(). 
I
> avoided making an accessor for name() here since then we could reach the
mailbox
> name through two paths -- mailbox().name and name(), which could be possibly
> dangerous in the future (code getting out of sync, etc.)

As it's just an accessor, I'd prefer a 2nd path here than reaching through so
many objects.

https://chromiumcodereview.appspot.com/105743004/diff/460001/content/renderer...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/content/renderer...
content/renderer/media/android/webmediaplayer_android.cc:69:
scoped_ptr<gpu::MailboxHolder> mailbox_holder) {
On 2014/01/06 22:44:44, sheu wrote:
> On 2014/01/06 20:13:42, danakj wrote:
> > I'm a little uncertain about returning a scoped_ptr here for a couple
reasons:
> > 
> > 1) It's not a pattern used anywhere else we've used mailboxes
> > 2) The underlying mailbox is actually ref-counted, and the original creator
of
> > the mailbox is already holding onto a reference to the mailbox with a
texture
> > id. So this scoped_ptr isn't really matching any lifetime of the underlying
> > mailbox data.
> > 
> > That said I'm not an owner here so I won't block this change either if y'all
> > think it's appropriate.
> 
> Well, it's scoped_ptr-ing the MailboxHolder, not the Mailbox itself.  I think
of
> the Mailbox as the primitive that gets passed around/copied as necessary, and
> the MailboxHolder being wrapping also some more state to go with it.  The
former
> doesn't need to be scoped_ptr; the latter should.

I understand, but I don't think scoped_ptr is adding value for you here in the
callback. It's not like there is only one place that has access to the contents
of the mailbox. The site that created the VideoFrame still has a texture id
which refs the mailbox, so this isn't an ownership transfer of the mailbox
itself. Further, they don't need to receive back the mailbox name or the target,
they only need the sync point. The scoped_ptr just feels like a bit of a lie to
me, destroying the scoped_ptr held object does not do anything at all.

https://chromiumcodereview.appspot.com/105743004/diff/460001/media/base/video...
File media/base/video_frame.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/media/base/video...
media/base/video_frame.h:264: scoped_ptr<gpu::MailboxHolder> mailbox_holder_;
On 2014/01/06 22:44:44, sheu wrote:
> On 2014/01/06 20:13:42, danakj wrote:
> > There's really no reason to use a scoped_ptr here at all anymore, you could
> just
> > store a MailboxHolder directly, and pass it in by const-ref in the
> > WrapNativeTexture method, instead of dealing with scoped_ptrs. This would
also
> > lead to not passing scoped ptrs in the callback anymore.
> 
> I made this a pointer so things that don't have a mailbox can have this member
> be NULL.  The scoped_ptr bits just fall out of that decision -- it's
> scoped_ptr-ing the MailboxHolder, not the Mailbox itself.

Ok, sure. Using a scoped_ptr here to avoid the size of the MailboxHolder is
fine.

[sheu, 2014-01-07 01:40:58]

Updated.  danakj@: PTAL

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/resources/res...
File cc/resources/resource_provider.cc (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/cc/resources/res...
cc/resources/resource_provider.cc:748: resource->mailbox.mailbox().name));
On 2014/01/06 23:28:33, danakj wrote:
> On 2014/01/06 22:44:44, sheu wrote:
> > On 2014/01/06 20:13:42, danakj wrote:
> > > Law of Demeter is sad about this. Can we have a name() method on
> > TextureMailbox
> > > that returns the mailbox().name?
> > 
> > We need to have the mailbox() accessible for other things, through
mailbox(). 
> I
> > avoided making an accessor for name() here since then we could reach the
> mailbox
> > name through two paths -- mailbox().name and name(), which could be possibly
> > dangerous in the future (code getting out of sync, etc.)
> 
> As it's just an accessor, I'd prefer a 2nd path here than reaching through so
> many objects.

Done.

https://chromiumcodereview.appspot.com/105743004/diff/460001/content/renderer...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://chromiumcodereview.appspot.com/105743004/diff/460001/content/renderer...
content/renderer/media/android/webmediaplayer_android.cc:69:
scoped_ptr<gpu::MailboxHolder> mailbox_holder) {
On 2014/01/06 23:28:33, danakj wrote:
> On 2014/01/06 22:44:44, sheu wrote:
> > On 2014/01/06 20:13:42, danakj wrote:
> > > I'm a little uncertain about returning a scoped_ptr here for a couple
> reasons:
> > > 
> > > 1) It's not a pattern used anywhere else we've used mailboxes
> > > 2) The underlying mailbox is actually ref-counted, and the original
creator
> of
> > > the mailbox is already holding onto a reference to the mailbox with a
> texture
> > > id. So this scoped_ptr isn't really matching any lifetime of the
underlying
> > > mailbox data.
> > > 
> > > That said I'm not an owner here so I won't block this change either if
y'all
> > > think it's appropriate.
> > 
> > Well, it's scoped_ptr-ing the MailboxHolder, not the Mailbox itself.  I
think
> of
> > the Mailbox as the primitive that gets passed around/copied as necessary,
and
> > the MailboxHolder being wrapping also some more state to go with it.  The
> former
> > doesn't need to be scoped_ptr; the latter should.
> 
> I understand, but I don't think scoped_ptr is adding value for you here in the
> callback. It's not like there is only one place that has access to the
contents
> of the mailbox. The site that created the VideoFrame still has a texture id
> which refs the mailbox, so this isn't an ownership transfer of the mailbox
> itself. Further, they don't need to receive back the mailbox name or the
target,
> they only need the sync point. The scoped_ptr just feels like a bit of a lie
to
> me, destroying the scoped_ptr held object does not do anything at all.

I think the ownership transfer semantics are really enforcing the idea that the
syncpoint is serializing all accesses on the mailbox.  If
DoReleaseRemotePlaybackTexture takes just a sync_point and not the whole
MailboxHolder struct, then it can't update the MailboxHolder's sync point
afterwards.  This is alright as long as the the implication of the release
callback is that the MailboxHolder is destroyed immediately after the callback
is run, i.e. the callback does not need to continue the "chain" of syncpoints.

This implication is safe for now since with this change
VideoFrame::WrapNativeTexture takes ownership of the MailboxHolder passed to it.
 If it were not the case, then it's possible that the caller of
VideoFrame::WrapNativeTexture could keep a copy of the MailboxHolder passed to
the VideoFrame, and not be expecting that the VideoFrame destructor calls a
callback that waits on the mailbox but does not update its syncpoint.  So I can
remove the ownership semantics here, as long as I don't remove them from
VideoFrame::WrapNativeTexture.

[piman, 2014-01-08 04:49:54]

LGTM once Dana is happy.

[danakj, 2014-01-08 17:59:14]

LGTM thanks!

https://chromiumcodereview.appspot.com/105743004/diff/1030001/media/base/vide...
File media/base/video_frame.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/1030001/media/base/vide...
media/base/video_frame.h:90: typedef base::Callback<void(gpu::MailboxHolder*)>
ReleaseMailboxCB;
what do you think of making it a const*?

[sheu, 2014-01-08 19:25:34]

More OWNERs.  PTAL, thanks!

enne@:  webkit/renderer/compositor_bindings/web_external_texture_layer_impl.cc
fischman@: media/*
cevans@: content/common/cc_messages.h

[enne (OOO), 2014-01-08 19:27:38]

webkit/renderer/compositor_bindings/ lgtm

[Chris Evans, 2014-01-09 03:33:59]

On 2014/01/08 19:27:38, enne wrote:
> webkit/renderer/compositor_bindings/ lgtm

Unless I missed something, IPC change looks trivial. lgtm.

[sheu, 2014-01-13 19:09:44]

Adding for OWNERs.

scherkus@: PTAL at media/*

Thanks!

[scherkus (not reviewing), 2014-01-13 19:25:26]

lgtm w/ pedantic nit

https://chromiumcodereview.appspot.com/105743004/diff/1690001/media/base/vide...
File media/base/video_frame.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/1690001/media/base/vide...
media/base/video_frame.h:18: 
pedantic nits: remove extra line of whitespace here and on line 20

[sheu, 2014-01-13 19:28:08]

https://chromiumcodereview.appspot.com/105743004/diff/1690001/media/base/vide...
File media/base/video_frame.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/1690001/media/base/vide...
media/base/video_frame.h:18: 
On 2014/01/13 19:25:27, scherkus wrote:
> pedantic nits: remove extra line of whitespace here and on line 20

More pedantry: I've seen the namespace declaration braces both with and without
leading/trailing whitespace.  Seems to differ file to file.  I did it here for
consistency because the rest of the file does have it -- see line 23.

Not a good reason to do it?  (I'll remove it if it's more "correct".)

[scherkus (not reviewing), 2014-01-13 19:34:52]

https://chromiumcodereview.appspot.com/105743004/diff/1690001/media/base/vide...
File media/base/video_frame.h (right):

https://chromiumcodereview.appspot.com/105743004/diff/1690001/media/base/vide...
media/base/video_frame.h:18: 
On 2014/01/13 19:28:09, sheu wrote:
> On 2014/01/13 19:25:27, scherkus wrote:
> > pedantic nits: remove extra line of whitespace here and on line 20
> 
> More pedantry: I've seen the namespace declaration braces both with and
without
> leading/trailing whitespace.  Seems to differ file to file.  I did it here for
> consistency because the rest of the file does have it -- see line 23.
> 
> Not a good reason to do it?  (I'll remove it if it's more "correct".)

I really don't care about this too much, but the general convention I've
witnessed is to skip the extra whitespace for namespaced forward decls

Here's an example for forward decl'ing base in media code:
https://code.google.com/p/chromium/codesearch#search/&q=%5Enamespace%5C%20bas...

[commit-bot: I haz the power, 2014-01-13 19:51:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/105743004/1830001

[commit-bot: I haz the power, 2014-01-13 20:34:38]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-14 22:43:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/105743004/2090001

[commit-bot: I haz the power, 2014-01-15 00:53:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/105743004/2090001

[commit-bot: I haz the power, 2014-01-16 20:27:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/105743004/2400001

[commit-bot: I haz the power, 2014-01-16 20:27:24]

Failed to apply patch for cc/trees/layer_tree_host_unittest_context.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/trees/layer_tree_host_unittest_context.cc
  Hunk #1 FAILED at 1029.
  Hunk #2 succeeded at 1044 (offset -26 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
cc/trees/layer_tree_host_unittest_context.cc.rej

Patch:       cc/trees/layer_tree_host_unittest_context.cc
Index: cc/trees/layer_tree_host_unittest_context.cc
diff --git a/cc/trees/layer_tree_host_unittest_context.cc
b/cc/trees/layer_tree_host_unittest_context.cc
index
b53127d4d9e09f4fabc70613478c228740de9088..f1b26fff23844aa2fd119bd99eb9affb9d8a83f1
100644
--- a/cc/trees/layer_tree_host_unittest_context.cc
+++ b/cc/trees/layer_tree_host_unittest_context.cc
@@ -1029,10 +1029,10 @@ class LayerTreeHostContextTestDontUseLostResources
     texture->SetAnchorPoint(gfx::PointF());
     texture->SetIsDrawable(true);
     texture->SetTextureMailbox(
-        TextureMailbox(mailbox, sync_point),
-        SingleReleaseCallback::Create(base::Bind(
-            &LayerTreeHostContextTestDontUseLostResources::
-                EmptyReleaseCallback)));
+        TextureMailbox(mailbox, GL_TEXTURE_2D, sync_point),
+        SingleReleaseCallback::Create(
+            base::Bind(&LayerTreeHostContextTestDontUseLostResources::
+                            EmptyReleaseCallback)));
     root->AddChild(texture);
 
     scoped_refptr<ContentLayer> mask = ContentLayer::Create(&client_);
@@ -1070,30 +1070,24 @@ class LayerTreeHostContextTestDontUseLostResources
 
     color_video_frame_ = VideoFrame::CreateColorFrame(
         gfx::Size(4, 4), 0x80, 0x80, 0x80, base::TimeDelta());
-    hw_video_frame_ = VideoFrame::WrapNativeTexture(
-        make_scoped_ptr(new VideoFrame::MailboxHolder(
-            mailbox,
-            sync_point,
-            VideoFrame::MailboxHolder::TextureNoLongerNeededCallback())),
-        GL_TEXTURE_2D,
-        gfx::Size(4, 4),
-        gfx::Rect(0, 0, 4, 4),
-        gfx::Size(4, 4),
-        base::TimeDelta(),
-        VideoFrame::ReadPixelsCB(),
-        base::Closure());
-    scaled_hw_video_frame_ = VideoFrame::WrapNativeTexture(
-        make_scoped_ptr(new VideoFrame::MailboxHolder(
-            mailbox,
-            sync_point,
-            VideoFrame::MailboxHolder::TextureNoLongerNeededCallback())),
-        GL_TEXTURE_2D,
-        gfx::Size(4, 4),
-        gfx::Rect(0, 0, 3, 2),
-        gfx::Size(4, 4),
-        base::TimeDelta(),
-        VideoFrame::ReadPixelsCB(),
-        base::Closure());
+    hw_video_frame_ =
+        VideoFrame::WrapNativeTexture(make_scoped_ptr(new gpu::MailboxHolder(
+                                          mailbox, GL_TEXTURE_2D, sync_point)),
+                                      media::VideoFrame::ReleaseMailboxCB(),
+                                      gfx::Size(4, 4),
+                                      gfx::Rect(0, 0, 4, 4),
+                                      gfx::Size(4, 4),
+                                      base::TimeDelta(),
+                                      VideoFrame::ReadPixelsCB());
+    scaled_hw_video_frame_ =
+        VideoFrame::WrapNativeTexture(make_scoped_ptr(new gpu::MailboxHolder(
+                                          mailbox, GL_TEXTURE_2D, sync_point)),
+                                      media::VideoFrame::ReleaseMailboxCB(),
+                                      gfx::Size(4, 4),
+                                      gfx::Rect(0, 0, 3, 2),
+                                      gfx::Size(4, 4),
+                                      base::TimeDelta(),
+                                      VideoFrame::ReadPixelsCB());
 
     color_frame_provider_.set_frame(color_video_frame_);
     hw_frame_provider_.set_frame(hw_video_frame_);

[commit-bot: I haz the power, 2014-01-17 00:46:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/105743004/2460001

[commit-bot: I haz the power, 2014-01-17 01:54:28]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2014-01-17 01:58:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/105743004/2460001

[commit-bot: I haz the power, 2014-01-17 02:06:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/105743004/2850001

[commit-bot: I haz the power, 2014-01-17 02:56:19]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2014-01-18 00:48:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/105743004/2980001

[commit-bot: I haz the power, 2014-01-18 01:15:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/105743004/2870002

[commit-bot: I haz the power, 2014-01-18 02:42:28]

Change committed as 245730