Fix WebMStreamParser to continue parsing after a cluster end

Fix WebMStreamParser to continue parsing after a cluster end

Previously the parser would stop after cluster end even if there is more
data to be parsed.

BUG=382807, 335676
R=acolwell@chromium.org, wolenetz@chromium.org

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

[Sergey Ulanov, 2014-06-10 05:58:28]

I'm not sure what's the right place to add a unittest. Looks like there are no
unittests for WebMStreamParser.

[Sergey Ulanov, 2014-06-12 18:21:09]

+wolenetz
Matt, can you please take a look? (Aaron is sheriffing today) Thanks!

[wolenetz, 2014-06-12 18:22:10]

On 2014/06/12 18:21:09, Sergey Ulanov wrote:
> +wolenetz
> Matt, can you please take a look? (Aaron is sheriffing today) Thanks!

Certainly. I'll add this to my short CR queue today :)

[wolenetz, 2014-06-12 21:32:48]

> I'm not sure what's the right place to add a unittest. Looks like there are no
> unittests for WebMStreamParser.

Correct, there are none specifically for WebMStreamParser. We've been
overloading chunk_demuxer_unittests.cc with these since it uses the
non-proprietary webm streams for related tests. See
ChunkDemuxerTest.WebMParsingMediaSegmentDetection, for example.

I worked through a repro (see bug) and it looks like you're on the right track
to getting both bug 382807 and 335676 fixed simultaneously. Please re-assign the
latter to yourself, if you concur:

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

https://codereview.chromium.org/328653002/diff/1/media/formats/webm/webm_stre...
media/formats/webm/webm_stream_parser.cc:309: } while (bytes_parsed > 0 ||
cluster_ended);
This looks wrong. Yes, in the case of a new append allowing the cluster parser
to detect the end of a previously 'kWebMUnknownSize' cluster, cluster_ended will
be true, (though bytes_parsed might not be 0, see below) and a single repeat
here would work. But the new append could be a sequence of at least one cluster
followed by an init segment, all at once. Some further peek-ahead to prevent
attempt to parse init segment as a cluster is needed, I think, instead of
blindly assuming in this loop that all of *data[0]..*data[size-1] is all media
segment (cluster) data.

bytes_parsed might not be 0 if the previous append did not completely send all
of an unknown-sized cluster, and the next append includes both the completion of
the first cluster and then some other data.

[Sergey Ulanov, 2014-06-13 00:26:08]

PTAL. Added unittests for both bugs.

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

https://codereview.chromium.org/328653002/diff/1/media/formats/webm/webm_stre...
media/formats/webm/webm_stream_parser.cc:309: } while (bytes_parsed > 0 ||
cluster_ended);
On 2014/06/12 21:32:48, wolenetz wrote:
> This looks wrong. Yes, in the case of a new append allowing the cluster parser
> to detect the end of a previously 'kWebMUnknownSize' cluster, cluster_ended
will
> be true, (though bytes_parsed might not be 0, see below) and a single repeat
> here would work. But the new append could be a sequence of at least one
cluster
> followed by an init segment, all at once. Some further peek-ahead to prevent
> attempt to parse init segment as a cluster is needed, I think, instead of
> blindly assuming in this loop that all of *data[0]..*data[size-1] is all media
> segment (cluster) data.

Done.

> 
> bytes_parsed might not be 0 if the previous append did not completely send all
> of an unknown-sized cluster, and the next append includes both the completion
of
> the first cluster and then some other data.

Yes. I think this code should handle that case correctly.

[Sergey Ulanov, 2014-06-13 23:44:44]

wolenetz: ping

[wolenetz, 2014-06-16 18:39:35]

Looking pretty good. Just a comment and a nit:

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

https://codereview.chromium.org/328653002/diff/60001/media/formats/webm/webm_...
media/formats/webm/webm_stream_parser.cc:284: }
Switch should have default case if not on enum. Also, ParseInfoAndTracks also
skips Cues and Chapters. To keep this ParseCluster() cleaner, ISTM better to
just replace this switch with:
if (id != kWebMIdCluster) {
  // Let ParseInfoAndTracks() handle all other level 1 element types.
  ChangeState(kParsingHeaders);
  return result;
}
and move matthewjheaney's TODO to the switch in ParseInfoAndTracks for now. This
allows similar coherent handling in one place of other level 0 and 1 elements
like SeekHead. Overall, it seems like there is really a fourth state that is
implicit beyond kWaitingForInit, kParsingHeaders and kParsingClusters, like
kFindingNextHeaderOrCluster. I'm fine with not including such an additional
state in this CL.

https://codereview.chromium.org/328653002/diff/60001/media/formats/webm/webm_...
media/formats/webm/webm_stream_parser.cc:321: // cluster. Try parsing again in
that case.
nit: s/"returns 0"..."cluster."/"returns 0 and |cluster_ended| is true if
previously parsing an unknown-size cluster and |data| does not continue that
cluster."/

[Sergey Ulanov, 2014-06-16 22:17:42]

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

https://codereview.chromium.org/328653002/diff/60001/media/formats/webm/webm_...
media/formats/webm/webm_stream_parser.cc:284: }
On 2014/06/16 18:39:35, wolenetz wrote:
> Switch should have default case if not on enum. Also, ParseInfoAndTracks also
> skips Cues and Chapters. To keep this ParseCluster() cleaner, ISTM better to
> just replace this switch with:
> if (id != kWebMIdCluster) {
>   // Let ParseInfoAndTracks() handle all other level 1 element types.
>   ChangeState(kParsingHeaders);
>   return result;
> }
> and move matthewjheaney's TODO to the switch in ParseInfoAndTracks for now.
This
> allows similar coherent handling in one place of other level 0 and 1 elements
> like SeekHead. Overall, it seems like there is really a fourth state that is
> implicit beyond kWaitingForInit, kParsingHeaders and kParsingClusters, like
> kFindingNextHeaderOrCluster. I'm fine with not including such an additional
> state in this CL.

Done.

https://codereview.chromium.org/328653002/diff/60001/media/formats/webm/webm_...
media/formats/webm/webm_stream_parser.cc:321: // cluster. Try parsing again in
that case.
On 2014/06/16 18:39:35, wolenetz wrote:
> nit: s/"returns 0"..."cluster."/"returns 0 and |cluster_ended| is true if
> previously parsing an unknown-size cluster and |data| does not continue that
> cluster."/

Done.

[wolenetz, 2014-06-16 22:38:27]

lgtm

[Sergey Ulanov, 2014-06-16 23:04:56]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-06-16 23:07:51]

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

[acolwell GONE FROM CHROMIUM, 2014-06-17 01:21:05]

lgtm

[Sergey Ulanov, 2014-06-17 03:04:00]

Committed patchset #5 manually as r277633 (presubmit successful).