Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2343543002:
MSE: Replace crossfade splicing overlap trimming. (Closed)
|
Created:
4 years, 3 months ago by chcunningham Modified:
4 years, 1 month ago Reviewers:
wolenetz CC:
chromium-reviews, feature-media-reviews_chromium.org, posciak+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMSE: Replace crossfade splicing overlap trimming.
Crossfading is quite complex and error prone (source of many
crashes). It is also not necessary for the gapless playback
scenario (handled entirely by partial append window trimming).
The simpler approach is to just trim the existing buffer,
eliminating the overlap.
See MSE spec proposal here:
https://github.com/w3c/media-source/issues/165
BUG=588351, 577438
TEST=new unit tests
Committed: https://crrev.com/967db2f72de7e17e8c3326438e29c90375e3a1be
Cr-Commit-Position: refs/heads/master@{#429394}
Patch Set 1 #
Total comments: 4
Patch Set 2 : No more silence, just trim the overlap. #
Total comments: 2
Patch Set 3 : No tiny splices #
Total comments: 6
Patch Set 4 : feedback: add new and old tests #Patch Set 5 : fix compile error #
Total comments: 58
Patch Set 6 : Feedback #
Total comments: 14
Patch Set 7 : Merge and feedback. #
Messages
Total messages: 48 (23 generated)
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== MSE: Replace crossfade splicing with generated silence. ========== to ========== MSE: Replace crossfade splicing with generated silence. BUG=588351, 577438 TEST=new unit tests ==========
chcunningham@chromium.org changed reviewers: + wolenetz@chromium.org
Hey Matt, PTAL Will do a follow up CL that deletes all of the splicing code from the renderers (including the AudioSplicer itself). Will also add some to this description in the morning.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
On 2016/09/14 03:44:33, chcunningham wrote: > Hey Matt, PTAL > > Will do a follow up CL that deletes all of the splicing code from the renderers > (including the AudioSplicer itself). > > Will also add some to this description in the morning. Sweet. I'll take a look today.
On 2016/09/14 19:02:18, wolenetz wrote: > On 2016/09/14 03:44:33, chcunningham wrote: > > Hey Matt, PTAL > > > > Will do a follow up CL that deletes all of the splicing code from the > renderers > > (including the AudioSplicer itself). > > > > Will also add some to this description in the morning. > > Sweet. I'll take a look today. Note: in quick chat, chcunningham@ will also look into whether or not forced config change across gapless (front_discard) is retained by this CL (and if it needs to be retained for correct decoder output across gapless media abutment). aside: abutment for gapless, not splice, since splice is tainted. WDYT? :)
I'll review this tomorrow morning - my review isn't wholly blocked, but chcunningham@ is working on an update to this CL currently.
Just chatted with chcunningham@ who indicated that the dependent CL is going to change to a rewrite of this CL that doesn't introduce silence for the dropped portion of the overlapped append, rather just end-discards the end-overlapped buffered to abut directly without crossfade the new buffer. As such, I'll hold off on review until that rewrite comes in. https://codereview.chromium.org/2343543002/diff/1/media/filters/chunk_demuxer.cc File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2343543002/diff/1/media/filters/chunk_demuxer... media/filters/chunk_demuxer.cc:188: // Enable partial append window support for most codecs (notably: not opus). nit: s/most codecs/most audio codecs/ https://codereview.chromium.org/2343543002/diff/1/media/filters/chunk_demuxer... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2343543002/diff/1/media/filters/chunk_demuxer... media/filters/chunk_demuxer_unittest.cc:174: "mu. Adding silence of length:" + s/mu/us/
Reworked as discussed. Re: config changes, the config change code for splicing is now entirely removed. We appear to not be currently forcing a config change for the preroll case, so that behavior is unchanged. https://codereview.chromium.org/2343543002/diff/1/media/filters/chunk_demuxer.cc File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2343543002/diff/1/media/filters/chunk_demuxer... media/filters/chunk_demuxer.cc:188: // Enable partial append window support for most codecs (notably: not opus). On 2016/09/15 23:47:52, wolenetz wrote: > nit: s/most codecs/most audio codecs/ Done. https://codereview.chromium.org/2343543002/diff/1/media/filters/chunk_demuxer... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2343543002/diff/1/media/filters/chunk_demuxer... media/filters/chunk_demuxer_unittest.cc:174: "mu. Adding silence of length:" + On 2016/09/15 23:47:52, wolenetz wrote: > s/mu/us/ Done.
Description was changed from ========== MSE: Replace crossfade splicing with generated silence. BUG=588351, 577438 TEST=new unit tests ========== to ========== MSE: Replace crossfade splicing overlap trimming. Crossfading is quite complex and error prone (source of many crashes). It is also not necessary for the gapless playback scenario (handled entirely by partial append window trimming). The simpler approach is to just trim the existing buffer, eliminating the overlap. BUG=588351, 577438 TEST=new unit tests ==========
https://codereview.chromium.org/2343543002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (left): https://codereview.chromium.org/2343543002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1766: // Don't generate splice frames which represent less than a millisecond (which Probably you want to carry this over to avoid bad media from generating false trims.
https://codereview.chromium.org/2343543002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (left): https://codereview.chromium.org/2343543002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1766: // Don't generate splice frames which represent less than a millisecond (which On 2016/09/16 21:01:04, DaleCurtis wrote: > Probably you want to carry this over to avoid bad media from generating false > trims. Done.
Should have left this comment the first time too, sorry. https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4298: TEST_F(SourceBufferStreamTest, Audio_SpliceFrame_NoMillisecondSplices) { Some of these rejection tests should be ported as well.
https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4298: TEST_F(SourceBufferStreamTest, Audio_SpliceFrame_NoMillisecondSplices) { On 2016/09/16 22:28:29, DaleCurtis wrote: > Some of these rejection tests should be ported as well. Done. Added back the following: - NoMillisecondSplices - SpliceFrame_NoSplice (with added verification) Also a new test to verify that existing discard is not overridden when splicing. https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4377: CheckExpectedRangesByTimestamp("{ [0,16) }"); Why this test was removed: This test used to cover a fix for a bug that required a very contrived set of circumstances to manifest. The logic has since changed (requiring appends to be more reasonable) causing this test to fail at this line. The failure seems reasonable and I can't seem to come up with a new test that would fail pre-fixing 469325 and also pass in today's code. So just deleting it.
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Friendly ping.
On 2016/09/22 17:50:15, chcunningham wrote: > Friendly ping. Was OoO sick. Looking at this now.
Please update the CL description to reference the MSE VNext spec issue I filed to allow this version of no-crossfade-splicing: https://github.com/w3c/media-source/issues/165
Description was changed from ========== MSE: Replace crossfade splicing overlap trimming. Crossfading is quite complex and error prone (source of many crashes). It is also not necessary for the gapless playback scenario (handled entirely by partial append window trimming). The simpler approach is to just trim the existing buffer, eliminating the overlap. BUG=588351, 577438 TEST=new unit tests ========== to ========== MSE: Replace crossfade splicing overlap trimming. Crossfading is quite complex and error prone (source of many crashes). It is also not necessary for the gapless playback scenario (handled entirely by partial append window trimming). The simpler approach is to just trim the existing buffer, eliminating the overlap. See MSE spec proposal here: https://github.com/w3c/media-source/issues/165 BUG=588351, 577438 TEST=new unit tests ==========
On 2016/09/22 20:57:10, wolenetz wrote: > Please update the CL description to reference the MSE VNext spec issue I filed > to allow this version of no-crossfade-splicing: > https://github.com/w3c/media-source/issues/165 Done.
partial CR. Lots of interesting spec side-effects and investigations are delaying this CR a little: Related to this CR, I: Filed https://github.com/w3c/media-source/issues/165 Filed https://github.com/w3c/media-source/issues/166 filed crbug 649530 Before completing review, I will need to look into the change in this CL that forces same-timestamp (e.g. alt-ref, or just badly muxed media) to be 0-duration (and also if the spec allows appropriate behavior there (for video, not just audio), as well as what the spec says about handling the scenario where 0KD10 is followed by 0D10 without intervening discontinuity triggered in the coded frame processing algorithm (for each of audio and video). tl;dr: this CR isn't simple :) https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:972: EXPECT_MEDIA_LOG(TrimmedSpliceOverlap(779000, 759000, 3000)); Why is this 3000 not 26000? https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1836: EXPECT_MEDIA_LOG(TrimmedSpliceOverlap(5000, 0, 5000)); To be clear, this trims an *additional* 5ms from the original 23ms frame which previously had 10ms trimmed from it, right? Or does this change to just discarding 5ms from the original 23ms (which would be incorrect)? To assist review/maintainability, can you read back the resulting buffers and verify their durations/discard amounts after the next line or at end of this unit test? e.g, at end of this test, I would expect the buffers to be: Audio: [0,5)[5,28)[28,45)[45,68) Video: [0,5)[5,38)[38,45)[45,78) ** Note that the video case is underspecified in the spec. I've filed https://github.com/w3c/media-source/issues/166 to track clarifying that (and Chrome only adjusts overlapped video frame durations if the overlapped frame had an estimated duration. Not very intuitive, and probably leading to other problems.) If we have this verification already in SourceBufferStreamTest, then maybe point to such test(s) from this one? (or add it over there and point to it from this test). https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:995: // in PreapreRangesForNextAppend(). nit typo
Looking pretty good, though I have unfortunately a lot of comments: https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:2903: "{ [0,23) [300,400) [520,590) [720,750) [900,970) }"); Wow, this appears to have been broken before. Why wasn't the last range previously also [900,950) before this CL? https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:2982: "{ [0,23) [300,400) [500,590) [700,750) [900,970) }"); ditto: why broken before? https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:2885: // - existing frame trimmed from [900, 970) to [900-930), nit: consistent [900,930) https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:2893: // Appending within buffered range should not affect video buffered ranges. nit: This isn't always true. But it is for the particular append done here. Rephrase? https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:2970: // Appending within buffered range should not affect video buffered ranges. nit ditto: rephrase? https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3337: EXPECT_EQ(last_timestamp.InMilliseconds(), 759); Is this 782 -> 759 change because the 782 buffer was needed for crossfade, but instead we now just discard tail of 759 buffer? https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (left): https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1542: if (pending_buffer_.get()) { pending buffer might be a preroll buffer still. GetSpliceBufferConfigId should return that config id for the preroll buffer still (for an out-of-splice-buffers-range index argument). I'm not convinced wholesale removal of this code block is correct, since preroll buffer might be different config than what came before, and not updating current_config_index_ here to be the new config will result in the old config being reported to the reader, not the new one. If I'm right, we also need a unit test to catch such a regression. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:61: // TODO(wolenetz): Once all stream parsers emit accurate frame durations, use aside: MSE v1 spec allows our current implementation of this fudge room :) https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:986: // At most one buffer should exist containing the time of the newly appended nit: "at most" vs dcheck_eq mismatch. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1008: if (overlap_duration == base::TimeDelta()) { Can this even occur with end_dts just 1us > splice_dts, and splice_dts != overlapped_buffer->timestamp() [assuming, safely for audio, that pts==dts]? convert to DCHECK_GT(overlap_duration, base::TimeDelta()) ? https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1022: LIMITED_MEDIA_LOG(DEBUG, media_log_, num_splice_logs_, kMaxAudioSpliceLogs) Include in log_string something like "Multiple occurrences may result in loss of A/V sync." https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1075: // deleting zero-duration VP9 alt-ref frames while still deleting frames I think from the fgalligan/strobe/you/me thread in January that the "decode all the things route" included retaining non-zero prev_duration same-timestamp overlapped buffer here. Or at least MEDIA_LOG'ing when we are dropping them. Do I misread? This wouldn't apply to audio (or maybe text) in this CL, just video. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:534: // the splice frame. If a timestamp within the preroll ends with C the config I think we still need some configchange testing (see my SBS.cc comment) https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4336: CheckExpectedBuffers("0K 2K 4K 6K 8K 10K 12K C 11P 13K 15K 17K"); We should still test that config change across different configs at the splice (discard-abutment) point including some preroll on the overlapping frames results in the right sequence of : original frames up to the overlapped truncated frame + config change + overlapping frames. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4366: CheckNoNextBuffer(); Why remove this? https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4077: // Make a 2 BufferQueues with a mix of buffers containing of start/end nit: Make a 2? containing of? https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4151: EXPECT_EQ(discardA1, read_buffer->discard_padding()); nit: s/discardA1/discardB1/ https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4182: // 0.75ms between the ranges. nit: s/ranges./ranges, but only 0.25ms end-overlap on the first overlapped frame./ https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4187: // A splice frame should not be generated (indicated by the lack of a config nit: this "lack of config change" would occur even in a generated-discard-abut splice. reword?
Updates from interactive review of comments w/chcunningham@: https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:2903: "{ [0,23) [300,400) [520,590) [720,750) [900,970) }"); On 2016/09/26 23:52:37, wolenetz wrote: > Wow, this appears to have been broken before. Why wasn't the last range > previously also [900,950) before this CL? Offline, yes looks like an artifact before (crossfade duration kept buffered range duration longer?) https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:2982: "{ [0,23) [300,400) [500,590) [700,750) [900,970) }"); On 2016/09/26 23:52:37, wolenetz wrote: > ditto: why broken before? Ditto Ack of offline. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1022: LIMITED_MEDIA_LOG(DEBUG, media_log_, num_splice_logs_, kMaxAudioSpliceLogs) On 2016/09/26 23:52:37, wolenetz wrote: > Include in log_string something like "Multiple occurrences may result in loss of > A/V sync." (nit) https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1075: // deleting zero-duration VP9 alt-ref frames while still deleting frames On 2016/09/26 23:52:37, wolenetz wrote: > I think from the fgalligan/strobe/you/me thread in January that the "decode all > the things route" included retaining non-zero prev_duration same-timestamp > overlapped buffer here. Or at least MEDIA_LOG'ing when we are dropping them. Do > I misread? This wouldn't apply to audio (or maybe text) in this CL, just video. In particular, I'm not sure this won't regress nonkeyframe video overlapping (a badly muxed) same timestamp >0 duration previously buffered keyframe. And I'm not sure about compliance yet with the text splice frame algorithm in this CL.
All comments addressed (finally). https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:2903: "{ [0,23) [300,400) [520,590) [720,750) [900,970) }"); On 2016/09/27 00:35:35, wolenetz_slow_oct11-14 wrote: > On 2016/09/26 23:52:37, wolenetz wrote: > > Wow, this appears to have been broken before. Why wasn't the last range > > previously also [900,950) before this CL? > > Offline, yes looks like an artifact before (crossfade duration kept buffered > range duration longer?) Right, not sure if this is a bug originally (much of the orignal splicing code is difficult to grok), but when converting the new appended buffer to a splice buffer (passing in the pre-splice buffers), we used the duration of the pre-splice buffers if it exceeds the overlap duration (which happens in this case since overlap = 20, pre-splice-duration=70). https://cs.chromium.org/chromium/src/media/base/stream_parser_buffer.cc?rcl=1... I look forward to deleting this code https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:972: EXPECT_MEDIA_LOG(TrimmedSpliceOverlap(779000, 759000, 3000)); On 2016/09/22 23:48:37, wolenetz wrote: > Why is this 3000 not 26000? First, let me list some numbers... A) splice time: 779000 B) start time of pre_splice_buffers->back() used in original calculation: 782000 C) start time of splice overlapped buffer: 759000 D) buffer durations (same for all buffers): 23000 Now to explain. The original impl fetched more than one pre_splice_buffer and the back of that list of buffers might have a PTS > than the splice PTS (true in this case). Said another way, the PTS for back of the pre_splice_buffers actually lands in the middle of the new splice buffer. The splice duration was then calculated as B + D - A = 26000. I think this may have been a bug. The buffers do overlap, but this does not actually correctly compute the duration of their overlap. Today's impl only grabs a single pre-splice buffer. The pre-splice buffer have a PTS < the splice buffer, and its tail is overlapped by the splice buffer. Here we compute the duration as C + D - A = 3000 In todays impl, the buffer B, with PTS > the splice buffer is still overlapped, but we have no use for it, so it gets completely removed. https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1836: EXPECT_MEDIA_LOG(TrimmedSpliceOverlap(5000, 0, 5000)); On 2016/09/22 23:48:37, wolenetz wrote: > To be clear, this trims an *additional* 5ms from the original 23ms frame which > previously had 10ms trimmed from it, right? Or does this change to just > discarding 5ms from the original 23ms (which would be incorrect)? > > To assist review/maintainability, can you read back the resulting buffers and > verify their durations/discard amounts after the next line or at end of this > unit test? > > e.g, at end of this test, I would expect the buffers to be: > Audio: [0,5)[5,28)[28,45)[45,68) > Video: [0,5)[5,38)[38,45)[45,78) ** Note that the video case is underspecified > in the spec. I've filed https://github.com/w3c/media-source/issues/166 to track > clarifying that (and Chrome only adjusts overlapped video frame durations if the > overlapped frame had an estimated duration. Not very intuitive, and probably > leading to other problems.) > > If we have this verification already in SourceBufferStreamTest, then maybe point > to such test(s) from this one? (or add it over there and point to it from this > test). Added buffer times for readability. Did not add duration verification - instead referenced SourceBufferStream tests that already do this. https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:2885: // - existing frame trimmed from [900, 970) to [900-930), On 2016/09/26 23:52:37, wolenetz wrote: > nit: consistent [900,930) Done. https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:2893: // Appending within buffered range should not affect video buffered ranges. On 2016/09/26 23:52:37, wolenetz wrote: > nit: This isn't always true. But it is for the particular append done here. > Rephrase? Done. https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:2970: // Appending within buffered range should not affect video buffered ranges. On 2016/09/26 23:52:37, wolenetz wrote: > nit ditto: rephrase? Done. https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3337: EXPECT_EQ(last_timestamp.InMilliseconds(), 759); On 2016/09/26 23:52:37, wolenetz_slow_oct11-14 wrote: > Is this 782 -> 759 change because the 782 buffer was needed for crossfade, but > instead we now just discard tail of 759 buffer? Yes. See detailed explanation in my comments on InitDemuxerWithConfigChangeData https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (left): https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1542: if (pending_buffer_.get()) { On 2016/09/26 23:52:37, wolenetz wrote: > pending buffer might be a preroll buffer still. GetSpliceBufferConfigId should > return that config id for the preroll buffer still (for an > out-of-splice-buffers-range index argument). I'm not convinced wholesale removal > of this code block is correct, since preroll buffer might be different config > than what came before, and not updating current_config_index_ here to be the new > config will result in the old config being reported to the reader, not the new > one. If I'm right, we also need a unit test to catch such a regression. I've added a new test, SourceBufferStreamTest.Audio_ConfigChangeWithPreroll. It passes with this code as-is. The config change still occurs because pre-roll buffers implicitly have the same config ID as the buffer they're attached to. See https://cs.chromium.org/chromium/src/media/base/stream_parser_buffer.cc?rcl=1... So when we are about to read out the buffer which has a preroll attached, We call GetNextBufferInternal and detect a config change at that time via the same mechanism as the non-preroll case (selected range config ID does not match). Let me know if you think this test is missing some edge case, but I think we may be good to without this pending buffer logic. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:61: // TODO(wolenetz): Once all stream parsers emit accurate frame durations, use On 2016/09/26 23:52:37, wolenetz_slow_oct11-14 wrote: > aside: MSE v1 spec allows our current implementation of this fudge room :) Acknowledged. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:986: // At most one buffer should exist containing the time of the newly appended On 2016/09/26 23:52:37, wolenetz_slow_oct11-14 wrote: > nit: "at most" vs dcheck_eq mismatch. Eh, I see your point, but I think this makes sense given the return on line 983 https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:995: // in PreapreRangesForNextAppend(). On 2016/09/22 23:48:37, wolenetz_slow_oct11-14 wrote: > nit typo Done. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1008: if (overlap_duration == base::TimeDelta()) { On 2016/09/26 23:52:37, wolenetz_slow_oct11-14 wrote: > Can this even occur with end_dts just 1us > splice_dts, and splice_dts != > overlapped_buffer->timestamp() [assuming, safely for audio, that pts==dts]? > convert to DCHECK_GT(overlap_duration, base::TimeDelta()) ? I think you're right. Changed to a DCHECK. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1022: LIMITED_MEDIA_LOG(DEBUG, media_log_, num_splice_logs_, kMaxAudioSpliceLogs) On 2016/09/26 23:52:37, wolenetz_slow_oct11-14 wrote: > Include in log_string something like "Multiple occurrences may result in loss of > A/V sync." Done. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1075: // deleting zero-duration VP9 alt-ref frames while still deleting frames On 2016/09/27 00:35:35, wolenetz_slow_oct11-14 wrote: > On 2016/09/26 23:52:37, wolenetz wrote: > > I think from the fgalligan/strobe/you/me thread in January that the "decode > all > > the things route" included retaining non-zero prev_duration same-timestamp > > overlapped buffer here. Or at least MEDIA_LOG'ing when we are dropping them. > Do > > I misread? This wouldn't apply to audio (or maybe text) in this CL, just > video. > > In particular, I'm not sure this won't regress nonkeyframe video overlapping (a > badly muxed) same timestamp >0 duration previously buffered keyframe. And I'm > not sure about compliance yet with the text splice frame algorithm in this CL. I think you're right. I've changed this as discussed and re-worked the comment. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:534: // the splice frame. If a timestamp within the preroll ends with C the config On 2016/09/26 23:52:37, wolenetz wrote: > I think we still need some configchange testing (see my SBS.cc comment) See new test, SourceBufferStreamTest.Audio_ConfigChangeWithPreroll https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4336: CheckExpectedBuffers("0K 2K 4K 6K 8K 10K 12K C 11P 13K 15K 17K"); On 2016/09/26 23:52:37, wolenetz wrote: > We should still test that config change across different configs at the splice > (discard-abutment) point including some preroll on the overlapping frames > results in the right sequence of : original frames up to the overlapped > truncated frame + config change + overlapping frames. Done. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4366: CheckNoNextBuffer(); On 2016/09/26 23:52:37, wolenetz wrote: > Why remove this? See explanation in earlier patch set: https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_bu... https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4077: // Make a 2 BufferQueues with a mix of buffers containing of start/end On 2016/09/26 23:52:37, wolenetz wrote: > nit: Make a 2? containing of? Done. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4151: EXPECT_EQ(discardA1, read_buffer->discard_padding()); On 2016/09/26 23:52:38, wolenetz wrote: > nit: s/discardA1/discardB1/ Done. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4182: // 0.75ms between the ranges. On 2016/09/26 23:52:38, wolenetz wrote: > nit: s/ranges./ranges, but only 0.25ms end-overlap on the first overlapped > frame./ Done, but a little different wording. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4187: // A splice frame should not be generated (indicated by the lack of a config On 2016/09/26 23:52:38, wolenetz wrote: > nit: this "lack of config change" would occur even in a generated-discard-abut > splice. reword? Done.
Looking almost ready to land: https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4377: CheckExpectedRangesByTimestamp("{ [0,16) }"); On 2016/09/19 22:51:37, chcunningham wrote: > Why this test was removed: > > This test used to cover a fix for a bug that required a very contrived set of > circumstances to manifest. The logic has since changed (requiring appends to be > more reasonable) causing this test to fail at this line. The failure seems > reasonable and I can't seem to come up with a new test that would fail > pre-fixing 469325 and also pass in today's code. So just deleting it. This test checks that 0D10K prepended to 10K..., followed by appending 0D10, *preserves* both 0D10K followed by 0D10 (and the original 10K...). This is indeed the same-timestamp, possibly bad/overlapping video duration case which I believe we should still pass now (per the fgalligan/strobe/you/me "preserve all the things" route in the thread in January and mentioned earlier in another CR comment on this CL (https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu...), augmented with "make sure an end abutment with previously buffered media doesn't trigger removal of that previously buffered media" logic that this test specifically targeted. If this case fails with latest patch set, please investigate because I believe it should pass. Otherwise, please include it or reference another test which covers the same scenario. https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:2903: "{ [0,23) [300,400) [520,590) [720,750) [900,970) }"); On 2016/10/27 23:36:06, chcunningham wrote: > On 2016/09/27 00:35:35, wolenetz_slow_oct11-14 wrote: > > On 2016/09/26 23:52:37, wolenetz wrote: > > > Wow, this appears to have been broken before. Why wasn't the last range > > > previously also [900,950) before this CL? > > > > Offline, yes looks like an artifact before (crossfade duration kept buffered > > range duration longer?) > > Right, not sure if this is a bug originally (much of the orignal splicing code > is difficult to grok), but when converting the new appended buffer to a splice > buffer (passing in the pre-splice buffers), we used the duration of the > pre-splice buffers if it exceeds the overlap duration (which happens in this > case since overlap = 20, pre-splice-duration=70). > > https://cs.chromium.org/chromium/src/media/base/stream_parser_buffer.cc?rcl=1... > > I look forward to deleting this code Acknowledged. https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:972: EXPECT_MEDIA_LOG(TrimmedSpliceOverlap(779000, 759000, 3000)); On 2016/10/27 23:36:06, chcunningham wrote: > On 2016/09/22 23:48:37, wolenetz wrote: > > Why is this 3000 not 26000? > > First, let me list some numbers... > > A) splice time: 779000 > B) start time of pre_splice_buffers->back() used in original calculation: 782000 > C) start time of splice overlapped buffer: 759000 > D) buffer durations (same for all buffers): 23000 > > Now to explain. The original impl fetched more than one pre_splice_buffer and > the back of that list of buffers might have a PTS > than the splice PTS (true in > this case). Said another way, the PTS for back of the pre_splice_buffers > actually lands in the middle of the new splice buffer. The splice duration was > then calculated as B + D - A = 26000. I think this may have been a bug. The > buffers do overlap, but this does not actually correctly compute the duration of > their overlap. > > Today's impl only grabs a single pre-splice buffer. The pre-splice buffer have a > PTS < the splice buffer, and its tail is overlapped by the splice buffer. Here > we compute the duration as C + D - A = 3000 > > In todays impl, the buffer B, with PTS > the splice buffer is still overlapped, > but we have no use for it, so it gets completely removed. Acknowledged. The change in expectations makes sense. I believe we previously had to have a > 3ms overlap (kCrossfadeDurationInMilliseconds = 5ms) to do the splice, hence we had previously kept B in pre-splice buffers. The net overlap of pre-splice and post-splice previously was indeed 26ms in this case, so on the surface, that doesn't look like a previous bug. Regardless, new expectation in this CL of *no crossfade* SGTM :) https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1836: EXPECT_MEDIA_LOG(TrimmedSpliceOverlap(5000, 0, 5000)); On 2016/10/27 23:36:06, chcunningham wrote: > On 2016/09/22 23:48:37, wolenetz wrote: > > To be clear, this trims an *additional* 5ms from the original 23ms frame which > > previously had 10ms trimmed from it, right? Or does this change to just > > discarding 5ms from the original 23ms (which would be incorrect)? > > > > To assist review/maintainability, can you read back the resulting buffers and > > verify their durations/discard amounts after the next line or at end of this > > unit test? > > > > e.g, at end of this test, I would expect the buffers to be: > > Audio: [0,5)[5,28)[28,45)[45,68) > > Video: [0,5)[5,38)[38,45)[45,78) ** Note that the video case is underspecified > > in the spec. I've filed https://github.com/w3c/media-source/issues/166 to > track > > clarifying that (and Chrome only adjusts overlapped video frame durations if > the > > overlapped frame had an estimated duration. Not very intuitive, and probably > > leading to other problems.) > > > > If we have this verification already in SourceBufferStreamTest, then maybe > point > > to such test(s) from this one? (or add it over there and point to it from this > > test). > > Added buffer times for readability. Did not add duration verification - instead > referenced SourceBufferStream tests that already do this. Ack. Thanks for the additional verification and reference to the SBSTest that verifies trimmed audio durations on overlap splice. https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:2885: // - existing frame trimmed from [900, 970) to [900-930), On 2016/10/27 23:36:06, chcunningham wrote: > On 2016/09/26 23:52:37, wolenetz wrote: > > nit: consistent [900,930) > > Done. Not done here actually :) (though done in at least one other place). This is a really small nit though. https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3337: EXPECT_EQ(last_timestamp.InMilliseconds(), 759); On 2016/10/27 23:36:06, chcunningham wrote: > On 2016/09/26 23:52:37, wolenetz_slow_oct11-14 wrote: > > Is this 782 -> 759 change because the 782 buffer was needed for crossfade, but > > instead we now just discard tail of 759 buffer? > > Yes. See detailed explanation in my comments on InitDemuxerWithConfigChangeData Acknowledged. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (left): https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1542: if (pending_buffer_.get()) { On 2016/10/27 23:36:06, chcunningham wrote: > On 2016/09/26 23:52:37, wolenetz wrote: > > pending buffer might be a preroll buffer still. GetSpliceBufferConfigId should > > return that config id for the preroll buffer still (for an > > out-of-splice-buffers-range index argument). I'm not convinced wholesale > removal > > of this code block is correct, since preroll buffer might be different config > > than what came before, and not updating current_config_index_ here to be the > new > > config will result in the old config being reported to the reader, not the new > > one. If I'm right, we also need a unit test to catch such a regression. > > I've added a new test, SourceBufferStreamTest.Audio_ConfigChangeWithPreroll. It > passes with this code as-is. The config change still occurs because pre-roll > buffers implicitly have the same config ID as the buffer they're attached to. > See > https://cs.chromium.org/chromium/src/media/base/stream_parser_buffer.cc?rcl=1... > > So when we are about to read out the buffer which has a preroll attached, We > call GetNextBufferInternal and detect a config change at that time via the same > mechanism as the non-preroll case (selected range config ID does not match). > > Let me know if you think this test is missing some edge case, but I think we may > be good to without this pending buffer logic. I think you're right, after grok'ing this code again. Thanks for adding the test to confirm. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:986: // At most one buffer should exist containing the time of the newly appended On 2016/10/27 23:36:06, chcunningham wrote: > On 2016/09/26 23:52:37, wolenetz_slow_oct11-14 wrote: > > nit: "at most" vs dcheck_eq mismatch. > > Eh, I see your point, but I think this makes sense given the return on line 983 Ack. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1008: if (overlap_duration == base::TimeDelta()) { On 2016/10/27 23:36:06, chcunningham wrote: > On 2016/09/26 23:52:37, wolenetz_slow_oct11-14 wrote: > > Can this even occur with end_dts just 1us > splice_dts, and splice_dts != > > overlapped_buffer->timestamp() [assuming, safely for audio, that pts==dts]? > > convert to DCHECK_GT(overlap_duration, base::TimeDelta()) ? > > I think you're right. Changed to a DCHECK. Acknowledged. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:534: // the splice frame. If a timestamp within the preroll ends with C the config On 2016/10/27 23:36:06, chcunningham wrote: > On 2016/09/26 23:52:37, wolenetz wrote: > > I think we still need some configchange testing (see my SBS.cc comment) > > See new test, SourceBufferStreamTest.Audio_ConfigChangeWithPreroll Acknowledged. https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4366: CheckNoNextBuffer(); On 2016/10/27 23:36:07, chcunningham wrote: > On 2016/09/26 23:52:37, wolenetz wrote: > > Why remove this? > > See explanation in earlier patch set: > https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_bu... I see. I commented now on that https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_bu.... Let's use that thread to discuss further. https://codereview.chromium.org/2343543002/diff/100001/media/base/test_helpers.h File media/base/test_helpers.h (right): https://codereview.chromium.org/2343543002/diff/100001/media/base/test_helper... media/base/test_helpers.h:197: MATCHER_P2(GeneratedSplice, duration_microseconds, time_microseconds, "") { This was relocated here from tests in the interim. I don't think this is needed, right? https://codereview.chromium.org/2343543002/diff/100001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2343543002/diff/100001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:2891: // Appending within buffered existing buffered range. nit: s/buffered existing buffered/existing buffered/ https://codereview.chromium.org/2343543002/diff/100001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:3229: /* Please uncomment this test's body :) https://codereview.chromium.org/2343543002/diff/100001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2343543002/diff/100001/media/filters/source_b... media/filters/source_buffer_stream.cc:1058: DCHECK_NE(overlap_duration, base::TimeDelta()); nit: s/NE/GT/ for extra safety :) https://codereview.chromium.org/2343543002/diff/100001/media/filters/source_b... media/filters/source_buffer_stream.cc:1124: // dependencies and there are no downsides to just keeping it. nit: s/no downsides/no downsides, other than potentially keeping some throw-away work for the decoder,/ https://codereview.chromium.org/2343543002/diff/100001/media/filters/source_b... media/filters/source_buffer_stream.cc:1125: // B. Type is audio and overlapped duration is 0. We've encountered Vorbis nit: s/audio/audio or text/ + add a TODO+crbug, above. Details: See https://www.w3.org/TR/media-source/#sourcebuffer-text-splice-frame-algorithm tl;dr: text coded frame overlap needs to preserve both the *first* overlapped cue and the overlapping cue, wwith the coded frame duration of the overlapped frame truncated to end where the overlapping frame starts (and any other later overlapped text frames in the overlapping presentation interval should be dropped). So, I think the comment needs inclusion of "text" here for same-timestamp overlapped cue preservation, as well as inclusion of TrimSpliceOverlap() processing, above, for kText. Since I understand the intent is to merge this change back ASAP to fix the high audio crossfade problem rate, I'd be fine with just updating the same-timestamp 0-duration cue comment here to include text and adding a TODO+crbug ref added above at invocation of TrimSpliceOverlap() to do similar processing for kText in this CL. https://codereview.chromium.org/2343543002/diff/100001/media/filters/source_b... media/filters/source_buffer_stream.cc:1353: DVLOG(3) << "Last output buffer DTS:" nit: Also include similar log in line 1328, and differentiate in the log messages where the buffer came from (track_buffer_ vs selected_range_)?
Though I'm out tomorrow (Monday Oct 31) I'll try to take a look in the afternoon to see if there's an update which L*TM. If not, I'll certainly check Tuesday morning.
Thanks Matt. I think I got them all this time https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4377: CheckExpectedRangesByTimestamp("{ [0,16) }"); On 2016/10/28 23:08:18, wolenetz wrote: > On 2016/09/19 22:51:37, chcunningham wrote: > > Why this test was removed: > > > > This test used to cover a fix for a bug that required a very contrived set of > > circumstances to manifest. The logic has since changed (requiring appends to > be > > more reasonable) causing this test to fail at this line. The failure seems > > reasonable and I can't seem to come up with a new test that would fail > > pre-fixing 469325 and also pass in today's code. So just deleting it. > > This test checks that 0D10K prepended to 10K..., followed by appending 0D10, > *preserves* both 0D10K followed by 0D10 (and the original 10K...). This is > indeed the same-timestamp, possibly bad/overlapping video duration case which I > believe we should still pass now (per the fgalligan/strobe/you/me "preserve all > the things" route in the thread in January and mentioned earlier in another CR > comment on this CL > (https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu...), > augmented with "make sure an end abutment with previously buffered media doesn't > trigger removal of that previously buffered media" logic that this test > specifically targeted. > If this case fails with latest patch set, please investigate because I believe > it should pass. Otherwise, please include it or reference another test which > covers the same scenario. Yep, this test passes now. My original comment was made obsolete by more recent changes. https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:2885: // - existing frame trimmed from [900, 970) to [900-930), On 2016/10/28 23:08:18, wolenetz wrote: > On 2016/10/27 23:36:06, chcunningham wrote: > > On 2016/09/26 23:52:37, wolenetz wrote: > > > nit: consistent [900,930) > > > > Done. > > Not done here actually :) (though done in at least one other place). > This is a really small nit though. Done. https://codereview.chromium.org/2343543002/diff/100001/media/base/test_helpers.h File media/base/test_helpers.h (right): https://codereview.chromium.org/2343543002/diff/100001/media/base/test_helper... media/base/test_helpers.h:197: MATCHER_P2(GeneratedSplice, duration_microseconds, time_microseconds, "") { On 2016/10/28 23:08:19, wolenetz wrote: > This was relocated here from tests in the interim. I don't think this is needed, > right? Done. Nice catch https://codereview.chromium.org/2343543002/diff/100001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2343543002/diff/100001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:2891: // Appending within buffered existing buffered range. On 2016/10/28 23:08:19, wolenetz wrote: > nit: s/buffered existing buffered/existing buffered/ Done. https://codereview.chromium.org/2343543002/diff/100001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:3229: /* On 2016/10/28 23:08:19, wolenetz wrote: > Please uncomment this test's body :) Lol, Done. https://codereview.chromium.org/2343543002/diff/100001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2343543002/diff/100001/media/filters/source_b... media/filters/source_buffer_stream.cc:1058: DCHECK_NE(overlap_duration, base::TimeDelta()); On 2016/10/28 23:08:19, wolenetz wrote: > nit: s/NE/GT/ for extra safety :) Done. https://codereview.chromium.org/2343543002/diff/100001/media/filters/source_b... media/filters/source_buffer_stream.cc:1124: // dependencies and there are no downsides to just keeping it. On 2016/10/28 23:08:19, wolenetz wrote: > nit: s/no downsides/no downsides, other than potentially keeping some throw-away > work for the decoder,/ Done. https://codereview.chromium.org/2343543002/diff/100001/media/filters/source_b... media/filters/source_buffer_stream.cc:1125: // B. Type is audio and overlapped duration is 0. We've encountered Vorbis On 2016/10/28 23:08:19, wolenetz wrote: > nit: s/audio/audio or text/ + add a TODO+crbug, above. > > Details: > > See https://www.w3.org/TR/media-source/#sourcebuffer-text-splice-frame-algorithm > > tl;dr: text coded frame overlap needs to preserve both the *first* overlapped > cue and the overlapping cue, wwith the coded frame duration of the overlapped > frame truncated to end where the overlapping frame starts (and any other later > overlapped text frames in the overlapping presentation interval should be > dropped). > > So, I think the comment needs inclusion of "text" here for same-timestamp > overlapped cue preservation, as well as inclusion of TrimSpliceOverlap() > processing, above, for kText. > > Since I understand the intent is to merge this change back ASAP to fix the high > audio crossfade problem rate, I'd be fine with just updating the same-timestamp > 0-duration cue comment here to include text and adding a TODO+crbug ref added > above at invocation of TrimSpliceOverlap() to do similar processing for kText in > this CL. Done. For posterity, our f2f decision was to treat Text like Video - exclude the same timestamp regardless of the previous buffer's duration. This preserves the previous behavior for Text. https://codereview.chromium.org/2343543002/diff/100001/media/filters/source_b... media/filters/source_buffer_stream.cc:1353: DVLOG(3) << "Last output buffer DTS:" On 2016/10/28 23:08:19, wolenetz wrote: > nit: Also include similar log in line 1328, and differentiate in the log > messages where the buffer came from (track_buffer_ vs selected_range_)? I actually don't mean to land this log. Its a bit confusing to describe the "last" output buffer here. Really what I wanted to achieve was just logging the timestamp of the buffer output from GetNextBuffer. I realize now that chunk_demuxer.cc already has a very thorough log for this. Removed the log from this file.
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4377: CheckExpectedRangesByTimestamp("{ [0,16) }"); On 2016/11/02 01:28:42, chcunningham wrote: > On 2016/10/28 23:08:18, wolenetz wrote: > > On 2016/09/19 22:51:37, chcunningham wrote: > > > Why this test was removed: > > > > > > This test used to cover a fix for a bug that required a very contrived set > of > > > circumstances to manifest. The logic has since changed (requiring appends to > > be > > > more reasonable) causing this test to fail at this line. The failure seems > > > reasonable and I can't seem to come up with a new test that would fail > > > pre-fixing 469325 and also pass in today's code. So just deleting it. > > > > This test checks that 0D10K prepended to 10K..., followed by appending 0D10, > > *preserves* both 0D10K followed by 0D10 (and the original 10K...). This is > > indeed the same-timestamp, possibly bad/overlapping video duration case which > I > > believe we should still pass now (per the fgalligan/strobe/you/me "preserve > all > > the things" route in the thread in January and mentioned earlier in another CR > > comment on this CL > > > (https://codereview.chromium.org/2343543002/diff/80001/media/filters/source_bu...), > > augmented with "make sure an end abutment with previously buffered media > doesn't > > trigger removal of that previously buffered media" logic that this test > > specifically targeted. > > If this case fails with latest patch set, please investigate because I believe > > it should pass. Otherwise, please include it or reference another test which > > covers the same scenario. > > Yep, this test passes now. My original comment was made obsolete by more recent > changes. Acknowledged.
The CQ bit was checked by wolenetz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MSE: Replace crossfade splicing overlap trimming. Crossfading is quite complex and error prone (source of many crashes). It is also not necessary for the gapless playback scenario (handled entirely by partial append window trimming). The simpler approach is to just trim the existing buffer, eliminating the overlap. See MSE spec proposal here: https://github.com/w3c/media-source/issues/165 BUG=588351, 577438 TEST=new unit tests ========== to ========== MSE: Replace crossfade splicing overlap trimming. Crossfading is quite complex and error prone (source of many crashes). It is also not necessary for the gapless playback scenario (handled entirely by partial append window trimming). The simpler approach is to just trim the existing buffer, eliminating the overlap. See MSE spec proposal here: https://github.com/w3c/media-source/issues/165 BUG=588351, 577438 TEST=new unit tests ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== MSE: Replace crossfade splicing overlap trimming. Crossfading is quite complex and error prone (source of many crashes). It is also not necessary for the gapless playback scenario (handled entirely by partial append window trimming). The simpler approach is to just trim the existing buffer, eliminating the overlap. See MSE spec proposal here: https://github.com/w3c/media-source/issues/165 BUG=588351, 577438 TEST=new unit tests ========== to ========== MSE: Replace crossfade splicing overlap trimming. Crossfading is quite complex and error prone (source of many crashes). It is also not necessary for the gapless playback scenario (handled entirely by partial append window trimming). The simpler approach is to just trim the existing buffer, eliminating the overlap. See MSE spec proposal here: https://github.com/w3c/media-source/issues/165 BUG=588351, 577438 TEST=new unit tests Committed: https://crrev.com/967db2f72de7e17e8c3326438e29c90375e3a1be Cr-Commit-Position: refs/heads/master@{#429394} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/967db2f72de7e17e8c3326438e29c90375e3a1be Cr-Commit-Position: refs/heads/master@{#429394} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
