Add support for config changes during playback to FFmpegVideoDecoder.

Add support for config changes during playback to FFmpegVideoDecoder.
Also fixed a minor config change related bug in SourceBufferStream.

BUG=122913
TEST=PipelineIntegrationTest.MediaSource_MP4ConfigChange, SourceBufferStreamTest.ConfigChange_Basic, PipelineIntegrationTest.MediaSource_Seek

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

[Ami GONE FROM CHROMIUM, 2012-08-17 15:16:59]

Very nice.

http://codereview.chromium.org/10836304/diff/1/media/filters/ffmpeg_video_dec...
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/10836304/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder.h:83: // Handles initializing the decoder
with a new config.
s/initializing/(re-)initializing/

http://codereview.chromium.org/10836304/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder.h:83: // Handles initializing the decoder
with a new config.
s/new/(new)/

http://codereview.chromium.org/10836304/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder.h:85: bool HandleConfigChange();
Since this handles the initial config as well, 
s/HandleConfigChange/ConfigureDecoder/ 
?

http://codereview.chromium.org/10836304/diff/1/media/filters/pipeline_integra...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10836304/diff/1/media/filters/pipeline_integra...
media/filters/pipeline_integration_test.cc:297: MockMediaSource
source("bear.640x360_dash.mp4", kMP4, kAppendWholeFile);
Did you forget to add this file to this CL?

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
File media/filters/source_buffer_stream.cc (right):

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
media/filters/source_buffer_stream.cc:286: audio_configs_[0] = new
AudioDecoderConfig();
FWIW, you could
audio_configs_.push_back(new AudioDecoderConfig());
audio_configs_.back()->CopyFrom(audio_config);
and drop the size ctor at l.273.
(and ditto for the VDC ctor below)

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
media/filters/source_buffer_stream.cc:951: if (!selected_range_ ||
!selected_range_->HasNextBuffer())
Can this really happen?
If not, then you can unconditionally set
config_change_pending_ = false;
at the top of the function and simplify a bit.

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
File media/filters/source_buffer_stream.h (right):

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
media/filters/source_buffer_stream.h:265: // and the next
GetCurrentXXXDecoderConfig() should update the current config.
This comment implies that the contract is that GNB() will keep reporting kCC
until the client gets the new current config.  You might test for that if that's
really what you want the API to be.
But probably the API should be that a single kCC is fired and must be acted
upon, and then you can CHECK(!config_change_pending_) at the top of GNB().

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
File media/filters/source_buffer_stream_unittest.cc (right):

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
media/filters/source_buffer_stream_unittest.cc:160: << " Got: " <<
actual.AsHumanReadableString();
s/" Got/"\nActual/ ?

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
media/filters/source_buffer_stream_unittest.cc:1864: // Consume the buffers
associated with the initial
s/initial/initial config./

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
media/filters/source_buffer_stream_unittest.cc:1875: CheckConfig(config_);
I think this paragraph can be dropped in favor of swapping the order of the
CheckConfig & EXPECT_EQ in the loop above.

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
media/filters/source_buffer_stream_unittest.cc:1891:
TEST_F(SourceBufferStreamTest, ConfigChange_Seek) {
What about edge/boundary conditions?

I.e. is it worth testing that when seeking to 
4.0001 * frame_duration_
the new config is vended?

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
media/filters/source_buffer_stream_unittest.cc:1906: Seek(5);
Add a
CheckConfig(config_);
between the Seek and the GetNextBuffer()?

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

http://codereview.chromium.org/10836304/diff/1/media/filters/ffmpeg_video_dec...
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/10836304/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder.h:83: // Handles initializing the decoder
with a new config.
On 2012/08/17 15:16:59, Ami Fischman wrote:
> s/initializing/(re-)initializing/

Done.

http://codereview.chromium.org/10836304/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder.h:83: // Handles initializing the decoder
with a new config.
On 2012/08/17 15:16:59, Ami Fischman wrote:
> s/new/(new)/

Done.

http://codereview.chromium.org/10836304/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder.h:85: bool HandleConfigChange();
On 2012/08/17 15:16:59, Ami Fischman wrote:
> Since this handles the initial config as well, 
> s/HandleConfigChange/ConfigureDecoder/ 
> ?

Done.

http://codereview.chromium.org/10836304/diff/1/media/filters/pipeline_integra...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10836304/diff/1/media/filters/pipeline_integra...
media/filters/pipeline_integration_test.cc:297: MockMediaSource
source("bear.640x360_dash.mp4", kMP4, kAppendWholeFile);
On 2012/08/17 15:16:59, Ami Fischman wrote:
> Did you forget to add this file to this CL?
No. Binary file updates are here (http://codereview.chromium.org/10854236/)

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
File media/filters/source_buffer_stream.cc (right):

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
media/filters/source_buffer_stream.cc:286: audio_configs_[0] = new
AudioDecoderConfig();
On 2012/08/17 15:16:59, Ami Fischman wrote:
> FWIW, you could
> audio_configs_.push_back(new AudioDecoderConfig());
> audio_configs_.back()->CopyFrom(audio_config);
> and drop the size ctor at l.273.
> (and ditto for the VDC ctor below)

Done.

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
media/filters/source_buffer_stream.cc:951: if (!selected_range_ ||
!selected_range_->HasNextBuffer())
On 2012/08/17 15:16:59, Ami Fischman wrote:
> Can this really happen?
> If not, then you can unconditionally set
> config_change_pending_ = false;
> at the top of the function and simplify a bit.
I believe it can happen with an unfortunate end overlap, but I believe the
proper thing to do would be to clear config_change_pending_ unconditionally
anyways so that GNB can be called again to reassess the situation.

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
File media/filters/source_buffer_stream.h (right):

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
media/filters/source_buffer_stream.h:265: // and the next
GetCurrentXXXDecoderConfig() should update the current config.
On 2012/08/17 15:16:59, Ami Fischman wrote:
> This comment implies that the contract is that GNB() will keep reporting kCC
> until the client gets the new current config.  You might test for that if
that's
> really what you want the API to be.
That was the original intent, but I like your suggestion below.

> But probably the API should be that a single kCC is fired and must be acted
> upon, and then you can CHECK(!config_change_pending_) at the top of GNB().

Done.

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
File media/filters/source_buffer_stream_unittest.cc (right):

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
media/filters/source_buffer_stream_unittest.cc:160: << " Got: " <<
actual.AsHumanReadableString();
On 2012/08/17 15:16:59, Ami Fischman wrote:
> s/" Got/"\nActual/ ?

Done.

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
media/filters/source_buffer_stream_unittest.cc:1864: // Consume the buffers
associated with the initial
On 2012/08/17 15:16:59, Ami Fischman wrote:
> s/initial/initial config./

Done.

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
media/filters/source_buffer_stream_unittest.cc:1875: CheckConfig(config_);
On 2012/08/17 15:16:59, Ami Fischman wrote:
> I think this paragraph can be dropped in favor of swapping the order of the
> CheckConfig & EXPECT_EQ in the loop above.

Done.

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
media/filters/source_buffer_stream_unittest.cc:1891:
TEST_F(SourceBufferStreamTest, ConfigChange_Seek) {
On 2012/08/17 15:16:59, Ami Fischman wrote:
> What about edge/boundary conditions?
> 
> I.e. is it worth testing that when seeking to 
> 4.0001 * frame_duration_
> the new config is vended?
I changed one of the seeks to a non-keyframe position. This along with
SourceBufferStreamTest.Seek_InBetweenTimestamps should be enough to cover your
concern.

http://codereview.chromium.org/10836304/diff/1/media/filters/source_buffer_st...
media/filters/source_buffer_stream_unittest.cc:1906: Seek(5);
On 2012/08/17 15:16:59, Ami Fischman wrote:
> Add a
> CheckConfig(config_);
> between the Seek and the GetNextBuffer()?

Done.

[Ami GONE FROM CHROMIUM, 2012-08-21 17:08:16]

LGTM; are you planning to land this as-is and let GVD pick up the pieces
post-facto?

http://codereview.chromium.org/10836304/diff/5003/media/filters/pipeline_inte...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10836304/diff/5003/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:329: #if defined(GOOGLE_CHROME_BUILD)
|| defined(USE_PROPRIETARY_CODECS)
Is this really || and not &&?  IDK, but please double-check.

http://codereview.chromium.org/10836304/diff/5003/media/filters/source_buffer...
File media/filters/source_buffer_stream_unittest.cc (right):

http://codereview.chromium.org/10836304/diff/5003/media/filters/source_buffer...
media/filters/source_buffer_stream_unittest.cc:1912: Seek(7);
Actually my hope was that you would seek to a target *before* the first frame
with a new config but after the last frame with the old config, to show that the
contract of seeking to the first frame following the seek target also makes the
right config current.

[acolwell GONE FROM CHROMIUM, 2012-08-21 18:18:40]

>are you planning to land this as-is and let GVD pick up the pieces post-facto?

I'm planning to land this as-is and then I'll take a stab at adding support to
GVD.

http://codereview.chromium.org/10836304/diff/5003/media/filters/pipeline_inte...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10836304/diff/5003/media/filters/pipeline_inte...
media/filters/pipeline_integration_test.cc:329: #if defined(GOOGLE_CHROME_BUILD)
|| defined(USE_PROPRIETARY_CODECS)
On 2012/08/21 17:08:16, Ami Fischman wrote:
> Is this really || and not &&?  IDK, but please double-check.
Sadly yes. Chrome builds don't define USE_PROPRIETARY_CODECS but local
development configurations to, but don't define GOOGLE_CHROME_BUILD. :(

http://codereview.chromium.org/10836304/diff/5003/media/filters/source_buffer...
File media/filters/source_buffer_stream_unittest.cc (right):

http://codereview.chromium.org/10836304/diff/5003/media/filters/source_buffer...
media/filters/source_buffer_stream_unittest.cc:1912: Seek(7);
On 2012/08/21 17:08:16, Ami Fischman wrote:
> Actually my hope was that you would seek to a target *before* the first frame
> with a new config but after the last frame with the old config, to show that
the
> contract of seeking to the first frame following the seek target also makes
the
> right config current.
Talked offline. No change needed.