MSE: Populate WebM missing duration with DefaultDuration, derived, or default

MSE: Populate WebM missing duration with DefaultDuration, derived, or default

This change modifes WebM stream parser to estimate missing WebM frame
durations using the following logic:
1) If the frame was a BlockGroup, it should already have a duration. Use
it. (This is previous behavior; no other WebM frames previously had
duration.)
2) If the frame was a SimpleBlock, derive its duration as follows:
2a) If the frame's TrackEntry had a DefaultDuration, use that value
capped at a precision no greater than TimeCodes with TimeCodeScale
applied.
2b) Otherwise, if there is a subsequent frame in the cluster, set the
duration to the difference in timestamps.
2c) Otherwise, use the maximum frame duration for the track encountered
so far, if any.
2d) Otherwise, use a hardcoded value.
Note, 2b-2c, for WebM audio, ideally would be calculated based on the
track's codebook. This is left for a future CL.

Adds related WebM track parser unit tests. Adjusts
existing ChunkDemuxerTests, PipelineIntegrationTests, and
WebMClusterParserTests based on new duration logic.

R=acolwell@chromium.org
TEST=All media_unittests and http/tests/media layout tests pass locally on Linux
BUG=351166
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260247

[wolenetz, 2014-03-26 22:14:44]

PTAL (acolwell@: everything, dalecurtis@: FYI).

This change requires https://codereview.chromium.org/213153008/ to land first.

A Blink MediaSource change will be required to similarly update layout test
expectations. See https://codereview.chromium.org/212593007.

Note: This change currently disables AudioSplicer frame generation in
SourceBufferStream (and assocated SourceBufferStreamTests) due to bug 356805.
Note: https://codereview.chromium.org/213473004/ may land first and already have
disabled AudioSplicer frame generation. If it were re-enabled, I found I would
also need to change a DCHECK in audio_splicer.cc that was getting hit by a test,
and add some fairly large number of kConfigChange expectations to
ConfigChange_Audio test due to many overlapping appends causing splicer config
changes. I've removed all of those work-arounds from this CL due to disabling
GenerateSpliceFrame().

[DaleCurtis, 2014-03-26 22:18:20]

Splicing requires exact durations, so even guesses are not good enough. In
practice this may mean we need to disable splice frame generation fro WebM until
durations are completely accurate.

[acolwell GONE FROM CHROMIUM, 2014-03-26 22:41:39]

On 2014/03/26 22:18:20, DaleCurtis wrote:
> Splicing requires exact durations, so even guesses are not good enough. In
> practice this may mean we need to disable splice frame generation fro WebM
until
> durations are completely accurate.

It is unlikely that this will be 100% possible especially since durations that
are actually encoded with individual blocks, are in timecode scale units (ie
milliseconds for WebM). Splicing will need to be able to tolerate slight
inaccuracies in the durations because the container doesn't always provide
accurate enough info. In general, I think the splicing code needs to be able to
handle inaccuracies on the order of 1-2 milliseconds.

[DaleCurtis, 2014-03-26 23:29:26]

I'm not sure that's practical when talking about a duration of 5ms for splices.
In any case, I think the splicer will work fine, but I don't see a way around
the config changes and splice frame generation.

[acolwell GONE FROM CHROMIUM, 2014-03-26 23:32:04]

looks pretty good. Just a few comments.

https://codereview.chromium.org/213253006/diff/1/media/filters/chunk_demuxer_...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/213253006/diff/1/media/filters/chunk_demuxer_...
media/filters/chunk_demuxer_unittest.cc:573: CheckExpectedRanges(kSourceId, "{
[0,2736) }");
nit: update values in comment above.

https://codereview.chromium.org/213253006/diff/1/media/filters/source_buffer_...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/213253006/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.cc:970: //
ChunkDemuxerTest.OutOfOrderClusters when all frames in a cluster have
nit: Remove comment since Dale has already disabled the generation for now and I
believe he's aware of the bug mentioned here.

https://codereview.chromium.org/213253006/diff/1/media/formats/webm/webm_clus...
File media/formats/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/213253006/diff/1/media/formats/webm/webm_clus...
media/formats/webm/webm_cluster_parser.cc:22: static int
kDefaultBufferDurationInMs = 50;
nit: If we are assuming this will be audio, then I think 23ms would be better.
This is equivalent to 1024 samples @ 44100, which is a common frame size for the
bitrates we care about.

https://codereview.chromium.org/213253006/diff/1/media/formats/webm/webm_clus...
media/formats/webm/webm_cluster_parser.cc:70: audio_.Flush();
nit: I think Reset() should be "clear everything" to be consistent w/ this
method's name. We should rename the Reset() you have to ClearBufferQueues() or
something like that so that it is clearer that not everything is cleared.

https://codereview.chromium.org/213253006/diff/1/media/formats/webm/webm_clus...
media/formats/webm/webm_cluster_parser.cc:470: if (buffer->duration() <
base::TimeDelta()) {
nit: Looks like we could avoid some duplicate code if we had a QueueBuffer()
method that did the duration checks, push_back(), and
estimated_next_frame_duration_ updates.

[wolenetz, 2014-03-27 18:01:48]

PTAL @ PS3. Thanks!

https://codereview.chromium.org/213253006/diff/1/media/filters/chunk_demuxer_...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/213253006/diff/1/media/filters/chunk_demuxer_...
media/filters/chunk_demuxer_unittest.cc:573: CheckExpectedRanges(kSourceId, "{
[0,2736) }");
On 2014/03/26 23:32:04, acolwell wrote:
> nit: update values in comment above.

Done.

https://codereview.chromium.org/213253006/diff/1/media/filters/source_buffer_...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/213253006/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.cc:970: //
ChunkDemuxerTest.OutOfOrderClusters when all frames in a cluster have
On 2014/03/26 23:32:04, acolwell wrote:
> nit: Remove comment since Dale has already disabled the generation for now and
I
> believe he's aware of the bug mentioned here.

Done (and rebased onto ToT that now includes Dale's
https://codereview.chromium.org/213283003/ that disables GenerateSpliceFrame()
outside of scoped tests.

https://codereview.chromium.org/213253006/diff/1/media/filters/source_buffer_...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/213253006/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream_unittest.cc:3625: // http://crbug.com/356805.
Likewise for these tests, I'm undoing my disabling in this CL and have rebased
onto ToT that has https://codereview.chromium.org/213283003/

https://codereview.chromium.org/213253006/diff/1/media/formats/webm/webm_clus...
File media/formats/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/213253006/diff/1/media/formats/webm/webm_clus...
media/formats/webm/webm_cluster_parser.cc:22: static int
kDefaultBufferDurationInMs = 50;
On 2014/03/26 23:32:04, acolwell wrote:
> nit: If we are assuming this will be audio, then I think 23ms would be better.
> This is equivalent to 1024 samples @ 44100, which is a common frame size for
the
> bitrates we care about.

Done. I've now differentiated audio vs video default buffer durations.

https://codereview.chromium.org/213253006/diff/1/media/formats/webm/webm_clus...
media/formats/webm/webm_cluster_parser.cc:70: audio_.Flush();
On 2014/03/26 23:32:04, acolwell wrote:
> nit: I think Reset() should be "clear everything" to be consistent w/ this
> method's name. We should rename the Reset() you have to ClearBufferQueues() or
> something like that so that it is clearer that not everything is cleared.

Done.

https://codereview.chromium.org/213253006/diff/1/media/formats/webm/webm_clus...
media/formats/webm/webm_cluster_parser.cc:470: if (buffer->duration() <
base::TimeDelta()) {
On 2014/03/26 23:32:04, acolwell wrote:
> nit: Looks like we could avoid some duplicate code if we had a QueueBuffer()
> method that did the duration checks, push_back(), and
> estimated_next_frame_duration_ updates.

Done.

[wolenetz, 2014-03-27 18:02:54]

On 2014/03/27 18:01:48, wolenetz wrote:
> PTAL @ PS3. Thanks!
> 
>
https://codereview.chromium.org/213253006/diff/1/media/filters/chunk_demuxer_...
> File media/filters/chunk_demuxer_unittest.cc (right):
> 
>
https://codereview.chromium.org/213253006/diff/1/media/filters/chunk_demuxer_...
> media/filters/chunk_demuxer_unittest.cc:573: CheckExpectedRanges(kSourceId, "{
> [0,2736) }");
> On 2014/03/26 23:32:04, acolwell wrote:
> > nit: update values in comment above.
> 
> Done.
> 
>
https://codereview.chromium.org/213253006/diff/1/media/filters/source_buffer_...
> File media/filters/source_buffer_stream.cc (right):
> 
>
https://codereview.chromium.org/213253006/diff/1/media/filters/source_buffer_...
> media/filters/source_buffer_stream.cc:970: //
> ChunkDemuxerTest.OutOfOrderClusters when all frames in a cluster have
> On 2014/03/26 23:32:04, acolwell wrote:
> > nit: Remove comment since Dale has already disabled the generation for now
and
> I
> > believe he's aware of the bug mentioned here.
> 
> Done (and rebased onto ToT that now includes Dale's
> https://codereview.chromium.org/213283003/ that disables GenerateSpliceFrame()
> outside of scoped tests.
> 
>
https://codereview.chromium.org/213253006/diff/1/media/filters/source_buffer_...
> File media/filters/source_buffer_stream_unittest.cc (right):
> 
>
https://codereview.chromium.org/213253006/diff/1/media/filters/source_buffer_...
> media/filters/source_buffer_stream_unittest.cc:3625: //
http://crbug.com/356805.
> Likewise for these tests, I'm undoing my disabling in this CL and have rebased
> onto ToT that has https://codereview.chromium.org/213283003/
> 
>
https://codereview.chromium.org/213253006/diff/1/media/formats/webm/webm_clus...
> File media/formats/webm/webm_cluster_parser.cc (right):
> 
>
https://codereview.chromium.org/213253006/diff/1/media/formats/webm/webm_clus...
> media/formats/webm/webm_cluster_parser.cc:22: static int
> kDefaultBufferDurationInMs = 50;
> On 2014/03/26 23:32:04, acolwell wrote:
> > nit: If we are assuming this will be audio, then I think 23ms would be
better.
> > This is equivalent to 1024 samples @ 44100, which is a common frame size for
> the
> > bitrates we care about.
> 
> Done. I've now differentiated audio vs video default buffer durations.
> 
>
https://codereview.chromium.org/213253006/diff/1/media/formats/webm/webm_clus...
> media/formats/webm/webm_cluster_parser.cc:70: audio_.Flush();
> On 2014/03/26 23:32:04, acolwell wrote:
> > nit: I think Reset() should be "clear everything" to be consistent w/ this
> > method's name. We should rename the Reset() you have to ClearBufferQueues()
or
> > something like that so that it is clearer that not everything is cleared.
> 
> Done.
> 
>
https://codereview.chromium.org/213253006/diff/1/media/formats/webm/webm_clus...
> media/formats/webm/webm_cluster_parser.cc:470: if (buffer->duration() <
> base::TimeDelta()) {
> On 2014/03/26 23:32:04, acolwell wrote:
> > nit: Looks like we could avoid some duplicate code if we had a QueueBuffer()
> > method that did the duration checks, push_back(), and
> > estimated_next_frame_duration_ updates.
> 
> Done.

s/PS3/PS2/... (PTAL @ PS2) Thanks!

[acolwell GONE FROM CHROMIUM, 2014-03-27 18:28:09]

lgtm.

You might want to land a temporary TestExpectations change that allows
appropriate LayoutTests to fail before landing this change so it doesn't get
reverted because of LayoutTest failures.

https://codereview.chromium.org/213253006/diff/20001/media/filters/pipeline_i...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/213253006/diff/20001/media/filters/pipeline_i...
media/filters/pipeline_integration_test.cc:1135:
base::TimeDelta::FromMilliseconds(167),
This changed because we are in the middle of a cluster and 1 buffer is being
held back?

https://codereview.chromium.org/213253006/diff/20001/media/formats/webm/webm_...
File media/formats/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/213253006/diff/20001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.cc:23: static int
kDefaultVideoBufferDurationInMs = 50;  // Larger may reduce stalls
nit: This implies 20fps which is pretty uncommon. 33 for 30fps, 42 for 24fps, or
40 for 25fps would be much more common and better blind guesses.

https://codereview.chromium.org/213253006/diff/20001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.cc:472: if (default_duration_ !=
kNoTimestamp()) {
nit: Move if chain to a helper function so we only have one set_duration() call
in this method? Either that or use a local var here to make the code a little
clearer/shorter.

[wolenetz, 2014-03-27 19:56:39]

Thank you.
I'll send this to CQ once the Blink-side
(https://codereview.chromium.org/212593007/) TestExpectations update has landed
and Blink has rolled.

https://codereview.chromium.org/213253006/diff/20001/media/filters/pipeline_i...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/213253006/diff/20001/media/filters/pipeline_i...
media/filters/pipeline_integration_test.cc:1135:
base::TimeDelta::FromMilliseconds(167),
On 2014/03/27 18:28:10, acolwell wrote:
> This changed because we are in the middle of a cluster and 1 buffer is being
> held back?

Correct.
...
Video buffer ts=167ms appended, held back due to no duration.
Video buffer ts=200ms appended, allowed previous buffer to get added to result
of parse, but 200ms buffer is held back due to no duration.
Video buffer ts=234ms appended, allowed previous buffer to get added to result
of parse, but 234ms buffer is held back due to no duration. Also, cluster is not
yet ended, so 234ms buffer remains held-back.
Test times out while waiting for current time to reach 200ms. I suspect there's
already a built-in 1-videobuffer delay elsewhere in the pipeline such that the
timestamp before this change wouldn't reach 234ms (I've confirmed that ToT times
out if waiting for 201ms instead of 200ms). This change introduces an extra
1-buffer delay when end-of-cluster has not yet been reached for webm.

https://codereview.chromium.org/213253006/diff/20001/media/formats/webm/webm_...
File media/formats/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/213253006/diff/20001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.cc:23: static int
kDefaultVideoBufferDurationInMs = 50;  // Larger may reduce stalls
On 2014/03/27 18:28:10, acolwell wrote:
> nit: This implies 20fps which is pretty uncommon. 33 for 30fps, 42 for 24fps,
or
> 40 for 25fps would be much more common and better blind guesses.

Ok. I'll use the longer of these (40) to help prevent stalls if the first ever
video frame parsed for a track is an end-of-cluster-simpleblock-frame and the
trackentry had no DefaultDuration, and the video framerate is low (25fps).

https://codereview.chromium.org/213253006/diff/20001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.cc:472: if (default_duration_ !=
kNoTimestamp()) {
On 2014/03/27 18:28:10, acolwell wrote:
> nit: Move if chain to a helper function so we only have one set_duration()
call
> in this method? Either that or use a local var here to make the code a little
> clearer/shorter.

Done (with a helper function).

[wolenetz, 2014-03-27 19:59:05]

On 2014/03/27 19:56:39, wolenetz wrote:
> Thank you.
> I'll send this to CQ once the Blink-side
> (https://codereview.chromium.org/212593007/) TestExpectations update has
landed
> and Blink has rolled.
> 
>
https://codereview.chromium.org/213253006/diff/20001/media/filters/pipeline_i...
> File media/filters/pipeline_integration_test.cc (right):
> 
>
https://codereview.chromium.org/213253006/diff/20001/media/filters/pipeline_i...
> media/filters/pipeline_integration_test.cc:1135:
> base::TimeDelta::FromMilliseconds(167),
> On 2014/03/27 18:28:10, acolwell wrote:
> > This changed because we are in the middle of a cluster and 1 buffer is being
> > held back?
> 
> Correct.
> ...
> Video buffer ts=167ms appended, held back due to no duration.
> Video buffer ts=200ms appended, allowed previous buffer to get added to result
> of parse, but 200ms buffer is held back due to no duration.
> Video buffer ts=234ms appended, allowed previous buffer to get added to result
> of parse, but 234ms buffer is held back due to no duration. Also, cluster is
not
> yet ended, so 234ms buffer remains held-back.
> Test times out while waiting for current time to reach 200ms. I suspect
there's
> already a built-in 1-videobuffer delay elsewhere in the pipeline such that the
> timestamp before this change wouldn't reach 234ms (I've confirmed that ToT
times
> out if waiting for 201ms instead of 200ms). This change introduces an extra
> 1-buffer delay when end-of-cluster has not yet been reached for webm.
> 
>
https://codereview.chromium.org/213253006/diff/20001/media/formats/webm/webm_...
> File media/formats/webm/webm_cluster_parser.cc (right):
> 
>
https://codereview.chromium.org/213253006/diff/20001/media/formats/webm/webm_...
> media/formats/webm/webm_cluster_parser.cc:23: static int
> kDefaultVideoBufferDurationInMs = 50;  // Larger may reduce stalls
> On 2014/03/27 18:28:10, acolwell wrote:
> > nit: This implies 20fps which is pretty uncommon. 33 for 30fps, 42 for
24fps,
> or
> > 40 for 25fps would be much more common and better blind guesses.
> 
> Ok. I'll use the longer of these (40) to help prevent stalls if the first ever
> video frame parsed for a track is an end-of-cluster-simpleblock-frame and the
> trackentry had no DefaultDuration, and the video framerate is low (25fps).
> 
>
https://codereview.chromium.org/213253006/diff/20001/media/formats/webm/webm_...
> media/formats/webm/webm_cluster_parser.cc:472: if (default_duration_ !=
> kNoTimestamp()) {
> On 2014/03/27 18:28:10, acolwell wrote:
> > nit: Move if chain to a helper function so we only have one set_duration()
> call
> > in this method? Either that or use a local var here to make the code a
little
> > clearer/shorter.
> 
> Done (with a helper function).

Hmm. I just noticed 42ms for 24fps is lower. I'll make that one change to
kDefaultVideoBufferDurationInMs (to use 42ms) in next patch set.

[wolenetz, 2014-03-27 20:02:27]

Ok. I'll send PS4 to CQ once the Blink-side
(https://codereview.chromium.org/212593007/) TestExpectations update has landed
and Blink has rolled.

[wolenetz, 2014-03-27 20:06:34]

On 2014/03/27 20:02:27, wolenetz wrote:
> Ok. I'll send PS4 to CQ once the Blink-side
> (https://codereview.chromium.org/212593007/) TestExpectations update has
landed
> and Blink has rolled.

Offline, I confirmed with dalecurtis@ (fyi reviewer) that no pending review
reply from him is necessary to get this landed.

[wolenetz, 2014-03-28 17:13:10]

On 2014/03/27 20:06:34, wolenetz wrote:
> On 2014/03/27 20:02:27, wolenetz wrote:
> > Ok. I'll send PS4 to CQ once the Blink-side
> > (https://codereview.chromium.org/212593007/) TestExpectations update has
> landed
> > and Blink has rolled.
> 
> Offline, I confirmed with dalecurtis@ (fyi reviewer) that no pending review
> reply from him is necessary to get this landed.

Blink-side (https://codereview.chromium.org/212593007/) TestExpectations update
has landed (https://src.chromium.org/viewvc/blink?view=rev&revision=170209) and
Blink has rolled
(http://src.chromium.org/viewvc/chrome?view=revision&revision=260086).

I'm sending this to CQ now.

[wolenetz, 2014-03-28 17:13:26]

The CQ bit was checked by wolenetz@chromium.org

[commit-bot: I haz the power, 2014-03-28 17:17:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/213253006/60001

[commit-bot: I haz the power, 2014-03-28 20:03:58]

Change committed as 260247