Update WebMClusterParser to compute durations for all StreamParserBuffers.

Update WebMClusterParser to compute durations for all StreamParserBuffers.


BUG=127916
TEST=WebMClusterParserTest.*


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

[acolwell GONE FROM CHROMIUM, 2012-05-16 18:19:19]

- Introduced a per-stream single buffer delay so we can compute the duration of
all StreamParserBuffers.
- Cleaned up ChunkDemuxer tests a bit so they are more readable & fixed minor
breakages caused by the 1 buffer delay.

[scherkus (not reviewing), 2012-05-17 02:45:45]

few q's

http://codereview.chromium.org/10382200/diff/9/media/filters/chunk_demuxer_un...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/10382200/diff/9/media/filters/chunk_demuxer_un...
media/filters/chunk_demuxer_unittest.cc:184: LOG(ERROR) << "Test code tried to
append a zero length buffer!";
NOTREACHED() / LOG(FATAL) / CHECK() instead?

http://codereview.chromium.org/10382200/diff/9/media/webm/webm_cluster_parser.cc
File media/webm/webm_cluster_parser.cc (right):

http://codereview.chromium.org/10382200/diff/9/media/webm/webm_cluster_parser...
media/webm/webm_cluster_parser.cc:209: scoped_refptr<StreamParserBuffer>*
delayed_buffer;
I'm finding this pointer-to-a-scoped_refptr<T> to be overly clever. I was
searching to figure out where delayed_audio_buffer_ was being set and it took me
a while to realize that it's on line 256 where we deref the pointer

do we need to have two separate delayed buffers? would we ever mistakenly queue
a video buffer into an audio queue? perhaps it's worth duplicating some code
here for the sake of clarity

would refactoring {duration, queue, delayed_buffer} into a struct/class and
having an audio instance and a video instance help here?

WDYT?

http://codereview.chromium.org/10382200/diff/9/media/webm/webm_cluster_parser...
media/webm/webm_cluster_parser.cc:244: if (new_duration <= base::TimeDelta())
when would this happen? out of order timestamps?

[acolwell GONE FROM CHROMIUM, 2012-05-17 18:28:51]

http://codereview.chromium.org/10382200/diff/9/media/filters/chunk_demuxer_un...
File media/filters/chunk_demuxer_unittest.cc (right):

http://codereview.chromium.org/10382200/diff/9/media/filters/chunk_demuxer_un...
media/filters/chunk_demuxer_unittest.cc:184: LOG(ERROR) << "Test code tried to
append a zero length buffer!";
On 2012/05/17 02:45:46, scherkus wrote:
> NOTREACHED() / LOG(FATAL) / CHECK() instead?

Done.

http://codereview.chromium.org/10382200/diff/9/media/webm/webm_cluster_parser.cc
File media/webm/webm_cluster_parser.cc (right):

http://codereview.chromium.org/10382200/diff/9/media/webm/webm_cluster_parser...
media/webm/webm_cluster_parser.cc:209: scoped_refptr<StreamParserBuffer>*
delayed_buffer;
On 2012/05/17 02:45:46, scherkus wrote:
> I'm finding this pointer-to-a-scoped_refptr<T> to be overly clever. I was
> searching to figure out where delayed_audio_buffer_ was being set and it took
me
> a while to realize that it's on line 256 where we deref the pointer
> 
> do we need to have two separate delayed buffers? would we ever mistakenly
queue
> a video buffer into an audio queue? perhaps it's worth duplicating some code
> here for the sake of clarity
> 
> would refactoring {duration, queue, delayed_buffer} into a struct/class and
> having an audio instance and a video instance help here?
> 
> WDYT?

Excellent idea! I moved this all into a helper class and it is so much nicer. :)

http://codereview.chromium.org/10382200/diff/9/media/webm/webm_cluster_parser...
media/webm/webm_cluster_parser.cc:244: if (new_duration <= base::TimeDelta())
On 2012/05/17 02:45:46, scherkus wrote:
> when would this happen? out of order timestamps?
Yes. It can happen if a muxer creates a cluster with out of order timestamps or
per stream timestamps that are not monotonically increasing.

[scherkus (not reviewing), 2012-05-18 01:34:02]

lgtm w/ nits

http://codereview.chromium.org/10382200/diff/15001/media/webm/webm_cluster_pa...
File media/webm/webm_cluster_parser.cc (right):

http://codereview.chromium.org/10382200/diff/15001/media/webm/webm_cluster_pa...
media/webm/webm_cluster_parser.cc:212: buffer->SetDuration(duration);
nit: move this inside of block_duration >= 0 section and remove decl of
duration?

http://codereview.chromium.org/10382200/diff/15001/media/webm/webm_cluster_pa...
media/webm/webm_cluster_parser.cc:234: 
remove blank line

[acolwell GONE FROM CHROMIUM, 2012-05-18 04:02:16]

http://codereview.chromium.org/10382200/diff/15001/media/webm/webm_cluster_pa...
File media/webm/webm_cluster_parser.cc (right):

http://codereview.chromium.org/10382200/diff/15001/media/webm/webm_cluster_pa...
media/webm/webm_cluster_parser.cc:212: buffer->SetDuration(duration);
On 2012/05/18 01:34:02, scherkus wrote:
> nit: move this inside of block_duration >= 0 section and remove decl of
> duration?

Done. But it required adding a SetDuration(kNoTimestamp()) to the
StreamParserBuffer constructor. This will be good though because it will allow
us to quickly detect StreamParser implementations that forget to set the
duration.

http://codereview.chromium.org/10382200/diff/15001/media/webm/webm_cluster_pa...
media/webm/webm_cluster_parser.cc:234: 
On 2012/05/18 01:34:02, scherkus wrote:
> remove blank line

Done.

[commit-bot: I haz the power, 2012-05-18 04:06:30]

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

[commit-bot: I haz the power, 2012-05-18 05:19:42]

Change committed as 137826