Move VideoDecoder out of media pipeline.

Move VideoDecoder out of media pipeline.

This is the first step to move VideoDecoder out of Filter hierarchy. In this CL, VideoDecoder is removed from the pipeline and is owned by the VideoRenderBase now. All calls to Seek()/Pause()/Play() are removed. The video decoder is flushed or stopped from the VideoRenderBase instead of from the composite filter.

In a follow-up CL, VideoDecoder will not be a Filter any more and Seek()/Pause()/Play() methods will be removed from its definition and all implementations.

BUG=108340
TEST=media_unittest, content_unittest and some basic playing of html5 video.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=128289

[xhwang, 2012-03-14 17:11:48]

Hello fischman and scherkus,

I'll do the "moving VideoDecoder out of Filter hierarchy" in two steps (this CL
being the first):
1, Remove all calls to Pause()/Seek()/Play() on VideoPlayer. Move it out of
pipeline and let video renderer own it.
2, Remove Pause()/Seek()/Play() from VideoDecoder and make it a non-Filter.

I am not sure if there'd be any threading issues with this change, so please be
fastidious!

xhwang

[Ami GONE FROM CHROMIUM, 2012-03-14 20:05:01]

http://codereview.chromium.org/9700006/diff/2001/media/base/filter_collection.h
File media/base/filter_collection.h (right):

http://codereview.chromium.org/9700006/diff/2001/media/base/filter_collection...
media/base/filter_collection.h:64: std::list<scoped_refptr<VideoDecoder> >
video_decoders_;
nit: IWBN to have _V_ideoDecoders follow _A_udio_decoders (here and in various
other places in this CL) to be lexicographically sorted.

http://codereview.chromium.org/9700006/diff/2001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/9700006/diff/2001/media/base/filters.h#newcode167
media/base/filters.h:167: };
Can you confirm that Pause/Seek/Play are never called on VideoDecoder by adding
private virtual overrides for them here (to hide the base implementation) and
then not implementing them (to trigger a linker failure if anyone tries to call
them)?

http://codereview.chromium.org/9700006/diff/2001/media/base/filters.h#newcode179
media/base/filters.h:179: const PipelineStatusCB& pipeline_status_cb,
s/pipeline_// ?

http://codereview.chromium.org/9700006/diff/2001/media/base/filters.h#newcode181
media/base/filters.h:181: const VideoTimeCB& video_time_cb) = 0;
IMO "Video" belongs in neither the param name nor the type name (look at the
typedef; there's nothing video-specific about it).

http://codereview.chromium.org/9700006/diff/2001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/9700006/diff/2001/media/base/pipeline.cc#newco...
media/base/pipeline.cc:1207: video_decoder_ =
pipeline_init_state_->video_decoder;
I don't get why you flipped the use of video_decoder_ and
pipeline_init_state_->video_decoder in this function.  Was it just aesthetics or
something deeper?

(I don't mind it b/c it makes clearer what to do when it comes time to kill the
video_decoder_ member, just curious)

http://codereview.chromium.org/9700006/diff/2001/media/base/pipeline_unittest.cc
File media/base/pipeline_unittest.cc (left):

http://codereview.chromium.org/9700006/diff/2001/media/base/pipeline_unittest...
media/base/pipeline_unittest.cc:138: EXPECT_CALL(*mocks_->video_decoder(),
Stop(_))
I'm a bit surprised that Stop() isn't being called.
Do you understand why?
(or is it being called and ignored by gmock?  is there logspam?)

http://codereview.chromium.org/9700006/diff/2001/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/9700006/diff/2001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:55: flush_callback_ = callback;
I'm surprised you didn't rename this :)

http://codereview.chromium.org/9700006/diff/2001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:460: decoder_->Flush(flush_callback_);
This change is unsafe if flushing the decoder causes the renderer to get flushed
again (the basic reason why ResetAndRunCB() isn't RunAndResetCB()).
The right way to do this is 
base::Closure flush_cb = flush_callback_;
flush_callback_.Reset();
decoder_->Flush(flush_cb);

[xhwang, 2012-03-14 22:28:40]

Hello fischman,

Mostly revised on comments. Please review again.

xhwang

http://codereview.chromium.org/9700006/diff/2001/media/base/filter_collection.h
File media/base/filter_collection.h (right):

http://codereview.chromium.org/9700006/diff/2001/media/base/filter_collection...
media/base/filter_collection.h:64: std::list<scoped_refptr<VideoDecoder> >
video_decoders_;
On 2012/03/14 20:05:01, Ami Fischman wrote:
> nit: IWBN to have _V_ideoDecoders follow _A_udio_decoders (here and in various
> other places in this CL) to be lexicographically sorted.

Done.

http://codereview.chromium.org/9700006/diff/2001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/9700006/diff/2001/media/base/filters.h#newcode167
media/base/filters.h:167: };
On 2012/03/14 20:05:01, Ami Fischman wrote:
> Can you confirm that Pause/Seek/Play are never called on VideoDecoder by
adding
> private virtual overrides for them here (to hide the base implementation) and
> then not implementing them (to trigger a linker failure if anyone tries to
call
> them)?

I like this! Done.

http://codereview.chromium.org/9700006/diff/2001/media/base/filters.h#newcode179
media/base/filters.h:179: const PipelineStatusCB& pipeline_status_cb,
On 2012/03/14 20:05:01, Ami Fischman wrote:
> s/pipeline_// ?

I chose this verbose name because status_cb and stats_cb (once used in various
places) were too similar. Now since we use statistics_cb for the latter, this
doesn't matter any more. Anything bad except it's too long? :)

http://codereview.chromium.org/9700006/diff/2001/media/base/filters.h#newcode181
media/base/filters.h:181: const VideoTimeCB& video_time_cb) = 0;
On 2012/03/14 20:05:01, Ami Fischman wrote:
> IMO "Video" belongs in neither the param name nor the type name (look at the
> typedef; there's nothing video-specific about it).

Will do in another CL.

http://codereview.chromium.org/9700006/diff/2001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/9700006/diff/2001/media/base/pipeline.cc#newco...
media/base/pipeline.cc:1207: video_decoder_ =
pipeline_init_state_->video_decoder;
On 2012/03/14 20:05:01, Ami Fischman wrote:
> I don't get why you flipped the use of video_decoder_ and
> pipeline_init_state_->video_decoder in this function.  Was it just aesthetics
or
> something deeper?
> 
> (I don't mind it b/c it makes clearer what to do when it comes time to kill
the
> video_decoder_ member, just curious)

Nothing deep :) I found InitializeVideoDecoder and InitializeAudioDecoder are
doing pretty much the same thing but are written in different ways. That
confused me a bit. I compared carefully to make sure that they are actually the
same logic. To prevent further confusion, I modified InitializeVideoDecoder so
that it looks just like InitializeAudioDecoder.

http://codereview.chromium.org/9700006/diff/2001/media/base/pipeline_unittest.cc
File media/base/pipeline_unittest.cc (left):

http://codereview.chromium.org/9700006/diff/2001/media/base/pipeline_unittest...
media/base/pipeline_unittest.cc:138: EXPECT_CALL(*mocks_->video_decoder(),
Stop(_))
On 2012/03/14 20:05:01, Ami Fischman wrote:
> I'm a bit surprised that Stop() isn't being called.
> Do you understand why?
> (or is it being called and ignored by gmock?  is there logspam?)

video_decoder()->Stop was called by pipeline. Now it's called by the
VideoRenderer. That's why it's removed here. I added it back in
VideoRendererBaseTest. I thought this test should focus on pipeline operations
and shouldn't expect what the video renderer will do.

http://codereview.chromium.org/9700006/diff/2001/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/9700006/diff/2001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:55: flush_callback_ = callback;
On 2012/03/14 20:05:01, Ami Fischman wrote:
> I'm surprised you didn't rename this :)

Doh! Done. Probably rename all seek_callback, play_callback, etc in another CL
to avoid this CL being buried in renames. Thanks!

http://codereview.chromium.org/9700006/diff/2001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:460: decoder_->Flush(flush_callback_);
On 2012/03/14 20:05:01, Ami Fischman wrote:
> This change is unsafe if flushing the decoder causes the renderer to get
flushed
> again (the basic reason why ResetAndRunCB() isn't RunAndResetCB()).
> The right way to do this is 
> base::Closure flush_cb = flush_callback_;
> flush_callback_.Reset();
> decoder_->Flush(flush_cb);

Done.

[Ami GONE FROM CHROMIUM, 2012-03-14 22:42:50]

LGTM % nits

http://codereview.chromium.org/9700006/diff/2001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/9700006/diff/2001/media/base/filters.h#newcode179
media/base/filters.h:179: const PipelineStatusCB& pipeline_status_cb,
On 2012/03/14 22:28:41, xhwang wrote:
> On 2012/03/14 20:05:01, Ami Fischman wrote:
> > s/pipeline_// ?
> 
> I chose this verbose name because status_cb and stats_cb (once used in various
> places) were too similar. Now since we use statistics_cb for the latter, this
> doesn't matter any more. Anything bad except it's too long? :)

My only objection is the length.

http://codereview.chromium.org/9700006/diff/5002/media/base/filters.cc
File media/base/filters.cc (right):

http://codereview.chromium.org/9700006/diff/5002/media/base/filters.cc#newcode79
media/base/filters.cc:79: CHECK(false);
CHECK(false);
 -> 
LOG(FATAL) << "WAT?";

http://codereview.chromium.org/9700006/diff/5002/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/9700006/diff/5002/media/base/filters.h#newcode171
media/base/filters.h:171: // TODO(xhwang): Remove them when VideoDecoder is not
a Filter any more.
Include the crbug # in the TODO

[xhwang, 2012-03-14 22:59:42]

http://codereview.chromium.org/9700006/diff/2001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/9700006/diff/2001/media/base/filters.h#newcode179
media/base/filters.h:179: const PipelineStatusCB& pipeline_status_cb,
On 2012/03/14 22:42:50, Ami Fischman wrote:
> On 2012/03/14 22:28:41, xhwang wrote:
> > On 2012/03/14 20:05:01, Ami Fischman wrote:
> > > s/pipeline_// ?
> > 
> > I chose this verbose name because status_cb and stats_cb (once used in
various
> > places) were too similar. Now since we use statistics_cb for the latter,
this
> > doesn't matter any more. Anything bad except it's too long? :)
> 
> My only objection is the length.

Okay, will change this in the next CL too.

http://codereview.chromium.org/9700006/diff/5002/media/base/filters.cc
File media/base/filters.cc (right):

http://codereview.chromium.org/9700006/diff/5002/media/base/filters.cc#newcode79
media/base/filters.cc:79: CHECK(false);
On 2012/03/14 22:42:50, Ami Fischman wrote:
> CHECK(false);
>  -> 
> LOG(FATAL) << "WAT?";

Done.

http://codereview.chromium.org/9700006/diff/5002/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/9700006/diff/5002/media/base/filters.h#newcode171
media/base/filters.h:171: // TODO(xhwang): Remove them when VideoDecoder is not
a Filter any more.
On 2012/03/14 22:42:50, Ami Fischman wrote:
> Include the crbug # in the TODO

Done.

[commit-bot: I haz the power, 2012-03-15 00:45:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/9700006/1013

[Ami GONE FROM CHROMIUM, 2012-03-15 01:00:52]

I expected you to wait for an LGTM from scherkus@, seeing as how you listed
him as a reviewer on the CL.

-a

On Wed, Mar 14, 2012 at 5:45 PM, <commit-bot@chromium.org> wrote:

> CQ is trying da patch. Follow status at
> https://chromium-status.**appspot.com/cq/xhwang@**
>
chromium.org/9700006/1013<https://chromium-status.appspot.com/cq/xhwang@chromium.org/9700006/1013>
>
>
>
http://codereview.chromium.**org/9700006/<http://codereview.chromium.org/9700...
>

[xhwang, 2012-03-15 01:14:02]

Okay, I guess I meant you OR scherkus@. I'll wait for him then. Thanks for
reminding me.  Commit cancelled

On 2012/03/15 01:00:52, Ami Fischman wrote:
> I expected you to wait for an LGTM from scherkus@, seeing as how you listed
> him as a reviewer on the CL.
> 
> -a
> 
> On Wed, Mar 14, 2012 at 5:45 PM, <mailto:commit-bot@chromium.org> wrote:
> 
> > CQ is trying da patch. Follow status at
> > https://chromium-status.**appspot.com/cq/xhwang%40**
> >
>
chromium.org/9700006/1013<https://chromium-status.appspot.com/cq/xhwang@chromium.org/9700006/1013>
> >
> >
> >
>
http://codereview.chromium.**org/9700006/%3Chttp://codereview.chromium.org/97...>
> >

[scherkus (not reviewing), 2012-03-22 13:43:22]

LGTM

[commit-bot: I haz the power, 2012-03-22 19:05:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/9700006/16001

[commit-bot: I haz the power, 2012-03-22 20:38:00]

Change committed as 128289