cc: Various code safety improvements in video compositing code.

cc: Various code safety improvements in video compositing code.

This CL adds comments and DHCECKs clarifying the thread safety aspects of
VideoFrameProviderClientImpl.

BUG=336733
Committed: https://crrev.com/a528172f25699f80a201e405d046eb376df9dfa1
Cr-Commit-Position: refs/heads/master@{#322521}

[sunnyps, 2015-03-24 01:47:11]

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

[sunnyps, 2015-03-24 01:47:11]

PTAL. I've split out the various code safety improvements I made out of my
previous CL: crrev.com/927423003. I believe I have addressed all comments there.

[danakj, 2015-03-24 17:38:01]

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_frame_provi...
File cc/layers/video_frame_provider_client_impl.cc (right):

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_frame_provi...
cc/layers/video_frame_provider_client_impl.cc:93: // Block the provider from
shutting down until the frame controller and it's
this is the only mention of a frame controller, i think this comment shouldn't
change?

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_frame_provi...
File cc/layers/video_frame_provider_client_impl.h (right):

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_frame_provi...
cc/layers/video_frame_provider_client_impl.h:17: }  // namespace media
(this isn't actually needed by the linter for small forward declare blocks)

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_frame_provi...
cc/layers/video_frame_provider_client_impl.h:20: 
why whitespace?

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_layer_impl.h
File cc/layers/video_layer_impl.h (right):

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_layer_impl....
cc/layers/video_layer_impl.h:21: 
why the whitespace change?

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_layer_impl....
cc/layers/video_layer_impl.h:45: media::VideoRotation video_rotation() const {
return video_rotation_; }
why is this moving?

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_layer_impl_...
File cc/layers/video_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_layer_impl_...
cc/layers/video_layer_impl_unittest.cc:20: // We can't use
DebugScopedSetImplThreadAndMainThreadBlocked for these tests
What if you SetRootLayer(nullptr) before exiting the tests?

https://codereview.chromium.org/1033563002/diff/1/cc/test/fake_video_frame_pr...
File cc/test/fake_video_frame_provider.h (right):

https://codereview.chromium.org/1033563002/diff/1/cc/test/fake_video_frame_pr...
cc/test/fake_video_frame_provider.h:29: void SendDidReceiveFrame();
The VLI::SetNeedsRedraw() method didn't go anywhere, so this test change isn't
needed is it?

[sunnyps, 2015-03-24 19:43:38]

PTAL.

Addressed all comments.

I renamed the Stop/Stopped methods to Destroy/Destroyed and removed
Start/Started - we (me and dalecurtis@) would like to use the Start/Stop names
to be used in the context of video playback and not the object lifecycle - this
is for an upcoming CL which integrates Dale's new video rendering improvements
with the compositor.

I also changed all simple accessors in VFPCI to use the thread checker which
means they are not simple accessors anymore and had to be renamed.

Thanks!

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_frame_provi...
File cc/layers/video_frame_provider_client_impl.cc (right):

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_frame_provi...
cc/layers/video_frame_provider_client_impl.cc:93: // Block the provider from
shutting down until the frame controller and it's
On 2015/03/24 17:38:01, danakj wrote:
> this is the only mention of a frame controller, i think this comment shouldn't
> change?

Done.

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_frame_provi...
File cc/layers/video_frame_provider_client_impl.h (right):

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_frame_provi...
cc/layers/video_frame_provider_client_impl.h:17: }  // namespace media
On 2015/03/24 17:38:01, danakj wrote:
> (this isn't actually needed by the linter for small forward declare blocks)

Done.

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_frame_provi...
cc/layers/video_frame_provider_client_impl.h:20: 
On 2015/03/24 17:38:01, danakj wrote:
> why whitespace?

Done.

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_layer_impl.h
File cc/layers/video_layer_impl.h (right):

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_layer_impl....
cc/layers/video_layer_impl.h:21: 
On 2015/03/24 17:38:01, danakj wrote:
> why the whitespace change?

Done.

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_layer_impl....
cc/layers/video_layer_impl.h:45: media::VideoRotation video_rotation() const {
return video_rotation_; }
On 2015/03/24 17:38:01, danakj wrote:
> why is this moving?

Done.

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_layer_impl_...
File cc/layers/video_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1033563002/diff/1/cc/layers/video_layer_impl_...
cc/layers/video_layer_impl_unittest.cc:20: // We can't use
DebugScopedSetImplThreadAndMainThreadBlocked for these tests
On 2015/03/24 17:38:01, danakj wrote:
> What if you SetRootLayer(nullptr) before exiting the tests?

I didn't use SetRootLayer(nullptr) but changed this to use a static function and
not require test fixtures.

https://codereview.chromium.org/1033563002/diff/1/cc/test/fake_video_frame_pr...
File cc/test/fake_video_frame_provider.h (right):

https://codereview.chromium.org/1033563002/diff/1/cc/test/fake_video_frame_pr...
cc/test/fake_video_frame_provider.h:29: void SendDidReceiveFrame();
On 2015/03/24 17:38:01, danakj wrote:
> The VLI::SetNeedsRedraw() method didn't go anywhere, so this test change isn't
> needed is it?

Done.

[sunnyps, 2015-03-26 18:24:50]

ping

[danakj, 2015-03-26 22:38:35]

LGTM

https://codereview.chromium.org/1033563002/diff/20001/cc/layers/video_frame_p...
File cc/layers/video_frame_provider.h (right):

https://codereview.chromium.org/1033563002/diff/20001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:22: class CC_EXPORT VideoFrameProvider {
we don't need to export pure virtual things right?

https://codereview.chromium.org/1033563002/diff/20001/cc/layers/video_frame_p...
File cc/layers/video_frame_provider_client_impl.cc (right):

https://codereview.chromium.org/1033563002/diff/20001/cc/layers/video_frame_p...
cc/layers/video_frame_provider_client_impl.cc:55: void
VideoFrameProviderClientImpl::Destroy() {
I would weakly prefer to keep these as Stop()/Stopped().

Destroy() is used in other places in the code to let an object delete itself
IIRC.

https://codereview.chromium.org/1033563002/diff/20001/cc/layers/video_frame_p...
File cc/layers/video_frame_provider_client_impl.h (right):

https://codereview.chromium.org/1033563002/diff/20001/cc/layers/video_frame_p...
cc/layers/video_frame_provider_client_impl.h:63: // frame controller are
accessing it's frame. Before being destroyed the
"is accessing"

https://codereview.chromium.org/1033563002/diff/20001/cc/layers/video_frame_p...
cc/layers/video_frame_provider_client_impl.h:65: // from returning till the
frame controller is done using the frame.
"until"

[sunnyps, 2015-03-27 00:38:46]

Thanks!

https://codereview.chromium.org/1033563002/diff/20001/cc/layers/video_frame_p...
File cc/layers/video_frame_provider.h (right):

https://codereview.chromium.org/1033563002/diff/20001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:22: class CC_EXPORT VideoFrameProvider {
On 2015/03/26 22:38:35, danakj wrote:
> we don't need to export pure virtual things right?

I think we do in this case - my CL fails on dll builds. I think it worked before
because we only used an inline function (active_video_layer) of
VideoFrameProviderClientImpl which is a subclass of VideoFrameProvider::Client.
As soon as I changed that to normal function call I got build errors. In the
test which uses active_video_layer there's a static cast from
VideoFrameProvider::Client (which was not exported) to
VideoFrameProviderClientImpl (which was/is exported). Could be that or the 1
million other special cases in C++. :)

https://codereview.chromium.org/1033563002/diff/20001/cc/layers/video_frame_p...
File cc/layers/video_frame_provider_client_impl.cc (right):

https://codereview.chromium.org/1033563002/diff/20001/cc/layers/video_frame_p...
cc/layers/video_frame_provider_client_impl.cc:55: void
VideoFrameProviderClientImpl::Destroy() {
On 2015/03/26 22:38:35, danakj wrote:
> I would weakly prefer to keep these as Stop()/Stopped().
> 
> Destroy() is used in other places in the code to let an object delete itself
> IIRC.

Done.

https://codereview.chromium.org/1033563002/diff/20001/cc/layers/video_frame_p...
File cc/layers/video_frame_provider_client_impl.h (right):

https://codereview.chromium.org/1033563002/diff/20001/cc/layers/video_frame_p...
cc/layers/video_frame_provider_client_impl.h:63: // frame controller are
accessing it's frame. Before being destroyed the
On 2015/03/26 22:38:35, danakj wrote:
> "is accessing"

Done.

https://codereview.chromium.org/1033563002/diff/20001/cc/layers/video_frame_p...
cc/layers/video_frame_provider_client_impl.h:65: // from returning till the
frame controller is done using the frame.
On 2015/03/26 22:38:35, danakj wrote:
> "until"

Done.

[sunnyps, 2015-03-27 00:38:51]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-03-27 00:38:51]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1033563002/#ps60001
(title: "Address comments")

[commit-bot: I haz the power, 2015-03-27 00:39:20]

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

[commit-bot: I haz the power, 2015-03-27 02:40:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-27 02:41:07]

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

[DaleCurtis, 2015-03-27 16:45:06]

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

[DaleCurtis, 2015-03-27 16:45:07]

https://codereview.chromium.org/1033563002/diff/60001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.h (right):

https://codereview.chromium.org/1033563002/diff/60001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.h:51: scoped_refptr<VideoFrameProviderClientImpl>
provider_client_impl,
I realize this is just copying existing style, but this should really be a
const& to avoid extra potential ref/unref steps (which if RefCountedThreadSafe
are atomic operations).

[danakj, 2015-03-27 16:55:15]

https://codereview.chromium.org/1033563002/diff/60001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.h (right):

https://codereview.chromium.org/1033563002/diff/60001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.h:51: scoped_refptr<VideoFrameProviderClientImpl>
provider_client_impl,
On 2015/03/27 16:45:07, DaleCurtis wrote:
> I realize this is just copying existing style, but this should really be a
> const& to avoid extra potential ref/unref steps (which if RefCountedThreadSafe
> are atomic operations).

I guess, this happens once per video being loaded, I think that it won't matter.
A const& would be fine, or alternatively, we can swap() it into the member
variable in the constructor.

We should really get r-value reference support for scoped_refptr so we can do
this properly with a Pass() or the like.

[sunnyps, 2015-03-27 18:58:28]

On 2015/03/27 16:55:15, danakj wrote:
>
https://codereview.chromium.org/1033563002/diff/60001/cc/layers/video_layer_i...
> File cc/layers/video_layer_impl.h (right):
> 
>
https://codereview.chromium.org/1033563002/diff/60001/cc/layers/video_layer_i...
> cc/layers/video_layer_impl.h:51: scoped_refptr<VideoFrameProviderClientImpl>
> provider_client_impl,
> On 2015/03/27 16:45:07, DaleCurtis wrote:
> > I realize this is just copying existing style, but this should really be a
> > const& to avoid extra potential ref/unref steps (which if
RefCountedThreadSafe
> > are atomic operations).
> 
> I guess, this happens once per video being loaded, I think that it won't
matter.
> A const& would be fine, or alternatively, we can swap() it into the member
> variable in the constructor.
> 
> We should really get r-value reference support for scoped_refptr so we can do
> this properly with a Pass() or the like.

Uploaded a CL for this: https://codereview.chromium.org/1039783002

[DaleCurtis, 2015-03-27 19:53:26]

https://codereview.chromium.org/1033563002/diff/60001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.h (right):

https://codereview.chromium.org/1033563002/diff/60001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.h:51: scoped_refptr<VideoFrameProviderClientImpl>
provider_client_impl,
On 2015/03/27 16:55:15, danakj wrote:
> On 2015/03/27 16:45:07, DaleCurtis wrote:
> > I realize this is just copying existing style, but this should really be a
> > const& to avoid extra potential ref/unref steps (which if
RefCountedThreadSafe
> > are atomic operations).
> 
> I guess, this happens once per video being loaded, I think that it won't
matter.
> A const& would be fine, or alternatively, we can swap() it into the member
> variable in the constructor.
> 
> We should really get r-value reference support for scoped_refptr so we can do
> this properly with a Pass() or the like.

Yeah, it's not really an issue in this case, just one of those in aggregate
style things.