Send hardware video frames with mailboxes.

Send hardware video frames with mailboxes.

Change media::VideoFrame from holding a raw texture id to
instead hold a gpu::Mailbox. The hardware video decoders
produce a mailbox on their context from the texture id
before putting it in the VideoFrame, and consume the
mailbox again when they receive the texture id back.

In the compositor, we hold onto the VideoFrame::MailboxHolder
as long as the mailboxed texture is in use in the compositor
(or its parent). This keeps the video decoders from writing
to the texture id, as they register a callback in the
VideoFrame to be called when it is destroyed.

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

[danakj, 2013-04-11 22:09:32]

This CL covers everything except android.

@scherkus could you please review the media stuff. I'll fill in the android
stuff if the rest is looking good.

@piman please review cc and mailbox-ness throughout.

[piman, 2013-04-11 22:57:57]

https://codereview.chromium.org/14199002/diff/2001/cc/resources/video_resourc...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/14199002/diff/2001/cc/resources/video_resourc...
cc/resources/video_resource_updater.cc:352: GLC(context,
context->waitSyncPoint(sync_point));
You need to (shallow) flush here to ensure the video decoder context processing
of the mailbox will be ordered wrt the wait.

Alternative is to pass the sync point all the way to the
RendererGpuVideoDecoderFactories so that it waits just before the consume. It's
the usual pattern, but in this case since you know both contexts are in the same
channel, you don't technically need that - up to you.

https://codereview.chromium.org/14199002/diff/2001/content/renderer/media/ren...
File content/renderer/media/renderer_gpu_video_decoder_factories.cc (right):

https://codereview.chromium.org/14199002/diff/2001/content/renderer/media/ren...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:192:
gles2->ProduceTextureCHROMIUM(GL_TEXTURE_2D, mailbox->name);
You probably need to shallow Flush so that the compositor context is properly
ordered wrt the Produce.
Alternatively, make a sync point and pass it through.

https://codereview.chromium.org/14199002/diff/2001/media/filters/gpu_video_de...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/14199002/diff/2001/media/filters/gpu_video_de...
media/filters/gpu_video_decoder.cc:436: gpu::Mailbox mailbox =
factories_->ProduceTextureToMailbox(pb.texture_id());
The first time you produce (or when you consume), the mailbox is associated with
the texture ID, and you can't produce again to a different mailbox (you'll
trigger asserts I believe). Could we keep the mailbox in the PictureBuffer and
create it lazily, passing it to ProduceTextureToMailbox?

https://codereview.chromium.org/14199002/diff/2001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/14199002/diff/2001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:725: bool
WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(
Is this the same as the webgl thing for printing? I.e. do you need to wait for
refcounting + read-only sharing for this to land?

[danakj, 2013-04-11 23:49:08]

Thanks, I'll fix up the flushing etc soon.

https://codereview.chromium.org/14199002/diff/2001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/14199002/diff/2001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:725: bool
WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(
On 2013/04/11 22:57:58, piman wrote:
> Is this the same as the webgl thing for printing? I.e. do you need to wait for
> refcounting + read-only sharing for this to land?

I think this should work fine now without ubercomp. It would break if you turned
on ubercomp without ref-counting landing first. Or is that a misunderstanding.
If that's true is it okay to land?

[danakj, 2013-04-12 00:01:39]

https://codereview.chromium.org/14199002/diff/2001/content/renderer/media/ren...
File content/renderer/media/renderer_gpu_video_decoder_factories.cc (right):

https://codereview.chromium.org/14199002/diff/2001/content/renderer/media/ren...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:192:
gles2->ProduceTextureCHROMIUM(GL_TEXTURE_2D, mailbox->name);
Should be fine to stick a sync point into the VideoFrame for the outgoing
mailbox.

The one coming back is trickier, the compositor doesn't have visibility into the
VideoFrame for the callback. Maybe the compositor just needs to replace the
sync_point in the video frame with its own, and then have the VideoFrame pass
its sync point in the release callback. I'll see how that looks.

https://codereview.chromium.org/14199002/diff/2001/media/filters/gpu_video_de...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/14199002/diff/2001/media/filters/gpu_video_de...
media/filters/gpu_video_decoder.cc:436: gpu::Mailbox mailbox =
factories_->ProduceTextureToMailbox(pb.texture_id());
Oh okay. I had in my head that Gen and Produce are the same thing at first, but
they are separate. I should do the Gen when the texture id is made, and then do
produce here as usual, I guess. I had originally had the mailbox in the Picture
but that didn't work with doing Gen here. I'll do just what you suggest, thanks.

[piman, 2013-04-12 00:54:25]

https://codereview.chromium.org/14199002/diff/2001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/14199002/diff/2001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:725: bool
WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(
On 2013/04/11 23:49:08, danakj wrote:
> On 2013/04/11 22:57:58, piman wrote:
> > Is this the same as the webgl thing for printing? I.e. do you need to wait
for
> > refcounting + read-only sharing for this to land?
> 
> I think this should work fine now without ubercomp. It would break if you
turned
> on ubercomp without ref-counting landing first. Or is that a misunderstanding.
> If that's true is it okay to land?

As it is today, Produce/Consume is (locally) destructive, so  I'm going to ask
more questions:
1- is this called exclusively while the current frame is "owned" by the media
player side?
2- the locking seems insufficient, but I don't know the threading expectations.
What prevents another thread from consuming the mailbox too, at the same time?
3- don't you need to produce the texture again before deleting?

[piman, 2013-04-12 00:56:32]

On Thu, Apr 11, 2013 at 5:01 PM, <danakj@chromium.org> wrote:

>
> https://codereview.chromium.**org/14199002/diff/2001/**
>
content/renderer/media/**renderer_gpu_video_decoder_**factories.cc<https://codereview.chromium.org/14199002/diff/2001/content/renderer/media/renderer_gpu_video_decoder_factories.cc>
> File content/renderer/media/**renderer_gpu_video_decoder_**factories.cc
> (right):
>
> https://codereview.chromium.**org/14199002/diff/2001/**
> content/renderer/media/**renderer_gpu_video_decoder_**
>
factories.cc#newcode192<https://codereview.chromium.org/14199002/diff/2001/content/renderer/media/renderer_gpu_video_decoder_factories.cc#newcode192>
> content/renderer/media/**renderer_gpu_video_decoder_**factories.cc:192:
> gles2->ProduceTextureCHROMIUM(**GL_TEXTURE_2D, mailbox->name);
> Should be fine to stick a sync point into the VideoFrame for the
> outgoing mailbox.
>
> The one coming back is trickier, the compositor doesn't have visibility
> into the VideoFrame for the callback. Maybe the compositor just needs to
> replace the sync_point in the video frame with its own, and then have
> the VideoFrame pass its sync point in the release callback. I'll see how
> that looks.


Yeah, either way. A shallow flush rather than passing around sync points is
fine too in this case (InsertSyncPoint does a shallow flush anyway - if it
matters).

>
>
> https://codereview.chromium.**org/14199002/diff/2001/media/**
>
filters/gpu_video_decoder.cc<https://codereview.chromium.org/14199002/diff/2001/media/filters/gpu_video_decoder.cc>
> File media/filters/gpu_video_**decoder.cc (right):
>
> https://codereview.chromium.**org/14199002/diff/2001/media/**
>
filters/gpu_video_decoder.cc#**newcode436<https://codereview.chromium.org/14199002/diff/2001/media/filters/gpu_video_decoder.cc#newcode436>
> media/filters/gpu_video_**decoder.cc:436: gpu::Mailbox mailbox =
> factories_->**ProduceTextureToMailbox(pb.**texture_id());
> Oh okay. I had in my head that Gen and Produce are the same thing at
> first, but they are separate. I should do the Gen when the texture id is
> made, and then do produce here as usual, I guess. I had originally had
> the mailbox in the Picture but that didn't work with doing Gen here.
> I'll do just what you suggest, thanks.
>

That sounds right.


>
>
https://codereview.chromium.**org/14199002/<https://codereview.chromium.org/1...
>

[danakj, 2013-04-12 17:08:06]

On 2013/04/12 00:54:25, piman wrote:
>
https://codereview.chromium.org/14199002/diff/2001/webkit/media/webmediaplaye...
> File webkit/media/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/14199002/diff/2001/webkit/media/webmediaplaye...
> webkit/media/webmediaplayer_impl.cc:725: bool
> WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(
> On 2013/04/11 23:49:08, danakj wrote:
> > On 2013/04/11 22:57:58, piman wrote:
> > > Is this the same as the webgl thing for printing? I.e. do you need to wait
> for
> > > refcounting + read-only sharing for this to land?
> > 
> > I think this should work fine now without ubercomp. It would break if you
> turned
> > on ubercomp without ref-counting landing first. Or is that a
misunderstanding.
> > If that's true is it okay to land?
> 
> As it is today, Produce/Consume is (locally) destructive, so  I'm going to ask
> more questions:

OK Thanks!

> 1- is this called exclusively while the current frame is "owned" by the media
> player side?

On android, this is called via WebMediaPlayerClientImpl::paint() only when
compositing is not active.

Otherwise:
WebGLRenderingContext::texImage2D
-> HTMLVideoElement::copyVideoTextureToPlatformTexture
-> MediaPlayer::copyVideoTextureToPlatformTexture
-> WebMediaPlayerClientImpl::copyVideoTextureToPlatformTexture
-> WebMediaPlayerImpl::copyVideoTextureToPlatformTexture

This can definitely happen while the VideoFrame is held by the compositor, and
the texture is in the browser.

Does ref-counted mailboxes not let me consume a mailbox that is currently
consumed elsewhere?

Would I have to do the consume on the mailbox in WebMediaPlayerImpl, then
produce a new mailbox to give to the compositor?

> 2- the locking seems insufficient, but I don't know the threading
expectations.
> What prevents another thread from consuming the mailbox too, at the same time?

While the VideoFrame has a reference on it, the texture won't be returned to the
decoder. That's the only other place doing a consume, so I think from the
media-player-side, only one thing will consume at a time since this code holds a
reference to the VideoFrame.

> 3- don't you need to produce the texture again before deleting?

Oh, I guess I saw the delete as an implicit produce :) Done.

[piman, 2013-04-12 17:45:04]

On Fri, Apr 12, 2013 at 10:08 AM, <danakj@chromium.org> wrote:

> On 2013/04/12 00:54:25, piman wrote:
>
> https://codereview.chromium.**org/14199002/diff/2001/webkit/**
>
media/webmediaplayer_impl.cc<https://codereview.chromium.org/14199002/diff/2001/webkit/media/webmediaplayer_impl.cc>
>
>> File webkit/media/webmediaplayer_**impl.cc (right):
>>
>
>
> https://codereview.chromium.**org/14199002/diff/2001/webkit/**
>
media/webmediaplayer_impl.cc#**newcode725<https://codereview.chromium.org/14199002/diff/2001/webkit/media/webmediaplayer_impl.cc#newcode725>
>
>> webkit/media/webmediaplayer_**impl.cc:725: bool
>> WebMediaPlayerImpl::**copyVideoTextureToPlatformText**ure(
>> On 2013/04/11 23:49:08, danakj wrote:
>> > On 2013/04/11 22:57:58, piman wrote:
>> > > Is this the same as the webgl thing for printing? I.e. do you need to
>> wait
>> for
>> > > refcounting + read-only sharing for this to land?
>> >
>> > I think this should work fine now without ubercomp. It would break if
>> you
>> turned
>> > on ubercomp without ref-counting landing first. Or is that a
>>
> misunderstanding.
>
>> > If that's true is it okay to land?
>>
>
>  As it is today, Produce/Consume is (locally) destructive, so  I'm going
>> to ask
>> more questions:
>>
>
> OK Thanks!
>
>
>  1- is this called exclusively while the current frame is "owned" by the
>> media
>> player side?
>>
>
> On android, this is called via WebMediaPlayerClientImpl::**paint() only
> when
> compositing is not active.
>
> Otherwise:
> WebGLRenderingContext::**texImage2D
> -> HTMLVideoElement::**copyVideoTextureToPlatformText**ure
> -> MediaPlayer::**copyVideoTextureToPlatformText**ure
> -> WebMediaPlayerClientImpl::**copyVideoTextureToPlatformText**ure
> -> WebMediaPlayerImpl::**copyVideoTextureToPlatformText**ure
>
> This can definitely happen while the VideoFrame is held by the compositor,
> and
> the texture is in the browser.
>
> Does ref-counted mailboxes not let me consume a mailbox that is currently
> consumed elsewhere?
>

refcounted+shared mailboxes will give you that but it's not implemented
like that yet.
Right now, think of it this way: the mailbox is what it says - a mailbox.
The texture only exists in a single place at any time (either in one of the
clients or in the mailbox), you use Produce/Consume to take it in/out of
the mailbox, but you can't steal a texture from another client.
Refcounted mailboxes fix some ugly semantics wrt destruction to avoid leaks.
Shared mailboxes change the semantics so that you can Produce once and
Consume multiple times from different clients, so that the texture would be
"mapped" in each client. There's very subtle things wrt changes in one
client potentially affecting others (think, e.g. one client bound the
texture to a FBO, then another one changes MIN_FILTER to use mipmapping,
making the texture incomplete), so we may need to add restrictions such as
a single client having write permissions or something.

In any case, as it's implemented now, you can't Consume the texture if it's
been already Consumed by the compositor, whether Über or not - so if it's
needed, you're running into the same issue as WebGL.

>
> Would I have to do the consume on the mailbox in WebMediaPlayerImpl, then
> produce a new mailbox to give to the compositor?


I'm not sure what you mean exactly, but with the restriction "the texture
is only in a single place at any time", I don't see a way around having to
wait for shared mailboxes if this is called while the frame is sent to the
compositor.

>
>
>  2- the locking seems insufficient, but I don't know the threading
>>
> expectations.
>
>> What prevents another thread from consuming the mailbox too, at the same
>> time?
>>
>
> While the VideoFrame has a reference on it, the texture won't be returned
> to the
> decoder. That's the only other place doing a consume, so I think from the
> media-player-side, only one thing will consume at a time since this code
> holds a
> reference to the VideoFrame.


>
>  3- don't you need to produce the texture again before deleting?
>>
>
> Oh, I guess I saw the delete as an implicit produce :) Done.
>
>
https://codereview.chromium.**org/14199002/<https://codereview.chromium.org/1...
>

[danakj, 2013-04-12 21:25:51]

On 2013/04/12 17:45:04, piman wrote:
> refcounted+shared mailboxes will give you that but it's not implemented
> like that yet.
> Right now, think of it this way: the mailbox is what it says - a mailbox.
> The texture only exists in a single place at any time (either in one of the
> clients or in the mailbox), you use Produce/Consume to take it in/out of
> the mailbox, but you can't steal a texture from another client.
> Refcounted mailboxes fix some ugly semantics wrt destruction to avoid leaks.
> Shared mailboxes change the semantics so that you can Produce once and
> Consume multiple times from different clients, so that the texture would be
> "mapped" in each client. There's very subtle things wrt changes in one
> client potentially affecting others (think, e.g. one client bound the
> texture to a FBO, then another one changes MIN_FILTER to use mipmapping,
> making the texture incomplete), so we may need to add restrictions such as
> a single client having write permissions or something.
> 
> In any case, as it's implemented now, you can't Consume the texture if it's
> been already Consumed by the compositor, whether Über or not - so if it's
> needed, you're running into the same issue as WebGL.

I see. It would seem that I've already broken this path then, since the
VideoResourceUpdater is doing a produce on the texture. The readback here would
be using an empty texture_id until shared mailboxes happen.

[scherkus (not reviewing), 2013-04-17 20:09:34]

most of the media changes look fine -- just nits 'n q's

https://codereview.chromium.org/14199002/diff/17004/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/14199002/diff/17004/media/base/video_frame.h#...
media/base/video_frame.h:103: const TextureNoLongerNeededCallback&
texture_no_longer_needed_cb,
OOC think we can remove the non-mailbox path after all is said and done?

https://codereview.chromium.org/14199002/diff/17004/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/14199002/diff/17004/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:771: #ifdef VIDEO_FRAME_MAILBOX
nit: doesn't chromium prefer #if defined(FOO) / #if !defined(FOO)?

[danakj, 2013-04-17 20:12:37]

https://codereview.chromium.org/14199002/diff/17004/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/14199002/diff/17004/media/base/video_frame.h#...
media/base/video_frame.h:103: const TextureNoLongerNeededCallback&
texture_no_longer_needed_cb,
On 2013/04/17 20:09:34, scherkus wrote:
> OOC think we can remove the non-mailbox path after all is said and done?

Yes absolutely. The no_longer_needed_cb is used for some other stuff (software
decoder things) so I thought to add a new callback instead of replacing it.

https://codereview.chromium.org/14199002/diff/17004/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/14199002/diff/17004/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:771: #ifdef VIDEO_FRAME_MAILBOX
On 2013/04/17 20:09:34, scherkus wrote:
> nit: doesn't chromium prefer #if defined(FOO) / #if !defined(FOO)?

Does it? The google style guide and chromium style guide both make offhand
references to "#ifdef" which seems to say it's acceptable (this is pretty
transient anyways).

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit...

[scherkus (not reviewing), 2013-04-17 20:16:58]

lgtm

https://codereview.chromium.org/14199002/diff/17004/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/14199002/diff/17004/media/base/video_frame.h#...
media/base/video_frame.h:103: const TextureNoLongerNeededCallback&
texture_no_longer_needed_cb,
On 2013/04/17 20:12:37, danakj wrote:
> On 2013/04/17 20:09:34, scherkus wrote:
> > OOC think we can remove the non-mailbox path after all is said and done?
> 
> Yes absolutely. The no_longer_needed_cb is used for some other stuff (software
> decoder things) so I thought to add a new callback instead of replacing it.

Slick!

https://codereview.chromium.org/14199002/diff/17004/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/14199002/diff/17004/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:771: #ifdef VIDEO_FRAME_MAILBOX
On 2013/04/17 20:12:37, danakj wrote:
> On 2013/04/17 20:09:34, scherkus wrote:
> > nit: doesn't chromium prefer #if defined(FOO) / #if !defined(FOO)?
> 
> Does it? The google style guide and chromium style guide both make offhand
> references to "#ifdef" which seems to say it's acceptable (this is pretty
> transient anyways).
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit...

yeah I don't really care enough to have it changed

I was basing this off of the OS_XXX defines, which seem to be heavily in favour
of defined():

$ git grep "#ifdef.*OS_WIN" -- "*.h" "*.cc" | wc -l
3

$ git grep "#if defined(OS_WIN)" -- "*.h" "*.cc" | wc -l
2761

[scherkus (not reviewing), 2013-04-17 20:17:24]

crap I didn't mean to lgtm that

I'll take another look once you've sorted out piman's issues

[danakj, 2013-04-17 20:40:28]

On Wed, Apr 17, 2013 at 4:17 PM, <scherkus@chromium.org> wrote:

> crap I didn't mean to lgtm that
>
> I'll take another look once you've sorted out piman's issues
>

No problem. The latest patch should address his concerns. It's just missing
mac/android yet.


>
>
https://codereview.chromium.**org/14199002/<https://codereview.chromium.org/1...
>

[danakj, 2013-04-18 02:06:56]

PTAL

+sievers for the android stuff

This should work for Mac and Android now. I use the correct texture target when
producing all of the mailboxes.

However, I'm just getting a black texture on android. I'm not sure if it's
something I'm doing or something else.

(I have the VIDEO_FRAME_MAILBOX ifdef off in this CL since it depends on shared
mailboxes for canvas-video interaction.)

[danakj, 2013-04-18 02:19:45]

Here's a patch version without the ifdefing noise.

[danakj, 2013-04-18 02:38:34]

https://codereview.chromium.org/14199002/diff/36001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/14199002/diff/36001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.cc:79: 0);  // TODO(danakj): How do
I get this sync point?
Daniel/Antoine, besides "things I did horribly wrong", I'd especially like your
input here.

There's a callback on the VideoFrame that will fire with the sync point we want
for this. But the callback happens then the VideoFrame is released from the
compositor, which means this class could be gone already.

Options that I see
1) Make stream_texture_factory_ a refptr, and hold a ref inside the VideoFrame.
(I'm not sure of the consequences of making it live longer).
2) We're assuming shared mailboxes, so..
2a) maybe this class just deletes its client id when it stuffs it into a video
frame for the first time? But then what if it needs to make a new frame for some
reason?
2b) maybe we don't need to consume the mailbox we produced and can just delete
the client id here directly. Would that cause a leak?

[piman, 2013-04-18 04:11:18]

https://codereview.chromium.org/14199002/diff/36001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/14199002/diff/36001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.cc:79: 0);  // TODO(danakj): How do
I get this sync point?
On 2013/04/18 02:38:34, danakj wrote:
> Daniel/Antoine, besides "things I did horribly wrong", I'd especially like
your
> input here.
> 
> There's a callback on the VideoFrame that will fire with the sync point we
want
> for this. But the callback happens then the VideoFrame is released from the
> compositor, which means this class could be gone already.
> 
> Options that I see
> 1) Make stream_texture_factory_ a refptr, and hold a ref inside the
VideoFrame.
> (I'm not sure of the consequences of making it live longer).
> 2) We're assuming shared mailboxes, so..
> 2a) maybe this class just deletes its client id when it stuffs it into a video
> frame for the first time? But then what if it needs to make a new frame for
some
> reason?
> 2b) maybe we don't need to consume the mailbox we produced and can just delete
> the client id here directly. Would that cause a leak?

Reading the code, my interpretation of how StreamTextures work is that the
content is updated when the texture is bound, as long as a stream exists.
If the stream is destroyed, the existing data will still be there, it just won't
update on a bind.

So with the sharing model, it would be no problem at all to destroy the texture
at any time (before any sync point), as well as the stream, with the (expected)
behavior that the texture won't update in other contexts after that.
I.e. after sharing, 2b is correct, and no that won't cause a leak.

With ÜC I realize that the texture will also not update if it's only bound in a
context that's on the same channel, but that looks reasonably easy to fix (by
sharing the StreamTextureManagerAndroid across channels - like the mailbox
manager - while giving enough routing info so that the callbacks do happen on
the right channel).

[no sievers, 2013-04-18 17:37:07]

https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/st...
File content/renderer/media/stream_texture_factory_impl_android.cc (right):

https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/st...
content/renderer/media/stream_texture_factory_impl_android.cc:130:
*texture_mailbox_sync_point = context_->insertSyncPoint();
You'll have to move this after createStreamTexture() for the texture in the
mailbox to be connected to the video stream. That's a synchronous call that
allocates the actual stream object and connects it to the texture. If you
produce it before, the mailbox will contain an external texture that is not
connected to any stream.

https://codereview.chromium.org/14199002/diff/36001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/14199002/diff/36001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.cc:79: 0);  // TODO(danakj): How do
I get this sync point?
On 2013/04/18 04:11:19, piman wrote:
> On 2013/04/18 02:38:34, danakj wrote:
> > Daniel/Antoine, besides "things I did horribly wrong", I'd especially like
> your
> > input here.
> > 
> > There's a callback on the VideoFrame that will fire with the sync point we
> want
> > for this. But the callback happens then the VideoFrame is released from the
> > compositor, which means this class could be gone already.
> > 
> > Options that I see
> > 1) Make stream_texture_factory_ a refptr, and hold a ref inside the
> VideoFrame.
> > (I'm not sure of the consequences of making it live longer).
> > 2) We're assuming shared mailboxes, so..
> > 2a) maybe this class just deletes its client id when it stuffs it into a
video
> > frame for the first time? But then what if it needs to make a new frame for
> some
> > reason?
> > 2b) maybe we don't need to consume the mailbox we produced and can just
delete
> > the client id here directly. Would that cause a leak?
> 
> Reading the code, my interpretation of how StreamTextures work is that the
> content is updated when the texture is bound, as long as a stream exists.
> If the stream is destroyed, the existing data will still be there, it just
won't
> update on a bind.
> 
> So with the sharing model, it would be no problem at all to destroy the
texture
> at any time (before any sync point), as well as the stream, with the
(expected)
> behavior that the texture won't update in other contexts after that.
> I.e. after sharing, 2b is correct, and no that won't cause a leak.


Yep, and notice that deleting the client texture (that is currently connected to
the stream) also implies a DestroyStreamTexture(). For sharing, we'll have to
remove that bit from GLES2DecoderImpl::DeleteTexturesHelper() and instead delete
the stream object from the shared texture when it goes away.


> With ÜC I realize that the texture will also not update if it's only bound in
a
> context that's on the same channel, but that looks reasonably easy to fix (by
> sharing the StreamTextureManagerAndroid across channels - like the mailbox
> manager - while giving enough routing info so that the callbacks do happen on
> the right channel).

I really want to merge StreamTexture(Manager) with GLImage/ImageManager.
However, ImageManager is also currently per channel. But maybe if I unify the
code a bit we can solve mailbox problems for both.

By the way, now that I think about it, updating the stream from BindTexture()
seems fishy. If the external binding never changes, we would not update. It
should probably either be an explicit GL call, or implied at draw time.

[piman, 2013-04-18 18:16:28]

On Thu, Apr 18, 2013 at 10:37 AM, <sievers@chromium.org> wrote:

>
> https://codereview.chromium.**org/14199002/diff/36001/**
>
content/renderer/media/stream_**texture_factory_impl_android.**cc<https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/stream_texture_factory_impl_android.cc>
> File content/renderer/media/stream_**texture_factory_impl_android.**cc
> (right):
>
> https://codereview.chromium.**org/14199002/diff/36001/**
> content/renderer/media/stream_**texture_factory_impl_android.**
>
cc#newcode130<https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/stream_texture_factory_impl_android.cc#newcode130>
> content/renderer/media/stream_**texture_factory_impl_android.**cc:130:
> *texture_mailbox_sync_point = context_->insertSyncPoint();
> You'll have to move this after createStreamTexture() for the texture in
> the mailbox to be connected to the video stream. That's a synchronous
> call that allocates the actual stream object and connects it to the
> texture. If you produce it before, the mailbox will contain an external
> texture that is not connected to any stream.


With sharing that wouldn't matter, but with the current scheme, it would,
yes.


>
>
> https://codereview.chromium.**org/14199002/diff/36001/**
>
webkit/media/android/**webmediaplayer_android.cc<https://codereview.chromium.org/14199002/diff/36001/webkit/media/android/webmediaplayer_android.cc>
> File webkit/media/android/**webmediaplayer_android.cc (right):
>
> https://codereview.chromium.**org/14199002/diff/36001/**
>
webkit/media/android/**webmediaplayer_android.cc#**newcode79<https://codereview.chromium.org/14199002/diff/36001/webkit/media/android/webmediaplayer_android.cc#newcode79>
> webkit/media/android/**webmediaplayer_android.cc:79: 0);  // TODO(danakj):
> How do I get this sync point?
> On 2013/04/18 04:11:19, piman wrote:
>
>> On 2013/04/18 02:38:34, danakj wrote:
>> > Daniel/Antoine, besides "things I did horribly wrong", I'd
>>
> especially like
>
>> your
>> > input here.
>> >
>> > There's a callback on the VideoFrame that will fire with the sync
>>
> point we
>
>> want
>> > for this. But the callback happens then the VideoFrame is released
>>
> from the
>
>> > compositor, which means this class could be gone already.
>> >
>> > Options that I see
>> > 1) Make stream_texture_factory_ a refptr, and hold a ref inside the
>> VideoFrame.
>> > (I'm not sure of the consequences of making it live longer).
>> > 2) We're assuming shared mailboxes, so..
>> > 2a) maybe this class just deletes its client id when it stuffs it
>>
> into a video
>
>> > frame for the first time? But then what if it needs to make a new
>>
> frame for
>
>> some
>> > reason?
>> > 2b) maybe we don't need to consume the mailbox we produced and can
>>
> just delete
>
>> > the client id here directly. Would that cause a leak?
>>
>
>  Reading the code, my interpretation of how StreamTextures work is that
>>
> the
>
>> content is updated when the texture is bound, as long as a stream
>>
> exists.
>
>> If the stream is destroyed, the existing data will still be there, it
>>
> just won't
>
>> update on a bind.
>>
>
>  So with the sharing model, it would be no problem at all to destroy
>>
> the texture
>
>> at any time (before any sync point), as well as the stream, with the
>>
> (expected)
>
>> behavior that the texture won't update in other contexts after that.
>> I.e. after sharing, 2b is correct, and no that won't cause a leak.
>>
>
>
> Yep, and notice that deleting the client texture (that is currently
> connected to the stream) also implies a DestroyStreamTexture(). For
> sharing, we'll have to remove that bit from
> GLES2DecoderImpl::**DeleteTexturesHelper() and instead delete the stream
> object from the shared texture when it goes away.


I missed that. You're right, if we move the StreamTextureManager to be
common, this should be moved to delete the stream when the texture is
deleted.


>
>
>
>  With ÜC I realize that the texture will also not update if it's only
>>
> bound in a
>
>> context that's on the same channel, but that looks reasonably easy to
>>
> fix (by
>
>> sharing the StreamTextureManagerAndroid across channels - like the
>>
> mailbox
>
>> manager - while giving enough routing info so that the callbacks do
>>
> happen on
>
>> the right channel).
>>
>
> I really want to merge StreamTexture(Manager) with GLImage/ImageManager.
> However, ImageManager is also currently per channel. But maybe if I
> unify the code a bit we can solve mailbox problems for both.
>

That'd be nice.
I think you're right, the ImageManager is essentially the same exact
problem.


>
> By the way, now that I think about it, updating the stream from
> BindTexture() seems fishy. If the external binding never changes, we
> would not update. It should probably either be an explicit GL call, or
> implied at draw time.
>

In the compositor, we'll always bind it before drawing.
That said, because the renderer gets notified of the new frame, it could
make sense to make it explicit then.


>
https://codereview.chromium.**org/14199002/<https://codereview.chromium.org/1...
>

[danakj, 2013-04-19 18:03:52]

N

https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/st...
File content/renderer/media/stream_texture_factory_impl_android.cc (right):

https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/st...
content/renderer/media/stream_texture_factory_impl_android.cc:130:
*texture_mailbox_sync_point = context_->insertSyncPoint();
On 2013/04/18 17:37:08, Daniel Sievers wrote:
> You'll have to move this after createStreamTexture() for the texture in the
> mailbox to be connected to the video stream. That's a synchronous call that
> allocates the actual stream object and connects it to the texture. If you
> produce it before, the mailbox will contain an external texture that is not
> connected to any stream.

Ok done, but I still am getting black output for this path.. hm..

Will the flush still be needed if the sync point is waited on before accessing
the mailbox? I moved it above the produce for now as that seemed safest (maybe
it's needed before produce?)

https://codereview.chromium.org/14199002/diff/36001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/14199002/diff/36001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.cc:79: 0);  // TODO(danakj): How do
I get this sync point?
On 2013/04/18 17:37:08, Daniel Sievers wrote:
> On 2013/04/18 04:11:19, piman wrote:
> > On 2013/04/18 02:38:34, danakj wrote:
> > > Daniel/Antoine, besides "things I did horribly wrong", I'd especially like
> > your
> > > input here.
> > > 
> > > There's a callback on the VideoFrame that will fire with the sync point we
> > want
> > > for this. But the callback happens then the VideoFrame is released from
the
> > > compositor, which means this class could be gone already.
> > > 
> > > Options that I see
> > > 1) Make stream_texture_factory_ a refptr, and hold a ref inside the
> > VideoFrame.
> > > (I'm not sure of the consequences of making it live longer).
> > > 2) We're assuming shared mailboxes, so..
> > > 2a) maybe this class just deletes its client id when it stuffs it into a
> video
> > > frame for the first time? But then what if it needs to make a new frame
for
> > some
> > > reason?
> > > 2b) maybe we don't need to consume the mailbox we produced and can just
> delete
> > > the client id here directly. Would that cause a leak?
> > 
> > Reading the code, my interpretation of how StreamTextures work is that the
> > content is updated when the texture is bound, as long as a stream exists.
> > If the stream is destroyed, the existing data will still be there, it just
> won't
> > update on a bind.
> > 
> > So with the sharing model, it would be no problem at all to destroy the
> texture
> > at any time (before any sync point), as well as the stream, with the
> (expected)
> > behavior that the texture won't update in other contexts after that.
> > I.e. after sharing, 2b is correct, and no that won't cause a leak.
> 
> 
> Yep, and notice that deleting the client texture (that is currently connected
to
> the stream) also implies a DestroyStreamTexture(). For sharing, we'll have to
> remove that bit from GLES2DecoderImpl::DeleteTexturesHelper() and instead
delete
> the stream object from the shared texture when it goes away.
> 
> 
> > With ÜC I realize that the texture will also not update if it's only bound
in
> a
> > context that's on the same channel, but that looks reasonably easy to fix
(by
> > sharing the StreamTextureManagerAndroid across channels - like the mailbox
> > manager - while giving enough routing info so that the callbacks do happen
on
> > the right channel).
> 
> I really want to merge StreamTexture(Manager) with GLImage/ImageManager.
> However, ImageManager is also currently per channel. But maybe if I unify the
> code a bit we can solve mailbox problems for both.
> 
> By the way, now that I think about it, updating the stream from BindTexture()
> seems fishy. If the external binding never changes, we would not update. It
> should probably either be an explicit GL call, or implied at draw time.

Oh, I have it just deleting the client texture and the stream texture now, with
a todo inside against sievers to make the StreamTexture get deleted later when
the last texture referencing it is destroyed.

[no sievers, 2013-04-19 18:12:07]

https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/st...
File content/renderer/media/stream_texture_factory_impl_android.cc (right):

https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/st...
content/renderer/media/stream_texture_factory_impl_android.cc:130:
*texture_mailbox_sync_point = context_->insertSyncPoint();
On 2013/04/19 18:03:52, danakj wrote:
> On 2013/04/18 17:37:08, Daniel Sievers wrote:
> > You'll have to move this after createStreamTexture() for the texture in the
> > mailbox to be connected to the video stream. That's a synchronous call that
> > allocates the actual stream object and connects it to the texture. If you
> > produce it before, the mailbox will contain an external texture that is not
> > connected to any stream.
> 
> Ok done, but I still am getting black output for this path.. hm..
> 
> Will the flush still be needed if the sync point is waited on before accessing
> the mailbox? I moved it above the produce for now as that seemed safest (maybe
> it's needed before produce?)

Not sure where the flush in the code comes from, createStreamTexture is
synchronous :)
And even if you are changing the order, insertSyncPoint() implies any flushing
for synchronization (with regard to previous commands and the sync point itself
being visible to other clients).

[no sievers, 2013-04-19 18:14:37]

On 2013/04/19 18:12:07, Daniel Sievers wrote:
>
https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/st...
> File content/renderer/media/stream_texture_factory_impl_android.cc (right):
> 
>
https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/st...
> content/renderer/media/stream_texture_factory_impl_android.cc:130:
> *texture_mailbox_sync_point = context_->insertSyncPoint();
> On 2013/04/19 18:03:52, danakj wrote:
> > On 2013/04/18 17:37:08, Daniel Sievers wrote:
> > > You'll have to move this after createStreamTexture() for the texture in
the
> > > mailbox to be connected to the video stream. That's a synchronous call
that
> > > allocates the actual stream object and connects it to the texture. If you
> > > produce it before, the mailbox will contain an external texture that is
not
> > > connected to any stream.
> > 
> > Ok done, but I still am getting black output for this path.. hm..
> > 
> > Will the flush still be needed if the sync point is waited on before
accessing
> > the mailbox? I moved it above the produce for now as that seemed safest
(maybe
> > it's needed before produce?)
> 
> Not sure where the flush in the code comes from, createStreamTexture is
> synchronous :)
> And even if you are changing the order, insertSyncPoint() implies any flushing
> for synchronization (with regard to previous commands and the sync point
itself
> being visible to other clients).

I meant: insertSyncPoint() implies any flushing *needed*

So yea, you should be good. You might want to see if you get any GL errors with
--enable-gpu-debugging. If you uploaded your latest patch, would I need anything
else to give it a try?

[piman, 2013-04-19 18:15:14]

On Fri, Apr 19, 2013 at 11:03 AM, <danakj@chromium.org> wrote:

> N
>
>
> https://codereview.chromium.**org/14199002/diff/36001/**
>
content/renderer/media/stream_**texture_factory_impl_android.**cc<https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/stream_texture_factory_impl_android.cc>
> File content/renderer/media/stream_**texture_factory_impl_android.**cc
> (right):
>
> https://codereview.chromium.**org/14199002/diff/36001/**
> content/renderer/media/stream_**texture_factory_impl_android.**
>
cc#newcode130<https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/stream_texture_factory_impl_android.cc#newcode130>
> content/renderer/media/stream_**texture_factory_impl_android.**cc:130:
> *texture_mailbox_sync_point = context_->insertSyncPoint();
> On 2013/04/18 17:37:08, Daniel Sievers wrote:
>
>> You'll have to move this after createStreamTexture() for the texture
>>
> in the
>
>> mailbox to be connected to the video stream. That's a synchronous call
>>
> that
>
>> allocates the actual stream object and connects it to the texture. If
>>
> you
>
>> produce it before, the mailbox will contain an external texture that
>>
> is not
>
>> connected to any stream.
>>
>
> Ok done, but I still am getting black output for this path.. hm..
>
> Will the flush still be needed if the sync point is waited on before
> accessing the mailbox? I moved it above the produce for now as that
> seemed safest (maybe it's needed before produce?)


You shouldn't need a flush before produce. insertSyncPoint does an implicit
shallow flush. We may need a deep flush to make sure texture changes are
visible cross-context, the best place is right before the insertSyncPoint -
we drop the shallow flush if no command was inserted, so it piggy-backs on
the deep flush's shallow flush (if you can follow that statement).

>
>
> https://codereview.chromium.**org/14199002/diff/36001/**
>
webkit/media/android/**webmediaplayer_android.cc<https://codereview.chromium.org/14199002/diff/36001/webkit/media/android/webmediaplayer_android.cc>
> File webkit/media/android/**webmediaplayer_android.cc (right):
>
> https://codereview.chromium.**org/14199002/diff/36001/**
>
webkit/media/android/**webmediaplayer_android.cc#**newcode79<https://codereview.chromium.org/14199002/diff/36001/webkit/media/android/webmediaplayer_android.cc#newcode79>
> webkit/media/android/**webmediaplayer_android.cc:79: 0);  // TODO(danakj):
> How do I get this sync point?
> On 2013/04/18 17:37:08, Daniel Sievers wrote:
>
>> On 2013/04/18 04:11:19, piman wrote:
>> > On 2013/04/18 02:38:34, danakj wrote:
>> > > Daniel/Antoine, besides "things I did horribly wrong", I'd
>>
> especially like
>
>> > your
>> > > input here.
>> > >
>> > > There's a callback on the VideoFrame that will fire with the sync
>>
> point we
>
>> > want
>> > > for this. But the callback happens then the VideoFrame is released
>>
> from the
>
>> > > compositor, which means this class could be gone already.
>> > >
>> > > Options that I see
>> > > 1) Make stream_texture_factory_ a refptr, and hold a ref inside
>>
> the
>
>> > VideoFrame.
>> > > (I'm not sure of the consequences of making it live longer).
>> > > 2) We're assuming shared mailboxes, so..
>> > > 2a) maybe this class just deletes its client id when it stuffs it
>>
> into a
>
>> video
>> > > frame for the first time? But then what if it needs to make a new
>>
> frame for
>
>> > some
>> > > reason?
>> > > 2b) maybe we don't need to consume the mailbox we produced and can
>>
> just
>
>> delete
>> > > the client id here directly. Would that cause a leak?
>> >
>> > Reading the code, my interpretation of how StreamTextures work is
>>
> that the
>
>> > content is updated when the texture is bound, as long as a stream
>>
> exists.
>
>> > If the stream is destroyed, the existing data will still be there,
>>
> it just
>
>> won't
>> > update on a bind.
>> >
>> > So with the sharing model, it would be no problem at all to destroy
>>
> the
>
>> texture
>> > at any time (before any sync point), as well as the stream, with the
>> (expected)
>> > behavior that the texture won't update in other contexts after that.
>> > I.e. after sharing, 2b is correct, and no that won't cause a leak.
>>
>
>
>  Yep, and notice that deleting the client texture (that is currently
>>
> connected to
>
>> the stream) also implies a DestroyStreamTexture(). For sharing, we'll
>>
> have to
>
>> remove that bit from GLES2DecoderImpl::**DeleteTexturesHelper() and
>>
> instead delete
>
>> the stream object from the shared texture when it goes away.
>>
>
>
>  > With ÜC I realize that the texture will also not update if it's only
>>
> bound in
>
>> a
>> > context that's on the same channel, but that looks reasonably easy
>>
> to fix (by
>
>> > sharing the StreamTextureManagerAndroid across channels - like the
>>
> mailbox
>
>> > manager - while giving enough routing info so that the callbacks do
>>
> happen on
>
>> > the right channel).
>>
>
>  I really want to merge StreamTexture(Manager) with
>>
> GLImage/ImageManager.
>
>> However, ImageManager is also currently per channel. But maybe if I
>>
> unify the
>
>> code a bit we can solve mailbox problems for both.
>>
>
>  By the way, now that I think about it, updating the stream from
>>
> BindTexture()
>
>> seems fishy. If the external binding never changes, we would not
>>
> update. It
>
>> should probably either be an explicit GL call, or implied at draw
>>
> time.
>
> Oh, I have it just deleting the client texture and the stream texture
> now, with a todo inside against sievers to make the StreamTexture get
> deleted later when the last texture referencing it is destroyed.
>
>
https://codereview.chromium.**org/14199002/<https://codereview.chromium.org/1...
>

[no sievers, 2013-04-19 18:17:48]

On 2013/04/19 18:15:14, piman wrote:
> On Fri, Apr 19, 2013 at 11:03 AM, <mailto:danakj@chromium.org> wrote:
> 
> > N
> >
> >
> > https://codereview.chromium.**org/14199002/diff/36001/**
> >
>
content/renderer/media/stream_**texture_factory_impl_android.**cc<https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/stream_texture_factory_impl_android.cc>
> > File content/renderer/media/stream_**texture_factory_impl_android.**cc
> > (right):
> >
> > https://codereview.chromium.**org/14199002/diff/36001/**
> > content/renderer/media/stream_**texture_factory_impl_android.**
> >
>
cc#newcode130<https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/stream_texture_factory_impl_android.cc#newcode130>
> > content/renderer/media/stream_**texture_factory_impl_android.**cc:130:
> > *texture_mailbox_sync_point = context_->insertSyncPoint();
> > On 2013/04/18 17:37:08, Daniel Sievers wrote:
> >
> >> You'll have to move this after createStreamTexture() for the texture
> >>
> > in the
> >
> >> mailbox to be connected to the video stream. That's a synchronous call
> >>
> > that
> >
> >> allocates the actual stream object and connects it to the texture. If
> >>
> > you
> >
> >> produce it before, the mailbox will contain an external texture that
> >>
> > is not
> >
> >> connected to any stream.
> >>
> >
> > Ok done, but I still am getting black output for this path.. hm..
> >
> > Will the flush still be needed if the sync point is waited on before
> > accessing the mailbox? I moved it above the produce for now as that
> > seemed safest (maybe it's needed before produce?)
> 
> 
> You shouldn't need a flush before produce. insertSyncPoint does an implicit
> shallow flush. We may need a deep flush to make sure texture changes are
> visible cross-context, the best place is right before the insertSyncPoint -
> we drop the shallow flush if no command was inserted, so it piggy-backs on
> the deep flush's shallow flush (if you can follow that statement).
> 

By deep flush you mean glFlush()? In that case, I don't think we need it here,
since the stream texture is an external texture and we don't really change it at
all from anywhere in the renderer.

[danakj, 2013-04-19 18:19:45]

On Fri, Apr 19, 2013 at 2:14 PM, <sievers@chromium.org> wrote:

> I meant: insertSyncPoint() implies any flushing *needed*
>
> So yea, you should be good. You might want to see if you get any GL errors
> with
> --enable-gpu-debugging. If you uploaded your latest patch, would I need
> anything
> else to give it a try?
>

It's uploded, and you shouldn't need anything else - assuming shared
mailboxes aren't needed for this path to display anything, which is my
understanding.


>
>
>
https://codereview.chromium.**org/14199002/<https://codereview.chromium.org/1...
>

[piman, 2013-04-19 18:40:01]

On Fri, Apr 19, 2013 at 11:17 AM, <sievers@chromium.org> wrote:

> On 2013/04/19 18:15:14, piman wrote:
>
>> On Fri, Apr 19, 2013 at 11:03 AM, <mailto:danakj@chromium.org> wrote:
>>
>
>  > N
>> >
>> >
>> > https://codereview.chromium.****org/14199002/diff/36001/**
>> >
>>
>
> content/renderer/media/stream_****texture_factory_impl_**android.**cc<
> https://**codereview.chromium.org/**14199002/diff/36001/content/**
>
renderer/media/stream_texture_**factory_impl_android.cc<https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/stream_texture_factory_impl_android.cc>
> >
>
>> > File content/renderer/media/stream_****texture_factory_impl_**
>> android.**cc
>> > (right):
>> >
>> > https://codereview.chromium.****org/14199002/diff/36001/**
>> > content/renderer/media/stream_****texture_factory_impl_**android.**
>> >
>>
>
> cc#newcode130<https://**codereview.chromium.org/**
> 14199002/diff/36001/content/**renderer/media/stream_texture_**
>
factory_impl_android.cc#**newcode130<https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/stream_texture_factory_impl_android.cc#newcode130>
> >
>
>> > content/renderer/media/stream_****texture_factory_impl_**
>> android.**cc:130:
>>
>> > *texture_mailbox_sync_point = context_->insertSyncPoint();
>> > On 2013/04/18 17:37:08, Daniel Sievers wrote:
>> >
>> >> You'll have to move this after createStreamTexture() for the texture
>> >>
>> > in the
>> >
>> >> mailbox to be connected to the video stream. That's a synchronous call
>> >>
>> > that
>> >
>> >> allocates the actual stream object and connects it to the texture. If
>> >>
>> > you
>> >
>> >> produce it before, the mailbox will contain an external texture that
>> >>
>> > is not
>> >
>> >> connected to any stream.
>> >>
>> >
>> > Ok done, but I still am getting black output for this path.. hm..
>> >
>> > Will the flush still be needed if the sync point is waited on before
>> > accessing the mailbox? I moved it above the produce for now as that
>> > seemed safest (maybe it's needed before produce?)
>>
>
>
>  You shouldn't need a flush before produce. insertSyncPoint does an
>> implicit
>> shallow flush. We may need a deep flush to make sure texture changes are
>> visible cross-context, the best place is right before the insertSyncPoint
>> -
>> we drop the shallow flush if no command was inserted, so it piggy-backs on
>> the deep flush's shallow flush (if you can follow that statement).
>>
>
>
> By deep flush you mean glFlush()?


context->flush(), which is the combination of adding a glFlush command and
flushing the command buffer (shallow flush).


> In that case, I don't think we need it here,
> since the stream texture is an external texture and we don't really change
> it at
> all from anywhere in the renderer.
>

It's about ordering at the GL level. glGenTextures + glBindTextures (and
possibly other state changes done by createStreamTextureCHROMIUM) in the GL
context backing this context may not be visible in the GL context backing
the renderer unless you glFlush() first. It's probably the reason why you
had a flush() here previously already.


>
>
>
https://codereview.chromium.**org/14199002/<https://codereview.chromium.org/1...
>

[no sievers, 2013-04-19 18:50:20]

On 2013/04/19 18:40:01, piman wrote:
> On Fri, Apr 19, 2013 at 11:17 AM, <mailto:sievers@chromium.org> wrote:
> 
> > On 2013/04/19 18:15:14, piman wrote:
> >
> >> On Fri, Apr 19, 2013 at 11:03 AM, <mailto:danakj@chromium.org> wrote:
> >>
> >
> >  > N
> >> >
> >> >
> >> > https://codereview.chromium.****org/14199002/diff/36001/**
> >> >
> >>
> >
> > content/renderer/media/stream_****texture_factory_impl_**android.**cc<
> > https://**codereview.chromium.org/**14199002/diff/36001/content/**
> >
>
renderer/media/stream_texture_**factory_impl_android.cc<https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/stream_texture_factory_impl_android.cc>
> > >
> >
> >> > File content/renderer/media/stream_****texture_factory_impl_**
> >> android.**cc
> >> > (right):
> >> >
> >> > https://codereview.chromium.****org/14199002/diff/36001/**
> >> > content/renderer/media/stream_****texture_factory_impl_**android.**
> >> >
> >>
> >
> > cc#newcode130<https://**codereview.chromium.org/**
> > 14199002/diff/36001/content/**renderer/media/stream_texture_**
> >
>
factory_impl_android.cc#**newcode130<https://codereview.chromium.org/14199002/diff/36001/content/renderer/media/stream_texture_factory_impl_android.cc#newcode130>
> > >
> >
> >> > content/renderer/media/stream_****texture_factory_impl_**
> >> android.**cc:130:
> >>
> >> > *texture_mailbox_sync_point = context_->insertSyncPoint();
> >> > On 2013/04/18 17:37:08, Daniel Sievers wrote:
> >> >
> >> >> You'll have to move this after createStreamTexture() for the texture
> >> >>
> >> > in the
> >> >
> >> >> mailbox to be connected to the video stream. That's a synchronous call
> >> >>
> >> > that
> >> >
> >> >> allocates the actual stream object and connects it to the texture. If
> >> >>
> >> > you
> >> >
> >> >> produce it before, the mailbox will contain an external texture that
> >> >>
> >> > is not
> >> >
> >> >> connected to any stream.
> >> >>
> >> >
> >> > Ok done, but I still am getting black output for this path.. hm..
> >> >
> >> > Will the flush still be needed if the sync point is waited on before
> >> > accessing the mailbox? I moved it above the produce for now as that
> >> > seemed safest (maybe it's needed before produce?)
> >>
> >
> >
> >  You shouldn't need a flush before produce. insertSyncPoint does an
> >> implicit
> >> shallow flush. We may need a deep flush to make sure texture changes are
> >> visible cross-context, the best place is right before the insertSyncPoint
> >> -
> >> we drop the shallow flush if no command was inserted, so it piggy-backs on
> >> the deep flush's shallow flush (if you can follow that statement).
> >>
> >
> >
> > By deep flush you mean glFlush()?
> 
> 
> context->flush(), which is the combination of adding a glFlush command and
> flushing the command buffer (shallow flush).
> 
> 
> > In that case, I don't think we need it here,
> > since the stream texture is an external texture and we don't really change
> > it at
> > all from anywhere in the renderer.
> >
> 
> It's about ordering at the GL level. glGenTextures + glBindTextures (and
> possibly other state changes done by createStreamTextureCHROMIUM) in the GL
> context backing this context may not be visible in the GL context backing
> the renderer unless you glFlush() first. It's probably the reason why you
> had a flush() here previously already.
> 


Ah right, createStreamTexture() is synchronous but does not imply a glFlush().
Ok, sorry Dana, keep the flush and do what Antoine said :)

Your latest patch looks functionally in the right order then.
But like Antoine said, if you move the glFlush() right before insertSyncPoint()
we avoid flushing commands again (insertSyncPoint is actually an IPC).



> 
> >
> >
> >
>
https://codereview.chromium.**org/14199002/%3Chttps://codereview.chromium.org...>
> >

[danakj, 2013-04-19 18:53:22]

On Fri, Apr 19, 2013 at 2:50 PM, <sievers@chromium.org> wrote:

> Ah right, createStreamTexture() is synchronous but does not imply a
> glFlush().
> Ok, sorry Dana, keep the flush and do what Antoine said :)
>
> Your latest patch looks functionally in the right order then.
> But like Antoine said, if you move the glFlush() right before
> insertSyncPoint()
> we avoid flushing commands again (insertSyncPoint is actually an IPC).
>
>
Yup, done.

I'm trying to get the libva hardware decode path going on my desktop so I
can try this path out properly for non-android. Maybe something
non-stream-texture-related is broken.

[danakj, 2013-04-19 20:44:43]

https://codereview.chromium.org/14199002/diff/55001/content/renderer/media/re...
File content/renderer/media/renderer_gpu_video_decoder_factories.cc (right):

https://codereview.chromium.org/14199002/diff/55001/content/renderer/media/re...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:207:
async_waiter_.Signal();
I was missing these Signal()s which broke everything. Now hardware video decode
is working for linux/chromeos. Android is still a black texture though.

[danakj, 2013-04-19 21:06:37]

On 2013/04/19 20:44:43, danakj wrote:
>
https://codereview.chromium.org/14199002/diff/55001/content/renderer/media/re...
> File content/renderer/media/renderer_gpu_video_decoder_factories.cc (right):
> 
>
https://codereview.chromium.org/14199002/diff/55001/content/renderer/media/re...
> content/renderer/media/renderer_gpu_video_decoder_factories.cc:207:
> async_waiter_.Signal();
> I was missing these Signal()s which broke everything. Now hardware video
decode
> is working for linux/chromeos. Android is still a black texture though.

I figured out how to printf debug on android, and I'm seeing lots of this:

E/chromium( 2593):
[2593:2623:0419/210555:11964887603:ERROR:gles2_cmd_decoder.cc(3904)]
.CompositorContext: GL ERROR :GL_INVALID_OPERATION : glBindTexture: texture
bound to more than 1 target.

So I guess we're using not-OES somewhere still.

[danakj, 2013-04-19 21:39:43]

Ok, this should all be correct now. PTAL!

https://codereview.chromium.org/14199002/diff/60001/cc/resources/video_resour...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/14199002/diff/60001/cc/resources/video_resour...
cc/resources/video_resource_updater.cc:339: video_frame->texture_target(),
Woops, I hadn't hooked the VideoFrame::texture_target() up. Now I get no GL
errors while playing HTML5 video on android and video shows up! \o/

[no sievers, 2013-04-19 22:07:41]

Cool! Looks good for Android. But SurfaceTextureTransportClient won't work yet
because it's hardcoded for a non-cmdbuffer context and real textures in there
(so mailbox calls are NOPs). Which would break WebView. So if you can wait a bit
until I figure that out, that would be great. But I guess you are still blocked
on sharing anyhow :)

[scherkus (not reviewing), 2013-04-19 23:32:07]

media lgtm w/ one nit

https://codereview.chromium.org/14199002/diff/60001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/14199002/diff/60001/media/base/video_frame.cc...
media/base/video_frame.cc:261: timestamp_(timestamp) {
init texture_mailbox_sync_point_ to some sort of known value for
non-native_texture paths?

[danakj, 2013-04-19 23:52:06]

Thanks!

https://codereview.chromium.org/14199002/diff/60001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/14199002/diff/60001/media/base/video_frame.cc...
media/base/video_frame.cc:261: timestamp_(timestamp) {
On 2013/04/19 23:32:07, scherkus wrote:
> init texture_mailbox_sync_point_ to some sort of known value for
> non-native_texture paths?

Done.

[danakj, 2013-05-11 00:11:14]

I reworked this CL a bit so that the lifetime of VideoFrame is not different for
the texture mailbox path than it is for other paths. That should make things
much nicer to reason about.

VideoFrame loses its accessors for the texture mailbox pieces, instead it has
two functions.

UseTextureMailbox() returns all the pieces to use the mailbox. You don't take
ownership of the mailbox.

TakeTextureMailbox() is like the above, but it passes responsibility for
releasing the texture mailbox to someone else. This lets the mailbox outlive the
VideoFrame.

The one awkward piece is if you TakeTextureMailbox() but then release it before
VideoFrame is destroyed, then you have a VideoFrame with a mailbox that
shouldn't be there.

I thought about how to solve that but was coming up with a WeakPtr to the
VideoFrame, which is already refcounted-thread-safe, so not great.

[danakj, 2013-05-11 01:42:23]

Ok I sorted out the callback timing stuff with TakeTextureMailbox. Added some
VideoFrameTest unittests to verify behaviour.

[danakj, 2013-06-14 19:56:22]

New patch, using a MailboxHolder refcounted object that VideoFrame holds and
returns, to make the release callback fire back to the decoder at the right
time.

[piman, 2013-06-14 21:47:28]

At a high level it LGTM.

https://codereview.chromium.org/14199002/diff/115001/content/browser/renderer...
File content/browser/renderer_host/surface_texture_transport_client_android.cc
(right):

https://codereview.chromium.org/14199002/diff/115001/content/browser/renderer...
content/browser/renderer_host/surface_texture_transport_client_android.cc:110:
context->bindTexture(kGLTextureExternalOES, texture_id_);
nit: you just bound above.

https://codereview.chromium.org/14199002/diff/115001/content/renderer/media/r...
File content/renderer/media/renderer_gpu_video_decoder_factories.cc (right):

https://codereview.chromium.org/14199002/diff/115001/content/renderer/media/r...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:143:
gles2->GenMailboxCHROMIUM(created_texture_mailboxes_[i].name);
With the new semantics, we can simply produce now, and not need to worry about
it any more (though we probably still need the sync point logic).

https://codereview.chromium.org/14199002/diff/115001/content/renderer/media/r...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:250:
gles2->ConsumeTextureCHROMIUM(texture_target, mailbox.name);
Same here, we don't actually need to consume any more. Though we still need the
waitsyncpoint.

https://codereview.chromium.org/14199002/diff/115001/webkit/renderer/media/we...
File webkit/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/14199002/diff/115001/webkit/renderer/media/we...
webkit/renderer/media/webmediaplayer_impl.cc:632:
mailbox_holder->mailbox().name);
no need for this produce (which ends up being a noop anyway).

[danakj, 2013-06-14 21:54:39]

https://codereview.chromium.org/14199002/diff/115001/content/browser/renderer...
File content/browser/renderer_host/surface_texture_transport_client_android.cc
(right):

https://codereview.chromium.org/14199002/diff/115001/content/browser/renderer...
content/browser/renderer_host/surface_texture_transport_client_android.cc:110:
context->bindTexture(kGLTextureExternalOES, texture_id_);
On 2013/06/14 21:47:29, piman wrote:
> nit: you just bound above.

Done.

https://codereview.chromium.org/14199002/diff/115001/content/renderer/media/r...
File content/renderer/media/renderer_gpu_video_decoder_factories.cc (right):

https://codereview.chromium.org/14199002/diff/115001/content/renderer/media/r...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:143:
gles2->GenMailboxCHROMIUM(created_texture_mailboxes_[i].name);
On 2013/06/14 21:47:29, piman wrote:
> With the new semantics, we can simply produce now, and not need to worry about
> it any more (though we probably still need the sync point logic).

Done. Changed produce method to take a set of mailboxes, and produce them right
after gen.

https://codereview.chromium.org/14199002/diff/115001/content/renderer/media/r...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:250:
gles2->ConsumeTextureCHROMIUM(texture_target, mailbox.name);
On 2013/06/14 21:47:29, piman wrote:
> Same here, we don't actually need to consume any more. Though we still need
the
> waitsyncpoint.

Done. Changed this method to WaitSyncPoint()

https://codereview.chromium.org/14199002/diff/115001/webkit/renderer/media/we...
File webkit/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/14199002/diff/115001/webkit/renderer/media/we...
webkit/renderer/media/webmediaplayer_impl.cc:632:
mailbox_holder->mailbox().name);
On 2013/06/14 21:47:29, piman wrote:
> no need for this produce (which ends up being a noop anyway).

Done, leftovers from before.

[piman, 2013-06-14 22:55:27]

https://codereview.chromium.org/14199002/diff/123001/media/filters/gpu_video_...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/14199002/diff/123001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:514: factories_->ProduceTexturesToMailboxes(
Why not do that directly inside of CreateTextures?

[danakj, 2013-06-15 00:12:57]

https://codereview.chromium.org/14199002/diff/123001/media/filters/gpu_video_...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/14199002/diff/123001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:514: factories_->ProduceTexturesToMailboxes(
On 2013/06/14 22:55:28, piman wrote:
> Why not do that directly inside of CreateTextures?

It seemed more flexible for the future and stuff this way as an API, but no real
reason other than that. Shall I switch it?

[no sievers, 2013-06-15 00:24:23]

lgtm for android

[piman, 2013-06-15 00:42:19]

https://codereview.chromium.org/14199002/diff/123001/media/filters/gpu_video_...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/14199002/diff/123001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:514: factories_->ProduceTexturesToMailboxes(
On 2013/06/15 00:12:58, danakj wrote:
> On 2013/06/14 22:55:28, piman wrote:
> > Why not do that directly inside of CreateTextures?
> 
> It seemed more flexible for the future and stuff this way as an API, but no
real
> reason other than that. Shall I switch it?

My thinking is that, we already know we're associating those textures with the
mailboxes (since you return the mailboxes from CreateTextures), might as well
associate them directly.

If we want the ability to do the mailbox thing only optionally/lazily, then
maybe we should make CreateTextures just generate the texture ids, and have
ProduceTexturesToMailboxes both generate the mailbox and produce?

[danakj, 2013-06-16 14:02:37]

On Fri, Jun 14, 2013 at 8:42 PM, <piman@chromium.org> wrote:

>
> https://codereview.chromium.**org/14199002/diff/123001/**
>
media/filters/gpu_video_**decoder.cc<https://codereview.chromium.org/14199002/diff/123001/media/filters/gpu_video_decoder.cc>
> File media/filters/gpu_video_**decoder.cc (right):
>
> https://codereview.chromium.**org/14199002/diff/123001/**
>
media/filters/gpu_video_**decoder.cc#newcode514<https://codereview.chromium.org/14199002/diff/123001/media/filters/gpu_video_decoder.cc#newcode514>
> media/filters/gpu_video_**decoder.cc:514:
> factories_->**ProduceTexturesToMailboxes(
> On 2013/06/15 00:12:58, danakj wrote:
>
>> On 2013/06/14 22:55:28, piman wrote:
>> > Why not do that directly inside of CreateTextures?
>>
>
>  It seemed more flexible for the future and stuff this way as an API,
>>
> but no real
>
>> reason other than that. Shall I switch it?
>>
>
> My thinking is that, we already know we're associating those textures
> with the mailboxes (since you return the mailboxes from CreateTextures),
> might as well associate them directly.
>
> If we want the ability to do the mailbox thing only optionally/lazily,
> then maybe we should make CreateTextures just generate the texture ids,
> and have ProduceTexturesToMailboxes both generate the mailbox and
> produce?
>

Ya, I was thinking about this some yesterday and came to the same point.
I'll merge!


>
>
https://codereview.chromium.**org/14199002/<https://codereview.chromium.org/1...
>

[danakj, 2013-06-16 14:02:54]

On Fri, Jun 14, 2013 at 8:24 PM, <sievers@chromium.org> wrote:

> lgtm for android
>

Great, thanks.

[danakj, 2013-06-17 19:09:51]

Merged produce into gen/create. Also verified that this patch version is working
on device.

[danakj, 2013-06-17 19:10:09]

piman@ PTAL if you like

[piman, 2013-06-17 19:58:28]

one last thing, then ok.

https://codereview.chromium.org/14199002/diff/139001/content/renderer/media/r...
File content/renderer/media/renderer_gpu_video_decoder_factories.cc (right):

https://codereview.chromium.org/14199002/diff/139001/content/renderer/media/r...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:150: *sync_point
= gles2->InsertSyncPointCHROMIUM();
move the sync point after the flush (the other context will want to wait until
after the flush is done). It's also more efficient (removes one shallow flush).

[scherkus (not reviewing), 2013-06-17 20:20:37]

https://codereview.chromium.org/14199002/diff/139001/content/renderer/media/s...
File content/renderer/media/stream_texture_factory_impl_android.h (right):

https://codereview.chromium.org/14199002/diff/139001/content/renderer/media/s...
content/renderer/media/stream_texture_factory_impl_android.h:34: unsigned
texture_target,
indent by 2 more spaces

https://codereview.chromium.org/14199002/diff/139001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/14199002/diff/139001/media/base/video_frame.c...
media/base/video_frame.cc:383: release_callback_.Run(sync_point_);
are we worried about clients calling texture_mailbox() and keeping a reference
to it and the MailboxHolder object alive longer than the VideoFrame object, or
is that the intended usage?

https://codereview.chromium.org/14199002/diff/139001/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/14199002/diff/139001/media/base/video_frame.h...
media/base/video_frame.h:93: const gpu::Mailbox& texture_mailbox,
thoughts on replacing the 3 args with a MailboxHolder, given we expose a
MailboxHolder accessor?

https://codereview.chromium.org/14199002/diff/139001/media/filters/gpu_video_...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/14199002/diff/139001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:652: 
remove extra blank line

[danakj, 2013-06-18 00:45:04]

https://codereview.chromium.org/14199002/diff/139001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/14199002/diff/139001/media/base/video_frame.c...
media/base/video_frame.cc:383: release_callback_.Run(sync_point_);
On 2013/06/17 20:20:37, scherkus wrote:
> are we worried about clients calling texture_mailbox() and keeping a reference
> to it and the MailboxHolder object alive longer than the VideoFrame object, or
> is that the intended usage?

That is intended. The compositor may hold onto the mailbox and use it, though it
doesn't hold onto the VideoFrame.

https://codereview.chromium.org/14199002/diff/139001/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/14199002/diff/139001/media/base/video_frame.h...
media/base/video_frame.h:93: const gpu::Mailbox& texture_mailbox,
On 2013/06/17 20:20:37, scherkus wrote:
> thoughts on replacing the 3 args with a MailboxHolder, given we expose a
> MailboxHolder accessor?

Hm... I think that sounds like a good idea. Will do.

https://codereview.chromium.org/14199002/diff/139001/media/filters/gpu_video_...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/14199002/diff/139001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:652: 
On 2013/06/17 20:20:37, scherkus wrote:
> remove extra blank line

oops yep

[scherkus (not reviewing), 2013-06-18 00:52:18]

thanks for the answers -- lgtm assuming you'll address the nits, etc..

https://codereview.chromium.org/14199002/diff/139001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/14199002/diff/139001/media/base/video_frame.c...
media/base/video_frame.cc:383: release_callback_.Run(sync_point_);
On 2013/06/18 00:45:05, danakj wrote:
> On 2013/06/17 20:20:37, scherkus wrote:
> > are we worried about clients calling texture_mailbox() and keeping a
reference
> > to it and the MailboxHolder object alive longer than the VideoFrame object,
or
> > is that the intended usage?
> 
> That is intended. The compositor may hold onto the mailbox and use it, though
it
> doesn't hold onto the VideoFrame.

Gotcha -- thanks for the background info!

Seeing as MailboxHolder is a public class w/ an API (however small it is), how
about a mention about lifetimes and the release_callback?

[danakj, 2013-06-18 16:54:12]

https://codereview.chromium.org/14199002/diff/139001/content/renderer/media/r...
File content/renderer/media/renderer_gpu_video_decoder_factories.cc (right):

https://codereview.chromium.org/14199002/diff/139001/content/renderer/media/r...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:150: *sync_point
= gles2->InsertSyncPointCHROMIUM();
On 2013/06/17 19:58:29, piman wrote:
> move the sync point after the flush (the other context will want to wait until
> after the flush is done). It's also more efficient (removes one shallow
flush).

Done.

https://codereview.chromium.org/14199002/diff/139001/content/renderer/media/s...
File content/renderer/media/stream_texture_factory_impl_android.h (right):

https://codereview.chromium.org/14199002/diff/139001/content/renderer/media/s...
content/renderer/media/stream_texture_factory_impl_android.h:34: unsigned
texture_target,
On 2013/06/17 20:20:37, scherkus wrote:
> indent by 2 more spaces

Done.

https://codereview.chromium.org/14199002/diff/139001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/14199002/diff/139001/media/base/video_frame.c...
media/base/video_frame.cc:383: release_callback_.Run(sync_point_);
On 2013/06/18 00:52:18, scherkus wrote:
> On 2013/06/18 00:45:05, danakj wrote:
> > On 2013/06/17 20:20:37, scherkus wrote:
> > > are we worried about clients calling texture_mailbox() and keeping a
> reference
> > > to it and the MailboxHolder object alive longer than the VideoFrame
object,
> or
> > > is that the intended usage?
> > 
> > That is intended. The compositor may hold onto the mailbox and use it,
though
> it
> > doesn't hold onto the VideoFrame.
> 
> Gotcha -- thanks for the background info!
> 
> Seeing as MailboxHolder is a public class w/ an API (however small it is), how
> about a mention about lifetimes and the release_callback?

Done.

https://codereview.chromium.org/14199002/diff/139001/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/14199002/diff/139001/media/base/video_frame.h...
media/base/video_frame.h:93: const gpu::Mailbox& texture_mailbox,
On 2013/06/18 00:45:05, danakj wrote:
> On 2013/06/17 20:20:37, scherkus wrote:
> > thoughts on replacing the 3 args with a MailboxHolder, given we expose a
> > MailboxHolder accessor?
> 
> Hm... I think that sounds like a good idea. Will do.

Done.

https://codereview.chromium.org/14199002/diff/139001/media/filters/gpu_video_...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/14199002/diff/139001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:652: 
On 2013/06/18 00:45:05, danakj wrote:
> On 2013/06/17 20:20:37, scherkus wrote:
> > remove extra blank line
> 
> oops yep

Done.

[commit-bot: I haz the power, 2013-06-18 17:40:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14199002/166003

[commit-bot: I haz the power, 2013-06-18 17:57:16]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, chromedriver2_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, nacl_integration, net_unittests, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-06-18 18:00:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14199002/171003

[commit-bot: I haz the power, 2013-06-18 18:16:25]

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

[danakj, 2013-06-18 18:59:24]

I split the command_buffer_common target out of gpu.gyp to
gpu/command_buffer/command_buffer_common.gyp, so that we can include it for
gpu::Mailbox without circular gyp files.

[commit-bot: I haz the power, 2013-06-19 00:49:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14199002/171003

[commit-bot: I haz the power, 2013-06-19 01:02:27]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-19 01:05:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14199002/208001

[commit-bot: I haz the power, 2013-06-19 01:24:21]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
compile, content_unittests, crypto_unittests, media_unittests, net_unittests,
sql_unittests, ui_unittests, url_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-06-20 14:57:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14199002/225003

[commit-bot: I haz the power, 2013-06-20 15:07:24]

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

[danakj, 2013-06-20 15:11:49]

+jamesr for webkit/support/


On Thu, Jun 20, 2013 at 11:07 AM, <commit-bot@chromium.org> wrote:

> Retried try job too often on chromium_presubmit for step(s) presubmit
> http://build.chromium.org/p/**tryserver.chromium/**
>
buildstatus?builder=chromium_**presubmit&number=10897<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=10897>
>
>
https://chromiumcodereview.**appspot.com/14199002/<https://chromiumcodereview...
>

[commit-bot: I haz the power, 2013-06-20 15:13:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14199002/225003

[commit-bot: I haz the power, 2013-06-20 15:31:32]

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

[jamesr, 2013-06-20 15:50:49]

lgtm on webkit/support/, although that file's not long for this world

[commit-bot: I haz the power, 2013-06-20 18:07:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14199002/152002

[commit-bot: I haz the power, 2013-06-20 19:34:12]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-06-20 19:35:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14199002/152002

[commit-bot: I haz the power, 2013-06-20 23:31:26]

Failed to apply patch for webkit/support/test_stream_texture_factory_android.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: webkit/support/test_stream_texture_factory_android.cc
  |diff --git a/webkit/support/test_stream_texture_factory_android.cc
b/webkit/support/test_stream_texture_factory_android.cc
  |index
6d17ab6c3547274709fdad093f7f8b3be7a19a3a..f2b1d251db9031d90675580319b36fcf6847cc83
100644
  |--- a/webkit/support/test_stream_texture_factory_android.cc
  |+++ b/webkit/support/test_stream_texture_factory_android.cc
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       webkit/support/test_stream_texture_factory_android.cc
Index: webkit/support/test_stream_texture_factory_android.cc
diff --git a/webkit/support/test_stream_texture_factory_android.cc
b/webkit/support/test_stream_texture_factory_android.cc
index
6d17ab6c3547274709fdad093f7f8b3be7a19a3a..f2b1d251db9031d90675580319b36fcf6847cc83
100644
--- a/webkit/support/test_stream_texture_factory_android.cc
+++ b/webkit/support/test_stream_texture_factory_android.cc
@@ -10,7 +10,11 @@ webkit_media::StreamTextureProxy*
TestStreamTextureFactory::CreateProxy() {
   return NULL;
 }
 
-unsigned TestStreamTextureFactory::CreateStreamTexture(unsigned* texture_id) {
+unsigned TestStreamTextureFactory::CreateStreamTexture(
+    unsigned texture_target,
+    unsigned* texture_id,
+    gpu::Mailbox* texture_mailbox,
+    unsigned* texture_mailbox_sync_point) {
   return 0;
 }

[commit-bot: I haz the power, 2013-06-25 16:58:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14199002/277034

[commit-bot: I haz the power, 2013-06-25 17:29:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14199002/277034

[commit-bot: I haz the power, 2013-06-25 17:31:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14199002/277034

[commit-bot: I haz the power, 2013-06-25 18:42:54]

Retried try job too often on mac_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-06-25 18:43:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14199002/277034

[commit-bot: I haz the power, 2013-06-25 19:37:34]

Retried try job too often on mac_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-06-25 19:43:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14199002/277034

[commit-bot: I haz the power, 2013-06-25 22:31:31]

Change committed as 208568