MSE: Use frame duration, if available, in LegacyFrameProcessor

MSE: Use frame duration, if available, in LegacyFrameProcessor

In preparation for testing the new compliant
coded frame processor (see bug 249422), this change modifies LegacyFrameProcessor and
ChunkDemuxer to use frame durations, if available. If not
available, SourceBufferStream's duration estimation logic is
used instead.
Updates relevant ChunkDemuxerTests and PipelineIntegrationTests.

BUG=351166, 249422
R=acolwell@chromium.org
TEST=All media_unittests pass locally on Linux with ffmpeg ChromeOS proprietary codecs enabled

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

[wolenetz, 2014-03-20 01:31:38]

PTAL. Much of this CL is required to get the tests to pass with the upcoming
compliant frame processor (that I expect to depend on this CL landing first).
I'll update the work-in-progress for that other CL tomorrow morning.

[wolenetz, 2014-03-20 02:16:15]

On 2014/03/20 01:31:38, wolenetz wrote:
> PTAL. Much of this CL is required to get the tests to pass with the upcoming
> compliant frame processor (that I expect to depend on this CL landing first).
> I'll update the work-in-progress for that other CL tomorrow morning.

Note that some layout tests will need to be updated. Without updates, at least
the following failures occur:

mediasource-buffered:
 FAIL Demuxed content with different lengths assert_equals:
mediaSource.activeSourceBuffers[0] expected "{ [0.000, 2.023) }" but got "{
[0.000, 2.044) }"
 FAIL Muxed tracks with different lengths assert_equals:
mediaSource.activeSourceBuffers[0] expected "{ [0.000, 2.023) }" but got "{
[0.000, 2.044) }"
 FAIL Demuxed content with an empty buffered range on one SourceBuffer
assert_equals: mediaSource.activeSourceBuffers[0] expected "{ [0.000, 2.023) }"
but got "{ [0.000, 2.044) }"

mediasource-remove:
 FAIL Test removing all appended data. assert_equals: Initial buffered range.
expected "{ [0.000, 6.042) }" but got "{ [0.000, 6.043) }"
 FAIL Test removing beginning of appended data. assert_equals: Initial buffered
range. expected "{ [0.000, 6.042) }" but got "{ [0.000, 6.043) }"
 FAIL Test removing the middle of appended data. assert_equals: Initial buffered
range. expected "{ [0.000, 6.042) }" but got "{ [0.000, 6.043) }"
 FAIL Test removing the end of appended data. assert_equals: Initial buffered
range. expected "{ [0.000, 6.042) }" but got "{ [0.000, 6.043) }"

webkitmediasource-duration-changed:
 Unexpected duration. Expected 1.593 got 1.594 FAIL
 (also, test is truncated due to this failure, so there could be other issues)

mediasource-duration:
 FAIL Test seek starts on duration truncation below currentTime assert_equals:
mediaElement fullDuration expected 6.042 but got 6.051
 FAIL Test appendBuffer completes previous seek to truncated duration
assert_equals: mediaElement fullDuration expected 6.042 but got 6.051
 FAIL Test endOfStream completes previous seek to truncated duration
assert_equals: mediaElement fullDuration expected 6.042 but got 6.051
 FAIL Test setting same duration multiple times does not fire duplicate
durationchange assert_equals: mediaElement fullDuration expected 6.042 but got
6.051

[acolwell GONE FROM CHROMIUM, 2014-03-21 15:50:14]

lgtm % comments

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

https://codereview.chromium.org/205703003/diff/1/media/filters/chunk_demuxer_...
media/filters/chunk_demuxer_unittest.cc:692: if (track_number == kVideoTrackNum)
{
heh.. this is a left over from my previous attempt at making the parser generate
durations.

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

https://codereview.chromium.org/205703003/diff/1/media/filters/chunk_demuxer_...
media/filters/chunk_demuxer_unittest.cc:574: EXPECT_CALL(host_,
SetDuration(base::TimeDelta::FromMilliseconds(2744)));
What triggers this? Does this need to be here or can it be moved closer to the
code that triggers it?

https://codereview.chromium.org/205703003/diff/1/media/filters/chunk_demuxer_...
media/filters/chunk_demuxer_unittest.cc:3037: // window and extend beyond the
end of the window are not included.
Why did this change? Is this an artifact of supporting step 1.10 of the coded
frame processing algorithm?

https://codereview.chromium.org/205703003/diff/1/media/filters/legacy_frame_p...
File media/filters/legacy_frame_processor.cc (right):

https://codereview.chromium.org/205703003/diff/1/media/filters/legacy_frame_p...
media/filters/legacy_frame_processor.cc:217: // TODO(wolenetz, acolwell):
Directly calculate this value from buffers in
nit: Should this TODO really be here. This sounds like it is talking about how
the other CFP should be behaving. That doesn't seem like a TODO for this code.

[wolenetz, 2014-03-21 18:14:00]

Thanks. PTAL @ PS3. I'm not sure all your previous comments were nits :). Note
also my earlier comment that several mediasource layout tests will be impacted
(fail) due to this change (underlying files probably need init segment duration
updated, and test duration metadata/expectations need updating).

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

https://codereview.chromium.org/205703003/diff/1/media/filters/chunk_demuxer_...
media/filters/chunk_demuxer_unittest.cc:574: EXPECT_CALL(host_,
SetDuration(base::TimeDelta::FromMilliseconds(2744)));
On 2014/03/21 15:50:15, acolwell wrote:
> What triggers this? Does this need to be here or can it be moved closer to the
> code that triggers it?

The file has 2744 encoded in its init segment. Parsing the init segment causes
SetDuration(2744) (which needs to be expected). However, the full file append,
along with the usage of parser's frame duration or estimate, causes the duration
to be not just the highest presentation timestamp, but increased to the
HPET+frame duration (==2768 with this change's usage of
SBS::GetBufferedDuration() to estimate the full duration). Bug 354284 tracks
getting the file fixed. Note, fixing that bug and the expectations here will
likely be impacted by change(s) we're working on to get WebM stream parser to
emit at least an estimated frame duration (part of fixing bug 351166), so that
the coded frame processor can use it to directly calculate the duration increase
instead of SBS::GetBufferedDuration().

Done, here and in similar places elsewhere in this file that reference
http://crbug.com/354284.

https://codereview.chromium.org/205703003/diff/1/media/filters/chunk_demuxer_...
media/filters/chunk_demuxer_unittest.cc:3037: // window and extend beyond the
end of the window are not included.
On 2014/03/21 15:50:15, acolwell wrote:
> Why did this change? Is this an artifact of supporting step 1.10 of the coded
> frame processing algorithm?

Yes. I included that step's support in this change. Previously, only the frame
PTS was compared to append window end. Now, if there is a frame duration, frame
PTS+duration is compared to append window end. And the tests here now always
provide duration.

Window is [20,280]. Audio duration is 23ms. Audio frames appended at 270,300,330
are dropped. Similar occurs for video and text in AppendWindow_Text, below.

https://codereview.chromium.org/205703003/diff/1/media/filters/legacy_frame_p...
File media/filters/legacy_frame_processor.cc (right):

https://codereview.chromium.org/205703003/diff/1/media/filters/legacy_frame_p...
media/filters/legacy_frame_processor.cc:217: // TODO(wolenetz, acolwell):
Directly calculate this value from buffers in
On 2014/03/21 15:50:15, acolwell wrote:
> nit: Should this TODO really be here. This sounds like it is talking about how
> the other CFP should be behaving. That doesn't seem like a TODO for this code.

Done.

[wolenetz, 2014-03-21 19:16:30]

On 2014/03/21 18:14:00, wolenetz wrote:
> Thanks. PTAL @ PS3. I'm not sure all your previous comments were nits :). Note
> also my earlier comment that several mediasource layout tests will be impacted
> (fail) due to this change (underlying files probably need init segment
duration
> updated, and test duration metadata/expectations need updating).
> 
>
https://codereview.chromium.org/205703003/diff/1/media/filters/chunk_demuxer_...
> File media/filters/chunk_demuxer_unittest.cc (right):
> 
>
https://codereview.chromium.org/205703003/diff/1/media/filters/chunk_demuxer_...
> media/filters/chunk_demuxer_unittest.cc:574: EXPECT_CALL(host_,
> SetDuration(base::TimeDelta::FromMilliseconds(2744)));
> On 2014/03/21 15:50:15, acolwell wrote:
> > What triggers this? Does this need to be here or can it be moved closer to
the
> > code that triggers it?
> 
> The file has 2744 encoded in its init segment. Parsing the init segment causes
> SetDuration(2744) (which needs to be expected). However, the full file append,
> along with the usage of parser's frame duration or estimate, causes the
duration
> to be not just the highest presentation timestamp, but increased to the
> HPET+frame duration (==2768 with this change's usage of
> SBS::GetBufferedDuration() to estimate the full duration). Bug 354284 tracks
> getting the file fixed. Note, fixing that bug and the expectations here will
> likely be impacted by change(s) we're working on to get WebM stream parser to
> emit at least an estimated frame duration (part of fixing bug 351166), so that
> the coded frame processor can use it to directly calculate the duration
increase
> instead of SBS::GetBufferedDuration().
> 
> Done, here and in similar places elsewhere in this file that reference
> http://crbug.com/354284.
> 
>
https://codereview.chromium.org/205703003/diff/1/media/filters/chunk_demuxer_...
> media/filters/chunk_demuxer_unittest.cc:3037: // window and extend beyond the
> end of the window are not included.
> On 2014/03/21 15:50:15, acolwell wrote:
> > Why did this change? Is this an artifact of supporting step 1.10 of the
coded
> > frame processing algorithm?
> 
> Yes. I included that step's support in this change. Previously, only the frame
> PTS was compared to append window end. Now, if there is a frame duration,
frame
> PTS+duration is compared to append window end. And the tests here now always
> provide duration.
> 
> Window is [20,280]. Audio duration is 23ms. Audio frames appended at
270,300,330
> are dropped. Similar occurs for video and text in AppendWindow_Text, below.
> 
>
https://codereview.chromium.org/205703003/diff/1/media/filters/legacy_frame_p...
> File media/filters/legacy_frame_processor.cc (right):
> 
>
https://codereview.chromium.org/205703003/diff/1/media/filters/legacy_frame_p...
> media/filters/legacy_frame_processor.cc:217: // TODO(wolenetz, acolwell):
> Directly calculate this value from buffers in
> On 2014/03/21 15:50:15, acolwell wrote:
> > nit: Should this TODO really be here. This sounds like it is talking about
how
> > the other CFP should be behaving. That doesn't seem like a TODO for this
code.
> 
> Done.

I chatted offline w/acolwell@. PS3 is lgth, though I'm working to hopefully
reduce layout test and media unittest MSE init segment / expectation churn by
getting a partial fix for http://crbug.com/351166 landed before this CL lands.

[wolenetz, 2014-03-28 02:04:28]

Please take another look due to the rebase, etc now in PS4.

Note: The following must happen in sequence before this CL can land (besides it
lgty again):
1) Blink roll to pull in
https://src.chromium.org/viewvc/blink?view=rev&revision=170209
2) Land Chromium CL https://codereview.chromium.org/213253006/
3) Land and roll Blink CL https://codereview.chromium.org/215863002/

Thanks!

[wolenetz, 2014-03-28 17:41:41]

On 2014/03/28 02:04:28, wolenetz wrote:
> Please take another look due to the rebase, etc now in PS4.
> 
> Note: The following must happen in sequence before this CL can land (besides
it
> lgty again):
> 1) Blink roll to pull in
> https://src.chromium.org/viewvc/blink?view=rev&revision=170209
> 2) Land Chromium CL https://codereview.chromium.org/213253006/
> 3) Land and roll Blink CL https://codereview.chromium.org/215863002/
> 
> Thanks!

Also, I will pend landing this on:
4) Discuss optimal landing time with acolwell@.

[acolwell GONE FROM CHROMIUM, 2014-03-28 21:01:34]

still lgtm % question.

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

https://codereview.chromium.org/205703003/diff/60001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:3170: "0K 30 60 90K 120K 150 180 210");
Why did this become a keyframe?

[wolenetz, 2014-03-28 22:17:43]

Thank you.
Still pending:
3) Land and roll Blink CL https://codereview.chromium.org/215863002/
4) ETA for sending to CQ: early next week.

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

https://codereview.chromium.org/205703003/diff/60001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:3170: "0K 30 60 90K 120K 150 180 210");
On 2014/03/28 21:01:34, acolwell wrote:
> Why did this become a keyframe?

Thanks for catching that. It slipped through the recent rebase. In an earlier CL
it was necessary, since the seek incorrectly went to an "overlapping 90-123"
transitively back to the 0K-33 frame. Intra-cluster frame durations are now
based on track's frame timestamp deltas if not otherwise specified (such as in
the test simpleblocks), so there is no video frame overlapping the 120K seek
target.

[wolenetz, 2014-03-31 21:03:29]

The CQ bit was checked by wolenetz@chromium.org

[commit-bot: I haz the power, 2014-03-31 21:04:18]

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

[commit-bot: I haz the power, 2014-04-01 00:37:44]

Change committed as 260719