Pass in media message loop to VideoRendererBase and enforce calling on the right thread.

Pass in media message loop to VideoRendererBase and enforce calling on the right thread.

As a result VideoDecoder implementations have been updated to expect they get called on the correct thread instead of trampolining.

The newly added media::WrapCB() facilitates running callbacks on separate execution stacks to avoid reentrancy in calling code.

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

[Ami GONE FROM CHROMIUM, 2012-11-29 23:34:23]

Unsurprisingly, I like this approach.

I especially like that you can have rules like "in vrb.cc, any calls on decoder_
must be done using PostTask unless you know something special".

https://codereview.chromium.org/11428095/diff/1/media/filters/ffmpeg_video_de...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/11428095/diff/1/media/filters/ffmpeg_video_de...
media/filters/ffmpeg_video_decoder.cc:161: DoRead(read_cb);
Either force-post or change the API contract of VideoDecoder::Read() and inline
DoRead into here?

https://codereview.chromium.org/11428095/diff/1/media/filters/video_renderer_...
File media/filters/video_renderer_base.cc (right):

https://codereview.chromium.org/11428095/diff/1/media/filters/video_renderer_...
media/filters/video_renderer_base.cc:38: decoder_message_loop_(message_loop) {
Could DCHECK that this is *not* the current loop, for sanity.

https://codereview.chromium.org/11428095/diff/1/media/filters/video_renderer_...
File media/filters/video_renderer_base.h (right):

https://codereview.chromium.org/11428095/diff/1/media/filters/video_renderer_...
media/filters/video_renderer_base.h:56: const
scoped_refptr<base::MessageLoopProxy>& message_loop);
could alternatively let the decoder tell the renderer what loop it lives on.

[scherkus (not reviewing), 2012-11-29 23:45:56]

yeah it didn't turn out as bad as I feared (it's yet to hit one of my CHECK()
tripwires during testing, to boot!)

https://codereview.chromium.org/11428095/diff/1/media/filters/ffmpeg_video_de...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/11428095/diff/1/media/filters/ffmpeg_video_de...
media/filters/ffmpeg_video_decoder.cc:161: DoRead(read_cb);
On 2012/11/29 23:34:23, Ami Fischman wrote:
> Either force-post or change the API contract of VideoDecoder::Read() and
inline
> DoRead into here?

This was lazy prototyping. Inlining the code took too many vim mode changes and
keystrokes!

https://codereview.chromium.org/11428095/diff/1/media/filters/video_renderer_...
File media/filters/video_renderer_base.h (right):

https://codereview.chromium.org/11428095/diff/1/media/filters/video_renderer_...
media/filters/video_renderer_base.h:56: const
scoped_refptr<base::MessageLoopProxy>& message_loop);
On 2012/11/29 23:34:23, Ami Fischman wrote:
> could alternatively let the decoder tell the renderer what loop it lives on.

I'm mulling over this one

the "caller has to know the right msg loop" model requires that the caller is
given a handle at some point. either it's provided up front (this PS) or as you
suggest the decoder can expose it

my gut tells me the latter is less optimal for some reason -- perhaps because we
have everything running on the same thread today so it seems unnecessary in the
long run i.e., VRB should be fixed to run on message_loop_

[scherkus (not reviewing), 2012-12-06 21:22:14]

acolwell: time to review!

I know I said yesterday I was going to add WrapCB() in
https://codereview.chromium.org/11411332/ but I actually need to land this CL
first

I'm totally open to suggestions for naming + implementation of
callback_wrapper.h / WrapCB().

https://codereview.chromium.org/11428095/diff/13001/media/filters/video_rende...
File media/filters/video_renderer_base.cc (right):

https://codereview.chromium.org/11428095/diff/13001/media/filters/video_rende...
media/filters/video_renderer_base.cc:586: state_ == kEnded) {
due to task-posting it's possible to run AttemptRead() after ending

I felt this was better than inspecting whether we're on |message_loop| or the
internal thread on line 591 and optionally PostTask()'ing

[scherkus (not reviewing), 2012-12-06 21:57:57]

https://codereview.chromium.org/11428095/diff/13001/media/base/callback_wrapp...
File media/base/callback_wrapper.h (right):

https://codereview.chromium.org/11428095/diff/13001/media/base/callback_wrapp...
media/base/callback_wrapper.h:26: void RunWrappedCB(
I just realized I was completely mistaken about BindToLoop()!!!

It *does* do a force-post, which means we can replace all of this with a helper
like BindToCurrentLoop()

[acolwell GONE FROM CHROMIUM, 2012-12-06 23:02:37]

LGTM

[scherkus (not reviewing), 2012-12-07 00:55:59]

xhwang: can you take a look at DVD?

[xhwang, 2012-12-07 01:21:07]

reviewed DVD only

https://codereview.chromium.org/11428095/diff/10020/media/filters/decrypting_...
File media/filters/decrypting_video_decoder.cc (right):

https://codereview.chromium.org/11428095/diff/10020/media/filters/decrypting_...
media/filters/decrypting_video_decoder.cc:144: BindToCurrentLoop(closure).Run();
Is this used to force posting the task to prevent reentrance? (Note that we are
already on the right thread). In that case, I feel BindToCurrentLoop a little
confusing. How about renaming BindToLoop/BindToCurrentLoop to
PostToLoop/PostToCurrentLoop?

Also, can the closure itself be a BindToCurrentLoop(...) in the VRB so that the
decoder can call it at any time without worrying about force posting it? If so
we can make it part of the API contract.

https://codereview.chromium.org/11428095/diff/10020/media/filters/decrypting_...
media/filters/decrypting_video_decoder.cc:259:
&DecryptingVideoDecoder::DeliverFrame, this,
indent

[scherkus (not reviewing), 2012-12-07 01:43:16]

https://codereview.chromium.org/11428095/diff/10020/media/filters/decrypting_...
File media/filters/decrypting_video_decoder.cc (right):

https://codereview.chromium.org/11428095/diff/10020/media/filters/decrypting_...
media/filters/decrypting_video_decoder.cc:144: BindToCurrentLoop(closure).Run();
On 2012/12/07 01:21:07, xhwang wrote:
> Is this used to force posting the task to prevent reentrance? (Note that we
are
> already on the right thread). In that case, I feel BindToCurrentLoop a little
> confusing. How about renaming BindToLoop/BindToCurrentLoop to
> PostToLoop/PostToCurrentLoop?

I agree the naming could be better but I'd like to save that for a different CL.

> Also, can the closure itself be a BindToCurrentLoop(...) in the VRB so that
the
> decoder can call it at any time without worrying about force posting it? If so
> we can make it part of the API contract.

Yeah I've been thinking about that... strictly speaking the API contract is that
these are async methods and thus should be executed async-ly (i.e., the onus is
on the implementer). Having the caller (e.g., VRB) wrap the closure means they
knows that the implementations of these methods are in fact executed
synchronously.

That being said, we have N VideoDecoder implementations and 1 VRB! I also wonder
whether some of these methods should be async at all!

I just fixed up GpuVideoDecoder as well and it's making me think that perhaps
these APIs shouldn't make guarantees and that the onus is on the caller to
BindToCurrentLoop() if they aren't safe against reentrancy... hmmmmmm..........

https://codereview.chromium.org/11428095/diff/10020/media/filters/decrypting_...
media/filters/decrypting_video_decoder.cc:259:
&DecryptingVideoDecoder::DeliverFrame, this,
On 2012/12/07 01:21:07, xhwang wrote:
> indent

Done.

[xhwang, 2012-12-07 02:22:19]

lgtm on DVD % more discussion

https://codereview.chromium.org/11428095/diff/10020/media/filters/decrypting_...
File media/filters/decrypting_video_decoder.cc (right):

https://codereview.chromium.org/11428095/diff/10020/media/filters/decrypting_...
media/filters/decrypting_video_decoder.cc:144: BindToCurrentLoop(closure).Run();
On 2012/12/07 01:43:16, scherkus wrote:
> On 2012/12/07 01:21:07, xhwang wrote:
> > Is this used to force posting the task to prevent reentrance? (Note that we
> are
> > already on the right thread). In that case, I feel BindToCurrentLoop a
little
> > confusing. How about renaming BindToLoop/BindToCurrentLoop to
> > PostToLoop/PostToCurrentLoop?
> 
> I agree the naming could be better but I'd like to save that for a different
CL.

SGTM
> 
> > Also, can the closure itself be a BindToCurrentLoop(...) in the VRB so that
> the
> > decoder can call it at any time without worrying about force posting it? If
so
> > we can make it part of the API contract.
> 
> Yeah I've been thinking about that... strictly speaking the API contract is
that
> these are async methods and thus should be executed async-ly (i.e., the onus
is
> on the implementer). Having the caller (e.g., VRB) wrap the closure means they
> knows that the implementations of these methods are in fact executed
> synchronously.
> 
> That being said, we have N VideoDecoder implementations and 1 VRB! I also
wonder
> whether some of these methods should be async at all!
> 
> I just fixed up GpuVideoDecoder as well and it's making me think that perhaps
> these APIs shouldn't make guarantees and that the onus is on the caller to
> BindToCurrentLoop() if they aren't safe against reentrancy...
hmmmmmm..........

I feel it's cleaner to have the VRB do that so that all decoders don't need to
worry about that. It's easy to miss this in VideoDecoder implementation. If we
do that, we probably can also remove the BindToCurrentLoop on lines 42, 73, 96.
But really I don't have strong opinions on this. As long as we make the API
contract clear I am fine.