Android media: VideoFrame should not store so many sync points.

Android media: VideoFrame should not store so many sync points.

VideoFrame based on StreamTexture is reused on many frames. However, clients (e.g.
compositor, WebGL) inserts a sync point every frame. So VideoFrame on Android
can keep several mega byte sync points unnecessarily.

This CL makes VideoFrame keep only one sync point. When a client set new sync point
to VideoFrame and VideoFrame already keeps the sync point of another client, VideoFrame
waits the previous sync point before inserting a new sync point. A client must provide
a VideoFrame::SyncPointProvider so that VideoFrame can insert or wait a sync point.

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

[dshwang, 2014-06-03 15:39:40]

VideoFrame can keep multiple sync points, and the sync points are usually
resolved at the end of VideoFrame.
However, webmediaplayer_android can reuse one VideoFrame on its life time
because of StreamTexture. It means that VideoFrame of Android keeps so many sync
points.

Suppose 1 hour video with 60FPS. VideoFrame will keep 216000 sync points (60min
x 60sec x 60FPS), which is 0.8MB. If there is WebGL client, it becomes two times
bigger; 1.6MB.

This CL makes Android create new VideoFrame every frame like other decoders
implementation.

This CL can affect performance adversely, although other platforms have the same
code of this CL.
I submits alternative to not effect performance of Android ASAP;
https://codereview.chromium.org/313623003/
But the alternative require to touch many files.

Could I ask which is better?

https://codereview.chromium.org/312803002/diff/1/content/renderer/media/andro...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/312803002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:1052: // FIXME: reuse
texture pool like gpu_video_decoder.
Currently every frame create new texture. We can avoid via texture pool like
gpu_video_decoder.
If you agree on this approach, I'll improve about it.

https://codereview.chromium.org/312803002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:1118: // handle it.
I use PutCurrentFrame() as the end of each frame, but it feels alien because
other decoders do same thing when the frame is ready.
We can match this impl to other decoder via
VideoFrameProviderClientImpl::DidReceiveFrame() callback.

TL;DR
However, if VideoFrameProviderClientImpl::DidReceiveFrame() calls
WebMediaPlayerAndroid::DidReceiveFrame(), it looks weird too because
VideoFrameProviderClientImpl is subclass of VideoFrameProvider::Client and
WebMediaPlayerAndroid is subclass of VideoFrameProvider. Is it weird for
VideoFrameProvider::Client::DidReceiveFrame() to call
VideoFrameProvider::DidReceiveFrame()?

[Ami GONE FROM CHROMIUM, 2014-06-03 16:10:42]

I defer to sievers/danakj but ISTM tracking one sync point per context is the
right thing to do (i.e. option 2).  IWBN to actually use context identity
instead of ad-hoc-created strings.

[no sievers, 2014-06-03 17:35:06]

What if the video is hidden? Then Get/PutCurrentFrame never gets called, so
would it still keep growing the list of sync points then?

[no sievers, 2014-06-03 17:35:40]

On 2014/06/03 17:35:06, sievers wrote:
> What if the video is hidden? Then Get/PutCurrentFrame never gets called, so
> would it still keep growing the list of sync points then?

I mean the video element itself being hidden. But then displayed through a
canvas or so.

[dshwang, 2014-06-04 10:35:18]

On 2014/06/03 16:10:42, Ami leaving Chromium June 9th wrote:
> I defer to sievers/danakj but ISTM tracking one sync point per context is the
> right thing to do (i.e. option 2).  IWBN to actually use context identity
> instead of ad-hoc-created strings.

I agree. If WebGraphicsContext3D has unique id, it's the best.

On 2014/06/03 17:35:40, sievers wrote:
> On 2014/06/03 17:35:06, sievers wrote:
> > What if the video is hidden? Then Get/PutCurrentFrame never gets called, so
> > would it still keep growing the list of sync points then?
> 
> I mean the video element itself being hidden. But then displayed through a
> canvas or so.

In this implementation, the list of sync points is growing. Option2 doesn't have
this issue.

[no sievers, 2014-06-04 18:16:23]

+piman, jbauman

Ok, so this is a real problem on other platforms too then?
If we insert new sync points without waiting for the old sync points, such as
with hidden video elements, the list of sync points we store in the VideoFrame
just keeps growing?

Why do we need to store multiple sync points? Is this an optimization for not
having to wait for previous sync points in some places where we insert new ones?
Is it an important optimization?

We could also collapse the sync points. It would be easiest on the service side
(easier to avoid having the problem of a growing list of sync points there,
because we know rightaway when they retire). For example the ability to insert a
sync point with another sync point as a dependency would allow us to only store
a single sync point in the client.

[jbauman, 2014-06-04 21:11:18]

On 2014/06/04 18:16:23, sievers wrote:
> +piman, jbauman
> 
> Ok, so this is a real problem on other platforms too then?
> If we insert new sync points without waiting for the old sync points, such as
> with hidden video elements, the list of sync points we store in the VideoFrame
> just keeps growing?
> 
> Why do we need to store multiple sync points? Is this an optimization for not
> having to wait for previous sync points in some places where we insert new
ones?
> Is it an important optimization?

The same texture mailbox could be sent to multiple different compositors with
delegated rendering; say for example the tab the frame is on is dragged out from
one window to another. Obviously this shouldn't happen on android, but in
principle the creators of the sync points may not know about each other.

It might be simplest to just wait on older sync points once a certain number (2
or more) were received.
> 
> We could also collapse the sync points. It would be easiest on the service
side
> (easier to avoid having the problem of a growing list of sync points there,
> because we know rightaway when they retire). For example the ability to insert
a
> sync point with another sync point as a dependency would allow us to only
store
> a single sync point in the client.

[no sievers, 2014-06-04 22:56:18]

On 2014/06/04 21:11:18, jbauman wrote:
> On 2014/06/04 18:16:23, sievers wrote:
> > +piman, jbauman
> > 
> > Ok, so this is a real problem on other platforms too then?
> > If we insert new sync points without waiting for the old sync points, such
as
> > with hidden video elements, the list of sync points we store in the
VideoFrame
> > just keeps growing?
> > 
> > Why do we need to store multiple sync points? Is this an optimization for
not
> > having to wait for previous sync points in some places where we insert new
> ones?
> > Is it an important optimization?
> 
> The same texture mailbox could be sent to multiple different compositors with
> delegated rendering; say for example the tab the frame is on is dragged out
from
> one window to another. Obviously this shouldn't happen on android, but in
> principle the creators of the sync points may not know about each other.
> 
> It might be simplest to just wait on older sync points once a certain number
(2
> or more) were received.
> > 
> > We could also collapse the sync points. It would be easiest on the service
> side
> > (easier to avoid having the problem of a growing list of sync points there,
> > because we know rightaway when they retire). For example the ability to
insert
> a
> > sync point with another sync point as a dependency would allow us to only
> store
> > a single sync point in the client.

Ok, let's just pick a limit for now then and wait for the oldest sync point when
inserting a new one.

[dshwang, 2014-06-05 14:38:29]

On 2014/06/04 22:56:18, sievers wrote:
> Ok, let's just pick a limit for now then and wait for the oldest sync point
when
> inserting a new one.

Thank you for good discussion.

I limit sync point via map. Now VideoFrame keeps only one sync point per client.

sievers@, Your solution is good, but it requires that VideoFrame keeps graphics
3d context reference. It's quite tricky and looks like layer violation.

Could you review again?

https://codereview.chromium.org/312803002/diff/20001/cc/resources/video_resou...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/312803002/diff/20001/cc/resources/video_resou...
cc/resources/video_resource_updater.cc:291: sync_point);
reinterpret_cast is ugly but I could not find better way.
In detail,
we want to make VideoFrame keep sync point per graphic 3d client. Here
compositor uses ContextProvider as graphics 3d context, while WebMediaPlayer
uses WebGraphicsContext3D as graphics 3d context. Moreover, both don't have kind
of id() method.
More elegant solution might be to add id() method in WebGraphicsContext3D and
expose the method to ContextProvider in both in_process and command_buffer impl.
But it seems to be overkill.

https://codereview.chromium.org/312803002/diff/20001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/312803002/diff/20001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:546:
web_graphics_context->insertSyncPoint());
reinterpret_cast here

https://codereview.chromium.org/312803002/diff/20001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/312803002/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:687:
reinterpret_cast<uintptr_t>(web_graphics_context),
reinterpret_cast here

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

https://codereview.chromium.org/312803002/diff/20001/media/base/video_frame.h...
media/base/video_frame.h:265: void AppendReleaseSyncPoint(uintptr_t client,
uint32 sync_point);
As explained video_resource_updater.cc, clients pass pointer of graphic 3d
context as |client|.

[Ami GONE FROM CHROMIUM, 2014-06-05 14:44:31]

Is it reasonable to have VideoFrame::AddSyncPoint return its oldest sync
point if the added point is more than the 5th or 10th or whatever, and rely
on the caller to wait for it?  Or is it important that the context that
generated a point be the one to wait for it?


On Thu, Jun 5, 2014 at 7:38 AM, <dongseong.hwang@intel.com> wrote:

> On 2014/06/04 22:56:18, sievers wrote:
>
>> Ok, let's just pick a limit for now then and wait for the oldest sync
>> point
>>
> when
>
>> inserting a new one.
>>
>
> Thank you for good discussion.
>
> I limit sync point via map. Now VideoFrame keeps only one sync point per
> client.
>
> sievers@, Your solution is good, but it requires that VideoFrame keeps
> graphics
> 3d context reference. It's quite tricky and looks like layer violation.
>
> Could you review again?
>
>
> https://codereview.chromium.org/312803002/diff/20001/cc/
> resources/video_resource_updater.cc
> File cc/resources/video_resource_updater.cc (right):
>
> https://codereview.chromium.org/312803002/diff/20001/cc/
> resources/video_resource_updater.cc#newcode291
> cc/resources/video_resource_updater.cc:291: sync_point);
> reinterpret_cast is ugly but I could not find better way.
> In detail,
> we want to make VideoFrame keep sync point per graphic 3d client. Here
> compositor uses ContextProvider as graphics 3d context, while
> WebMediaPlayer uses WebGraphicsContext3D as graphics 3d context.
> Moreover, both don't have kind of id() method.
> More elegant solution might be to add id() method in
> WebGraphicsContext3D and expose the method to ContextProvider in both
> in_process and command_buffer impl. But it seems to be overkill.
>
> https://codereview.chromium.org/312803002/diff/20001/
> content/renderer/media/android/webmediaplayer_android.cc
> File content/renderer/media/android/webmediaplayer_android.cc (right):
>
> https://codereview.chromium.org/312803002/diff/20001/
> content/renderer/media/android/webmediaplayer_android.cc#newcode546
> content/renderer/media/android/webmediaplayer_android.cc:546:
> web_graphics_context->insertSyncPoint());
> reinterpret_cast here
>
> https://codereview.chromium.org/312803002/diff/20001/
> content/renderer/media/webmediaplayer_impl.cc
> File content/renderer/media/webmediaplayer_impl.cc (right):
>
> https://codereview.chromium.org/312803002/diff/20001/
> content/renderer/media/webmediaplayer_impl.cc#newcode687
> content/renderer/media/webmediaplayer_impl.cc:687:
> reinterpret_cast<uintptr_t>(web_graphics_context),
> reinterpret_cast here
>
> https://codereview.chromium.org/312803002/diff/20001/
> media/base/video_frame.h
> File media/base/video_frame.h (right):
>
> https://codereview.chromium.org/312803002/diff/20001/
> media/base/video_frame.h#newcode265
> media/base/video_frame.h:265: void AppendReleaseSyncPoint(uintptr_t
> client, uint32 sync_point);
> As explained video_resource_updater.cc, clients pass pointer of graphic
> 3d context as |client|.
>
> https://codereview.chromium.org/312803002/
>

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

[dshwang, 2014-06-05 14:53:18]

https://codereview.chromium.org/312803002/diff/20001/cc/resources/video_resou...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/312803002/diff/20001/cc/resources/video_resou...
cc/resources/video_resource_updater.cc:291: sync_point);
On 2014/06/05 14:44:31, Ami leaving Chromium June 9th wrote:
> Is it reasonable to have VideoFrame::AddSyncPoint return its oldest sync
> point if the added point is more than the 5th or 10th or whatever, and rely
> on the caller to wait for it?  Or is it important that the context that
> generated a point be the one to wait for it?

That's good question. If one client waits for sync point made by another client,
VideoFrame can keep only one latest sync point. It's not important to keep sync
points for all clients.
However, VideoResourceUpdater can not wait a sync point because
VideoResourceUpdater can not use any graphics 3d context at this point. If we
really want to do that, we need hack that ResouceProvider passes graphics 3d
context to this callback. IMO, it's more ugly.

[no sievers, 2014-06-05 17:12:27]

On 2014/06/05 14:53:18, dshwang wrote:
>
https://codereview.chromium.org/312803002/diff/20001/cc/resources/video_resou...
> File cc/resources/video_resource_updater.cc (right):
> 
>
https://codereview.chromium.org/312803002/diff/20001/cc/resources/video_resou...
> cc/resources/video_resource_updater.cc:291: sync_point);
> On 2014/06/05 14:44:31, Ami leaving Chromium June 9th wrote:
> > Is it reasonable to have VideoFrame::AddSyncPoint return its oldest sync
> > point if the added point is more than the 5th or 10th or whatever, and rely
> > on the caller to wait for it?  Or is it important that the context that
> > generated a point be the one to wait for it?
> 
> That's good question. If one client waits for sync point made by another
client,
> VideoFrame can keep only one latest sync point. It's not important to keep
sync
> points for all clients.
> However, VideoResourceUpdater can not wait a sync point because
> VideoResourceUpdater can not use any graphics 3d context at this point. If we
> really want to do that, we need hack that ResouceProvider passes graphics 3d
> context to this callback. IMO, it's more ugly.

Wait, whoever inserts a new sync point obviously has a context, so can wait on
the oldest one, no?

If you keep enough sync points the oldest one will always be already retired and
not end up blocking that context in practice.

[dshwang, 2014-06-05 17:30:53]

On 2014/06/05 17:12:27, sievers wrote:
> Wait, whoever inserts a new sync point obviously has a context, so can wait on
> the oldest one, no?

In theory, you're right. but in implementation detail, we need ugly change.
In the compositor, ResourceProvider inserts a sync pointer but callback in
video_resource_updater.cc appends sync pointer to VideoFrame.
If we want to wait old sync pointer, ResourceProvider should pass context 3d to
the callback, but the signature of callback is used in so many place;
typedef base::Callback<void(uint32 sync_point, bool is_lost)> ReleaseCallback;

> If you keep enough sync points the oldest one will always be already retired
and
> not end up blocking that context in practice.

Yes, right. In addition, we can wait the latest sync point per context 3d also.

[danakj, 2014-06-06 17:50:40]

On Thu, Jun 5, 2014 at 7:30 PM, <dongseong.hwang@intel.com> wrote:

> On 2014/06/05 17:12:27, sievers wrote:
>
>> Wait, whoever inserts a new sync point obviously has a context, so can
>> wait on
>> the oldest one, no?
>>
>
> In theory, you're right. but in implementation detail, we need ugly change.
> In the compositor, ResourceProvider inserts a sync pointer but callback in
> video_resource_updater.cc appends sync pointer to VideoFrame.
> If we want to wait old sync pointer, ResourceProvider should pass context
> 3d to
> the callback, but the signature of callback is used in so many place;
> typedef base::Callback<void(uint32 sync_point, bool is_lost)>
> ReleaseCallback;


It would be possible to have the ReturnTexture callback have a weakptr to
the VideoResourceUpdater in it and use the context there. The
RecycleResource method does similarly.


>
>
>  If you keep enough sync points the oldest one will always be already
>> retired
>>
> and
>
>> not end up blocking that context in practice.
>>
>
> Yes, right. In addition, we can wait the latest sync point per context 3d
> also.
>
> https://codereview.chromium.org/312803002/
>

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

[danakj, 2014-06-06 17:57:58]

On Thu, Jun 5, 2014 at 4:44 PM, Ami Fischman <fischman@chromium.org> wrote:

> Is it reasonable to have VideoFrame::AddSyncPoint return its oldest sync
> point if the added point is more than the 5th or 10th or whatever, and rely
> on the caller to wait for it?  Or is it important that the context that
> generated a point be the one to wait for it?
>

Returning it is too late, you need to wait before inserting to get correct
semantics. However anyone who inserts a sync point should have some way to
wait on the existing sync point. I think this was mostly done because
VideoResourceUpdater's ReturnTexture() was adding a new sync point and
doesn't have a context, but it could have stuff bound into the callback to
let it have a context. That may be a much better approach? Unless I'm
forgetting some context now..


>
>
> On Thu, Jun 5, 2014 at 7:38 AM, <dongseong.hwang@intel.com> wrote:
>
>> On 2014/06/04 22:56:18, sievers wrote:
>>
>>> Ok, let's just pick a limit for now then and wait for the oldest sync
>>> point
>>>
>> when
>>
>>> inserting a new one.
>>>
>>
>> Thank you for good discussion.
>>
>> I limit sync point via map. Now VideoFrame keeps only one sync point per
>> client.
>>
>> sievers@, Your solution is good, but it requires that VideoFrame keeps
>> graphics
>> 3d context reference. It's quite tricky and looks like layer violation.
>>
>> Could you review again?
>>
>>
>> https://codereview.chromium.org/312803002/diff/20001/cc/
>> resources/video_resource_updater.cc
>> File cc/resources/video_resource_updater.cc (right):
>>
>> https://codereview.chromium.org/312803002/diff/20001/cc/
>> resources/video_resource_updater.cc#newcode291
>> cc/resources/video_resource_updater.cc:291: sync_point);
>> reinterpret_cast is ugly but I could not find better way.
>> In detail,
>> we want to make VideoFrame keep sync point per graphic 3d client. Here
>> compositor uses ContextProvider as graphics 3d context, while
>> WebMediaPlayer uses WebGraphicsContext3D as graphics 3d context.
>> Moreover, both don't have kind of id() method.
>> More elegant solution might be to add id() method in
>> WebGraphicsContext3D and expose the method to ContextProvider in both
>> in_process and command_buffer impl. But it seems to be overkill.
>>
>> https://codereview.chromium.org/312803002/diff/20001/
>> content/renderer/media/android/webmediaplayer_android.cc
>> File content/renderer/media/android/webmediaplayer_android.cc (right):
>>
>> https://codereview.chromium.org/312803002/diff/20001/
>> content/renderer/media/android/webmediaplayer_android.cc#newcode546
>> content/renderer/media/android/webmediaplayer_android.cc:546:
>> web_graphics_context->insertSyncPoint());
>> reinterpret_cast here
>>
>> https://codereview.chromium.org/312803002/diff/20001/
>> content/renderer/media/webmediaplayer_impl.cc
>> File content/renderer/media/webmediaplayer_impl.cc (right):
>>
>> https://codereview.chromium.org/312803002/diff/20001/
>> content/renderer/media/webmediaplayer_impl.cc#newcode687
>> content/renderer/media/webmediaplayer_impl.cc:687:
>> reinterpret_cast<uintptr_t>(web_graphics_context),
>> reinterpret_cast here
>>
>> https://codereview.chromium.org/312803002/diff/20001/
>> media/base/video_frame.h
>> File media/base/video_frame.h (right):
>>
>> https://codereview.chromium.org/312803002/diff/20001/
>> media/base/video_frame.h#newcode265
>> media/base/video_frame.h:265: void AppendReleaseSyncPoint(uintptr_t
>> client, uint32 sync_point);
>> As explained video_resource_updater.cc, clients pass pointer of graphic
>> 3d context as |client|.
>>
>> https://codereview.chromium.org/312803002/
>>
>
>

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

[dshwang, 2014-06-06 18:29:25]

Thank you for review.

On 2014/06/06 17:50:40, danakj wrote:
> On Thu, Jun 5, 2014 at 7:30 PM, <mailto:dongseong.hwang@intel.com> wrote:
> 
> > On 2014/06/05 17:12:27, sievers wrote:
> >
> >> Wait, whoever inserts a new sync point obviously has a context, so can
> >> wait on
> >> the oldest one, no?
> >>
> >
> > In theory, you're right. but in implementation detail, we need ugly change.
> > In the compositor, ResourceProvider inserts a sync pointer but callback in
> > video_resource_updater.cc appends sync pointer to VideoFrame.
> > If we want to wait old sync pointer, ResourceProvider should pass context
> > 3d to
> > the callback, but the signature of callback is used in so many place;
> > typedef base::Callback<void(uint32 sync_point, bool is_lost)>
> > ReleaseCallback;
> 
> 
> It would be possible to have the ReturnTexture callback have a weakptr to
> the VideoResourceUpdater in it and use the context there. The
> RecycleResource method does similarly.
> 

ResourceProvider, given VideoFrame and ReturnTexture callback bind object can
outlive over VideoResourceUpdater. e.g. when compositor is destructed
So I think it's not good for ReturnTexture callvask to use weakptr to the
VideoResourceUpdater 

On 2014/06/06 17:57:58, danakj wrote:
> On Thu, Jun 5, 2014 at 4:44 PM, Ami Fischman <mailto:fischman@chromium.org>
wrote:
> 
> > Is it reasonable to have VideoFrame::AddSyncPoint return its oldest sync
> > point if the added point is more than the 5th or 10th or whatever, and rely
> > on the caller to wait for it?  Or is it important that the context that
> > generated a point be the one to wait for it?
> >
> 
> Returning it is too late, you need to wait before inserting to get correct
> semantics. However anyone who inserts a sync point should have some way to
> wait on the existing sync point. I think this was mostly done because
> VideoResourceUpdater's ReturnTexture() was adding a new sync point and
> doesn't have a context, but it could have stuff bound into the callback to
> let it have a context. That may be a much better approach? Unless I'm
> forgetting some context now..

Let me explain context. This story consists of three chapters.
1) Creation of VideoFrame
When VideoFrame is created, VideoFrame::MailboxHolder::sync_point is set.
2) Clients consume VideoFrame
When a client consumes the VideoFrame, the client waits
VideoFrame::MailboxHolder::sync_point, and uses the mailbox, and then insert
release-sync-point via VideoFrame::AppendSyncPoint().
1 x Compositor, n x WebGL, n x Canvas can be a client.
3) Destruction of VideoFrame
When VideoFrame is destructed, all sync points inserted by clients are waited.

Suppose ReturnTexture callback is about inserting sync point for compositor.
If VideoFrame returns the old sync point inserted by compositor, ReturnTexture
callback doesn't need to wait the old sync point because old sync point was
issued by compositor.
If VideoFrame returns the old sync point inserted by WebGL, ReturnTexture
callback must wait the old sync point, but the order is not important because
ReturnTexture callback just waits the old sync point more earlier than
Destruction of VideoFrame, which originally waits the old sync point.

In conclusion, sievers@'s suggestion is good except for the case when
ResourceProvider outlives VideoResourceUpdater.
So I guess touching many files is inevitable. If you have better idea, please
let me know.

[danakj, 2014-06-06 21:59:06]

On Fri, Jun 6, 2014 at 2:29 PM, <dongseong.hwang@intel.com> wrote:

> Thank you for review.
>
> On 2014/06/06 17:50:40, danakj wrote:
>
>  On Thu, Jun 5, 2014 at 7:30 PM, <mailto:dongseong.hwang@intel.com> wrote:
>>
>
>  > On 2014/06/05 17:12:27, sievers wrote:
>> >
>> >> Wait, whoever inserts a new sync point obviously has a context, so can
>> >> wait on
>> >> the oldest one, no?
>> >>
>> >
>> > In theory, you're right. but in implementation detail, we need ugly
>> change.
>> > In the compositor, ResourceProvider inserts a sync pointer but callback
>> in
>> > video_resource_updater.cc appends sync pointer to VideoFrame.
>> > If we want to wait old sync pointer, ResourceProvider should pass
>> context
>> > 3d to
>> > the callback, but the signature of callback is used in so many place;
>> > typedef base::Callback<void(uint32 sync_point, bool is_lost)>
>> > ReleaseCallback;
>>
>
>
>  It would be possible to have the ReturnTexture callback have a weakptr to
>> the VideoResourceUpdater in it and use the context there. The
>> RecycleResource method does similarly.
>>
>
>
> ResourceProvider, given VideoFrame and ReturnTexture callback bind object
> can
> outlive over VideoResourceUpdater. e.g. when compositor is destructed
> So I think it's not good for ReturnTexture callvask to use weakptr to the
> VideoResourceUpdater
>

Two options at that point:

1. Instead of WeakPtr<VideoResourceUpdater> have a
scoped_refptr<ContextProvider> bound to the method. This is kinda meh, cuz
ref counting..

2. If the weakptr is invalid, mark the texture as lost instead of inserting
a sync point. Situations like this are why cc::ReleaseCallback has both a
sync point and a lost bool. When the texture is reported as lost, no one
can use it anymore exist to just delete local references to it. Would this
break things?
- This situation only happens when either:
a) The context provider is lost, so VideoLayerImpl::ReleaseResources
happens. Since we're losing our context, the ResourceProvider will be
reporting the texture as lost next anyway. Currently ReturnTexture() just
ignores this flag and gives back a sync point of 0... we could just
continue to do this...
b) The video layer is removed. This means the video is no longer being
displayed, do we care about the decoder getting back a correct sync point
in this case? Will it actually want to re-use the texture, or would it just
delete it? If it could reuse it for something, could we pass along a "lost"
bool instead to prevent it and make it create a new texture?


>
> On 2014/06/06 17:57:58, danakj wrote:
>
>> On Thu, Jun 5, 2014 at 4:44 PM, Ami Fischman <mailto:
>> fischman@chromium.org>
>>
> wrote:
>
>  > Is it reasonable to have VideoFrame::AddSyncPoint return its oldest sync
>> > point if the added point is more than the 5th or 10th or whatever, and
>> rely
>> > on the caller to wait for it?  Or is it important that the context that
>> > generated a point be the one to wait for it?
>> >
>>
>
>  Returning it is too late, you need to wait before inserting to get correct
>> semantics. However anyone who inserts a sync point should have some way to
>> wait on the existing sync point. I think this was mostly done because
>> VideoResourceUpdater's ReturnTexture() was adding a new sync point and
>> doesn't have a context, but it could have stuff bound into the callback to
>> let it have a context. That may be a much better approach? Unless I'm
>> forgetting some context now..
>>
>
> Let me explain context. This story consists of three chapters.
> 1) Creation of VideoFrame
> When VideoFrame is created, VideoFrame::MailboxHolder::sync_point is set.
> 2) Clients consume VideoFrame
> When a client consumes the VideoFrame, the client waits
> VideoFrame::MailboxHolder::sync_point, and uses the mailbox, and then
> insert
> release-sync-point via VideoFrame::AppendSyncPoint().
> 1 x Compositor, n x WebGL, n x Canvas can be a client.
> 3) Destruction of VideoFrame
> When VideoFrame is destructed, all sync points inserted by clients are
> waited.
>
> Suppose ReturnTexture callback is about inserting sync point for
> compositor.
> If VideoFrame returns the old sync point inserted by compositor,
> ReturnTexture
> callback doesn't need to wait the old sync point because old sync point was
> issued by compositor.
> If VideoFrame returns the old sync point inserted by WebGL, ReturnTexture
> callback must wait the old sync point, but the order is not important
> because
> ReturnTexture callback just waits the old sync point more earlier than
> Destruction of VideoFrame, which originally waits the old sync point.
>
> In conclusion, sievers@'s suggestion is good except for the case when
> ResourceProvider outlives VideoResourceUpdater.
> So I guess touching many files is inevitable. If you have better idea,
> please
> let me know.
>
> https://codereview.chromium.org/312803002/
>

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

[dshwang, 2014-06-09 12:33:46]

Thank you for good advice. Your points are all right.

On 2014/06/06 21:59:06, danakj wrote:
> Two options at that point:
> 
> 1. Instead of WeakPtr<VideoResourceUpdater> have a
> scoped_refptr<ContextProvider> bound to the method. This is kinda meh, cuz
> ref counting..

That's good point. we can wait old sync point via
scoped_refptr<ContextProvider>.

> 2. If the weakptr is invalid, mark the texture as lost instead of inserting
> a sync point. Situations like this are why cc::ReleaseCallback has both a
> sync point and a lost bool. When the texture is reported as lost, no one
> can use it anymore exist to just delete local references to it. Would this
> break things?
> - This situation only happens when either:
> a) The context provider is lost, so VideoLayerImpl::ReleaseResources
> happens. Since we're losing our context, the ResourceProvider will be
> reporting the texture as lost next anyway. Currently ReturnTexture() just
> ignores this flag and gives back a sync point of 0... we could just
> continue to do this...

Indeed.
currently, release callback of VideoFrame deletes the texture regardless of lost
context. When lost context, it is no-op.
Gpu video seems to need to handle lost context more delicately.

> b) The video layer is removed. This means the video is no longer being
> displayed, do we care about the decoder getting back a correct sync point
> in this case? Will it actually want to re-use the texture, or would it just
> delete it? If it could reuse it for something, could we pass along a "lost"
> bool instead to prevent it and make it create a new texture?

VideoResourceUpdater is about inserting sync point at the moment although the
video layer is removed.
It means video frame should be displayed on this frame, while no longer being
displayed from next frame. So we want sync point to ensure gpu video decoder
doesn't delete texture until this frame is displayed.

We have two options to resolve this issue.
1. use map like Patch Set 2
2. Make VideoFrame keep only one sync point. Clients must wait old sync point
when inserting new sync point into VideoFrame.

Which is better? both are its own pros. and cons.
1. pros.: easy for client to use | cons.: key type is ugly
2. pros.: simple data structure of VideoFrame | cons.: hard for client to use

[danakj, 2014-06-13 15:47:52]

On Mon, Jun 9, 2014 at 8:33 AM, <dongseong.hwang@intel.com> wrote:

> Thank you for good advice. Your points are all right.
>
>
> On 2014/06/06 21:59:06, danakj wrote:
>
>> Two options at that point:
>>
>
>  1. Instead of WeakPtr<VideoResourceUpdater> have a
>> scoped_refptr<ContextProvider> bound to the method. This is kinda meh, cuz
>> ref counting..
>>
>
> That's good point. we can wait old sync point via
> scoped_refptr<ContextProvider>.
>
>
>  2. If the weakptr is invalid, mark the texture as lost instead of
>> inserting
>> a sync point. Situations like this are why cc::ReleaseCallback has both a
>> sync point and a lost bool. When the texture is reported as lost, no one
>> can use it anymore exist to just delete local references to it. Would this
>> break things?
>> - This situation only happens when either:
>> a) The context provider is lost, so VideoLayerImpl::ReleaseResources
>> happens. Since we're losing our context, the ResourceProvider will be
>> reporting the texture as lost next anyway. Currently ReturnTexture() just
>> ignores this flag and gives back a sync point of 0... we could just
>> continue to do this...
>>
>
> Indeed.
> currently, release callback of VideoFrame deletes the texture regardless
> of lost
> context. When lost context, it is no-op.
> Gpu video seems to need to handle lost context more delicately.
>
>
>  b) The video layer is removed. This means the video is no longer being
>> displayed, do we care about the decoder getting back a correct sync point
>> in this case? Will it actually want to re-use the texture, or would it
>> just
>> delete it? If it could reuse it for something, could we pass along a
>> "lost"
>> bool instead to prevent it and make it create a new texture?
>>
>
> VideoResourceUpdater is about inserting sync point at the moment although
> the
> video layer is removed.
> It means video frame should be displayed on this frame, while no longer
> being
> displayed from next frame. So we want sync point to ensure gpu video
> decoder
> doesn't delete texture until this frame is displayed.
>
> We have two options to resolve this issue.
> 1. use map like Patch Set 2
> 2. Make VideoFrame keep only one sync point. Clients must wait old sync
> point
> when inserting new sync point into VideoFrame.
>
> Which is better? both are its own pros. and cons.
> 1. pros.: easy for client to use | cons.: key type is ugly
> 2. pros.: simple data structure of VideoFrame | cons.: hard for client to
> use
>

Seems like there's only one client it's hard for (VideoResourceUpdater)?
And it could just lose the texture when it needs to as long as we make the
decoder respect that. It sounds like a good way to go, do you think?

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

[dshwang, 2014-06-14 15:34:25]

On 2014/06/13 15:47:52, danakj wrote:
> On Mon, Jun 9, 2014 at 8:33 AM, <mailto:dongseong.hwang@intel.com> wrote:
> > We have two options to resolve this issue.
> > 1. use map like Patch Set 2
> > 2. Make VideoFrame keep only one sync point. Clients must wait old sync
> > point
> > when inserting new sync point into VideoFrame.
> >
> > Which is better? both are its own pros. and cons.
> > 1. pros.: easy for client to use | cons.: key type is ugly
> > 2. pros.: simple data structure of VideoFrame | cons.: hard for client to
> > use
> >
> 
> Seems like there's only one client it's hard for (VideoResourceUpdater)?
> And it could just lose the texture when it needs to as long as we make the
> decoder respect that. It sounds like a good way to go, do you think?

Yes, Only (VideoResourceUpdater) has difficulty, and we need to ensure decoder
don't delete/recycle the texture until compositor use it.
What's your preference? I'll go to that direction.
1. using map to keep sync point per client
2. waiting old sync point by a client

[danakj, 2014-06-17 22:50:05]

On Sat, Jun 14, 2014 at 8:34 AM, <dongseong.hwang@intel.com> wrote:

> On 2014/06/13 15:47:52, danakj wrote:
>
>> On Mon, Jun 9, 2014 at 8:33 AM, <mailto:dongseong.hwang@intel.com> wrote:
>> > We have two options to resolve this issue.
>> > 1. use map like Patch Set 2
>> > 2. Make VideoFrame keep only one sync point. Clients must wait old sync
>> > point
>> > when inserting new sync point into VideoFrame.
>> >
>> > Which is better? both are its own pros. and cons.
>> > 1. pros.: easy for client to use | cons.: key type is ugly
>> > 2. pros.: simple data structure of VideoFrame | cons.: hard for client
>> to
>> > use
>> >
>>
>
>  Seems like there's only one client it's hard for (VideoResourceUpdater)?
>> And it could just lose the texture when it needs to as long as we make the
>> decoder respect that. It sounds like a good way to go, do you think?
>>
>
> Yes, Only (VideoResourceUpdater) has difficulty, and we need to ensure
> decoder
> don't delete/recycle the texture until compositor use it.
> What's your preference? I'll go to that direction.
> 1. using map to keep sync point per client
>

The part that seems weird about this is now we need some unique identifier
per client? That seems overly complicated/strange.


> 2. waiting old sync point by a client
>

I'm worried about potential deadlocks here if we hold a mutex while doing
wait/insert sync point.. But I guess we could grab the sync point via
mutex, release it, wait/insert, then save it back in the videoframe via
mutex again, and if it's changed in the meantime, repeat the process.

It sounds to me like 2 would mean the simplest API on the VideoFrame which
sounds like a good reason to go that way. So I'd lean that way personally.



>
>
> https://codereview.chromium.org/312803002/
>

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

[dshwang, 2014-06-18 00:06:10]

On 2014/06/17 22:50:05, danakj wrote:
> On Sat, Jun 14, 2014 at 8:34 AM, <mailto:dongseong.hwang@intel.com> wrote:
> > Yes, Only (VideoResourceUpdater) has difficulty, and we need to ensure
> > decoder
> > don't delete/recycle the texture until compositor use it.
> > What's your preference? I'll go to that direction.
> > 1. using map to keep sync point per client
> >
> 
> The part that seems weird about this is now we need some unique identifier
> per client? That seems overly complicated/strange.

right. this option needs weird unique identifier, as which he patch set uses
pointer value.

> > 2. waiting old sync point by a client
> >
> 
> I'm worried about potential deadlocks here if we hold a mutex while doing
> wait/insert sync point.. But I guess we could grab the sync point via
> mutex, release it, wait/insert, then save it back in the videoframe via
> mutex again, and if it's changed in the meantime, repeat the process.

How about this pseudo code for client.

uint32 old_sync_point = video_frame.AppendSyncPoint(new_sync_point);
gl->WaitSyncPoint(old_sync_point);

The client should wait the old sync point before video_frame is destructed.

Speaking deadlock, AppendSyncPoint() lock&release the mutex by itself, so there
is not potential deadlock.

> It sounds to me like 2 would mean the simplest API on the VideoFrame which
> sounds like a good reason to go that way. So I'd lean that way personally.

Thank you for feedback. I'll update soon.

[Ami GONE FROM CHROMIUM, 2014-06-18 00:56:00]

I think danakj@ and sievers@ have this well in hand, and I'm no longer an OWNER
(so what use am I?!?), so removing myself from R=.  scherkus@ will be your
media/ OWNER for this CL probably.

Let me know if I can provide any further info.

[dshwang, 2014-06-18 15:53:54]

Hi, danakj@ and sievers@
I update this CL using Option2 and amend CL's description.
Now, VideoFrame keeps only one sync point. And the client MUST wait the previous
sync point that VideoFrame returns.

Could you review again?

On 2014/06/18 00:56:00, Ami GONE FROM CHROMIUM wrote:
> I think danakj@ and sievers@ have this well in hand, and I'm no longer an
OWNER
> (so what use am I?!?), so removing myself from R=.  scherkus@ will be your
> media/ OWNER for this CL probably.
> 
> Let me know if I can provide any further info.

Thank you for kind explanation.

[danakj, 2014-06-18 16:00:11]

On Tue, Jun 17, 2014 at 5:06 PM, <dongseong.hwang@intel.com> wrote:

> On 2014/06/17 22:50:05, danakj wrote:
>
>  On Sat, Jun 14, 2014 at 8:34 AM, <mailto:dongseong.hwang@intel.com>
>> wrote:
>> > Yes, Only (VideoResourceUpdater) has difficulty, and we need to ensure
>> > decoder
>> > don't delete/recycle the texture until compositor use it.
>> > What's your preference? I'll go to that direction.
>> > 1. using map to keep sync point per client
>> >
>>
>
>  The part that seems weird about this is now we need some unique identifier
>> per client? That seems overly complicated/strange.
>>
>
> right. this option needs weird unique identifier, as which he patch set
> uses
> pointer value.
>
>
>  > 2. waiting old sync point by a client
>> >
>>
>
>  I'm worried about potential deadlocks here if we hold a mutex while doing
>> wait/insert sync point.. But I guess we could grab the sync point via
>> mutex, release it, wait/insert, then save it back in the videoframe via
>> mutex again, and if it's changed in the meantime, repeat the process.
>>
>
> How about this pseudo code for client.
>
> uint32 old_sync_point = video_frame.AppendSyncPoint(new_sync_point);
> gl->WaitSyncPoint(old_sync_point);
>

You must wait on the old sync point before InsertSyncPoint() or you aren't
guaranteeing that the wait will happen in order.

Writing it out, it would look something like...

while (1) {
  {
    lock
    old = videoframe.GetSyncPoint();
  }
  WaitSyncPoint(old);
  new = InsertSyncPoint();
  {
    lock
    old_again = videoframe.GetSyncPoint();
    if (old == old_again) {
      videoframe.SetSyncPoint(new);
      break;
  }
}

Since you only set the sync point on the VideoFrame when you exit the loop,
you know that in ties someone will set first and break the tie and leave
the loop, so this shouldn't be able to fight with itself on another thread
forever.

It could be implemented by passing the old and the new sync point to the
Set() method, and if the old doesn't match the current sync point, it just
early out and return false. Then the calling code is like

do {
  old = videoframe.GetSyncPoint();
  WaitSyncPoint(old);
  new = InsertSyncPoint();
} while (!videoframe.SetSyncPoint(old, new));

VideoFrame::SetSyncPoint(old, new) {
  lock;
  if (old != current_) return false;
  current_ = new;
  return true;
}

sievers@ WDYT, does this look reasonable?


>
> The client should wait the old sync point before video_frame is destructed.
>
> Speaking deadlock, AppendSyncPoint() lock&release the mutex by itself, so
> there
> is not potential deadlock.
>
>
>  It sounds to me like 2 would mean the simplest API on the VideoFrame which
>> sounds like a good reason to go that way. So I'd lean that way personally.
>>
>
> Thank you for feedback. I'll update soon.
>
> https://codereview.chromium.org/312803002/
>
>

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

[dshwang, 2014-06-18 16:10:20]

On 2014/06/18 16:00:11, danakj wrote:
> You must wait on the old sync point before InsertSyncPoint() or you aren't
> guaranteeing that the wait will happen in order.
> 
> Writing it out, it would look something like...
> 
> while (1) {
>   {
>     lock
>     old = videoframe.GetSyncPoint();
>   }
>   WaitSyncPoint(old);
>   new = InsertSyncPoint();
>   {
>     lock
>     old_again = videoframe.GetSyncPoint();
>     if (old == old_again) {
>       videoframe.SetSyncPoint(new);
>       break;
>   }
> }
> 
> Since you only set the sync point on the VideoFrame when you exit the loop,
> you know that in ties someone will set first and break the tie and leave
> the loop, so this shouldn't be able to fight with itself on another thread
> forever.
> 
> It could be implemented by passing the old and the new sync point to the
> Set() method, and if the old doesn't match the current sync point, it just
> early out and return false. Then the calling code is like
> 
> do {
>   old = videoframe.GetSyncPoint();
>   WaitSyncPoint(old);
>   new = InsertSyncPoint();
> } while (!videoframe.SetSyncPoint(old, new));
> 
> VideoFrame::SetSyncPoint(old, new) {
>   lock;
>   if (old != current_) return false;
>   current_ = new;
>   return true;
> }
> 
> sievers@ WDYT, does this look reasonable?

Thank you for quick review.

No, we don't need to wait old sync point before setting new sync point, because
we don't need to order among release-sync-points.
, because there is not any interaction between clients. All clients are just
consumer which reads mailbox the decoder produced.

Let me explain in more detail.
Here is current summary of VideoFrame lifecycle.
1) Creation of VideoFrame
When the decoder creates VideoFrame, VideoFrame::MailboxHolder::sync_point is
set.
2) Clients consume VideoFrame
When a client consumes the VideoFrame, the client waits
VideoFrame::MailboxHolder::sync_point, and uses the mailbox, and then insert
release-sync-point via VideoFrame::AppendSyncPoint().
client consist of 1 x Compositor, n x WebGL, n x Canvas and etc.
3) Destruction of VideoFrame
When VideoFrame is destructed, all sync points inserted by clients are waited.

Currently, all release-sync-points are waited in the destructor of VideoFrame.

This patch set just move the time to wait release-sync-points eariler; when each
client set the new sync point.
If each client wait the old sync point before the VideoFrame is destucted,
everything is fine.

[danakj, 2014-06-18 16:17:30]

On Wed, Jun 18, 2014 at 9:10 AM, <dongseong.hwang@intel.com> wrote:

> On 2014/06/18 16:00:11, danakj wrote:
>
>> You must wait on the old sync point before InsertSyncPoint() or you aren't
>> guaranteeing that the wait will happen in order.
>>
>
>  Writing it out, it would look something like...
>>
>
>  while (1) {
>>    {
>>      lock
>>      old = videoframe.GetSyncPoint();
>>    }
>>    WaitSyncPoint(old);
>>    new = InsertSyncPoint();
>>    {
>>      lock
>>      old_again = videoframe.GetSyncPoint();
>>      if (old == old_again) {
>>        videoframe.SetSyncPoint(new);
>>        break;
>>    }
>> }
>>
>
>  Since you only set the sync point on the VideoFrame when you exit the
>> loop,
>> you know that in ties someone will set first and break the tie and leave
>> the loop, so this shouldn't be able to fight with itself on another thread
>> forever.
>>
>
>  It could be implemented by passing the old and the new sync point to the
>> Set() method, and if the old doesn't match the current sync point, it just
>> early out and return false. Then the calling code is like
>>
>
>  do {
>>    old = videoframe.GetSyncPoint();
>>    WaitSyncPoint(old);
>>    new = InsertSyncPoint();
>> } while (!videoframe.SetSyncPoint(old, new));
>>
>
>  VideoFrame::SetSyncPoint(old, new) {
>>    lock;
>>    if (old != current_) return false;
>>    current_ = new;
>>    return true;
>> }
>>
>
>  sievers@ WDYT, does this look reasonable?
>>
>
> Thank you for quick review.
>
> No, we don't need to wait old sync point before setting new sync point,
> because
> we don't need to order among release-sync-points.
> , because there is not any interaction between clients. All clients are
> just
> consumer which reads mailbox the decoder produced.
>
> Let me explain in more detail.
> Here is current summary of VideoFrame lifecycle.
> 1) Creation of VideoFrame
> When the decoder creates VideoFrame, VideoFrame::MailboxHolder::sync_point
> is
> set.
> 2) Clients consume VideoFrame
> When a client consumes the VideoFrame, the client waits
> VideoFrame::MailboxHolder::sync_point, and uses the mailbox, and then
> insert
> release-sync-point via VideoFrame::AppendSyncPoint().
> client consist of 1 x Compositor, n x WebGL, n x Canvas and etc.
> 3) Destruction of VideoFrame
> When VideoFrame is destructed, all sync points inserted by clients are
> waited.
>
> Currently, all release-sync-points are waited in the destructor of
> VideoFrame.
>
> This patch set just move the time to wait release-sync-points eariler;
> when each
> client set the new sync point.
> If each client wait the old sync point before the VideoFrame is destucted,
> everything is fine.
>

When the video decoder gets back the VideoFrame, it's going to wait on the
single sync point in the VideoFrame. The guarantee it wants is that no GPU
commands are going to make use of the texture after that wait, so it can
start changing the texture.

However, if someone else did InsertSyncPoint(); WaitSyncPoint(); then, the
video decoder is going to wait on that sync point and guarantee that no
commands from that last context are run afterward, but it has no guarantee
for the context that inserted the sync point before that.

Context A: Compositor
Context B: WebGL
Context C: Video Decoder

A: Draws from VideoFrame to screen
A: InsertSyncPoint A1
B: Copies VideoFrame to another Texture
B: InsertSyncPoint B1
B: Waits for sync point A1
   - we can now guarantee that actions B takes will occur after A1
C: Waits for sync point B1
  - we can now guarantee that actions C takes will occur after B1, but have
no guarantee about A1

Reversing the order B's insert/wait will give us a better guarantee on C.

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

[dshwang, 2014-06-18 16:33:35]

On 2014/06/18 16:17:30, danakj wrote:
> On Wed, Jun 18, 2014 at 9:10 AM, <mailto:dongseong.hwang@intel.com> wrote:
> 
> > On 2014/06/18 16:00:11, danakj wrote:
> >
> >> You must wait on the old sync point before InsertSyncPoint() or you aren't
> >> guaranteeing that the wait will happen in order.
> >>
> >
> >  Writing it out, it would look something like...
> >>
> >
> >  while (1) {
> >>    {
> >>      lock
> >>      old = videoframe.GetSyncPoint();
> >>    }
> >>    WaitSyncPoint(old);
> >>    new = InsertSyncPoint();
> >>    {
> >>      lock
> >>      old_again = videoframe.GetSyncPoint();
> >>      if (old == old_again) {
> >>        videoframe.SetSyncPoint(new);
> >>        break;
> >>    }
> >> }
> >>
> >
> >  Since you only set the sync point on the VideoFrame when you exit the
> >> loop,
> >> you know that in ties someone will set first and break the tie and leave
> >> the loop, so this shouldn't be able to fight with itself on another thread
> >> forever.
> >>
> >
> >  It could be implemented by passing the old and the new sync point to the
> >> Set() method, and if the old doesn't match the current sync point, it just
> >> early out and return false. Then the calling code is like
> >>
> >
> >  do {
> >>    old = videoframe.GetSyncPoint();
> >>    WaitSyncPoint(old);
> >>    new = InsertSyncPoint();
> >> } while (!videoframe.SetSyncPoint(old, new));
> >>
> >
> >  VideoFrame::SetSyncPoint(old, new) {
> >>    lock;
> >>    if (old != current_) return false;
> >>    current_ = new;
> >>    return true;
> >> }
> >>
> >
> >  sievers@ WDYT, does this look reasonable?
> >>
> >
> > Thank you for quick review.
> >
> > No, we don't need to wait old sync point before setting new sync point,
> > because
> > we don't need to order among release-sync-points.
> > , because there is not any interaction between clients. All clients are
> > just
> > consumer which reads mailbox the decoder produced.
> >
> > Let me explain in more detail.
> > Here is current summary of VideoFrame lifecycle.
> > 1) Creation of VideoFrame
> > When the decoder creates VideoFrame, VideoFrame::MailboxHolder::sync_point
> > is
> > set.
> > 2) Clients consume VideoFrame
> > When a client consumes the VideoFrame, the client waits
> > VideoFrame::MailboxHolder::sync_point, and uses the mailbox, and then
> > insert
> > release-sync-point via VideoFrame::AppendSyncPoint().
> > client consist of 1 x Compositor, n x WebGL, n x Canvas and etc.
> > 3) Destruction of VideoFrame
> > When VideoFrame is destructed, all sync points inserted by clients are
> > waited.
> >
> > Currently, all release-sync-points are waited in the destructor of
> > VideoFrame.
> >
> > This patch set just move the time to wait release-sync-points eariler;
> > when each
> > client set the new sync point.
> > If each client wait the old sync point before the VideoFrame is destucted,
> > everything is fine.
> >
> 
> When the video decoder gets back the VideoFrame, it's going to wait on the
> single sync point in the VideoFrame. The guarantee it wants is that no GPU
> commands are going to make use of the texture after that wait, so it can
> start changing the texture.
> 
> However, if someone else did InsertSyncPoint(); WaitSyncPoint(); then, the
> video decoder is going to wait on that sync point and guarantee that no
> commands from that last context are run afterward, but it has no guarantee
> for the context that inserted the sync point before that.
> 
> Context A: Compositor
> Context B: WebGL
> Context C: Video Decoder
> 
> A: Draws from VideoFrame to screen
> A: InsertSyncPoint A1
> B: Copies VideoFrame to another Texture
> B: InsertSyncPoint B1
> B: Waits for sync point A1
>    - we can now guarantee that actions B takes will occur after A1
> C: Waits for sync point B1
>   - we can now guarantee that actions C takes will occur after B1, but have
> no guarantee about A1
> 
> Reversing the order B's insert/wait will give us a better guarantee on C.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

That's excellent example. All clients still waits A1. There is two sync points
in VideoFrame: mailbox_holder_::sync_point and release_sync_points.
Let me change the story a bit to show this CL's logic.

Context A: Compositor
Context B: WebGL
Context C: Video Decoder

C: Create VideoFrame
C: InsertSyncPoint C1
A: Waits for sync point C1
A: Draws from VideoFrame to screen
A: InsertSyncPoint A1
B: Waits for sync point C1
B: Copies VideoFrame to another Texture
B: InsertSyncPoint B1
B: Waits for sync point A1
   - ordering A and B is not important. If both action A and B takes occur after
C1, everything is fine.
C: Waits for sync point B1
  - we can now guarantee that actions C takes will occur after B1, but have no
guarantee about A1 <- it's good question.

Each client guarantee about A1. For example, video_resource_updater.cc

static void ReturnTexture(
    const scoped_refptr<media::VideoFrame>& frame,
    const scoped_refptr<ContextProvider>& context_provider,
    uint32 sync_point,
    bool lost_resource) {
  uint32 previous_sync_point = frame->SetReleaseSyncPoint(sync_point);
  GLC(context_provider->ContextGL(),
      context_provider->ContextGL()->WaitSyncPointCHROMIUM(
          previous_sync_point));
}

ReturnTexture() function waits A1 before VideoFrame destruction, because the
callback holds scoped_refptr<media::VideoFrame>. Sometime layer after that,
VideoFrame will be destructed. At that time, C Waits for sync point B1. We know
A1 was already waited.

[danakj, 2014-06-18 16:44:43]

On Wed, Jun 18, 2014 at 9:33 AM, <dongseong.hwang@intel.com> wrote:

> On 2014/06/18 16:17:30, danakj wrote:
>
>  On Wed, Jun 18, 2014 at 9:10 AM, <mailto:dongseong.hwang@intel.com>
>> wrote:
>>
>
>  > On 2014/06/18 16:00:11, danakj wrote:
>> >
>> >> You must wait on the old sync point before InsertSyncPoint() or you
>> aren't
>> >> guaranteeing that the wait will happen in order.
>> >>
>> >
>> >  Writing it out, it would look something like...
>> >>
>> >
>> >  while (1) {
>> >>    {
>> >>      lock
>> >>      old = videoframe.GetSyncPoint();
>> >>    }
>> >>    WaitSyncPoint(old);
>> >>    new = InsertSyncPoint();
>> >>    {
>> >>      lock
>> >>      old_again = videoframe.GetSyncPoint();
>> >>      if (old == old_again) {
>> >>        videoframe.SetSyncPoint(new);
>> >>        break;
>> >>    }
>> >> }
>> >>
>> >
>> >  Since you only set the sync point on the VideoFrame when you exit the
>> >> loop,
>> >> you know that in ties someone will set first and break the tie and
>> leave
>> >> the loop, so this shouldn't be able to fight with itself on another
>> thread
>> >> forever.
>> >>
>> >
>> >  It could be implemented by passing the old and the new sync point to
>> the
>> >> Set() method, and if the old doesn't match the current sync point, it
>> just
>> >> early out and return false. Then the calling code is like
>> >>
>> >
>> >  do {
>> >>    old = videoframe.GetSyncPoint();
>> >>    WaitSyncPoint(old);
>> >>    new = InsertSyncPoint();
>> >> } while (!videoframe.SetSyncPoint(old, new));
>> >>
>> >
>> >  VideoFrame::SetSyncPoint(old, new) {
>> >>    lock;
>> >>    if (old != current_) return false;
>> >>    current_ = new;
>> >>    return true;
>> >> }
>> >>
>> >
>> >  sievers@ WDYT, does this look reasonable?
>> >>
>> >
>> > Thank you for quick review.
>> >
>> > No, we don't need to wait old sync point before setting new sync point,
>> > because
>> > we don't need to order among release-sync-points.
>> > , because there is not any interaction between clients. All clients are
>> > just
>> > consumer which reads mailbox the decoder produced.
>> >
>> > Let me explain in more detail.
>> > Here is current summary of VideoFrame lifecycle.
>> > 1) Creation of VideoFrame
>> > When the decoder creates VideoFrame, VideoFrame::MailboxHolder::
>> sync_point
>> > is
>> > set.
>> > 2) Clients consume VideoFrame
>> > When a client consumes the VideoFrame, the client waits
>> > VideoFrame::MailboxHolder::sync_point, and uses the mailbox, and then
>> > insert
>> > release-sync-point via VideoFrame::AppendSyncPoint().
>> > client consist of 1 x Compositor, n x WebGL, n x Canvas and etc.
>> > 3) Destruction of VideoFrame
>> > When VideoFrame is destructed, all sync points inserted by clients are
>> > waited.
>> >
>> > Currently, all release-sync-points are waited in the destructor of
>> > VideoFrame.
>> >
>> > This patch set just move the time to wait release-sync-points eariler;
>> > when each
>> > client set the new sync point.
>> > If each client wait the old sync point before the VideoFrame is
>> destucted,
>> > everything is fine.
>> >
>>
>
>  When the video decoder gets back the VideoFrame, it's going to wait on the
>> single sync point in the VideoFrame. The guarantee it wants is that no GPU
>> commands are going to make use of the texture after that wait, so it can
>> start changing the texture.
>>
>
>  However, if someone else did InsertSyncPoint(); WaitSyncPoint(); then, the
>> video decoder is going to wait on that sync point and guarantee that no
>> commands from that last context are run afterward, but it has no guarantee
>> for the context that inserted the sync point before that.
>>
>
>  Context A: Compositor
>> Context B: WebGL
>> Context C: Video Decoder
>>
>
>  A: Draws from VideoFrame to screen
>> A: InsertSyncPoint A1
>> B: Copies VideoFrame to another Texture
>> B: InsertSyncPoint B1
>> B: Waits for sync point A1
>>     - we can now guarantee that actions B takes will occur after A1
>> C: Waits for sync point B1
>>    - we can now guarantee that actions C takes will occur after B1, but
>> have
>> no guarantee about A1
>>
>
>  Reversing the order B's insert/wait will give us a better guarantee on C.
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> That's excellent example. All clients still waits A1. There is two sync
> points
> in VideoFrame: mailbox_holder_::sync_point and release_sync_points.
> Let me change the story a bit to show this CL's logic.
>
>
> Context A: Compositor
> Context B: WebGL
> Context C: Video Decoder
>
> C: Create VideoFrame
> C: InsertSyncPoint C1
> A: Waits for sync point C1
>
> A: Draws from VideoFrame to screen
> A: InsertSyncPoint A1
> B: Waits for sync point C1
>
> B: Copies VideoFrame to another Texture
> B: InsertSyncPoint B1
> B: Waits for sync point A1
>    - ordering A and B is not important. If both action A and B takes occur
> after
> C1, everything is fine.


Yes, for the clients of VideoFrame, they will be fine, because they wait on
the sync point from the decoder always before inserting their own.


>
> C: Waits for sync point B1
>   - we can now guarantee that actions C takes will occur after B1, but
> have no
> guarantee about A1 <- it's good question.
>
> Each client guarantee about A1. For example, video_resource_updater.cc
>
> static void ReturnTexture(
>     const scoped_refptr<media::VideoFrame>& frame,
>     const scoped_refptr<ContextProvider>& context_provider,
>     uint32 sync_point,
>     bool lost_resource) {
>   uint32 previous_sync_point = frame->SetReleaseSyncPoint(sync_point);
>   GLC(context_provider->ContextGL(),
>       context_provider->ContextGL()->WaitSyncPointCHROMIUM(
>           previous_sync_point));
> }
>
> ReturnTexture() function waits A1 before VideoFrame destruction, because
> the
> callback holds scoped_refptr<media::VideoFrame>. Sometime layer after
> that,
> VideoFrame will be destructed. At that time, C Waits for sync point B1. We
> know
> A1 was already waited.
>

You know that WaitSyncPoint(A1) was called, yes, but you don't know about
the ordering of that wait in the command buffer stream. The wait isn't a
synchronous block or anything, it just inserts a wait into the command
buffer. So while the decoder waits on B1, it will prevent the decoder's
command buffer from executing things before B1 is reached in the B command
buffer, but A's command buffer may not have reached A1 yet, and it might
still be drawing the texture to the screen which C's command buffer to
change the texture is also being run. So knowing that it's called isn't
enough, you need to know that your command buffer will wait for A and B's
command buffers.


>
>
> https://codereview.chromium.org/312803002/
>

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

[dshwang, 2014-06-18 17:01:30]

On 2014/06/18 16:44:43, danakj wrote:
> > Context A: Compositor
> > Context B: WebGL
> > Context C: Video Decoder
> >
> > C: Create VideoFrame
> > C: InsertSyncPoint C1
> > A: Waits for sync point C1
> >
> > A: Draws from VideoFrame to screen
> > A: InsertSyncPoint A1
> > B: Waits for sync point C1
> >
> > B: Copies VideoFrame to another Texture
> > B: InsertSyncPoint B1
> > B: Waits for sync point A1
> >    - ordering A and B is not important. If both action A and B takes occur
> > after
> > C1, everything is fine.
> 
> 
> Yes, for the clients of VideoFrame, they will be fine, because they wait on
> the sync point from the decoder always before inserting their own.
> 
> 
> >
> > C: Waits for sync point B1
> >   - we can now guarantee that actions C takes will occur after B1, but
> > have no
> > guarantee about A1 <- it's good question.
> >
> > Each client guarantee about A1. For example, video_resource_updater.cc
> >
> > static void ReturnTexture(
> >     const scoped_refptr<media::VideoFrame>& frame,
> >     const scoped_refptr<ContextProvider>& context_provider,
> >     uint32 sync_point,
> >     bool lost_resource) {
> >   uint32 previous_sync_point = frame->SetReleaseSyncPoint(sync_point);
> >   GLC(context_provider->ContextGL(),
> >       context_provider->ContextGL()->WaitSyncPointCHROMIUM(
> >           previous_sync_point));
> > }
> >
> > ReturnTexture() function waits A1 before VideoFrame destruction, because
> > the
> > callback holds scoped_refptr<media::VideoFrame>. Sometime layer after
> > that,
> > VideoFrame will be destructed. At that time, C Waits for sync point B1. We
> > know
> > A1 was already waited.
> >
> 
> You know that WaitSyncPoint(A1) was called, yes, but you don't know about
> the ordering of that wait in the command buffer stream. The wait isn't a
> synchronous block or anything, it just inserts a wait into the command
> buffer. So while the decoder waits on B1, it will prevent the decoder's
> command buffer from executing things before B1 is reached in the B command
> buffer, but A's command buffer may not have reached A1 yet, and it might
> still be drawing the texture to the screen which C's command buffer to
> change the texture is also being run. So knowing that it's called isn't
> enough, you need to know that your command buffer will wait for A and B's
> command buffers.

Ah, you are right. I understand now. Thank you for kind explanation.
I need to use your pseudo code; while(1) {lock; get; if set;}
Unfortunately, video_resource_updater and webmediaplayer_impl need separate
implementation, because each uses ContextProvider and WebGraphicsContext3D
respectively.
The next patch set will bloat code in video_resource_updater.cc,
webmediaplayer_impl.cc and webmediaplayer_android.cc

before updating,
On 2014/06/18 16:00:11, danakj wrote:
> You must wait on the old sync point before InsertSyncPoint() or you aren't
> guaranteeing that the wait will happen in order.
> 
> Writing it out, it would look something like...
> 
> while (1) {
>   {
>     lock
>     old = videoframe.GetSyncPoint();
>   }
>   WaitSyncPoint(old);
>   new = InsertSyncPoint();
>   {
>     lock
>     old_again = videoframe.GetSyncPoint();
>     if (old == old_again) {
>       videoframe.SetSyncPoint(new);
>       break;
>   }
> }
> 
> Since you only set the sync point on the VideoFrame when you exit the loop,
> you know that in ties someone will set first and break the tie and leave
> the loop, so this shouldn't be able to fight with itself on another thread
> forever.
> 
> It could be implemented by passing the old and the new sync point to the
> Set() method, and if the old doesn't match the current sync point, it just
> early out and return false. Then the calling code is like
> 
> do {
>   old = videoframe.GetSyncPoint();
>   WaitSyncPoint(old);
>   new = InsertSyncPoint();
> } while (!videoframe.SetSyncPoint(old, new));
> 
> VideoFrame::SetSyncPoint(old, new) {
>   lock;
>   if (old != current_) return false;
>   current_ = new;
>   return true;
> }
> 
> sievers@ WDYT, does this look reasonable?

sievers@ WDYT?

[dshwang, 2014-06-19 10:28:17]

Hi, danakj@, sievers@, I addressed danakj@'s concern in this patch set. Could
you review again?

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

https://codereview.chromium.org/312803002/diff/60001/media/base/video_frame.c...
media/base/video_frame.cc:820: release_sync_point_ = provider.InsertSyncPoint();
I makes this API atomic to avoid while(){lock} code in client site.
WDYT, danakj@?

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

https://codereview.chromium.org/312803002/diff/60001/media/base/video_frame.h...
media/base/video_frame.h:255: class SyncPointProvider {
I'm not good at naming. If you have better name, please let me know.

[danakj, 2014-06-19 15:56:27]

Thanks, I'm happy with the Client interface so we don't have to give GL pointers
directly to VideoFrame

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

https://codereview.chromium.org/312803002/diff/60001/cc/resources/video_resou...
cc/resources/video_resource_updater.cc:314: const
scoped_refptr<ContextProvider>& context_provider,
Can we pass a WeakPtr to the VRUpdater instead so we don't have to extend the
lifetime of the ContextProvider? And if the WeakPtr is invalid, we report that
the texture is lost?

https://codereview.chromium.org/312803002/diff/60001/cc/resources/video_resou...
cc/resources/video_resource_updater.cc:318: provider.WaitSyncPoint(sync_point);
why waiting on the sync point here? this same context inserted the sync point in
the first place

https://codereview.chromium.org/312803002/diff/60001/cc/resources/video_resou...
cc/resources/video_resource_updater.cc:356: &ReturnTexture, video_frame,
make_scoped_refptr(context_provider_)));
make_scoped_refptr() is not needed

https://codereview.chromium.org/312803002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/312803002/diff/60001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:74: void
SyncPointProviderImpl::WaitSyncPoint(uint32 sync_point) {
Why is this ok to drop the current sync point in the VideoFrame on the floor
here?

https://codereview.chromium.org/312803002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/312803002/diff/60001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:34: #elif
defined(OS_LINUX) && defined(ARCH_CPU_X86_FAMILY) && defined(USE_X11)
unrelated change?

https://codereview.chromium.org/312803002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/312803002/diff/60001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:409: #elif
defined(OS_LINUX) && defined(ARCH_CPU_X86_FAMILY)
and here?

https://codereview.chromium.org/312803002/diff/60001/content/common/sandbox_l...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/312803002/diff/60001/content/common/sandbox_l...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:223: I965DrvVideoPath =
"/usr/lib/x86_64-linux-gnu/dri/i965_drv_video.so";
and here?

https://codereview.chromium.org/312803002/diff/60001/content/content_common.gypi
File content/content_common.gypi (right):

https://codereview.chromium.org/312803002/diff/60001/content/content_common.g...
content/content_common.gypi:678: ['(chromeos==1 or OS=="linux") and use_x11==1
and target_arch != "arm"', {
and here?

https://codereview.chromium.org/312803002/diff/60001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/312803002/diff/60001/content/content_tests.gy...
content/content_tests.gypi:1365: ['(chromeos==1 or (OS=="linux" and use_x11==1
and target_arch != "arm")) or OS=="win" or OS=="android"', {
and this?

https://codereview.chromium.org/312803002/diff/60001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/312803002/diff/60001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:87: virtual uint32
InsertSyncPoint();
since you're in the .cc you could define these methods inline here if you want,
it would be less LOC overall.. same for the other Impls

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

https://codereview.chromium.org/312803002/diff/60001/media/base/video_frame.c...
media/base/video_frame.cc:820: release_sync_point_ = provider.InsertSyncPoint();
On 2014/06/19 10:28:17, dshwang wrote:
> I makes this API atomic to avoid while(){lock} code in client site.
> WDYT, danakj@?

Very good!

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

https://codereview.chromium.org/312803002/diff/60001/media/base/video_frame.h...
media/base/video_frame.h:255: class SyncPointProvider {
On 2014/06/19 10:28:17, dshwang wrote:
> I'm not good at naming. If you have better name, please let me know.

Mmmh SyncPointClient if you like that better? This is fine too.

https://codereview.chromium.org/312803002/diff/60001/media/base/video_frame.h...
media/base/video_frame.h:272: void UpdateReleaseSyncPoint(SyncPointProvider&);
this is a non-const ref which is not allowed. make this a pointer, and give it a
variable name here please for chromium style

[dshwang, 2014-06-23 18:33:20]

Thank you for detailed review. I delay to reply due to Midsummer holiday, which
is one of biggest holiday in Scandinavia. :)

Could you review again?

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

https://codereview.chromium.org/312803002/diff/60001/cc/resources/video_resou...
cc/resources/video_resource_updater.cc:314: const
scoped_refptr<ContextProvider>& context_provider,
Yes, I use WeakPtr in new patch set.

https://codereview.chromium.org/312803002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/312803002/diff/60001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:74: void
SyncPointProviderImpl::WaitSyncPoint(uint32 sync_point) {
I misunderstood that this file is used by only unittest. I fix it in new patch
set.

https://codereview.chromium.org/312803002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/312803002/diff/60001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:34: #elif
defined(OS_LINUX) && defined(ARCH_CPU_X86_FAMILY) && defined(USE_X11)
Sorry for bothering you due to unrelated change.
I usually test GPU Video change on Linux, not Cros. I made the mistake that is
to submit the downstream patch enabling GPU Video on Linux together.

https://codereview.chromium.org/312803002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/312803002/diff/60001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:409: #elif
defined(OS_LINUX) && defined(ARCH_CPU_X86_FAMILY)
ditto. it's not related change.

https://codereview.chromium.org/312803002/diff/60001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/312803002/diff/60001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:87: virtual uint32
InsertSyncPoint();
Done.

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

https://codereview.chromium.org/312803002/diff/60001/media/base/video_frame.h...
media/base/video_frame.h:255: class SyncPointProvider {
I choose SyncPointClient :)

https://codereview.chromium.org/312803002/diff/60001/media/base/video_frame.h...
media/base/video_frame.h:272: void UpdateReleaseSyncPoint(SyncPointProvider&);
Done.

https://codereview.chromium.org/312803002/diff/80001/cc/resources/video_resou...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/312803002/diff/80001/cc/resources/video_resou...
cc/resources/video_resource_updater.cc:309: DCHECK(context_provider.get() ||
lost_resource);
This callback is called from ResourceProvider, which uses the same context
provider. If |context_provider| is null, |sync_point| can not be inserted.

https://codereview.chromium.org/312803002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

https://codereview.chromium.org/312803002/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:524:
ASSERT_GT(release_syncpoint_vectors[i], mailbox_syncpoints[i]);
I sorta abuse sync point implementation detail; sync point is monotonically
increasing.
I wanted to check release sync point but I can not change the implementation of
InsertSyncPoint()/WaitSyncPoint() of
ImageTransportFactory::GetInstance()->GetGLHelper().

[danakj, 2014-06-23 18:34:59]

https://codereview.chromium.org/312803002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

https://codereview.chromium.org/312803002/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:524:
ASSERT_GT(release_syncpoint_vectors[i], mailbox_syncpoints[i]);
On 2014/06/23 18:33:20, dshwang wrote:
> I sorta abuse sync point implementation detail; sync point is monotonically
> increasing.
> I wanted to check release sync point but I can not change the implementation
of
> InsertSyncPoint()/WaitSyncPoint() of
> ImageTransportFactory::GetInstance()->GetGLHelper().

Is _NE not enough to show it's been updated since the video frame was created?

[dshwang, 2014-06-23 18:50:38]

https://codereview.chromium.org/312803002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

https://codereview.chromium.org/312803002/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:524:
ASSERT_GT(release_syncpoint_vectors[i], mailbox_syncpoints[i]);
yes, it's not enough.
This check's goal is to ensure mailbox_syncpoints[i] had occurred if we wait
release_syncpoint_vectors[i]. Unfortunately, I could not find sufficient way to
show it.

[dshwang, 2014-06-23 18:53:53]

https://codereview.chromium.org/312803002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

https://codereview.chromium.org/312803002/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:524:
ASSERT_GT(release_syncpoint_vectors[i], mailbox_syncpoints[i]);
Ah, sorry for wrong answer of your question.
_NE ensures as much as _GT, so I'll use _NE. Thx.

[dshwang, 2014-06-23 19:42:07]

https://codereview.chromium.org/312803002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/312803002/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:76: GLHelper*
gl_helper = ImageTransportFactory::GetInstance()->GetGLHelper();
I found that android can not use ImageTransportFactory.
I'll make Android use ImageTransportFactoryAndroid soon.

[dshwang, 2014-06-24 14:04:22]

I made in-process impl of ImageTransportFactoryAndroid for unittests, because
video_capture_controller_unittest needs to use ImageTransportFactory.
Could you review again?

https://codereview.chromium.org/312803002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/image_transport_factory_android.cc (right):

https://codereview.chromium.org/312803002/diff/160001/content/browser/rendere...
content/browser/renderer_host/image_transport_factory_android.cc:107: class
NoImageTransportFactory : public ImageTransportFactoryAndroid {
This class is very similar to NoTransportImageTransportFactory except for
inheriting ImageTransportFactoryAndroid.

https://codereview.chromium.org/312803002/diff/160001/content/browser/rendere...
content/browser/renderer_host/image_transport_factory_android.cc:138: void
ImageTransportFactoryAndroid::InitializeForUnitTests(
Implement it like ImageTransportFactory::InitializeForUnitTests() in order to
use it in unittests.

[dshwang, 2014-06-30 19:05:36]

Hi, would you have a chance to review again?

[danakj, 2014-07-02 21:43:03]

https://codereview.chromium.org/312803002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/image_transport_factory_android.cc (right):

https://codereview.chromium.org/312803002/diff/160001/content/browser/rendere...
content/browser/renderer_host/image_transport_factory_android.cc:107: class
NoImageTransportFactory : public ImageTransportFactoryAndroid {
one class per file, can you move this out?

https://codereview.chromium.org/312803002/diff/160001/content/browser/rendere...
content/browser/renderer_host/image_transport_factory_android.cc:148:
DCHECK(!g_initialized_for_unit_tests);
i don't think this DCHECK buys much, since there's no Initialize() for this
class unlike the other ITF. what are you trying to guard against? i think just
remove it and the global bool.

what would be better is if this whole NoImageTransportFactory class was only
compiled into test targets. can we do that instead?

[dshwang, 2014-07-03 18:18:35]

Thank you for review.

On 2014/07/02 21:43:03, danakj wrote:
>
https://codereview.chromium.org/312803002/diff/160001/content/browser/rendere...
> File content/browser/renderer_host/image_transport_factory_android.cc (right):
> 
>
https://codereview.chromium.org/312803002/diff/160001/content/browser/rendere...
> content/browser/renderer_host/image_transport_factory_android.cc:107: class
> NoImageTransportFactory : public ImageTransportFactoryAndroid {
> one class per file, can you move this out?

DONE.

>
https://codereview.chromium.org/312803002/diff/160001/content/browser/rendere...
> content/browser/renderer_host/image_transport_factory_android.cc:148:
> DCHECK(!g_initialized_for_unit_tests);
> i don't think this DCHECK buys much, since there's no Initialize() for this
> class unlike the other ITF. what are you trying to guard against? i think just
> remove it and the global bool.

I removed redundant bool.

> what would be better is if this whole NoImageTransportFactory class was only
> compiled into test targets. can we do that instead?

DONE.

[dshwang, 2014-07-10 15:39:26]

danakj@: Please review changes in cc/
sievers@: Please review changes in content/browser/renderer_host
xhwang@: Please review changes in content/*/media and media/

[danakj, 2014-07-10 17:09:15]

https://codereview.chromium.org/312803002/diff/220001/cc/output/context_provi...
File cc/output/context_provider.h (right):

https://codereview.chromium.org/312803002/diff/220001/cc/output/context_provi...
cc/output/context_provider.h:25: public base::SupportsWeakPtr<ContextProvider> {
ಠ_ಠ let's not do this please

https://codereview.chromium.org/312803002/diff/220001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/312803002/diff/220001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:306: base::WeakPtr<ContextProvider>
context_provider,
Why not WeakPtr to the VideoResourceUpdater, and pull the context_provider off
of there? See the RecycleTexture method.

Also SupportsWeakPtr is a very heavy hammer, for a DCHECK.

https://codereview.chromium.org/312803002/diff/220001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:307: uint32 /* sync_point */,
we don't comment out parameter names like this in chrome

https://codereview.chromium.org/312803002/diff/220001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:311: return;
can you TODO that this lost_resource should be forwarded to the decoder?

https://codereview.chromium.org/312803002/diff/220001/content/browser/rendere...
File content/browser/renderer_host/no_image_transport_factory_android.h (right):

https://codereview.chromium.org/312803002/diff/220001/content/browser/rendere...
content/browser/renderer_host/no_image_transport_factory_android.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
Think this class could live in content/browser/compositor/test/?

[no sievers, 2014-07-10 18:52:29]

https://codereview.chromium.org/312803002/diff/220001/content/browser/media/c...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/312803002/diff/220001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:301:
cb->Run(sync_point, false);
maybe remove this function since it doesn't do any extra work anymore and use
|cb| directly?

https://codereview.chromium.org/312803002/diff/220001/content/browser/rendere...
File content/browser/renderer_host/image_transport_factory_android.cc (right):

https://codereview.chromium.org/312803002/diff/220001/content/browser/rendere...
content/browser/renderer_host/image_transport_factory_android.cc:107: void
ImageTransportFactoryAndroid::InitializeForUnitTests(
Do we need a method for teardown also?

With unit tests it can create this nasty problems when you leak static objects
depending on how they run.

https://codereview.chromium.org/312803002/diff/220001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/312803002/diff/220001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:88:
gl_helper->WaitSyncPoint(sync_point);
This might deserve a comment why we have to deal with another sync point here.

https://codereview.chromium.org/312803002/diff/220001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

https://codereview.chromium.org/312803002/diff/220001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:500:
std::vector<uint32> release_syncpoint_vectors(mailbox_buffers);
nit: |release_syncpoints_|?

https://codereview.chromium.org/312803002/diff/220001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:541:
ASSERT_NE(mailbox_syncpoints[i], release_syncpoint_vectors[i]);
At what point are those equal though? Can we ASSERT that also above?

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

https://codereview.chromium.org/312803002/diff/220001/media/base/video_frame....
media/base/video_frame.cc:666: base::AutoLock locker(release_sync_point_lock_);
Why do we need a lock in the destructor? Somebody might access this object while
it's being destructed?

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

https://codereview.chromium.org/312803002/diff/220001/media/base/video_frame....
media/base/video_frame.h:268: // Set a new sync point to |release_sync_point_|
which will be passed to
nit: don't use internals like the private member |release_sync_point_| to
describe what's going on. how about some brief comment along the lines of saying
that "it uses |client| to insert a new sync point and potentially waits on older
sync points"?

[danakj, 2014-07-10 19:15:57]

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

https://codereview.chromium.org/312803002/diff/220001/media/base/video_frame....
media/base/video_frame.cc:666: base::AutoLock locker(release_sync_point_lock_);
On 2014/07/10 18:52:29, sievers wrote:
> Why do we need a lock in the destructor? Somebody might access this object
while
> it's being destructed?

Thread barrier, someone may have changed the value on another thread that you're
running the destructor on, and you need to get that value. The lock ensures you
do.

[dshwang, 2014-07-10 19:23:12]

danakj@, sievers@, Thank you for reviewing. I'll apply all your comment to next
patch set.

danakj@, could you give me opinion again about weak_ptr hammer?

https://codereview.chromium.org/312803002/diff/220001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/312803002/diff/220001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:306: base::WeakPtr<ContextProvider>
context_provider,
I was worried the last frame glitch before video_layer_impl dies.
ContextProvider lives nearly until render process dies, but VideoResourceUpdater
dies when video_layer_impl dies.

For example,
Render process                       Browser process
----------------------------------------------------------
video_layer_impl issue drawQuad
video_layer_impl dies                Draw video drawQuad
                                     Reclaim the resource
ReturnTexture() is skipped 
due to WeakPtr<VideoResourceUpdater>

WDYT? Can we ignore the last frame glitch?

https://codereview.chromium.org/312803002/diff/220001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:311: return;
Yes, I'll update

https://codereview.chromium.org/312803002/diff/220001/content/browser/media/c...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/312803002/diff/220001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:301:
cb->Run(sync_point, false);
you're right. I'll update.

https://codereview.chromium.org/312803002/diff/220001/content/browser/rendere...
File content/browser/renderer_host/image_transport_factory_android.cc (right):

https://codereview.chromium.org/312803002/diff/220001/content/browser/rendere...
content/browser/renderer_host/image_transport_factory_android.cc:107: void
ImageTransportFactoryAndroid::InitializeForUnitTests(
Ah, you're right. I mimic this method after
ImageTransportFactory::InitializeForUnitTests(scoped_ptr<ImageTransportFactory>
factory).
ImageTransportFactory class has Terminate() method for this reason!. I'll
update.

https://codereview.chromium.org/312803002/diff/220001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

https://codereview.chromium.org/312803002/diff/220001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:541:
ASSERT_NE(mailbox_syncpoints[i], release_syncpoint_vectors[i]);
Unfortunately, we cannot know what is release_syncpoint_vectors[i] because real
gl context generated the sync point.

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

https://codereview.chromium.org/312803002/diff/220001/media/base/video_frame....
media/base/video_frame.cc:666: base::AutoLock locker(release_sync_point_lock_);
Thank you for answer!

[no sievers, 2014-07-10 19:27:47]

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

https://codereview.chromium.org/312803002/diff/220001/media/base/video_frame....
media/base/video_frame.cc:666: base::AutoLock locker(release_sync_point_lock_);
On 2014/07/10 19:15:57, danakj wrote:
> On 2014/07/10 18:52:29, sievers wrote:
> > Why do we need a lock in the destructor? Somebody might access this object
> while
> > it's being destructed?
> 
> Thread barrier, someone may have changed the value on another thread that
you're
> running the destructor on, and you need to get that value. The lock ensures
you
> do.

Ah thanks. I guess the comment says that already. If you change it to something
like "To ensure that changes to |release_sync_point_| are visible on this thread
(imply a memory barrier)" then even I will understand it :)

[danakj, 2014-07-10 20:00:49]

https://codereview.chromium.org/312803002/diff/220001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/312803002/diff/220001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:306: base::WeakPtr<ContextProvider>
context_provider,
On 2014/07/10 19:23:11, dshwang wrote:
> I was worried the last frame glitch before video_layer_impl dies.
> ContextProvider lives nearly until render process dies, but
VideoResourceUpdater
> dies when video_layer_impl dies.
> 
> For example,
> Render process                       Browser process
> ----------------------------------------------------------
> video_layer_impl issue drawQuad
> video_layer_impl dies                Draw video drawQuad
>                                      Reclaim the resource
> ReturnTexture() is skipped 
> due to WeakPtr<VideoResourceUpdater>
> 
> WDYT? Can we ignore the last frame glitch?

We could pass on lost_resource=true to the videoframe in that case to say that
we don't have a valid sync point to give out. So, TODO?

[dshwang, 2014-07-10 21:44:26]

danakj@, sievers@, Thank you for detail review. I apply your all comments to
this patch set. Could you review again?

https://codereview.chromium.org/312803002/diff/240001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/312803002/diff/240001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:314: // resource.
danakj@, use WeakPtr<VideoResourceUpdater> and add TODO

https://codereview.chromium.org/312803002/diff/240001/content/browser/media/c...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/312803002/diff/240001/content/browser/media/c...
content/browser/media/capture/desktop_capture_device_aura.cc:298: uint32
sync_point) {
sievers@, this function cannot be removed because SingleReleaseCallback::Run
requires additional boolean. The signature is different.

https://codereview.chromium.org/312803002/diff/240001/content/browser/rendere...
File content/browser/renderer_host/image_transport_factory_android.cc (right):

https://codereview.chromium.org/312803002/diff/240001/content/browser/rendere...
content/browser/renderer_host/image_transport_factory_android.cc:114: void
ImageTransportFactoryAndroid::TerminateForUnitTests() {
sievers@, Terminate is added to teardown.

https://codereview.chromium.org/312803002/diff/240001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/312803002/diff/240001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:89: // wait the
given |sync_point| using |gl_helper|.
sievers@, add a comment

https://codereview.chromium.org/312803002/diff/240001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

https://codereview.chromium.org/312803002/diff/240001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:499:
std::vector<uint32> release_syncpoints(mailbox_buffers);
sievers@, nits done.

https://codereview.chromium.org/312803002/diff/240001/content/browser/rendere...
File
content/browser/renderer_host/test/no_transport_image_transport_factory_android.cc
(right):

https://codereview.chromium.org/312803002/diff/240001/content/browser/rendere...
content/browser/renderer_host/test/no_transport_image_transport_factory_android.cc:5:
#include
"content/browser/renderer_host/test/no_transport_image_transport_factory_android.h"
danakj@, I move it to renderer_host/test

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

https://codereview.chromium.org/312803002/diff/240001/media/base/video_frame....
media/base/video_frame.cc:666: // thread (imply a memory barrier).
sievers@, improve this comment.

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

https://codereview.chromium.org/312803002/diff/240001/media/base/video_frame....
media/base/video_frame.h:270: // VideoFrame.
sievers@, yep, I use your great rephrase.

[danakj, 2014-07-10 21:49:26]

Thanks! cc LGTM

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

https://codereview.chromium.org/312803002/diff/240001/media/base/video_frame....
media/base/video_frame.cc:825: // Must wait the previous sync point before
inserting a new sync point so that
nit: wait on

https://codereview.chromium.org/312803002/diff/240001/media/base/video_frame....
media/base/video_frame.cc:827: // when it waits |release_sync_point_|.
nit: waits on

[dshwang, 2014-07-10 22:12:16]

On 2014/07/10 21:49:26, danakj wrote:
> Thanks! cc LGTM

Thanks!

>
https://codereview.chromium.org/312803002/diff/240001/media/base/video_frame.cc
> File media/base/video_frame.cc (right):
> 
>
https://codereview.chromium.org/312803002/diff/240001/media/base/video_frame....
> media/base/video_frame.cc:825: // Must wait the previous sync point before
> inserting a new sync point so that
> nit: wait on

Done.

>
https://codereview.chromium.org/312803002/diff/240001/media/base/video_frame....
> media/base/video_frame.cc:827: // when it waits |release_sync_point_|.
> nit: waits on

Done.

[dshwang, 2014-07-14 13:57:13]

sievers@: Please review changes in content/browser/renderer_host
xhwang@: Please review changes in content/*/media and media/

[xhwang, 2014-07-14 15:40:31]

I am not very familiar with mailbox/syncpoint code. So if you'd like to find
someone who is more familiar with it to review, feel free to do so. Otherwise,
rslgtm % nit.

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

https://codereview.chromium.org/312803002/diff/260001/media/base/video_frame....
media/base/video_frame.h:144: const std::vector<int> dmabuf_fds,
keep include <vector> for this

[no sievers, 2014-07-14 19:52:51]

On 2014/07/14 13:57:13, dshwang wrote:
> sievers@: Please review changes in content/browser/renderer_host

lgtm

[dshwang, 2014-07-15 15:08:37]

Thank you for review, all.

nasko@: Please review changes in content/common/media/video_capture_messages.h

[nasko, 2014-07-16 08:28:12]

IPC LGTM

[dshwang, 2014-07-18 15:06:58]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-07-18 15:07:54]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/312803002/30...

[commit-bot: I haz the power, 2014-07-18 17:57:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-18 18:00:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-18 18:00:10]

Try jobs failed on following builders:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[dshwang, 2014-07-22 08:12:13]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-07-22 08:13:23]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/312803002/30...

[commit-bot: I haz the power, 2014-07-22 08:21:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[commit-bot: I haz the power, 2014-07-22 08:23:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-22 08:23:56]

Try jobs failed on following builders:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[dshwang, 2014-07-22 08:42:44]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-07-22 08:43:28]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/312803002/32...

[commit-bot: I haz the power, 2014-07-22 10:32:02]

Change committed as 284658