Implement support for 2 source ids (1 for audio and 1 for video).

Implement support for 2 source ids (1 for audio and 1 for video).

This patch allows data to be appended to 2 different sources (1 audio and 1 video).  Also makes sure that appended data matches the mimetype provided in AddId().

BUG=122909
TEST=ChunkDemuxerTest.*


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

[annacc, 2012-06-07 22:42:06]

PTAL!  I feel like the changes to chunk_demuxer.[h/cc] are nice, but the changes
to the unittest are a bit messy.  I'm happy to talk over suggestions (or rewrite
some of the other tests to make things more clear).

There are a few comments inline to try to clarify the changes I needed to make. 
Let me know if anything else is not clear.

Thanks!

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

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer.cc:752: return;
This check is the source of a lot of the AddId() changes in the unittest.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
File media/filters/chunk_demuxer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:81:
EXPECT_TRUE(buffer->IsEndOfStream());
I thought about modifying EndOfStreamHelper to check for just one stream, but
found this to be simpler.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:204: }
This is required because TestInit appends info tracks with no audio and no
video.  I don't think we want to allow that in real life (in fact the codecs
specified in AddId() must be > 0).  I'm not sure if this is the right
solution...

[acolwell GONE FROM CHROMIUM, 2012-06-08 00:29:07]

This looks really good. Just a few minor comments.

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

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer.cc:467: base::AutoLock auto_lock(lock_);
I think we should apply the lock to the whole method. I'm not sure why I left
the lock out originally. That was a mistake.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer.cc:515: stream_parser_map_.erase(id);
I think this leaks the parser.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer.cc:542: if (stream_parser_map_.count(id) == 0) {
This whole block & the comment can be remove. The necessary patch has landed.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer.cc:665: stream_parser_map_.clear();
This is a memory leak. You need to iterate over the map and delete each element.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer.cc:684: DCHECK_NE(state_, INITIALIZED);
You need a loop here that deletes all the parsers in the stream_parser_map_.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer.cc:754: duration_ = duration;
I wonder if we should move the duration_ assignment before the conditional
above. Perhaps always making duration_ be the max duration we see.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
File media/filters/chunk_demuxer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:204: }
On 2012/06/07 22:42:07, annacc wrote:
> This is required because TestInit appends info tracks with no audio and no
> video.  I don't think we want to allow that in real life (in fact the codecs
> specified in AddId() must be > 0).  I'm not sure if this is the right
> solution...
This looks right to me.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:301: success =
AppendInfoTracks(audio_id, true, false, false);
Move this assignement to the line above.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:302: success = success &&
AppendInfoTracks(video_id, false, true, false);
You could use &= here

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:372: scoped_ptr<Cluster>
GenerateSingleStreamCluster(int audio_timecode,
how about just timecode since I don't see anything audio specific here?

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:391: // Make the last block a
BlockGroups so that it doesn't get delayed by the
s/BlockGroups/BlockGroup

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:1219: EXPECT_TRUE(video_read_done);
Consider creating the single stream equivalent of GenerateExpectedReads() and
verify that we get the expected buffers.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:1317: // Seek to 3.
Might want to also indicate that 3 is used because its an unbuffered region.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:1336:
audio->Read(base::Bind(&OnReadDone,
WDYT about putting these reads right above the cluster_a2 declaration and adding
EXPECT_FALSE(audio_read_done) &EXPECT_FALSE(video_read_done) just to make sure
they don't accidentally return early for some reason?

[annacc, 2012-06-08 22:12:23]

Thanks!

Just FYI, this patch breaks the video-media-source-add-and-remove-ids.html
LayoutTest.  I will need to fix that (and add another) before this patch should
land.

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

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer.cc:467: base::AutoLock auto_lock(lock_);
On 2012/06/08 00:29:07, acolwell wrote:
> I think we should apply the lock to the whole method. I'm not sure why I left
> the lock out originally. That was a mistake.

Done.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer.cc:515: stream_parser_map_.erase(id);
On 2012/06/08 00:29:07, acolwell wrote:
> I think this leaks the parser.

Good catch, thanks!

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer.cc:542: if (stream_parser_map_.count(id) == 0) {
On 2012/06/08 00:29:07, acolwell wrote:
> This whole block & the comment can be remove. The necessary patch has landed.

Hmm, seems like it is still needed.  Are you sure this has been fixed?

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer.cc:665: stream_parser_map_.clear();
On 2012/06/08 00:29:07, acolwell wrote:
> This is a memory leak. You need to iterate over the map and delete each
element.

Whoops.  Done.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer.cc:684: DCHECK_NE(state_, INITIALIZED);
On 2012/06/08 00:29:07, acolwell wrote:
> You need a loop here that deletes all the parsers in the stream_parser_map_.

Done.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer.cc:754: duration_ = duration;
On 2012/06/08 00:29:07, acolwell wrote:
> I wonder if we should move the duration_ assignment before the conditional
> above. Perhaps always making duration_ be the max duration we see.

Oh, good idea.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
File media/filters/chunk_demuxer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:301: success =
AppendInfoTracks(audio_id, true, false, false);
On 2012/06/08 00:29:07, acolwell wrote:
> Move this assignement to the line above.

Done.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:302: success = success &&
AppendInfoTracks(video_id, false, true, false);
On 2012/06/08 00:29:07, acolwell wrote:
> You could use &= here

Done.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:372: scoped_ptr<Cluster>
GenerateSingleStreamCluster(int audio_timecode,
On 2012/06/08 00:29:07, acolwell wrote:
> how about just timecode since I don't see anything audio specific here?

Whoops, leftover from a previous idea.  Done.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:391: // Make the last block a
BlockGroups so that it doesn't get delayed by the
On 2012/06/08 00:29:07, acolwell wrote:
> s/BlockGroups/BlockGroup

Done.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:1219: EXPECT_TRUE(video_read_done);
On 2012/06/08 00:29:07, acolwell wrote:
> Consider creating the single stream equivalent of GenerateExpectedReads() and
> verify that we get the expected buffers.

Done.

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:1317: // Seek to 3.
On 2012/06/08 00:29:07, acolwell wrote:
> Might want to also indicate that 3 is used because its an unbuffered region.

Done.

[acolwell GONE FROM CHROMIUM, 2012-06-11 16:09:33]

LGTM % nits

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

https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
media/filters/chunk_demuxer.cc:542: if (stream_parser_map_.count(id) == 0) {
On 2012/06/08 22:12:23, annacc wrote:
> On 2012/06/08 00:29:07, acolwell wrote:
> > This whole block & the comment can be remove. The necessary patch has
landed.
> 
> Hmm, seems like it is still needed.  Are you sure this has been fixed?
Yes. The only test that I saw break when I removed this in my local checkout was
ChunkDemuxerTest.TestAppendDataBeforeInit . This test just needs to be updated
with an AddId() before the CreateInitSegment() call. Are there other breakages
you are seeing?

https://chromiumcodereview.appspot.com/10545066/diff/10001/media/filters/chun...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10545066/diff/10001/media/filters/chun...
media/filters/chunk_demuxer.cc:669: delete it->second;
nit: Add {} since this spans multiple lines.

https://chromiumcodereview.appspot.com/10545066/diff/10001/media/filters/chun...
media/filters/chunk_demuxer.cc:692: delete it->second;
nit: Add {} since this spans multiple lines.

[annacc, 2012-06-11 17:54:28]

On 2012/06/11 16:09:33, acolwell wrote:
> LGTM % nits
> 
>
https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
> File media/filters/chunk_demuxer.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10545066/diff/1004/media/filters/chunk...
> media/filters/chunk_demuxer.cc:542: if (stream_parser_map_.count(id) == 0) {
> On 2012/06/08 22:12:23, annacc wrote:
> > On 2012/06/08 00:29:07, acolwell wrote:
> > > This whole block & the comment can be remove. The necessary patch has
> landed.
> > 
> > Hmm, seems like it is still needed.  Are you sure this has been fixed?
> Yes. The only test that I saw break when I removed this in my local checkout
was
> ChunkDemuxerTest.TestAppendDataBeforeInit . This test just needs to be updated
> with an AddId() before the CreateInitSegment() call. Are there other breakages
> you are seeing?

OK, the errors I was seeing are not related to this block directly.  I'm not
sure the cause yet, but I'm getting parser errors on a layout test that uses
test-25fps-a.webm.  Looks like the pipeline shuts us down when we report a parse
error.  Then, if AppendData() is called again after that, state_ is SHUTDOWN,
and stream_parser_map_ is empty, and we hit a DCHECK.  We could add a state
check that returns early in AppendData(), but I'm not sure why AppendData()
would get called again after a Shutdown().  Digging some more... but let me know
if you have thoughts on any of that.

> 
>
https://chromiumcodereview.appspot.com/10545066/diff/10001/media/filters/chun...
> File media/filters/chunk_demuxer.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10545066/diff/10001/media/filters/chun...
> media/filters/chunk_demuxer.cc:669: delete it->second;
> nit: Add {} since this spans multiple lines.
> 
>
https://chromiumcodereview.appspot.com/10545066/diff/10001/media/filters/chun...
> media/filters/chunk_demuxer.cc:692: delete it->second;
> nit: Add {} since this spans multiple lines.

[acolwell GONE FROM CHROMIUM, 2012-06-11 18:07:17]

On 2012/06/11 17:54:28, annacc wrote:
> OK, the errors I was seeing are not related to this block directly.  I'm not
> sure the cause yet, but I'm getting parser errors on a layout test that uses
> test-25fps-a.webm.  Looks like the pipeline shuts us down when we report a
parse
> error.  Then, if AppendData() is called again after that, state_ is SHUTDOWN,
> and stream_parser_map_ is empty, and we hit a DCHECK.  We could add a state
> check that returns early in AppendData(), but I'm not sure why AppendData()
> would get called again after a Shutdown().  Digging some more... but let me
know
> if you have thoughts on any of that.

AppendData() can be called after shutdown if WebKit hasn't been notified of the
error yet. This typically happens when JavaScript calls append in a loop and
ignores the exceptions being thrown. I think you just need to move the "map is
not empty" DCHECK into the INITIALIZING & INITIALIZED case blocks in the switch
below. This check should have been inside the lock anyways.

[annacc, 2012-06-11 18:28:08]

On 2012/06/11 18:07:17, acolwell wrote:
> On 2012/06/11 17:54:28, annacc wrote:
> > OK, the errors I was seeing are not related to this block directly.  I'm not
> > sure the cause yet, but I'm getting parser errors on a layout test that uses
> > test-25fps-a.webm.  Looks like the pipeline shuts us down when we report a
> parse
> > error.  Then, if AppendData() is called again after that, state_ is
SHUTDOWN,
> > and stream_parser_map_ is empty, and we hit a DCHECK.  We could add a state
> > check that returns early in AppendData(), but I'm not sure why AppendData()
> > would get called again after a Shutdown().  Digging some more... but let me
> know
> > if you have thoughts on any of that.
> 
> AppendData() can be called after shutdown if WebKit hasn't been notified of
the
> error yet. This typically happens when JavaScript calls append in a loop and
> ignores the exceptions being thrown. I think you just need to move the "map is
> not empty" DCHECK into the INITIALIZING & INITIALIZED case blocks in the
switch
> below. This check should have been inside the lock anyways.

OK thanks, that solved the DCHECK issue.  Still investigating the parser error
in my layout test.

[annacc, 2012-06-11 19:23:08]

Thanks!

https://chromiumcodereview.appspot.com/10545066/diff/10001/media/filters/chun...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10545066/diff/10001/media/filters/chun...
media/filters/chunk_demuxer.cc:669: delete it->second;
On 2012/06/11 16:09:33, acolwell wrote:
> nit: Add {} since this spans multiple lines.

Done.

https://chromiumcodereview.appspot.com/10545066/diff/10001/media/filters/chun...
media/filters/chunk_demuxer.cc:692: delete it->second;
On 2012/06/11 16:09:33, acolwell wrote:
> nit: Add {} since this spans multiple lines.

Done.

[commit-bot: I haz the power, 2012-06-12 15:44:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/annacc@chromium.org/10545066/7011

[commit-bot: I haz the power, 2012-06-12 17:29:04]

Change committed as 141679