cc: Send BeginFrame to VideoFrameController when added if we're inside a frame.

cc: Send BeginFrame to VideoFrameController when added.

When a VideoFrameController starts rendering it has to wait until the
next frame to get a BeginFrame message. This means that it renders
nothing for the first frame if it is able to draw in that frame.

Committed: https://crrev.com/c052ba992cee6d00e84ee00d434453700fe89731
Cr-Commit-Position: refs/heads/master@{#328238}

[sunnyps, 2015-05-01 00:07:23]

sunnyps@chromium.org changed reviewers:
+ brianderson@chromium.org, dalecurtis@chromium.org

[sunnyps, 2015-05-01 00:07:23]

PTAL

This should solve the LayoutTests that break on turning on the new video
renderer. This requires mithro's CL: https://codereview.chromium.org/1099703004/
for setting the current_begin_frame_args_ correctly in LTHI.

[DaleCurtis, 2015-05-01 00:09:58]

Could this deadlock if AcquireLockAndCurrentFrame() has been previously called,
since we attempt to acquire the provider lock in the OnBeginFrame?

[sunnyps, 2015-05-01 00:18:06]

On 2015/05/01 00:09:58, DaleCurtis wrote:
> Could this deadlock if AcquireLockAndCurrentFrame() has been previously
called,
> since we attempt to acquire the provider lock in the OnBeginFrame?

Only if there's a way for StartRendering to be called in between calling
AcquireLockAndCurrentFrame and ReleaseLock. Is that possible?

[DaleCurtis, 2015-05-01 00:33:25]

On 2015/05/01 00:18:06, sunnyps wrote:
> On 2015/05/01 00:09:58, DaleCurtis wrote:
> > Could this deadlock if AcquireLockAndCurrentFrame() has been previously
> called,
> > since we attempt to acquire the provider lock in the OnBeginFrame?
> 
> Only if there's a way for StartRendering to be called in between calling
> AcquireLockAndCurrentFrame and ReleaseLock. Is that possible?

I'm not super familiar with the VideoLayer side of things. If the compositor
message loop is pumped between those points, it's certainly possible that
StartRendering could be called as the layer is trying to acquire the current
frame for some reason.

[sunnyps, 2015-05-01 01:14:22]

On 2015/05/01 00:33:25, DaleCurtis wrote:
> On 2015/05/01 00:18:06, sunnyps wrote:
> > On 2015/05/01 00:09:58, DaleCurtis wrote:
> > > Could this deadlock if AcquireLockAndCurrentFrame() has been previously
> > called,
> > > since we attempt to acquire the provider lock in the OnBeginFrame?
> > 
> > Only if there's a way for StartRendering to be called in between calling
> > AcquireLockAndCurrentFrame and ReleaseLock. Is that possible?
> 
> I'm not super familiar with the VideoLayer side of things. If the compositor
> message loop is pumped between those points, it's certainly possible that
> StartRendering could be called as the layer is trying to acquire the current
> frame for some reason.

That won't happen - Drawing is a synchronous operation in the compositor - i.e.
the message loop cannot be pumped in between WillDraw and DidDraw.

That being said - I do think there's a bug in VideoLayerImpl here:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_la...

If WillDraw returns false, DidDraw will not be called so the video layer should
release the lock when it returns false - it doesn't do so in one of it's "false"
exits. I'll upload a separate CL for that.

[DaleCurtis, 2015-05-02 00:24:19]

lgtm, this fixes the layout tests with some additional tweaks on my side.

[DaleCurtis, 2015-05-02 00:25:48]

Whoops, forgot, one more nit: you need to update VideoFrameProviderClientImpl to
move the renderering_ set to before calling client_ to add or remove the
controller.

[sunnyps, 2015-05-02 00:33:05]

On 2015/05/02 00:25:48, DaleCurtis wrote:
> Whoops, forgot, one more nit: you need to update VideoFrameProviderClientImpl
to
> move the renderering_ set to before calling client_ to add or remove the
> controller.

This is in the latest patch, unless you're talking about something else - it's
required to set rendering_ before calling AddVideoFrameController but the same
isn't required before calling RemoveVideoFrameController, correct?

[DaleCurtis, 2015-05-02 00:53:10]

Ah, yeah you're right, thanks! Hopefully we can get in all of these patches by
Monday and test flag flip on Tuesday :)

[DaleCurtis, 2015-05-04 22:27:28]

Sunny, given our conversation on the other thread, do you want to add a test for
this?

[sunnyps, 2015-05-04 23:22:45]

On 2015/05/04 22:27:28, DaleCurtis wrote:
> Sunny, given our conversation on the other thread, do you want to add a test
for
> this?

Added a test - it's not a LayerTreeTest because that would be really difficult
to write for what we're testing here.

[brianderson, 2015-05-04 23:39:47]

lgtm

https://codereview.chromium.org/1113283002/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1113283002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3088:
controller->OnBeginFrame(current_begin_frame_args_);
Not that the controller is really going to care, but should we set the type to
MISSED before forwarding it to the controller?

Fine with holding off on this until the controller actually wants to use it.
Especially since we'd get this for free if/when the BeginFrameSource supports
multiple subscribers.

[sunnyps, 2015-05-04 23:41:07]

Thanks!

https://codereview.chromium.org/1113283002/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1113283002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3088:
controller->OnBeginFrame(current_begin_frame_args_);
On 2015/05/04 23:39:47, brianderson wrote:
> Not that the controller is really going to care, but should we set the type to
> MISSED before forwarding it to the controller?
> 
> Fine with holding off on this until the controller actually wants to use it.
> Especially since we'd get this for free if/when the BeginFrameSource supports
> multiple subscribers.

Yeah, let's hold off on this until we need it.

[sunnyps, 2015-05-04 23:41:15]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-05-04 23:41:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1113283002/#ps40001
(title: "Add tests.")

[commit-bot: I haz the power, 2015-05-04 23:42:50]

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

[commit-bot: I haz the power, 2015-05-05 00:42:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-05 00:43:25]

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