MSE: Make WebMClusterParser hold back buffers at or beyond buffer missing duration

MSE: Make WebMClusterParser hold back buffers at or beyond buffer missing duration

If a buffer is missing duration and is being held back, then the cluster
parser now holds back all other tracks' buffers that have same or higher
(decode) timestamp. This keeps the timestamps emitted for a cluster
monotonically non-decreasing and in same order as parsed, even across
tracks within the cluster.

Adds a related new WebMClusterParserTest, updates related
ChunkDemuxerTests and removes an obsolete TODO now that bug 361786 is
fixed by r265340.

R=acolwell@chromium.org
BUG=363421
TEST=All media_unittests and MSE http layout tests pass locally on Linux

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

[wolenetz, 2014-04-15 23:34:42]

Patch set 1 is not ready for review yet, but I'm posting it as FYI/WIP. It does
contain a new test that is intended to verify the correct behavior without
relying on PipelineIntegrationTest on some WebM file to maybe catch the problem.

I'm continuing work; next patch set will include updated cluster parser for
which the new test passes.

[wolenetz, 2014-04-23 20:47:25]

PTAL @ patch set 3. Note that I've also tried patch set 4 of Dale's
https://codereview.chromium.org/222783007/ on top of this, and it no longer
fails any media_unittests on Linux for me.

Thanks!

[acolwell GONE FROM CHROMIUM, 2014-04-23 22:36:39]

looks pretty good. Just a few minor nits.

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

https://codereview.chromium.org/239343007/diff/40001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:1245: // buffer's timestamp (0.033s).
nit: What is the purpose of this comment? It doesn't appear to be relevant to
what is being tested. I'd say drop it since none of the code below appears to be
taking action based on this information.

https://codereview.chromium.org/239343007/diff/40001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:1776: // parser to determine durations
for each.
nit: I don't think it is worth having this here either since the audio
completing before the video isn't really the primary point of this test. The
primary goal is to make sure that buffers are incrementally handed out even if
the whole cluster isn't available.

https://codereview.chromium.org/239343007/diff/40001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:1779: EXPECT_FALSE(video_read_done);
nit: Remove since this test really shouldn't concern itself about the specific
order?

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
File media/formats/webm/webm_cluster_parser.cc (left):

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.cc:123: text_buffers_map_.clear();
Why move this code? It seems like this is more consistant with the audio & video
code.

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

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.cc:440: if (!buffers_.empty()) {
nit: reverse condition and early return.

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.cc:446: << before_timestamp.InSecondsF();
nit:early return.

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.cc:465: return ready_buffers_;
nit: looks like this is valuable at only one call site. Mind just calling
ready_buffers() there instead of returning here?

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.cc:659:
text_buffers_map_.insert(std::make_pair(itr->first, text_buffers));
Why are these staged here? I would expect these to be extracted at the same time
audio_ & video_ would.

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
File media/formats/webm/webm_cluster_parser.h (right):

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.h:52: // tracks being parsed by a
WebMClusterParser, the minimum value of their
nit: It seems like text from about here and below is TMI and could easily get
out of sync w/ the actual code.

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.h:60: // Extracts and returns
|ready_buffers_|, which are the initial portion of
nit: ditto. I think a smaller comment here would be more helpful and avoid this
text getting out of sync w/ the code.

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.h:123: // and cleared by Reset() and
ClearReadyBuffers().
nit: This comment doesn't really explain the difference between what is held in
buffers_ vs what is held in ready_buffers_. What does it mean to be "ready"?

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
File media/formats/webm/webm_cluster_parser_unittest.cc (right):

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser_unittest.cc:122: EXPECT_EQ(block_count,
buffer_count);
nit: put this in a DVLOG below since this condition will cause the test to fail
on the next 2 lines? It seems weird to double check it here.

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser_unittest.cc:151: EXPECT_LT(*offset,
buffers->size());
ditto

[wolenetz, 2014-04-25 20:04:21]

Thanks for comments. PTAL @ patch set 5.

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

https://codereview.chromium.org/239343007/diff/40001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:1245: // buffer's timestamp (0.033s).
On 2014/04/23 22:36:39, acolwell wrote:
> nit: What is the purpose of this comment? It doesn't appear to be relevant to
> what is being tested. I'd say drop it since none of the code below appears to
be
> taking action based on this information.

Done.

https://codereview.chromium.org/239343007/diff/40001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:1776: // parser to determine durations
for each.
On 2014/04/23 22:36:39, acolwell wrote:
> nit: I don't think it is worth having this here either since the audio
> completing before the video isn't really the primary point of this test. The
> primary goal is to make sure that buffers are incrementally handed out even if
> the whole cluster isn't available.

Done.

https://codereview.chromium.org/239343007/diff/40001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:1779: EXPECT_FALSE(video_read_done);
On 2014/04/23 22:36:39, acolwell wrote:
> nit: Remove since this test really shouldn't concern itself about the specific
> order?

Done.

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
File media/formats/webm/webm_cluster_parser.cc (left):

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.cc:123: text_buffers_map_.clear();
On 2014/04/23 22:36:39, acolwell wrote:
> Why move this code? It seems like this is more consistant with the audio &
video
> code.

This was just an optimization to iterate through the text track list just once
to setup the text map per each Parse...Get{*}Buffers... cycle.
UpdateReadyBuffers() needs to occur at most once per cycle, so that calculating
the upper bound and moving buffers into ready_buffers is done consistently
across all tracks. I could retain iteration here (to reconstruct the map, but
not redo extraction), and also retain iteration in UpdateReadyBuffers() (to
perform the once-per-cycle extraction). WDYT? (I've made this change to clarify
what this looks like.)

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

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.cc:440: if (!buffers_.empty()) {
On 2014/04/23 22:36:39, acolwell wrote:
> nit: reverse condition and early return.

Done.

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.cc:446: << before_timestamp.InSecondsF();
On 2014/04/23 22:36:39, acolwell wrote:
> nit:early return.

Done.

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.cc:465: return ready_buffers_;
On 2014/04/23 22:36:39, acolwell wrote:
> nit: looks like this is valuable at only one call site. Mind just calling
> ready_buffers() there instead of returning here?

Done.

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.cc:659:
text_buffers_map_.insert(std::make_pair(itr->first, text_buffers));
On 2014/04/23 22:36:39, acolwell wrote:
> Why are these staged here? I would expect these to be extracted at the same
time
> audio_ & video_ would.

Done (see my reply to GetTextBuffers() comment, above.)

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
File media/formats/webm/webm_cluster_parser.h (right):

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.h:52: // tracks being parsed by a
WebMClusterParser, the minimum value of their
On 2014/04/23 22:36:39, acolwell wrote:
> nit: It seems like text from about here and below is TMI and could easily get
> out of sync w/ the actual code.

Done.

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.h:60: // Extracts and returns
|ready_buffers_|, which are the initial portion of
On 2014/04/23 22:36:39, acolwell wrote:
> nit: ditto. I think a smaller comment here would be more helpful and avoid
this
> text getting out of sync w/ the code.

Done.

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.h:123: // and cleared by Reset() and
ClearReadyBuffers().
On 2014/04/23 22:36:39, acolwell wrote:
> nit: This comment doesn't really explain the difference between what is held
in
> buffers_ vs what is held in ready_buffers_. What does it mean to be "ready"?

Done.

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
File media/formats/webm/webm_cluster_parser_unittest.cc (right):

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser_unittest.cc:122: EXPECT_EQ(block_count,
buffer_count);
On 2014/04/23 22:36:39, acolwell wrote:
> nit: put this in a DVLOG below since this condition will cause the test to
fail
> on the next 2 lines? It seems weird to double check it here.

Done.

https://codereview.chromium.org/239343007/diff/40001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser_unittest.cc:151: EXPECT_LT(*offset,
buffers->size());
On 2014/04/23 22:36:39, acolwell wrote:
> ditto

Done.

[acolwell GONE FROM CHROMIUM, 2014-04-26 00:29:35]

lgtm

https://codereview.chromium.org/239343007/diff/70001/media/formats/webm/webm_...
File media/formats/webm/webm_cluster_parser.h (right):

https://codereview.chromium.org/239343007/diff/70001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.h:59: // Gets |ready_buffers_|.
nit: Remove. You can see what it does right here. :)

[wolenetz, 2014-04-26 00:37:38]

Thank you.

https://codereview.chromium.org/239343007/diff/70001/media/formats/webm/webm_...
File media/formats/webm/webm_cluster_parser.h (right):

https://codereview.chromium.org/239343007/diff/70001/media/formats/webm/webm_...
media/formats/webm/webm_cluster_parser.h:59: // Gets |ready_buffers_|.
On 2014/04/26 00:29:35, acolwell wrote:
> nit: Remove. You can see what it does right here. :)

Done.

[wolenetz, 2014-04-26 00:37:42]

The CQ bit was checked by wolenetz@chromium.org

[commit-bot: I haz the power, 2014-04-26 00:41:16]

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

[commit-bot: I haz the power, 2014-04-28 13:31:20]

Change committed as 266538