Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1491513002:
[MSE] Fix GC with media_time past the last appended buffer timestamp (Closed)
Description[MSE] Fix GC with media_time past the last appended buffer timestamp
Previously MSE GC algorithm didn't take into account properly the
duration of the last appended buffer. As a result GC would fail when the
media_time was beyond the timestamp of the last appended buffer. This CL
fixes that and adds a new unit test case to cover this situation.
BUG=563292
Committed: https://crrev.com/3ec8ac57b3788dd43da901a05ec99a4364e088bf
Cr-Commit-Position: refs/heads/master@{#363002}
Patch Set 1 #
Total comments: 6
Patch Set 2 : cr feedback #
Total comments: 1
Patch Set 3 : Implement media_time clamping to buffered ranges #
Messages
Total messages: 18 (6 generated)
wolenetz@chromium.org changed reviewers: + wolenetz@chromium.org
Just nits so far. I assume the clamping logic will become part of this CL in next PS. https://codereview.chromium.org/1491513002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1491513002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream.cc:143: last_appended_buffer_duration_(kNoTimestamp()), nit: some of these common initializers can now happen in .h instead to clean up these ctor impls. https://codereview.chromium.org/1491513002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1491513002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:3073: // the last appended buffer (330), but lower than the end of buffered ranges nit: s/lower/within/ in this comment block? (see my s/350/360 nit below) https://codereview.chromium.org/1491513002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:3077: DecodeTimestamp::FromMilliseconds(350), 0)); nit: s/350/360 to include the very edge of last appended frame's end.
Description was changed from ========== SBS GC fix BUG=563292 ========== to ========== [MSE] Fix GC with media_time past the last appended buffer pts Previously MSE GC algorithm didn't take into account properly the duration of the last appended buffer. As a result GC would fail when the media_time was beyond the pts of the last appended buffer. This CL fixes that and adds a new unit test case to cover this situation. BUG=563292 ==========
https://codereview.chromium.org/1491513002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1491513002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream.cc:143: last_appended_buffer_duration_(kNoTimestamp()), On 2015/12/01 01:12:43, wolenetz wrote: > nit: some of these common initializers can now happen in .h instead to clean up > these ctor impls. Done. https://codereview.chromium.org/1491513002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1491513002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:3073: // the last appended buffer (330), but lower than the end of buffered ranges On 2015/12/01 01:12:43, wolenetz wrote: > nit: s/lower/within/ in this comment block? (see my s/350/360 nit below) Done. https://codereview.chromium.org/1491513002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:3077: DecodeTimestamp::FromMilliseconds(350), 0)); On 2015/12/01 01:12:43, wolenetz wrote: > nit: s/350/360 to include the very edge of last appended frame's end. Done.
Per chat w/servolk@, I'll take over the work on this fix soon today.
On 2015/12/01 21:07:01, wolenetz wrote: > Per chat w/servolk@, I'll take over the work on this fix soon today. Sorry, crbug.com is currently down, so I can't update the bug. But I think this could be a second part of the fix (logically this is a different change, so I've decided to make a separate CL for now, but we can merge with this one if you prefer): https://codereview.chromium.org/1491813002/ That CL will clamp currentTime used for MSE GC to the SB.buffered ranges.
To reduce latency of potentially merging the fix for the bug to release branches, I recommend including the clamping logic in this as a single CL. This CL LG % description-=PTS, and inclusion of clamping and clamping testing https://codereview.chromium.org/1491513002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.h (right): https://codereview.chromium.org/1491513002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.h:409: DecodeTimestamp last_appended_buffer_timestamp_ = kNoDecodeTimestamp(); Note that this is a DTS, not a PTS (pre-existing problem: see crbugs with label:MSEptsdtsCleanup for this known issue). media_time is correctly PTS. Buffered ranges and last_appendeded incorrectly are based on DTS. Perhaps adjust the CL description to not mention PTS, so that it doesn't confuse readers. Note that DTS usually is < PTS, so the clamping CL follow-up is motivated not just for Chromium pipeline allowing audio clock to go slightly past end of buffered video, it's also motivated for clamping a slightly higher PTS into the (incorrectly DTS-based-currently) buffered ranges for sane GC in out-of-order decode streams like AVC.
On 2015/12/02 23:32:50, wolenetz wrote: > To reduce latency of potentially merging the fix for the bug to release > branches, I recommend including the clamping logic in this as a single CL. > > This CL LG % description-=PTS, and inclusion of clamping and clamping testing > > https://codereview.chromium.org/1491513002/diff/20001/media/filters/source_bu... > File media/filters/source_buffer_stream.h (right): > > https://codereview.chromium.org/1491513002/diff/20001/media/filters/source_bu... > media/filters/source_buffer_stream.h:409: DecodeTimestamp > last_appended_buffer_timestamp_ = kNoDecodeTimestamp(); > Note that this is a DTS, not a PTS (pre-existing problem: see crbugs with > label:MSEptsdtsCleanup for this known issue). media_time is correctly PTS. > Buffered ranges and last_appendeded incorrectly are based on DTS. > > Perhaps adjust the CL description to not mention PTS, so that it doesn't confuse > readers. > > Note that DTS usually is < PTS, so the clamping CL follow-up is motivated not > just for Chromium pipeline allowing audio clock to go slightly past end of > buffered video, it's also motivated for clamping a slightly higher PTS into the > (incorrectly DTS-based-currently) buffered ranges for sane GC in out-of-order > decode streams like AVC. s/DTS usually is < PTS/DTS is usually <= PTS/
Description was changed from ========== [MSE] Fix GC with media_time past the last appended buffer pts Previously MSE GC algorithm didn't take into account properly the duration of the last appended buffer. As a result GC would fail when the media_time was beyond the pts of the last appended buffer. This CL fixes that and adds a new unit test case to cover this situation. BUG=563292 ========== to ========== [MSE] Fix GC with media_time past the last appended buffer timestamp Previously MSE GC algorithm didn't take into account properly the duration of the last appended buffer. As a result GC would fail when the media_time was beyond the timestamp of the last appended buffer. This CL fixes that and adds a new unit test case to cover this situation. BUG=563292 ==========
On 2015/12/02 23:33:44, wolenetz wrote: > On 2015/12/02 23:32:50, wolenetz wrote: > > To reduce latency of potentially merging the fix for the bug to release > > branches, I recommend including the clamping logic in this as a single CL. > > > > This CL LG % description-=PTS, and inclusion of clamping and clamping testing > > > > > https://codereview.chromium.org/1491513002/diff/20001/media/filters/source_bu... > > File media/filters/source_buffer_stream.h (right): > > > > > https://codereview.chromium.org/1491513002/diff/20001/media/filters/source_bu... > > media/filters/source_buffer_stream.h:409: DecodeTimestamp > > last_appended_buffer_timestamp_ = kNoDecodeTimestamp(); > > Note that this is a DTS, not a PTS (pre-existing problem: see crbugs with > > label:MSEptsdtsCleanup for this known issue). media_time is correctly PTS. > > Buffered ranges and last_appendeded incorrectly are based on DTS. > > > > Perhaps adjust the CL description to not mention PTS, so that it doesn't > confuse > > readers. > > > > Note that DTS usually is < PTS, so the clamping CL follow-up is motivated not > > just for Chromium pipeline allowing audio clock to go slightly past end of > > buffered video, it's also motivated for clamping a slightly higher PTS into > the > > (incorrectly DTS-based-currently) buffered ranges for sane GC in out-of-order > > decode streams like AVC. > > s/DTS usually is < PTS/DTS is usually <= PTS/ All done, PTAL. I've also added a new unit test to cover the case where the GC algorithm is invoked with media_time outside of the current selected_range_ (which, in the absense of pending seek, should be the range where we are reading from and where media_time should belong).
LGTM! Ship it :) Thank you for following this through to a fix that looks to me like it will be more easily merge-able back to release branches, once baked.
The CQ bit was checked by wolenetz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491513002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491513002/40001
Message was sent while issue was closed.
Description was changed from ========== [MSE] Fix GC with media_time past the last appended buffer timestamp Previously MSE GC algorithm didn't take into account properly the duration of the last appended buffer. As a result GC would fail when the media_time was beyond the timestamp of the last appended buffer. This CL fixes that and adds a new unit test case to cover this situation. BUG=563292 ========== to ========== [MSE] Fix GC with media_time past the last appended buffer timestamp Previously MSE GC algorithm didn't take into account properly the duration of the last appended buffer. As a result GC would fail when the media_time was beyond the timestamp of the last appended buffer. This CL fixes that and adds a new unit test case to cover this situation. BUG=563292 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [MSE] Fix GC with media_time past the last appended buffer timestamp Previously MSE GC algorithm didn't take into account properly the duration of the last appended buffer. As a result GC would fail when the media_time was beyond the timestamp of the last appended buffer. This CL fixes that and adds a new unit test case to cover this situation. BUG=563292 ========== to ========== [MSE] Fix GC with media_time past the last appended buffer timestamp Previously MSE GC algorithm didn't take into account properly the duration of the last appended buffer. As a result GC would fail when the media_time was beyond the timestamp of the last appended buffer. This CL fixes that and adds a new unit test case to cover this situation. BUG=563292 Committed: https://crrev.com/3ec8ac57b3788dd43da901a05ec99a4364e088bf Cr-Commit-Position: refs/heads/master@{#363002} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3ec8ac57b3788dd43da901a05ec99a4364e088bf Cr-Commit-Position: refs/heads/master@{#363002}
Message was sent while issue was closed.
On 2015/12/03 17:25:34, wolenetz wrote: > LGTM! Ship it :) > > Thank you for following this through to a fix that looks to me like it will be > more easily merge-able back to release branches, once baked. Yep, we'll probably need to cherry-pick this into release branches after this bakes for some time in Canary builds. Thanks! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
