Fix WebMStreamParser to handle Cues between Clusters correctly.

Fix WebMStreamParser to handle Cues between Clusters correctly.

Previously webm parser was correctly handling Cues element between
clusters. It was broken in r277633. Also added tests to cover this
case.

BUG=386122
R=acolwell@chromium.org, wolenetz@chromium.org

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

[wolenetz, 2014-06-18 19:44:21]

lgtm. Thank you for fixing this.

[wolenetz, 2014-06-18 19:51:25]

Hold on a sec - one comment. not lgtm.

https://codereview.chromium.org/346613003/diff/1/media/formats/webm/webm_stre...
File media/formats/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/346613003/diff/1/media/formats/webm/webm_stre...
media/formats/webm/webm_stream_parser.cc:64: if (state_ != kParsingClusters)
Hmm. On second (third) look, it seems we should Reset() the parser if it is
not-null, even if |state_| isn't kParsingClusters, because it could have been
switched to kParsingHeaders due to cues, for instance, that followed previous
cluster parse.

[acolwell GONE FROM CHROMIUM, 2014-06-18 19:54:22]

https://codereview.chromium.org/346613003/diff/1/media/formats/webm/webm_stre...
File media/formats/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/346613003/diff/1/media/formats/webm/webm_stre...
media/formats/webm/webm_stream_parser.cc:241: ChangeState(kParsingClusters);
You should probably remove this so that the same code path is always used to
transition to kParseClusters.

https://codereview.chromium.org/346613003/diff/1/media/formats/webm/webm_stre...
media/formats/webm/webm_stream_parser.cc:314: } while (size > 0 && (bytes_parsed
> 0 || cluster_ended));
Do we still need this loop? Can't you just transition to kParseHeaders after a
single cluster has been parsed and use the outer loop that calls this method?

[wolenetz, 2014-06-18 21:10:23]

Note: I have a change in CQ now that changes chunk_demuxer_unittest.cc to use
TEST_F, not TEST_P. It will conflict with this change; likely this CL will need
to adapt. See https://codereview.chromium.org/315483002/

[Sergey Ulanov, 2014-06-18 21:47:55]

PTAL. Removed parsing_cluster_, which makes ParseCluster() much simpler.

https://codereview.chromium.org/346613003/diff/1/media/formats/webm/webm_stre...
File media/formats/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/346613003/diff/1/media/formats/webm/webm_stre...
media/formats/webm/webm_stream_parser.cc:64: if (state_ != kParsingClusters)
On 2014/06/18 19:51:25, wolenetz wrote:
> Hmm. On second (third) look, it seems we should Reset() the parser if it is
> not-null, even if |state_| isn't kParsingClusters, because it could have been
> switched to kParsingHeaders due to cues, for instance, that followed previous
> cluster parse. 

Done.

https://codereview.chromium.org/346613003/diff/1/media/formats/webm/webm_stre...
media/formats/webm/webm_stream_parser.cc:241: ChangeState(kParsingClusters);
On 2014/06/18 19:54:22, acolwell wrote:
> You should probably remove this so that the same code path is always used to
> transition to kParseClusters.

Done.

https://codereview.chromium.org/346613003/diff/1/media/formats/webm/webm_stre...
media/formats/webm/webm_stream_parser.cc:314: } while (size > 0 && (bytes_parsed
> 0 || cluster_ended));
On 2014/06/18 19:54:22, acolwell wrote:
> Do we still need this loop? Can't you just transition to kParseHeaders after a
> single cluster has been parsed and use the outer loop that calls this method?

see crbug.com/382807. We need to handle the case when a new buffer starts new
cluster that implicitly ends the previous cluster. In that case
cluster_parser_->Parse() will return 0, but if it's called again it will parse
more.
WebMStreamParser::Parse() will not call ParseCluster() again if ParseCluster()
returns 0.

Now that I look at it again, I think we don't need parsing_cluster_, and instead
can go back to the header-parsing start after end of each cluster. This
simplifies this code significantly. PTAL

[wolenetz, 2014-06-18 22:47:53]

I like the simplification. Looking pretty good:

https://codereview.chromium.org/346613003/diff/60001/media/filters/chunk_demu...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/346613003/diff/60001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:3492: CheckExpectedRanges(kSourceId, "{
[0,46) }");
nit: ClusterWithUnknownSize already does this. Can just one append be done in
this test for both clusters and the cues?

https://codereview.chromium.org/346613003/diff/60001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:3498: AppendData(&*data.begin(),
data.size());
nit: s/&*data.begin()/data.data()/ ?

https://codereview.chromium.org/346613003/diff/60001/media/formats/webm/webm_...
File media/formats/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/346613003/diff/60001/media/formats/webm/webm_...
media/formats/webm/webm_stream_parser.cc:61: cluster_parser_->Reset();
cluster_parser_ may still be null prior to completing the first init segment
parse. If Flush() occurred then, boom. Please protect against this.

https://codereview.chromium.org/346613003/diff/60001/media/formats/webm/webm_...
media/formats/webm/webm_stream_parser.cc:103: if (state_ == oldState && result
== 0)
aside: This state_ == oldState is key to the simplification.

[Sergey Ulanov, 2014-06-18 23:26:50]

https://codereview.chromium.org/346613003/diff/60001/media/filters/chunk_demu...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/346613003/diff/60001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:3492: CheckExpectedRanges(kSourceId, "{
[0,46) }");
On 2014/06/18 22:47:53, wolenetz wrote:
> nit: ClusterWithUnknownSize already does this. Can just one append be done in
> this test for both clusters and the cues?

Done.

https://codereview.chromium.org/346613003/diff/60001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:3498: AppendData(&*data.begin(),
data.size());
On 2014/06/18 22:47:53, wolenetz wrote:
> nit: s/&*data.begin()/data.data()/ ?

This doesn't compile on android, I already tried:
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...

https://codereview.chromium.org/346613003/diff/60001/media/formats/webm/webm_...
File media/formats/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/346613003/diff/60001/media/formats/webm/webm_...
media/formats/webm/webm_stream_parser.cc:61: cluster_parser_->Reset();
On 2014/06/18 22:47:53, wolenetz wrote:
> cluster_parser_ may still be null prior to completing the first init segment
> parse. If Flush() occurred then, boom. Please protect against this.

Done.

https://codereview.chromium.org/346613003/diff/60001/media/formats/webm/webm_...
media/formats/webm/webm_stream_parser.cc:103: if (state_ == oldState && result
== 0)
On 2014/06/18 22:47:53, wolenetz wrote:
> aside: This state_ == oldState is key to the simplification.

Yes

[wolenetz, 2014-06-18 23:54:29]

lgtm % nit + note that a rebase will probably be needed if
https://codereview.chromium.org/315483002/ lands first.

https://codereview.chromium.org/346613003/diff/60001/media/filters/chunk_demu...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/346613003/diff/60001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:3498: AppendData(&*data.begin(),
data.size());
On 2014/06/18 23:26:49, Sergey Ulanov wrote:
> On 2014/06/18 22:47:53, wolenetz wrote:
> > nit: s/&*data.begin()/data.data()/ ?
> 
> This doesn't compile on android, I already tried:
>
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...

I see. Thanks. (std::vector::data() is C++11)

https://codereview.chromium.org/346613003/diff/80001/media/filters/chunk_demu...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/346613003/diff/80001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:3491: // Add two cluster separated by
Cues in a single Append() call.
nit:s/cluster/clusters

[Sergey Ulanov, 2014-06-19 00:18:09]

https://codereview.chromium.org/346613003/diff/80001/media/filters/chunk_demu...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/346613003/diff/80001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:3491: // Add two cluster separated by
Cues in a single Append() call.
On 2014/06/18 23:54:29, wolenetz wrote:
> nit:s/cluster/clusters

Done.

[acolwell GONE FROM CHROMIUM, 2014-06-19 01:01:19]

lgtm

[Sergey Ulanov, 2014-06-19 17:35:24]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-06-19 17:36:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sergeyu@chromium.org/346613003/120001

[Sergey Ulanov, 2014-06-19 19:10:54]

Committed patchset #7 manually as r278452 (presubmit successful).