Add config change handling to SourceBufferStream & ChunkDemuxer

Add config change handling to SourceBufferStream & ChunkDemuxer


BUG=122913
TEST=None


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

[acolwell GONE FROM CHROMIUM, 2012-07-12 01:11:50]

Here are my changes to track decoder config changes. I know I still need
unittests, but I wanted to get some initial feedback. 

This CL depends on http://codereview.chromium.org/10669022/

[xhwang, 2012-07-12 18:09:12]

Drive-by to know what's going on since I'll probably need to rebase my work onto
this. Added a few comments.

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/base/stream_p...
File media/base/stream_parser_buffer.h (right):

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/base/stream_p...
media/base/stream_parser_buffer.h:28: void SetConfigID(int config_id);
Here and above, should we follow the style guide and make these
accessors/mutators lower case, e.g
config_id()
set_config_id()

The style guide also suggests inline accessors/mutators in .h file (as in
IsKeyframe()).

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/base/stream_p...
media/base/stream_parser_buffer.h:36: int config_id_;
Add comment? e.g. -1 means an invalid id?

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/sourc...
File media/filters/source_buffer_stream.cc (right):

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/sourc...
media/filters/source_buffer_stream.cc:95: int GetNextConfigID() const;
s/ID/Id?

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/sourc...
media/filters/source_buffer_stream.cc:277: video_configs_.clear();
Use STLDeleteElements here and above?

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/sourc...
media/filters/source_buffer_stream.cc:424:
(*itr)->SetConfigID(append_config_index_);
The comment in .h file uses set_config_id, which sounds right. Change
StreamParserBuffer::SetConfigID to set_config_id since it's a setter?

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/sourc...
media/filters/source_buffer_stream.cc:768: existing_config.extra_data_size())))
{
How about having helper functions for this and below (793)? e.g.
VideoDecoderConfig::Matches(const VideoDecoderConfig&)

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/sourc...
File media/filters/source_buffer_stream.h (right):

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/sourc...
media/filters/source_buffer_stream.h:192: void SetConfigIDs(const BufferQueue&
buffers);
s/ID/Id?

[vrk (LEFT CHROMIUM), 2012-07-12 19:37:54]

Looking good! Just a bunch of nits, and will wait for your unittests before
giving the ell-gee-tea-em.

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/chunk...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/chunk...
media/filters/chunk_demuxer.cc:375: // Do nothing and continue
nit: period at end

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/chunk...
media/filters/chunk_demuxer.cc:410: const AudioDecoderConfig&
ChunkDemuxerStream::audio_decoder_config() {
nit: {audio,video}_decoder_config() does more than being a simple accessor now,
so should be renamed.

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/chunk...
media/filters/chunk_demuxer.cc:427: stream_->UpdateCurrentConfigIndex();
Change method name to something like "CompleteConfigChange"

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/sourc...
File media/filters/source_buffer_stream.cc (right):

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/sourc...
media/filters/source_buffer_stream.cc:662: return
selected_range_->GetNextBuffer(out_buffer) ? kSuccess : kNeedBuffer;
At this point, shouldn't selected_range_->GetNextBuffer(out_buffer) always
return true?

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/sourc...
media/filters/source_buffer_stream.cc:736: DCHECK(!audio_configs_.empty());
DCHECK(video_configs_.empty())?

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/sourc...
media/filters/source_buffer_stream.cc:774: // No matches found so lets add this
one to the list.
nit: "let's"

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/sourc...
media/filters/source_buffer_stream.cc:783: DCHECK(!video_configs_.empty());
DCHECK(audio_configs_.empty())?

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/sourc...
media/filters/source_buffer_stream.cc:808: // No matches found so lets add this
one to the list.
nit: "let's"

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/sourc...
media/filters/source_buffer_stream.cc:1020: next_buffer_index_ <
static_cast<int>(buffers_.size());
nit: indentation + add "&& !waiting_for_keyframe_" just to be safe.

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/sourc...
File media/filters/source_buffer_stream.h (right):

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/sourc...
media/filters/source_buffer_stream.h:191: // Calls set_config_id() on each
buffer with |append_config_index_|.
nit: Since the "set_config_id()" method isn't in this class, maybe say something
more general like "Sets the config id for each buffer to
|append_config_index_|."

https://chromiumcodereview.appspot.com/10696182/diff/7001/media/filters/sourc...
media/filters/source_buffer_stream.h:197: int current_config_index_;
nit: add comments for new fields

[acolwell GONE FROM CHROMIUM, 2012-07-12 23:07:52]

Addressed CR comments. Need to create some tests still.

http://codereview.chromium.org/10696182/diff/7001/media/base/stream_parser_bu...
File media/base/stream_parser_buffer.h (right):

http://codereview.chromium.org/10696182/diff/7001/media/base/stream_parser_bu...
media/base/stream_parser_buffer.h:28: void SetConfigID(int config_id);
On 2012/07/12 18:09:12, xhwang wrote:
> Here and above, should we follow the style guide and make these
> accessors/mutators lower case, e.g
> config_id()
> set_config_id()
> 
> The style guide also suggests inline accessors/mutators in .h file (as in
> IsKeyframe()).
These match the style of getter/setter's in DecoderBuffer. We could change both
classes, but I think that might be better in a separate CL since that will cause
a large number of changes. I'd rather keep a consistent style, though
non-conforming, until both classes can be changed.

http://codereview.chromium.org/10696182/diff/7001/media/base/stream_parser_bu...
media/base/stream_parser_buffer.h:36: int config_id_;
On 2012/07/12 18:09:12, xhwang wrote:
> Add comment? e.g. -1 means an invalid id?
Done. Created a named constant to document the meaning of this value.

http://codereview.chromium.org/10696182/diff/7001/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/10696182/diff/7001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:375: // Do nothing and continue
On 2012/07/12 19:37:55, Victoria Kirst wrote:
> nit: period at end

Done.

http://codereview.chromium.org/10696182/diff/7001/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:427: stream_->UpdateCurrentConfigIndex();
On 2012/07/12 19:37:55, Victoria Kirst wrote:
> Change method name to something like "CompleteConfigChange"

Done.

http://codereview.chromium.org/10696182/diff/7001/media/filters/source_buffer...
File media/filters/source_buffer_stream.cc (right):

http://codereview.chromium.org/10696182/diff/7001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:95: int GetNextConfigID() const;
On 2012/07/12 18:09:12, xhwang wrote:
> s/ID/Id?

Done.

http://codereview.chromium.org/10696182/diff/7001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:277: video_configs_.clear();
On 2012/07/12 18:09:12, xhwang wrote:
> Use STLDeleteElements here and above?

Done.

http://codereview.chromium.org/10696182/diff/7001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:424:
(*itr)->SetConfigID(append_config_index_);
On 2012/07/12 18:09:12, xhwang wrote:
> The comment in .h file uses set_config_id, which sounds right. Change
> StreamParserBuffer::SetConfigID to set_config_id since it's a setter?
I just fixed the comment to reflect the SetConfigId() form. I originally had the
lower case form, but I used MixedCase to match the getter/setters in
StreamParserBuffer & DecoderBuffer.

http://codereview.chromium.org/10696182/diff/7001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:662: return
selected_range_->GetNextBuffer(out_buffer) ? kSuccess : kNeedBuffer;
On 2012/07/12 19:37:55, Victoria Kirst wrote:
> At this point, shouldn't selected_range_->GetNextBuffer(out_buffer) always
> return true?
I wasn't sure. The guard in SourceBufferRange::GetNextBuffer() has an added
check for waiting_for_keyframe_, but SourceBufferRange::HasNextBuffer() doesn't
check for this. I wasn't sure if there was a situation were HasNextBuffer()
could return true and waiting_for_keyframe_ was true.

http://codereview.chromium.org/10696182/diff/7001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:736: DCHECK(!audio_configs_.empty());
On 2012/07/12 19:37:55, Victoria Kirst wrote:
> DCHECK(video_configs_.empty())?

Done.

http://codereview.chromium.org/10696182/diff/7001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:768: existing_config.extra_data_size())))
{
On 2012/07/12 18:09:12, xhwang wrote:
> How about having helper functions for this and below (793)? e.g.
> VideoDecoderConfig::Matches(const VideoDecoderConfig&)

Done.

http://codereview.chromium.org/10696182/diff/7001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:774: // No matches found so lets add this
one to the list.
On 2012/07/12 19:37:55, Victoria Kirst wrote:
> nit: "let's"

Done.

http://codereview.chromium.org/10696182/diff/7001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:783: DCHECK(!video_configs_.empty());
On 2012/07/12 19:37:55, Victoria Kirst wrote:
> DCHECK(audio_configs_.empty())?

Done.

http://codereview.chromium.org/10696182/diff/7001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:808: // No matches found so lets add this
one to the list.
On 2012/07/12 19:37:55, Victoria Kirst wrote:
> nit: "let's"

Done.

http://codereview.chromium.org/10696182/diff/7001/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:1020: next_buffer_index_ <
static_cast<int>(buffers_.size());
On 2012/07/12 19:37:55, Victoria Kirst wrote:
> nit: indentation + add "&& !waiting_for_keyframe_" just to be safe.

Done. Changed GetNextBuffer() to use this now that the conditions match.

http://codereview.chromium.org/10696182/diff/7001/media/filters/source_buffer...
File media/filters/source_buffer_stream.h (right):

http://codereview.chromium.org/10696182/diff/7001/media/filters/source_buffer...
media/filters/source_buffer_stream.h:191: // Calls set_config_id() on each
buffer with |append_config_index_|.
On 2012/07/12 19:37:55, Victoria Kirst wrote:
> nit: Since the "set_config_id()" method isn't in this class, maybe say
something
> more general like "Sets the config id for each buffer to
> |append_config_index_|."

Done.

http://codereview.chromium.org/10696182/diff/7001/media/filters/source_buffer...
media/filters/source_buffer_stream.h:192: void SetConfigIDs(const BufferQueue&
buffers);
On 2012/07/12 18:09:12, xhwang wrote:
> s/ID/Id?

Done.

http://codereview.chromium.org/10696182/diff/7001/media/filters/source_buffer...
media/filters/source_buffer_stream.h:197: int current_config_index_;
On 2012/07/12 19:37:55, Victoria Kirst wrote:
> nit: add comments for new fields

Done.

[acolwell GONE FROM CHROMIUM, 2012-07-25 17:50:57]

PTAL. I've added test to verify config change behavior.

[vrk (LEFT CHROMIUM), 2012-07-25 21:18:22]

LGTM % minor nits

http://codereview.chromium.org/10696182/diff/13001/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/10696182/diff/13001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:359: bool
InitDemuxerWithConfigChangeData() {
I think you should add a comment before this method to explain what it does and
to highlight the hardcoded values that one would need to know in order to write
the TestConfigChange tests that you wrote below. I think you should mention
there are 2 configs: config 1 describes 320x240 video, config 2 describes
640x360. Then mention that this method fills the buffered region from [0, 2737)
and that (0, 467) is associated with config 1, (501 to 767) config 2, and (801,
end) config 1.

http://codereview.chromium.org/10696182/diff/13001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:376: // Append initialization segment
for bear2
nit: period at the end

http://codereview.chromium.org/10696182/diff/13001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:385: // Append a media segment that goes
from [0.477000, 0.988000)
nit: period at end

http://codereview.chromium.org/10696182/diff/13001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:1917:
ASSERT_TRUE(video_config.Matches(stream->video_decoder_config()));
nit: In this test and below, maybe s/video_config/&1 to make it extra clear that
it's the first video config.

[acolwell GONE FROM CHROMIUM, 2012-07-25 23:39:23]

http://codereview.chromium.org/10696182/diff/13001/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/10696182/diff/13001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:376: // Append initialization segment
for bear2
On 2012/07/25 21:18:22, Victoria Kirst wrote:
> nit: period at the end

Done.

http://codereview.chromium.org/10696182/diff/13001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:385: // Append a media segment that goes
from [0.477000, 0.988000)
On 2012/07/25 21:18:22, Victoria Kirst wrote:
> nit: period at end

Done.

http://codereview.chromium.org/10696182/diff/13001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:1917:
ASSERT_TRUE(video_config.Matches(stream->video_decoder_config()));
On 2012/07/25 21:18:22, Victoria Kirst wrote:
> nit: In this test and below, maybe s/video_config/&1 to make it extra clear
that
> it's the first video config.

Done.

[commit-bot: I haz the power, 2012-07-26 15:01:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/10696182/17002

[commit-bot: I haz the power, 2012-07-26 15:01:49]

Presubmit check for 10696182-17002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.(?: \*/)?\n
Found a bad license header in these files:
  media/test/data/bear-320x240-manifest.js \
  media/test/data/bear-640x360-manifest.js

Presubmit checks took 1.6s to calculate.

[commit-bot: I haz the power, 2012-07-26 15:06:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/10696182/13005

[commit-bot: I haz the power, 2012-07-26 16:38:05]

Change committed as 148563