Move VideoDecoder initialization into VideoRendererBase.

Move VideoDecoder initialization into VideoRendererBase to simplify implementing codec config changes during playback.


BUG=141533
TEST=Existing PipelineTest.*, VideoRendererBaseTest.*

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

[Ami GONE FROM CHROMIUM, 2012-08-09 20:30:18]

The CL description could be more informative as to what's driving this change.

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

http://codereview.chromium.org/10836167/diff/7005/media/base/filter_collectio...
media/base/filter_collection.h:43: bool IsEmpty() const;
I wonder if this is actually profitably used by anything.

http://codereview.chromium.org/10836167/diff/7005/media/base/filter_collectio...
media/base/filter_collection.h:56: VideoDecoderList& GetVideoDecoders();
Return a pointer, never a non-const-ref in chromium style.

http://codereview.chromium.org/10836167/diff/7005/media/base/mock_filters.h
File media/base/mock_filters.h (right):

http://codereview.chromium.org/10836167/diff/7005/media/base/mock_filters.h#n...
media/base/mock_filters.h:157: MOCK_METHOD0(PrepareForShutdownHack, void());
Do you really need this?

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

http://codereview.chromium.org/10836167/diff/7005/media/base/pipeline.cc#newc...
media/base/pipeline.cc:64: struct Pipeline::PipelineInitState {
drop this struct now (and bind the audio decoder into the callback for the
renderer)?

http://codereview.chromium.org/10836167/diff/7005/media/base/pipeline.cc#newc...
media/base/pipeline.cc:644: // Assuming audio renderer was created, create video
decoder.
s/decoder/renderer/

http://codereview.chromium.org/10836167/diff/7005/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/10836167/diff/7005/media/base/pipeline.h#newco...
media/base/pipeline.h:386: // audio/video stream does not exist.
drop "video"

http://codereview.chromium.org/10836167/diff/7005/media/base/pipeline.h#newco...
media/base/pipeline.h:532: // Renderer references used for setting the volume
and determining
This comment is lying by omission; I think you want to move the HACK comment
that used to live right above here down to here (with a bit of massaging).

http://codereview.chromium.org/10836167/diff/7005/media/base/video_renderer.h
File media/base/video_renderer.h (right):

http://codereview.chromium.org/10836167/diff/7005/media/base/video_renderer.h...
media/base/video_renderer.h:101: // the duration.  Default implementation does
nothing; derived decoders may
There is no default implementation anymore (so update the comment).

http://codereview.chromium.org/10836167/diff/7005/media/filters/video_rendere...
File media/filters/video_renderer_base.h (right):

http://codereview.chromium.org/10836167/diff/7005/media/filters/video_rendere...
media/filters/video_renderer_base.h:137: VideoDecoderList decoders_;
This is only used during initialization, so could also be bound into the
initializenextdecoder calls.

http://codereview.chromium.org/10836167/diff/7005/media/filters/video_rendere...
media/filters/video_renderer_base.h:139: scoped_refptr<DemuxerStream>
demuxer_stream_;
ditto

http://codereview.chromium.org/10836167/diff/7005/webkit/media/filter_helpers.cc
File webkit/media/filter_helpers.cc (left):

http://codereview.chromium.org/10836167/diff/7005/webkit/media/filter_helpers...
webkit/media/filter_helpers.cc:53: // NOTE: http://crbug.com/110800 is about
replacing this ad-hockery with
This comment should live on.

[xhwang, 2012-08-09 21:38:29]

Drive-by with one question.

http://codereview.chromium.org/10836167/diff/7005/media/filters/video_rendere...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/10836167/diff/7005/media/filters/video_rendere...
media/filters/video_renderer_base.cc:176: if (state_ == kStopped)
Q: if VBR is already stopped, do we want to InitializeNextDecoder above?

[acolwell GONE FROM CHROMIUM, 2012-08-09 22:23:31]

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

http://codereview.chromium.org/10836167/diff/7005/media/base/filter_collectio...
media/base/filter_collection.h:43: bool IsEmpty() const;
On 2012/08/09 20:30:18, Ami Fischman wrote:
> I wonder if this is actually profitably used by anything.

No. Removed.

http://codereview.chromium.org/10836167/diff/7005/media/base/filter_collectio...
media/base/filter_collection.h:56: VideoDecoderList& GetVideoDecoders();
On 2012/08/09 20:30:18, Ami Fischman wrote:
> Return a pointer, never a non-const-ref in chromium style.

Done.

http://codereview.chromium.org/10836167/diff/7005/media/base/mock_filters.h
File media/base/mock_filters.h (right):

http://codereview.chromium.org/10836167/diff/7005/media/base/mock_filters.h#n...
media/base/mock_filters.h:157: MOCK_METHOD0(PrepareForShutdownHack, void());
On 2012/08/09 20:30:18, Ami Fischman wrote:
> Do you really need this?
Yes. It is part of the VideoRenderer interface. :/

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

http://codereview.chromium.org/10836167/diff/7005/media/base/pipeline.cc#newc...
media/base/pipeline.cc:64: struct Pipeline::PipelineInitState {
On 2012/08/09 20:30:18, Ami Fischman wrote:
> drop this struct now (and bind the audio decoder into the callback for the
> renderer)?
I can't really do this w/o moving the kInitAudioDecoder block in
InitializeTask() out into a separate method. This may be moot once scherkus@
collapses init into one of his handy callback sequences. 

Another option is to defer this to another CL where I move AudioDecoder init
into AudioRendererImpl. That would likely need to happen anyway for supporting
audio decoder config changes.

http://codereview.chromium.org/10836167/diff/7005/media/base/pipeline.cc#newc...
media/base/pipeline.cc:644: // Assuming audio renderer was created, create video
decoder.
On 2012/08/09 20:30:18, Ami Fischman wrote:
> s/decoder/renderer/

Done.

http://codereview.chromium.org/10836167/diff/7005/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/10836167/diff/7005/media/base/pipeline.h#newco...
media/base/pipeline.h:386: // audio/video stream does not exist.
On 2012/08/09 20:30:18, Ami Fischman wrote:
> drop "video"

Done.

http://codereview.chromium.org/10836167/diff/7005/media/base/pipeline.h#newco...
media/base/pipeline.h:532: // Renderer references used for setting the volume
and determining
On 2012/08/09 20:30:18, Ami Fischman wrote:
> This comment is lying by omission; I think you want to move the HACK comment
> that used to live right above here down to here (with a bit of massaging).

Done.

http://codereview.chromium.org/10836167/diff/7005/media/base/video_renderer.h
File media/base/video_renderer.h (right):

http://codereview.chromium.org/10836167/diff/7005/media/base/video_renderer.h...
media/base/video_renderer.h:101: // the duration.  Default implementation does
nothing; derived decoders may
On 2012/08/09 20:30:18, Ami Fischman wrote:
> There is no default implementation anymore (so update the comment).

Done.

http://codereview.chromium.org/10836167/diff/7005/media/filters/video_rendere...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/10836167/diff/7005/media/filters/video_rendere...
media/filters/video_renderer_base.cc:176: if (state_ == kStopped)
On 2012/08/09 21:38:29, xhwang wrote:
> Q: if VBR is already stopped, do we want to InitializeNextDecoder above?
Good point. Moved this to the top.

http://codereview.chromium.org/10836167/diff/7005/media/filters/video_rendere...
File media/filters/video_renderer_base.h (right):

http://codereview.chromium.org/10836167/diff/7005/media/filters/video_rendere...
media/filters/video_renderer_base.h:137: VideoDecoderList decoders_;
On 2012/08/09 20:30:18, Ami Fischman wrote:
> This is only used during initialization, so could also be bound into the
> initializenextdecoder calls.

Done.

http://codereview.chromium.org/10836167/diff/7005/media/filters/video_rendere...
media/filters/video_renderer_base.h:139: scoped_refptr<DemuxerStream>
demuxer_stream_;
On 2012/08/09 20:30:18, Ami Fischman wrote:
> ditto

Done.

http://codereview.chromium.org/10836167/diff/7005/webkit/media/filter_helpers.cc
File webkit/media/filter_helpers.cc (left):

http://codereview.chromium.org/10836167/diff/7005/webkit/media/filter_helpers...
webkit/media/filter_helpers.cc:53: // NOTE: http://crbug.com/110800 is about
replacing this ad-hockery with
On 2012/08/09 20:30:18, Ami Fischman wrote:
> This comment should live on.

Done.

[Ami GONE FROM CHROMIUM, 2012-08-10 19:47:22]

lgtm

[acolwell GONE FROM CHROMIUM, 2012-08-10 20:24:24]

brettw@ could you OWNERS review the one line change to
content/renderer/render_view_impl.cc . Thanks.

[jamesr, 2012-08-10 21:51:36]

content/renderer lgtm

[scherkus (not reviewing), 2012-08-14 22:00:24]

nice CL!

post-commit drive by -- acolwell can you TBR the fixes?

http://codereview.chromium.org/10836167/diff/27/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/10836167/diff/27/media/base/pipeline.h#newcode501
media/base/pipeline.h:501: // Video Renderer reference used for determining when
playback has finished
this TODO isn't really applicable in this context anymore as we won't be
removing the video_renderer_ reference after that bug is fixed

I'd rather see this TODO by the ShutdownHack() method call / declaration

http://codereview.chromium.org/10836167/diff/27/media/base/video_renderer.h
File media/base/video_renderer.h (right):

http://codereview.chromium.org/10836167/diff/27/media/base/video_renderer.h#n...
media/base/video_renderer.h:40: // Initialize a VideoRenderer with the given
VideoDecoder, executing
docs need updating

http://codereview.chromium.org/10836167/diff/27/media/filters/video_renderer_...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/10836167/diff/27/media/filters/video_renderer_...
media/filters/video_renderer_base_unittest.cc:110:
.WillOnce(RunPipelineStatusCB1(PIPELINE_OK));
fix indent

http://codereview.chromium.org/10836167/diff/27/media/filters/video_renderer_...
media/filters/video_renderer_base_unittest.cc:699: 
remove extra newline

http://codereview.chromium.org/10836167/diff/27/media/filters/video_renderer_...
media/filters/video_renderer_base_unittest.cc:700: }
fix indent

http://codereview.chromium.org/10836167/diff/27/webkit/media/filter_helpers.cc
File webkit/media/filter_helpers.cc (right):

http://codereview.chromium.org/10836167/diff/27/webkit/media/filter_helpers.c...
webkit/media/filter_helpers.cc:52: // Remove any "traditional" decoders (e.g.
GpuVideoDecoder) from the
is this comment duplicated?

[acolwell GONE FROM CHROMIUM, 2012-08-15 21:36:56]

http://codereview.chromium.org/10836167/diff/27/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/10836167/diff/27/media/base/pipeline.h#newcode501
media/base/pipeline.h:501: // Video Renderer reference used for determining when
playback has finished
On 2012/08/14 22:00:24, scherkus wrote:
> this TODO isn't really applicable in this context anymore as we won't be
> removing the video_renderer_ reference after that bug is fixed
> 
> I'd rather see this TODO by the ShutdownHack() method call / declaration

Ami asked me to keep the comment here
http://codereview.chromium.org/10836167/diff/7005/media/base/pipeline.h#newco...

http://codereview.chromium.org/10836167/diff/27/media/base/video_renderer.h
File media/base/video_renderer.h (right):

http://codereview.chromium.org/10836167/diff/27/media/base/video_renderer.h#n...
media/base/video_renderer.h:40: // Initialize a VideoRenderer with the given
VideoDecoder, executing
On 2012/08/14 22:00:24, scherkus wrote:
> docs need updating

Done.

http://codereview.chromium.org/10836167/diff/27/media/filters/video_renderer_...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/10836167/diff/27/media/filters/video_renderer_...
media/filters/video_renderer_base_unittest.cc:110:
.WillOnce(RunPipelineStatusCB1(PIPELINE_OK));
On 2012/08/14 22:00:24, scherkus wrote:
> fix indent

Done.

http://codereview.chromium.org/10836167/diff/27/media/filters/video_renderer_...
media/filters/video_renderer_base_unittest.cc:699: 
On 2012/08/14 22:00:24, scherkus wrote:
> remove extra newline

Done.

http://codereview.chromium.org/10836167/diff/27/media/filters/video_renderer_...
media/filters/video_renderer_base_unittest.cc:700: }
On 2012/08/14 22:00:24, scherkus wrote:
> fix indent

Done.

http://codereview.chromium.org/10836167/diff/27/webkit/media/filter_helpers.cc
File webkit/media/filter_helpers.cc (right):

http://codereview.chromium.org/10836167/diff/27/webkit/media/filter_helpers.c...
webkit/media/filter_helpers.cc:52: // Remove any "traditional" decoders (e.g.
GpuVideoDecoder) from the
On 2012/08/14 22:00:24, scherkus wrote:
> is this comment duplicated?

Done.