Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2035003002:
MSE: Signal new coded frame group more aggressively (Closed)
|
Created:
4 years, 6 months ago by wolenetz Modified:
4 years, 6 months ago Reviewers:
chcunningham CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMSE: Signal new coded frame group more aggressively
For 'sequence' mode coded frame processing, we may need to signal a new
coded frame group because application may override timestampOffset and
cause the processed frame sequence to go backwards in time. Jumps
forward are allowed and kept within the same coded frame group. Jumps
backward are now detected and trigger new coded frame group signalling
so the overlap-append logic in SourceBufferStream handles this situation
more correctly.
TEST=*FrameProcessorTest.AudioVideo_Discontinuity_TimestampOffset*, http/tests/media/media-source/mediasource-crbug-616565.html
BUG=616565, 620523
Committed: https://crrev.com/b7982341ce30c1dfb165551b8c641d781eeb0159
Cr-Commit-Position: refs/heads/master@{#400249}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Checkpoint while working on new media_unittest for this (which is currently broken) #Patch Set 3 : Rebase and includes some (not to be landed) temporary SBS logging #
Total comments: 2
Patch Set 4 : Fixed unit test, referenced in TODO new bug 620523. Layout test forthcoming soon. #Patch Set 5 : Added layout test #
Total comments: 10
Patch Set 6 : Addressed chcunningham@'s comments from PS5 #
Dependent Patchsets: Messages
Total messages: 31 (14 generated)
wolenetz@chromium.org changed reviewers: + chcunningham@chromium.org
As I mentioned offline, I still need to add some tests demonstrating what was fixed by this. Notably, I could have made a somewhat simpler fix (reset in_coded_frame_group_ upon *any* discontinuity detection in CFP), but that would have violated the intent of 'sequence' mode in some cases (see previous comment around this from acolwell@ in https://codereview.chromium.org/180153003/diff/210001/media/filters/frame_pro...). Please take a look if you have time at PS1. I'll have another PS posted tomorrow that contains more tests.
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2035003002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2035003002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks good, but needs a unit test. https://codereview.chromium.org/2035003002/diff/1/media/filters/frame_process... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/2035003002/diff/1/media/filters/frame_process... media/filters/frame_processor.cc:298: // sequence mode. Reset it here only if in segments mode. Can you say why? I know I've asked before, but I've forgotten the answer and the comment doesn't tell me.
While I continue to work on the tests, please take a look to see if your non-test comments are now addressed by PS2. (PS2's new test isn't working yet. I also plan to add layout testing for interop compliance checking and inclusion of "sourceBuffer.remove()" in the scenario. https://codereview.chromium.org/2035003002/diff/1/media/filters/frame_process... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/2035003002/diff/1/media/filters/frame_process... media/filters/frame_processor.cc:298: // sequence mode. Reset it here only if in segments mode. On 2016/06/03 22:16:15, chcunningham wrote: > Can you say why? I know I've asked before, but I've forgotten the answer and the > comment doesn't tell me. Done.
On 2016/06/03 22:43:42, wolenetz_slow_reviews wrote: > While I continue to work on the tests, please take a look to see if your > non-test comments are now addressed by PS2. > > (PS2's new test isn't working yet. I also plan to add layout testing for interop > compliance checking and inclusion of "sourceBuffer.remove()" in the scenario. > > https://codereview.chromium.org/2035003002/diff/1/media/filters/frame_process... > File media/filters/frame_processor.cc (right): > > https://codereview.chromium.org/2035003002/diff/1/media/filters/frame_process... > media/filters/frame_processor.cc:298: // sequence mode. Reset it here only if in > segments mode. > On 2016/06/03 22:16:15, chcunningham wrote: > > Can you say why? I know I've asked before, but I've forgotten the answer and > the > > comment doesn't tell me. > > Done. (Also, refer to https://codereview.chromium.org/2035003002/#msg2 for previous discussion around your comment that I similarly had with acolwell@ in https://codereview.chromium.org/180153003/diff/210001/media/filters/frame_pro...).
Comments are great. Will review the tests when you're ready.
Description was changed from ========== MSE: Signal new coded frame group more aggressively For 'sequence' mode coded frame processing, we may need to signal a new coded frame group because application may override timestampOffset and cause the processed frame sequence to go backwards in time. Jumps forward are allowed and kept within the same coded frame group. Jumps backward are now detected and trigger new coded frame group signalling so the overlap-append logic in SourceBufferStream handles this situation more correctly. TEST=No regression locally. BUG=616565 ========== to ========== MSE: Signal new coded frame group more aggressively For 'sequence' mode coded frame processing, we may need to signal a new coded frame group because application may override timestampOffset and cause the processed frame sequence to go backwards in time. Jumps forward are allowed and kept within the same coded frame group. Jumps backward are now detected and trigger new coded frame group signalling so the overlap-append logic in SourceBufferStream handles this situation more correctly. TEST=No regression locally. BUG=616565,620253 ==========
Notes and reference to new bug, based on our discussion today: (I'll update the actual PS in this CL shortly.) https://codereview.chromium.org/2035003002/diff/40001/media/filters/frame_pro... File media/filters/frame_processor_unittest.cc (right): https://codereview.chromium.org/2035003002/diff/40001/media/filters/frame_pro... media/filters/frame_processor_unittest.cc:646: // Resulting in Actual: "{ [45,230) }" This is now understood by myself (and chcunningham@): Since the adjacency is gated on DTS+2*max_interbuffer_distance (and disregards coded frame group start time in this case), and since sequence mode was used with a large (explicit timestampOffset change) jump into the future, the adjacency threshold is really large now. https://codereview.chromium.org/2035003002/diff/40001/media/filters/frame_pro... media/filters/frame_processor_unittest.cc:655: // BIG TODO: Figure out why there is NOT a MergeWithAdjacentRangeIfNecessary Similarly, this is now understood: In Segments mode, the large jump forward by timestampOffset triggered a new coded frame group, so max_interbuffer_distance is still just 10ms and the adjacency threshold is still small and this range merger fails. Based on discussion with chcunningham@, the "max-interbuffer-distance" may grow too large in sequence mode coded frame groups that get large jumps forward due to timestampOffset. While subsequent range merges will have a much more lenient (large) adjacency threshold, if the app switches back to segments mode, that same (large) adjacency threshold would still apply. I've filed https://crbug.com/620523 to consider options to address this, if such lenient range merges indeed become a problem for apps using Chrome and mixed sequence+segments mode for the same SourceBuffer, with jumps forward using timestampOffset while in sequence mode.
Description was changed from ========== MSE: Signal new coded frame group more aggressively For 'sequence' mode coded frame processing, we may need to signal a new coded frame group because application may override timestampOffset and cause the processed frame sequence to go backwards in time. Jumps forward are allowed and kept within the same coded frame group. Jumps backward are now detected and trigger new coded frame group signalling so the overlap-append logic in SourceBufferStream handles this situation more correctly. TEST=No regression locally. BUG=616565,620253 ========== to ========== MSE: Signal new coded frame group more aggressively For 'sequence' mode coded frame processing, we may need to signal a new coded frame group because application may override timestampOffset and cause the processed frame sequence to go backwards in time. Jumps forward are allowed and kept within the same coded frame group. Jumps backward are now detected and trigger new coded frame group signalling so the overlap-append logic in SourceBufferStream handles this situation more correctly. TEST=No regression locally. BUG=616565,620523 ==========
Description was changed from ========== MSE: Signal new coded frame group more aggressively For 'sequence' mode coded frame processing, we may need to signal a new coded frame group because application may override timestampOffset and cause the processed frame sequence to go backwards in time. Jumps forward are allowed and kept within the same coded frame group. Jumps backward are now detected and trigger new coded frame group signalling so the overlap-append logic in SourceBufferStream handles this situation more correctly. TEST=No regression locally. BUG=616565,620523 ========== to ========== MSE: Signal new coded frame group more aggressively For 'sequence' mode coded frame processing, we may need to signal a new coded frame group because application may override timestampOffset and cause the processed frame sequence to go backwards in time. Jumps forward are allowed and kept within the same coded frame group. Jumps backward are now detected and trigger new coded frame group signalling so the overlap-append logic in SourceBufferStream handles this situation more correctly. TEST=*FrameProcessorTest.AudioVideo_Discontinuity_TimestampOffset* BUG=616565,620523 ==========
Description was changed from ========== MSE: Signal new coded frame group more aggressively For 'sequence' mode coded frame processing, we may need to signal a new coded frame group because application may override timestampOffset and cause the processed frame sequence to go backwards in time. Jumps forward are allowed and kept within the same coded frame group. Jumps backward are now detected and trigger new coded frame group signalling so the overlap-append logic in SourceBufferStream handles this situation more correctly. TEST=*FrameProcessorTest.AudioVideo_Discontinuity_TimestampOffset* BUG=616565,620523 ========== to ========== MSE: Signal new coded frame group more aggressively For 'sequence' mode coded frame processing, we may need to signal a new coded frame group because application may override timestampOffset and cause the processed frame sequence to go backwards in time. Jumps forward are allowed and kept within the same coded frame group. Jumps backward are now detected and trigger new coded frame group signalling so the overlap-append logic in SourceBufferStream handles this situation more correctly. TEST=*FrameProcessorTest.AudioVideo_Discontinuity_TimestampOffset*, http/tests/media/media-source/mediasource-crbug-616565.html BUG=616565,620523 ==========
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2035003002/80001
Please take a look. Note, without the fp.cc+fp.h part of this change, the new layout test indeed fails indicating it received the 'error' event (showing repro of bug 616565). The SegmentsMode version of the new unit test passes without the fp.cc+fp.h part of this change, but the SequenceMode crashes as expected. (When checking without the fp.cc+fp.h part of this change, I used the old version of fp_unittest.cc's "in_coded_frame_group()" helper to align with the old fp.cc+fp.h code, too). https://codereview.chromium.org/2035003002/diff/80001/media/filters/frame_pro... File media/filters/frame_processor_unittest.cc (right): https://codereview.chromium.org/2035003002/diff/80001/media/filters/frame_pro... media/filters/frame_processor_unittest.cc:680: audio_->AbortReads(); aside: I'll have a separate CL shortly that introduces a helper for these seeks (it'll be used by other tests in this file, too)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2035003002/80001
lgtm % nits https://codereview.chromium.org/2035003002/diff/80001/media/filters/frame_pro... File media/filters/frame_processor_unittest.cc (right): https://codereview.chromium.org/2035003002/diff/80001/media/filters/frame_pro... media/filters/frame_processor_unittest.cc:587: // Coded frame processing, even in 'sequence' mode if timestampOffset has The first sentence here is pretty hard to read. I'm not sure if it adds anything that isn't covered below it. https://codereview.chromium.org/2035003002/diff/80001/media/filters/frame_pro... media/filters/frame_processor_unittest.cc:628: // Note that, even for 'sequence' mode, we don't necessarily coalesce the end This comment seems inconsistent with the checks below. Sequence mode does (regrettably) see these ranges coalesce. https://codereview.chromium.org/2035003002/diff/80001/media/filters/frame_pro... media/filters/frame_processor_unittest.cc:632: base::TimeDelta fifty_five_ms = base::TimeDelta::FromMilliseconds(55); Why did this change from 45? https://codereview.chromium.org/2035003002/diff/80001/media/filters/frame_pro... media/filters/frame_processor_unittest.cc:664: audio_->AbortReads(); Do you need these first aborts? https://codereview.chromium.org/2035003002/diff/80001/media/filters/frame_pro... media/filters/frame_processor_unittest.cc:680: audio_->AbortReads(); On 2016/06/16 02:55:53, wolenetz_slow_reviews wrote: > aside: I'll have a separate CL shortly that introduces a helper for these seeks > (it'll be used by other tests in this file, too) Acknowledged.
Thanks for the review. https://codereview.chromium.org/2035003002/diff/80001/media/filters/frame_pro... File media/filters/frame_processor_unittest.cc (right): https://codereview.chromium.org/2035003002/diff/80001/media/filters/frame_pro... media/filters/frame_processor_unittest.cc:587: // Coded frame processing, even in 'sequence' mode if timestampOffset has On 2016/06/16 18:19:49, chcunningham wrote: > The first sentence here is pretty hard to read. I'm not sure if it adds anything > that isn't covered below it. Done. https://codereview.chromium.org/2035003002/diff/80001/media/filters/frame_pro... media/filters/frame_processor_unittest.cc:628: // Note that, even for 'sequence' mode, we don't necessarily coalesce the end On 2016/06/16 18:19:49, chcunningham wrote: > This comment seems inconsistent with the checks below. Sequence mode does > (regrettably) see these ranges coalesce. Done. https://codereview.chromium.org/2035003002/diff/80001/media/filters/frame_pro... media/filters/frame_processor_unittest.cc:632: base::TimeDelta fifty_five_ms = base::TimeDelta::FromMilliseconds(55); On 2016/06/16 18:19:49, chcunningham wrote: > Why did this change from 45? To make the comment in l.654 make sense. If 45, would never have been close enough even to a DTS of 100. Now that it's 55, DTS 85 is demonstrated to not be close enough to next range, even though that range has coded frame group start time of 100. https://codereview.chromium.org/2035003002/diff/80001/media/filters/frame_pro... media/filters/frame_processor_unittest.cc:664: audio_->AbortReads(); On 2016/06/16 18:19:49, chcunningham wrote: > Do you need these first aborts? Yeah, it's part of the ChunkDemuxerStream contract around seeking. In general, that contract protects against a read/seek race. In this test, we control precisely the read/seek sequence, but the production code has a state machine with DCHECKs to help enforce general protection. tl;dr: DCHECKs on CDStream's internal |state_| would fail without AbortReads() here, because we have previously called StartReturningData() in AddTestTracks(), above. Note, in the follow-up CL (https://codereview.chromium.org/2072673002) that has the seek helper, there *is* an unnecessary initial AbortReads() used in AddTestTracks(), since the helper always does abort->seek->startreturningdata. I think the simplification in that CL is a net win.
The CQ bit was checked by wolenetz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chcunningham@chromium.org Link to the patchset: https://codereview.chromium.org/2035003002/#ps100001 (title: "Addressed chcunningham@'s comments from PS5")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2035003002/100001
Message was sent while issue was closed.
Description was changed from ========== MSE: Signal new coded frame group more aggressively For 'sequence' mode coded frame processing, we may need to signal a new coded frame group because application may override timestampOffset and cause the processed frame sequence to go backwards in time. Jumps forward are allowed and kept within the same coded frame group. Jumps backward are now detected and trigger new coded frame group signalling so the overlap-append logic in SourceBufferStream handles this situation more correctly. TEST=*FrameProcessorTest.AudioVideo_Discontinuity_TimestampOffset*, http/tests/media/media-source/mediasource-crbug-616565.html BUG=616565,620523 ========== to ========== MSE: Signal new coded frame group more aggressively For 'sequence' mode coded frame processing, we may need to signal a new coded frame group because application may override timestampOffset and cause the processed frame sequence to go backwards in time. Jumps forward are allowed and kept within the same coded frame group. Jumps backward are now detected and trigger new coded frame group signalling so the overlap-append logic in SourceBufferStream handles this situation more correctly. TEST=*FrameProcessorTest.AudioVideo_Discontinuity_TimestampOffset*, http/tests/media/media-source/mediasource-crbug-616565.html BUG=616565,620523 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== MSE: Signal new coded frame group more aggressively For 'sequence' mode coded frame processing, we may need to signal a new coded frame group because application may override timestampOffset and cause the processed frame sequence to go backwards in time. Jumps forward are allowed and kept within the same coded frame group. Jumps backward are now detected and trigger new coded frame group signalling so the overlap-append logic in SourceBufferStream handles this situation more correctly. TEST=*FrameProcessorTest.AudioVideo_Discontinuity_TimestampOffset*, http/tests/media/media-source/mediasource-crbug-616565.html BUG=616565,620523 ========== to ========== MSE: Signal new coded frame group more aggressively For 'sequence' mode coded frame processing, we may need to signal a new coded frame group because application may override timestampOffset and cause the processed frame sequence to go backwards in time. Jumps forward are allowed and kept within the same coded frame group. Jumps backward are now detected and trigger new coded frame group signalling so the overlap-append logic in SourceBufferStream handles this situation more correctly. TEST=*FrameProcessorTest.AudioVideo_Discontinuity_TimestampOffset*, http/tests/media/media-source/mediasource-crbug-616565.html BUG=616565,620523 Committed: https://crrev.com/b7982341ce30c1dfb165551b8c641d781eeb0159 Cr-Commit-Position: refs/heads/master@{#400249} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b7982341ce30c1dfb165551b8c641d781eeb0159 Cr-Commit-Position: refs/heads/master@{#400249} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
