cc: VideoResourceUpdater: don't dismiss the sync_point given by the compositor

cc: VideoResourceUpdater: don't dismiss the sync_point given by the compositor

When the compositor is done using a video texture, it returns a
sync_point associated with the last use of this texture. We appear to
drop this sync_point on the floor, neither waiting on it, nor
associating it to the video frame returned to the GpuVideoDecoder.

This change emits a WaitSyncPointCHROMIUM on the compositor sync point
when updating the renderer's media::VideoFrame release sync point.

BUG=chrome-os-partner:37786
Committed: https://crrev.com/ee1b10bd38de9e6f96e491bd30c63577e22a9bae
Cr-Commit-Position: refs/heads/master@{#325416}

[llandwerlin-old, 2015-04-14 11:16:29]

lionel.g.landwerlin@intel.com changed reviewers:
+ piman@chromium.org

[llandwerlin-old, 2015-04-14 11:16:30]

PTAL, Thanks.

[danakj, 2015-04-14 15:56:27]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-04-14 15:56:28]

https://codereview.chromium.org/1088903002/diff/1/cc/resources/video_resource...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/1088903002/diff/1/cc/resources/video_resource...
cc/resources/video_resource_updater.cc:381: //
VideoFrame::UpdateReleaseSyncPoint() creates new sync point using the same
What about this comment? Why is it wrong?

[llandwerlin-old, 2015-04-14 16:21:54]

https://codereview.chromium.org/1088903002/diff/1/cc/resources/video_resource...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/1088903002/diff/1/cc/resources/video_resource...
cc/resources/video_resource_updater.cc:381: //
VideoFrame::UpdateReleaseSyncPoint() creates new sync point using the same
On 2015/04/14 15:56:27, danakj (away until april 19) wrote:
> What about this comment? Why is it wrong?

Thanks, I will update the comment.

It's wrong because prior to https://codereview.chromium.org/312803002/ we kept
adding sync points to the media::VideoFrame.

If we want to keep only one sync point, we need to build up a chain of
dependency. We can do this by inserting a WaitSyncPointCHROMIUM op in the
command stream and create a new sync point to be used by the media::VideoFrame,
but since we do not draw with the GL texture from the renderer, we can directly
replace the sync point inside the media::VideoFrame.

[piman, 2015-04-14 18:10:46]

On 2015/04/14 15:56:28, danakj (away until april 19) wrote:
>
https://codereview.chromium.org/1088903002/diff/1/cc/resources/video_resource...
> File cc/resources/video_resource_updater.cc (right):
> 
>
https://codereview.chromium.org/1088903002/diff/1/cc/resources/video_resource...
> cc/resources/video_resource_updater.cc:381: //
> VideoFrame::UpdateReleaseSyncPoint() creates new sync point using the same
> What about this comment? Why is it wrong?

I think it's not true, the sync point may not come from the same GL context (and
channel), it may be the sync point returned from the parent compositor.

I think this change makes things better, but it might break assumptions in other
places. The code in VideoFrame::UpdateReleaseSyncPoint seems to assume
SyncPointClient::InsertSyncPoint would insert on the same context as in
SyncPointClient::WaitSyncPoint (to establish a dependency).

So maybe what SyncPointClientImpl should do is, if WaitSyncPoint is called, wait
on |sync_point_| and reset it, and wait on |sync_point| (in any order, really).
Then when InsertSyncPoint is called, it would return sync_point_ if !0 but
otherwise would insert a new sync point and return that.

[llandwerlin-old, 2015-04-15 09:55:18]

On 2015/04/14 18:10:46, piman (Very slow to review) wrote:
> On 2015/04/14 15:56:28, danakj (away until april 19) wrote:
> >
>
https://codereview.chromium.org/1088903002/diff/1/cc/resources/video_resource...
> > File cc/resources/video_resource_updater.cc (right):
> > 
> >
>
https://codereview.chromium.org/1088903002/diff/1/cc/resources/video_resource...
> > cc/resources/video_resource_updater.cc:381: //
> > VideoFrame::UpdateReleaseSyncPoint() creates new sync point using the same
> > What about this comment? Why is it wrong?
> 
> I think it's not true, the sync point may not come from the same GL context
(and
> channel), it may be the sync point returned from the parent compositor.
> 
> I think this change makes things better, but it might break assumptions in
other
> places. The code in VideoFrame::UpdateReleaseSyncPoint seems to assume
> SyncPointClient::InsertSyncPoint would insert on the same context as in
> SyncPointClient::WaitSyncPoint (to establish a dependency).
> 
> So maybe what SyncPointClientImpl should do is, if WaitSyncPoint is called,
wait
> on |sync_point_| and reset it, and wait on |sync_point| (in any order,
really).
> Then when InsertSyncPoint is called, it would return sync_point_ if !0 but
> otherwise would insert a new sync point and return that.

Here is an update following your suggestion.

[piman, 2015-04-15 19:48:04]

LGTM, but worth a comment:
If the video_frame already had a sync point, we need to combine that sync point
with the returned sync point, which we do by waiting on both and inserting a new
one. Otherwise we can just pass the returned sync point through.

[llandwerlin-old, 2015-04-16 08:32:08]

On 2015/04/15 19:48:04, piman (Very slow to review) wrote:
> LGTM, but worth a comment:
> If the video_frame already had a sync point, we need to combine that sync
point
> with the returned sync point, which we do by waiting on both and inserting a
new
> one. Otherwise we can just pass the returned sync point through.

Thanks, updated as you described.

[llandwerlin-old, 2015-04-16 08:33:45]

The CQ bit was checked by lionel.g.landwerlin@intel.com

[llandwerlin-old, 2015-04-16 08:33:46]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1088903002/#ps60001
(title: "Pass compositor sync_point with no previous sync point in
media::VideoFrame")

[commit-bot: I haz the power, 2015-04-16 08:34:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1088903002/60001

[commit-bot: I haz the power, 2015-04-16 11:48:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-16 11:49:33]

Patchset 4 (id:??) landed as
https://crrev.com/ee1b10bd38de9e6f96e491bd30c63577e22a9bae
Cr-Commit-Position: refs/heads/master@{#325416}