Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1670033002:
Reland: MSE: Relax the 'media segment must begin with keyframe' requirement (Closed)
|
Created:
4 years, 10 months ago by wolenetz Modified:
4 years, 10 months ago Reviewers:
chcunningham CC:
chromium-reviews, 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. |
DescriptionReland: MSE: Relax the 'media segment must begin with keyframe' requirement
The current version of the MSE spec no longer requires that media segments
start with a keyframe. This change reworks the MSE implementation
so that it now allows tracks within media segments to not begin
with a keyframe.
To accomplish this requirement's relaxation, this CL:
1) Separates media segment signaling from coded frame group signaling:
MSE stream parsers need to report whether or not they are currently
in the middle of parsing a media segment, such as a WebM cluster, to
comply with spec disallowance of certain operations like changing
SourceBuffer appendMode when appendState is PARSING_MEDIA_SEGMENT.
However, SourceBufferStream only needs to be signaled when a new coded
frame group is beginning, for example when buffers are appended
following a discontinuity.
2) Since the FrameProcessor now can transitively signal SourceBufferStreams
when a new coded frame group is beginning (which might span multiple
media segments), the SourceBufferStream can rely upon the fact
that the next buffer appended must be a keyframe if it's been told
that a new coded frame group is starting. A CHECK() is included.
3) New ChunkDemuxerTests.
4) Various test cleanup to conform to the distinct "new coded frame group"
and "new media segment" signaling separation, as well as removal
of decode error verification when a media segment doesn't begin
with a keyframe.
5) Fixes regression found in previous attempt, tracked in bug 581125. In
short, relaxes the restriction on same timestamp sequences. A
MediaLog is added where decode error would occur previously.
6) Fixes regression found in previous attempt, tracked in bug 581458. In
short, prevents tiny gaps under the FrameProcessor's discontinuity
threshold from triggering buffered range gaps in SourceBufferStream if
there is an intervening removal of media from the range last appended
to, but the last appended GOP survived the removal.
7) Uses coded frame group start time more intelligently to determine
range adjacency on appends. This allows extremely jagged start
coded frame group appends to overlap an existing range, fixing a
DCHECK which could occur otherwise (and adhering more correctly
to muxed buffered range expectations.) Improves handling of range
removals that may occur between SBS::OnStartOfCodedFrameGroup()
and SBS::Append().
Note that the FrameProcessor already drops non-keyframes prior to the
first keyframe processed for a track following a discontinuity,
and the SourceBufferStream already drops non-keyframes prior to the
first keyframe processed for a track following a range removal that
removed the last appended position in the current coded frame group.
BUG=229412, 459546, 581125, 581458, 462575
R=chcunningham@chromium.org
TEST=Updated ChunkDemuxerTests, SourceBufferStreamTests,
FrameProcessorTests. Also locally tested: bug 459546's
original report no longer fails. A sample webm VP9 file
from b/26524063 no longer fails. GPM webm vp9/opus test no
longer fails. Bug 518854's original report's "flags_all.mp4"
no longer fails. Bugs 581125 and 581458 no longer repro.
Committed: https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb
Cr-Commit-Position: refs/heads/master@{#377368}
Patch Set 1 : Previous attempt that was reverted (from https://codereview.chromium.org/1091293005/#ps240001) onto… #
Total comments: 2
Patch Set 2 : Fixes and tests for bugs 581125 and 581458, which caused previous attempt to be reverted #
Total comments: 24
Patch Set 3 : Rebase plus interim patch set with a new test demonstrating there's more failure that needs fixing #
Total comments: 10
Patch Set 4 : Rebase plus handling item #7 from CL description #
Total comments: 15
Patch Set 5 : More tests for item #7 in CL description #
Total comments: 4
Patch Set 6 : Rebase to ToT (especially, ToT contains https://codereview.chromium.org/1692403002/) #Patch Set 7 : Fixes the new test that the rebase introduced to this CL #Patch Set 8 : Comment responses, rebase, new CHECK_LT in SBR is failing. Investigating still. #
Total comments: 2
Patch Set 9 : new CHECK_LT in SBR still failing. Updated related SBSTest to produce FrameProcessor-like output #Patch Set 10 : SBR's new CHECK_LT fixed to be CHECK_LE #
Total comments: 2
Patch Set 11 : Undid patch set 9's test change, since FrameProcessor *can* produce that output #
Messages
Total messages: 54 (20 generated)
Not quite ready for review yet. I need to do some cleanup of various log messages and "BIG TODO" items noted in patch set 1 still.
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/1670033002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1670033002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
To make this easier to review, PS#1 should be the old patch and PS#2 the second.
On 2016/02/05 18:43:41, DaleCurtis wrote: > To make this easier to review, PS#1 should be the old patch and PS#2 the second. Yep - that's the plan I discussed w/chcunningham@ EOD yesterday. I'm updating this CL shortly to match that sequence.
Patchset #1 (id:1) has been deleted
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Please take a look at patch set #2. Note that PS#1 is a rebased-onto-ToT version of the previously landed-then-reverted CL. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_range.h (right): https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.h:55: static bool IsUncommonSameTimestampSequence(bool prev_is_keyframe, Note that I've inverted the logic and the name here. This is handled at the callers correctly. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.h:207: // sequence after |buffers_.back()|, false othewise. nit-to-self: fix this typo. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:241: // Buffers within a coded frame group should be monotonically increasing. Note that IsMonotonicallyIncreasing() does the same work as the now-removed call to and method "IsNextTimestampValid". https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:440: // Update |range_for_next_append_| if it was previously |range| and should Note: this is the core of the fix for crbug 581458. Even tiny gaps *within* a media segment, prior to keyframe relaxation, would cause a buffered range gap if there were a removal of the first part of the media segment prior to the buffers following the tiny gap being appended. This fixes the same situation at the coded-frame-group level of abstraction. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:557: SourceBufferRange::IsUncommonSameTimestampSequence( Note, this is the core of the fix for crbug 581125. We now allow these uncommon sequences (which can occur across media segments even), but medialog (up to capped number) when they occur. This corresponds to the "don't drop b" version from our internal thread, chcunningham@. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4603: TEST_F(SourceBufferStreamTest, Note: I tried an equivalent (media-segment-version) of this test prior to keyframe relaxation and observed it also failed (a buffered range gap was created), as I suspected it would.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1670033002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1670033002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1670033002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1670033002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:564: new_coded_frame_group_ || Great find! This is *exactly* the sort of subtlety we were always worried about with separating media segment / GOP signalling. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:28: return current_is_keyframe && !prev_is_keyframe; Just to note: I wasn't sure how (un)common this is in the case of VP9 alt-ref frames. Word from fgalligan@ is that an alt-ref frame is *almost always* followed by a non-key frame. In other words, "uncommon" (rather than "impossible" or "error") is a good choice of naming here... https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:241: // Buffers within a coded frame group should be monotonically increasing. On 2016/02/05 23:27:24, wolenetz wrote: > Note that IsMonotonicallyIncreasing() does the same work as the now-removed call > to and method "IsNextTimestampValid". Acknowledged. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:440: // Update |range_for_next_append_| if it was previously |range| and should On 2016/02/05 23:27:24, wolenetz wrote: > Note: this is the core of the fix for crbug 581458. Even tiny gaps *within* a > media segment, prior to keyframe relaxation, would cause a buffered range gap if > there were a removal of the first part of the media segment prior to the buffers > following the tiny gap being appended. This fixes the same situation at the > coded-frame-group level of abstraction. Acknowledged. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:444: last_appended_buffer_timestamp_ != kNoDecodeTimestamp()) { I'm wondering about the case where we've just gotten a OnStartOfCodedFrameGroup, such that new_range should be the range_for_next_append_, but we don't pick it up here because last_appeneded_buffer_timestamp_ == kNoDecodeTimestamp()? I *think* what would happen is we would end up creating a *new* range on the next append... probably not what we want. I though MergeWithAjacentRange would save us, but then it seems that only merges with the range ahead in the range list, not the range behind. WDYT? It might be more correct to remove this guard on line 444 and change the lines below to read: DecodeTimestamp potential_next_append_timestamp = last_appended_buffer_timestamp_ == kNoTimestamp() ? coded_frame_group_start_time_ : last_appended_buffer_timestamp_ + base::TimeDelta::FromInternalValue(1); This probably needs a unit tests to be fully fleshed out. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:557: SourceBufferRange::IsUncommonSameTimestampSequence( On 2016/02/05 23:27:24, wolenetz wrote: > Note, this is the core of the fix for crbug 581125. We now allow these uncommon > sequences (which can occur across media segments even), but medialog (up to > capped number) when they occur. This corresponds to the "don't drop b" version > from our internal thread, chcunningham@. Acknowledged. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:941: // current append. Never be exclusive if a splice frame was generated because Does the splice buffers check really make sense here? If we never generate a splice in the event of same timestamp (not enough duration to make a good splice), then it should really be more of a DCHECK, and probably closer to the code where splices are generated instead of down here in PrepareRangesForNextAppend. Perhaps something for a follow up CL? https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4593: AdjacentCodedFrameGroupContinuation_NoGapCreatedByTinyGapInGroupContinuation) { Did this test fail before? I would expect the test with the remove to fail, but this one I expect to pass today because of the fudge factor in IsNextInSequence. Worth having either way, just want to check if I'm missing something. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4603: TEST_F(SourceBufferStreamTest, On 2016/02/05 23:27:24, wolenetz wrote: > Note: I tried an equivalent (media-segment-version) of this test prior to > keyframe relaxation and observed it also failed (a buffered range gap was > created), as I suspected it would. Acknowledged. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4608: RemoveInMs(0, 35, 60); Random aside: I have never liked the last "duration" argument to Remove. I know its part of the spec, but I don't follow why we need both duration and end timestamp. Could we not do with just end timestamp?
Just comment replies. https://codereview.chromium.org/1670033002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1670033002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:564: new_coded_frame_group_ || On 2016/02/09 00:21:08, chcunningham wrote: > Great find! This is *exactly* the sort of subtlety we were always worried about > with separating media segment / GOP signalling. Acknowledged. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:28: return current_is_keyframe && !prev_is_keyframe; On 2016/02/09 00:21:08, chcunningham wrote: > Just to note: I wasn't sure how (un)common this is in the case of VP9 alt-ref > frames. Word from fgalligan@ is that an alt-ref frame is *almost always* > followed by a non-key frame. In other words, "uncommon" (rather than > "impossible" or "error") is a good choice of naming here... Acknowledged. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:444: last_appended_buffer_timestamp_ != kNoDecodeTimestamp()) { On 2016/02/09 00:21:08, chcunningham wrote: > I'm wondering about the case where we've just gotten a OnStartOfCodedFrameGroup, > such that new_range should be the range_for_next_append_, but we don't pick it > up here because last_appeneded_buffer_timestamp_ == kNoDecodeTimestamp()? I > *think* what would happen is we would end up creating a *new* range on the next > append... probably not what we want. I though MergeWithAjacentRange would save > us, but then it seems that only merges with the range ahead in the range list, > not the range behind. WDYT? > > It might be more correct to remove this guard on line 444 and change the lines > below to read: > > DecodeTimestamp potential_next_append_timestamp = > last_appended_buffer_timestamp_ == kNoTimestamp() ? > coded_frame_group_start_time_ : > last_appended_buffer_timestamp_ + > base::TimeDelta::FromInternalValue(1); > > > This probably needs a unit tests to be fully fleshed out. I'll need to grok this a little more. I'll get an answer for this tomorrow morning :) https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:941: // current append. Never be exclusive if a splice frame was generated because On 2016/02/09 00:21:08, chcunningham wrote: > Does the splice buffers check really make sense here? If we never generate a > splice in the event of same timestamp (not enough duration to make a good > splice), then it should really be more of a DCHECK, and probably closer to the > code where splices are generated instead of down here in > PrepareRangesForNextAppend. Perhaps something for a follow up CL? > Hmm. PrepareRangesForNextAppend() actually calls GenerateSpliceFrame(), above (line 927). This DCHECK is part of protecting the borders to prevent a possibly divergent GenerateSpliceFrame() impl from breaking bigger things. Regardless, I think a change to the splice checking is out of scope of this CL. WDYT? https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4593: AdjacentCodedFrameGroupContinuation_NoGapCreatedByTinyGapInGroupContinuation) { On 2016/02/09 00:21:08, chcunningham wrote: > Did this test fail before? I would expect the test with the remove to fail, but > this one I expect to pass today because of the fudge factor in IsNextInSequence. > Worth having either way, just want to check if I'm missing something. It didn't fail (in an equivalent media-segment-version) prior to relaxing keyframe. I included it for completeness :) https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4608: RemoveInMs(0, 35, 60); On 2016/02/09 00:21:08, chcunningham wrote: > Random aside: I have never liked the last "duration" argument to Remove. I know > its part of the spec, but I don't follow why we need both duration and end > timestamp. Could we not do with just end timestamp? You're probably right that an equivalent version would use just end timestamp. Duration is simply more clearly understood in the spec, and is guaranteed to be >= the highest end timestamp across all tracks and SourceBuffers.
more comment replies. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:444: last_appended_buffer_timestamp_ != kNoDecodeTimestamp()) { On 2016/02/09 00:47:19, wolenetz wrote: > On 2016/02/09 00:21:08, chcunningham wrote: > > I'm wondering about the case where we've just gotten a > OnStartOfCodedFrameGroup, > > such that new_range should be the range_for_next_append_, but we don't pick it > > up here because last_appeneded_buffer_timestamp_ == kNoDecodeTimestamp()? I > > *think* what would happen is we would end up creating a *new* range on the > next > > append... probably not what we want. I though MergeWithAjacentRange would save > > us, but then it seems that only merges with the range ahead in the range list, > > not the range behind. WDYT? > > > > It might be more correct to remove this guard on line 444 and change the lines > > below to read: > > > > DecodeTimestamp potential_next_append_timestamp = > > last_appended_buffer_timestamp_ == kNoTimestamp() ? > > coded_frame_group_start_time_ : > > last_appended_buffer_timestamp_ + > > base::TimeDelta::FromInternalValue(1); > > > > > > This probably needs a unit tests to be fully fleshed out. > > I'll need to grok this a little more. I'll get an answer for this tomorrow > morning :) Great comment, Chris! We've never tested any case like "starting a new {media segment or coded frame group}, but don't append anything yet. Instead remove something and then maybe append stuff later". Indeed this causes problems (as I've found while just now adding an initial test like this locally). I'll proceed with adding more tests and fixing the various issues (currently DCHECK on line 50 of source_buffer_range.cc occurs for the one test I've tried so far...)
Not ready for review yet. Just demos newly found bad stuff demonstrated in patch set 3. https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4657: AppendBuffers("2000K 2010"); This line causes DCHECK boom. Future patch set will include more such tests and associated fixes.
Description was changed from ========== Reland: MSE: Relax the 'media segment must begin with keyframe' requirement The current version of the MSE spec no longer requires that media segments start with a keyframe. This change reworks the MSE implementation so that it now allows tracks within media segments to not begin with a keyframe. To accomplish this requirement's relaxation, this CL: 1) Separates media segment signaling from coded frame group signaling: MSE stream parsers need to report whether or not they are currently in the middle of parsing a media segment, such as a WebM cluster, to comply with spec disallowance of certain operations like changing SourceBuffer appendMode when appendState is PARSING_MEDIA_SEGMENT. However, SourceBufferStream only needs to be signaled when a new coded frame group is beginning, for example when buffers are appended following a discontinuity. 2) Since the FrameProcessor now can transitively signal SourceBufferStreams when a new coded frame group is beginning (which might span multiple media segments), the SourceBufferStream can rely upon the fact that the next buffer appended must be a keyframe if it's been told that a new coded frame group is starting. A CHECK() is included. 3) New ChunkDemuxerTests. 4) Various test cleanup to conform to the distinct "new coded frame group" and "new media segment" signaling separation, as well as removal of decode error verification when a media segment doesn't begin with a keyframe. 5) Fixes regression found in previous attempt, tracked in bug 581125. In short, relaxes the restriction on same timestamp sequences. A MediaLog is added where decode error would occur previously. 6) Fixes regression found in previous attempt, tracked in bug 581458. In short, prevents tiny gaps under the FrameProcessor's discontinuity threshold from triggering buffered range gaps in SourceBufferStream if there is an intervening removal of media from the range last appended to, but the last appended GOP survived the removal. Note that the FrameProcessor already drops non-keyframes prior to the first keyframe processed for a track following a discontinuity, and the SourceBufferStream already drops non-keyframes prior to the first keyframe processed for a track following a range removal that removed the last appended position in the current coded frame group. BUG=229412,459546,581125,581458 R=chcunningham@chromium.org TEST=Updated ChunkDemuxerTests, SourceBufferStreamTests, FrameProcessorTests. Also locally tested: bug 459546's original report no longer fails. A sample webm VP9 file from b/26524063 no longer fails. GPM webm vp9/opus test no longer fails. Bug 518854's original report's "flags_all.mp4" no longer fails. Bugs 581125 and 581458 no longer repro. ========== to ========== Reland: MSE: Relax the 'media segment must begin with keyframe' requirement The current version of the MSE spec no longer requires that media segments start with a keyframe. This change reworks the MSE implementation so that it now allows tracks within media segments to not begin with a keyframe. To accomplish this requirement's relaxation, this CL: 1) Separates media segment signaling from coded frame group signaling: MSE stream parsers need to report whether or not they are currently in the middle of parsing a media segment, such as a WebM cluster, to comply with spec disallowance of certain operations like changing SourceBuffer appendMode when appendState is PARSING_MEDIA_SEGMENT. However, SourceBufferStream only needs to be signaled when a new coded frame group is beginning, for example when buffers are appended following a discontinuity. 2) Since the FrameProcessor now can transitively signal SourceBufferStreams when a new coded frame group is beginning (which might span multiple media segments), the SourceBufferStream can rely upon the fact that the next buffer appended must be a keyframe if it's been told that a new coded frame group is starting. A CHECK() is included. 3) New ChunkDemuxerTests. 4) Various test cleanup to conform to the distinct "new coded frame group" and "new media segment" signaling separation, as well as removal of decode error verification when a media segment doesn't begin with a keyframe. 5) Fixes regression found in previous attempt, tracked in bug 581125. In short, relaxes the restriction on same timestamp sequences. A MediaLog is added where decode error would occur previously. 6) Fixes regression found in previous attempt, tracked in bug 581458. In short, prevents tiny gaps under the FrameProcessor's discontinuity threshold from triggering buffered range gaps in SourceBufferStream if there is an intervening removal of media from the range last appended to, but the last appended GOP survived the removal. Note that the FrameProcessor already drops non-keyframes prior to the first keyframe processed for a track following a discontinuity, and the SourceBufferStream already drops non-keyframes prior to the first keyframe processed for a track following a range removal that removed the last appended position in the current coded frame group. 7) Uses coded frame group start time more intelligently to determine range adjacency on appends. This allows extremely jagged start coded frame group appends to overlap an existing range, fixing a DCHECK which could occur otherwise (and adhering more correctly to muxed buffered range expectations. Improves handling of range removals that may occur between SBS::OnStartOfCodedFrameGroup() and SBS::Append(). BUG=229412,459546,581125,581458 R=chcunningham@chromium.org TEST=Updated ChunkDemuxerTests, SourceBufferStreamTests, FrameProcessorTests. Also locally tested: bug 459546's original report no longer fails. A sample webm VP9 file from b/26524063 no longer fails. GPM webm vp9/opus test no longer fails. Bug 518854's original report's "flags_all.mp4" no longer fails. Bugs 581125 and 581458 no longer repro. ==========
Description was changed from ========== Reland: MSE: Relax the 'media segment must begin with keyframe' requirement The current version of the MSE spec no longer requires that media segments start with a keyframe. This change reworks the MSE implementation so that it now allows tracks within media segments to not begin with a keyframe. To accomplish this requirement's relaxation, this CL: 1) Separates media segment signaling from coded frame group signaling: MSE stream parsers need to report whether or not they are currently in the middle of parsing a media segment, such as a WebM cluster, to comply with spec disallowance of certain operations like changing SourceBuffer appendMode when appendState is PARSING_MEDIA_SEGMENT. However, SourceBufferStream only needs to be signaled when a new coded frame group is beginning, for example when buffers are appended following a discontinuity. 2) Since the FrameProcessor now can transitively signal SourceBufferStreams when a new coded frame group is beginning (which might span multiple media segments), the SourceBufferStream can rely upon the fact that the next buffer appended must be a keyframe if it's been told that a new coded frame group is starting. A CHECK() is included. 3) New ChunkDemuxerTests. 4) Various test cleanup to conform to the distinct "new coded frame group" and "new media segment" signaling separation, as well as removal of decode error verification when a media segment doesn't begin with a keyframe. 5) Fixes regression found in previous attempt, tracked in bug 581125. In short, relaxes the restriction on same timestamp sequences. A MediaLog is added where decode error would occur previously. 6) Fixes regression found in previous attempt, tracked in bug 581458. In short, prevents tiny gaps under the FrameProcessor's discontinuity threshold from triggering buffered range gaps in SourceBufferStream if there is an intervening removal of media from the range last appended to, but the last appended GOP survived the removal. Note that the FrameProcessor already drops non-keyframes prior to the first keyframe processed for a track following a discontinuity, and the SourceBufferStream already drops non-keyframes prior to the first keyframe processed for a track following a range removal that removed the last appended position in the current coded frame group. 7) Uses coded frame group start time more intelligently to determine range adjacency on appends. This allows extremely jagged start coded frame group appends to overlap an existing range, fixing a DCHECK which could occur otherwise (and adhering more correctly to muxed buffered range expectations. Improves handling of range removals that may occur between SBS::OnStartOfCodedFrameGroup() and SBS::Append(). BUG=229412,459546,581125,581458 R=chcunningham@chromium.org TEST=Updated ChunkDemuxerTests, SourceBufferStreamTests, FrameProcessorTests. Also locally tested: bug 459546's original report no longer fails. A sample webm VP9 file from b/26524063 no longer fails. GPM webm vp9/opus test no longer fails. Bug 518854's original report's "flags_all.mp4" no longer fails. Bugs 581125 and 581458 no longer repro. ========== to ========== Reland: MSE: Relax the 'media segment must begin with keyframe' requirement The current version of the MSE spec no longer requires that media segments start with a keyframe. This change reworks the MSE implementation so that it now allows tracks within media segments to not begin with a keyframe. To accomplish this requirement's relaxation, this CL: 1) Separates media segment signaling from coded frame group signaling: MSE stream parsers need to report whether or not they are currently in the middle of parsing a media segment, such as a WebM cluster, to comply with spec disallowance of certain operations like changing SourceBuffer appendMode when appendState is PARSING_MEDIA_SEGMENT. However, SourceBufferStream only needs to be signaled when a new coded frame group is beginning, for example when buffers are appended following a discontinuity. 2) Since the FrameProcessor now can transitively signal SourceBufferStreams when a new coded frame group is beginning (which might span multiple media segments), the SourceBufferStream can rely upon the fact that the next buffer appended must be a keyframe if it's been told that a new coded frame group is starting. A CHECK() is included. 3) New ChunkDemuxerTests. 4) Various test cleanup to conform to the distinct "new coded frame group" and "new media segment" signaling separation, as well as removal of decode error verification when a media segment doesn't begin with a keyframe. 5) Fixes regression found in previous attempt, tracked in bug 581125. In short, relaxes the restriction on same timestamp sequences. A MediaLog is added where decode error would occur previously. 6) Fixes regression found in previous attempt, tracked in bug 581458. In short, prevents tiny gaps under the FrameProcessor's discontinuity threshold from triggering buffered range gaps in SourceBufferStream if there is an intervening removal of media from the range last appended to, but the last appended GOP survived the removal. 7) Uses coded frame group start time more intelligently to determine range adjacency on appends. This allows extremely jagged start coded frame group appends to overlap an existing range, fixing a DCHECK which could occur otherwise (and adhering more correctly to muxed buffered range expectations. Improves handling of range removals that may occur between SBS::OnStartOfCodedFrameGroup() and SBS::Append(). Note that the FrameProcessor already drops non-keyframes prior to the first keyframe processed for a track following a discontinuity, and the SourceBufferStream already drops non-keyframes prior to the first keyframe processed for a track following a range removal that removed the last appended position in the current coded frame group. BUG=229412,459546,581125,581458 R=chcunningham@chromium.org TEST=Updated ChunkDemuxerTests, SourceBufferStreamTests, FrameProcessorTests. Also locally tested: bug 459546's original report no longer fails. A sample webm VP9 file from b/26524063 no longer fails. GPM webm vp9/opus test no longer fails. Bug 518854's original report's "flags_all.mp4" no longer fails. Bugs 581125 and 581458 no longer repro. ==========
Description was changed from ========== Reland: MSE: Relax the 'media segment must begin with keyframe' requirement The current version of the MSE spec no longer requires that media segments start with a keyframe. This change reworks the MSE implementation so that it now allows tracks within media segments to not begin with a keyframe. To accomplish this requirement's relaxation, this CL: 1) Separates media segment signaling from coded frame group signaling: MSE stream parsers need to report whether or not they are currently in the middle of parsing a media segment, such as a WebM cluster, to comply with spec disallowance of certain operations like changing SourceBuffer appendMode when appendState is PARSING_MEDIA_SEGMENT. However, SourceBufferStream only needs to be signaled when a new coded frame group is beginning, for example when buffers are appended following a discontinuity. 2) Since the FrameProcessor now can transitively signal SourceBufferStreams when a new coded frame group is beginning (which might span multiple media segments), the SourceBufferStream can rely upon the fact that the next buffer appended must be a keyframe if it's been told that a new coded frame group is starting. A CHECK() is included. 3) New ChunkDemuxerTests. 4) Various test cleanup to conform to the distinct "new coded frame group" and "new media segment" signaling separation, as well as removal of decode error verification when a media segment doesn't begin with a keyframe. 5) Fixes regression found in previous attempt, tracked in bug 581125. In short, relaxes the restriction on same timestamp sequences. A MediaLog is added where decode error would occur previously. 6) Fixes regression found in previous attempt, tracked in bug 581458. In short, prevents tiny gaps under the FrameProcessor's discontinuity threshold from triggering buffered range gaps in SourceBufferStream if there is an intervening removal of media from the range last appended to, but the last appended GOP survived the removal. 7) Uses coded frame group start time more intelligently to determine range adjacency on appends. This allows extremely jagged start coded frame group appends to overlap an existing range, fixing a DCHECK which could occur otherwise (and adhering more correctly to muxed buffered range expectations. Improves handling of range removals that may occur between SBS::OnStartOfCodedFrameGroup() and SBS::Append(). Note that the FrameProcessor already drops non-keyframes prior to the first keyframe processed for a track following a discontinuity, and the SourceBufferStream already drops non-keyframes prior to the first keyframe processed for a track following a range removal that removed the last appended position in the current coded frame group. BUG=229412,459546,581125,581458 R=chcunningham@chromium.org TEST=Updated ChunkDemuxerTests, SourceBufferStreamTests, FrameProcessorTests. Also locally tested: bug 459546's original report no longer fails. A sample webm VP9 file from b/26524063 no longer fails. GPM webm vp9/opus test no longer fails. Bug 518854's original report's "flags_all.mp4" no longer fails. Bugs 581125 and 581458 no longer repro. ========== to ========== Reland: MSE: Relax the 'media segment must begin with keyframe' requirement The current version of the MSE spec no longer requires that media segments start with a keyframe. This change reworks the MSE implementation so that it now allows tracks within media segments to not begin with a keyframe. To accomplish this requirement's relaxation, this CL: 1) Separates media segment signaling from coded frame group signaling: MSE stream parsers need to report whether or not they are currently in the middle of parsing a media segment, such as a WebM cluster, to comply with spec disallowance of certain operations like changing SourceBuffer appendMode when appendState is PARSING_MEDIA_SEGMENT. However, SourceBufferStream only needs to be signaled when a new coded frame group is beginning, for example when buffers are appended following a discontinuity. 2) Since the FrameProcessor now can transitively signal SourceBufferStreams when a new coded frame group is beginning (which might span multiple media segments), the SourceBufferStream can rely upon the fact that the next buffer appended must be a keyframe if it's been told that a new coded frame group is starting. A CHECK() is included. 3) New ChunkDemuxerTests. 4) Various test cleanup to conform to the distinct "new coded frame group" and "new media segment" signaling separation, as well as removal of decode error verification when a media segment doesn't begin with a keyframe. 5) Fixes regression found in previous attempt, tracked in bug 581125. In short, relaxes the restriction on same timestamp sequences. A MediaLog is added where decode error would occur previously. 6) Fixes regression found in previous attempt, tracked in bug 581458. In short, prevents tiny gaps under the FrameProcessor's discontinuity threshold from triggering buffered range gaps in SourceBufferStream if there is an intervening removal of media from the range last appended to, but the last appended GOP survived the removal. 7) Uses coded frame group start time more intelligently to determine range adjacency on appends. This allows extremely jagged start coded frame group appends to overlap an existing range, fixing a DCHECK which could occur otherwise (and adhering more correctly to muxed buffered range expectations.) Improves handling of range removals that may occur between SBS::OnStartOfCodedFrameGroup() and SBS::Append(). Note that the FrameProcessor already drops non-keyframes prior to the first keyframe processed for a track following a discontinuity, and the SourceBufferStream already drops non-keyframes prior to the first keyframe processed for a track following a range removal that removed the last appended position in the current coded frame group. BUG=229412,459546,581125,581458 R=chcunningham@chromium.org TEST=Updated ChunkDemuxerTests, SourceBufferStreamTests, FrameProcessorTests. Also locally tested: bug 459546's original report no longer fails. A sample webm VP9 file from b/26524063 no longer fails. GPM webm vp9/opus test no longer fails. Bug 518854's original report's "flags_all.mp4" no longer fails. Bugs 581125 and 581458 no longer repro. ==========
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/1670033002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1670033002/100001
Please take a look at patch set 5. The marginal change to include the new work (see item #7 in CL description) is non-trivial. We can discuss breaking that out into a prerequisite CL, if you really wish. Ideally, I'd like to land this as one piece that hopefully sticks, to ease any potential merge back to M49. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_range.h (right): https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.h:207: // sequence after |buffers_.back()|, false othewise. On 2016/02/05 23:27:24, wolenetz wrote: > nit-to-self: fix this typo. Done. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:444: last_appended_buffer_timestamp_ != kNoDecodeTimestamp()) { On 2016/02/09 01:36:41, wolenetz wrote: > On 2016/02/09 00:47:19, wolenetz wrote: > > On 2016/02/09 00:21:08, chcunningham wrote: > > > I'm wondering about the case where we've just gotten a > > OnStartOfCodedFrameGroup, > > > such that new_range should be the range_for_next_append_, but we don't pick > it > > > up here because last_appeneded_buffer_timestamp_ == kNoDecodeTimestamp()? I > > > *think* what would happen is we would end up creating a *new* range on the > > next > > > append... probably not what we want. I though MergeWithAjacentRange would > save > > > us, but then it seems that only merges with the range ahead in the range > list, > > > not the range behind. WDYT? > > > > > > It might be more correct to remove this guard on line 444 and change the > lines > > > below to read: > > > > > > DecodeTimestamp potential_next_append_timestamp = > > > last_appended_buffer_timestamp_ == kNoTimestamp() ? > > > coded_frame_group_start_time_ : > > > last_appended_buffer_timestamp_ + > > > base::TimeDelta::FromInternalValue(1); > > > > > > > > > This probably needs a unit tests to be fully fleshed out. > > > > I'll need to grok this a little more. I'll get an answer for this tomorrow > > morning :) > > Great comment, Chris! > > We've never tested any case like "starting a new {media segment or coded frame > group}, but don't append anything yet. Instead remove something and then maybe > append stuff later". Indeed this causes problems (as I've found while just now > adding an initial test like this locally). > > I'll proceed with adding more tests and fixing the various issues (currently > DCHECK on line 50 of source_buffer_range.cc occurs for the one test I've tried > so far...) Done. (as of patch set #5) https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:941: // current append. Never be exclusive if a splice frame was generated because On 2016/02/09 00:47:19, wolenetz wrote: > On 2016/02/09 00:21:08, chcunningham wrote: > > Does the splice buffers check really make sense here? If we never generate a > > splice in the event of same timestamp (not enough duration to make a good > > splice), then it should really be more of a DCHECK, and probably closer to the > > code where splices are generated instead of down here in > > PrepareRangesForNextAppend. Perhaps something for a follow up CL? > > > > Hmm. PrepareRangesForNextAppend() actually calls GenerateSpliceFrame(), above > (line 927). This DCHECK is part of protecting the borders to prevent a possibly > divergent GenerateSpliceFrame() impl from breaking bigger things. Regardless, I > think a change to the splice checking is out of scope of this CL. WDYT? From chat, this is fodder for a low pri later CL. https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4657: AppendBuffers("2000K 2010"); On 2016/02/09 01:42:31, wolenetz wrote: > This line causes DCHECK boom. Future patch set will include more such tests and > associated fixes. This is a bit ugly: the DCHECK fails due to the range gap between the DTS of the last buffer in the range being appended to (1080ms) and the actual first buffer appended in the new coded frame group (2000). This is a bit extreme, but demonstrates an edge case for which our GetFudgeRoom logic is insufficient. I believe this issue predates keyframe relaxation. Patch set 5 includes fixes and more tests like this. https://codereview.chromium.org/1670033002/diff/100001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1670033002/diff/100001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:909: // bear-640x360.webm : [527-760) This is intentional. As of patch set 4-5, the removal range for a new coded frame group append extends earlier in time potentially than the first new frame for the track in the group (it can go back to the coded frame group start time.) Also, the test media includes audio discontinuities at every webm cluster boundary, triggering frequent new coded frame groups (and removal logic that now removes a little more). ChunkDemuxerTest.ConfigChange_Video is also adjusted to match this. https://codereview.chromium.org/1670033002/diff/100001/media/filters/source_b... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1670033002/diff/100001/media/filters/source_b... media/filters/source_buffer_stream_unittest.cc:4267: CheckExpectedBuffers("0K 0K 1K 1K"); This is intentional. Due to the changes in PS4-5 that extend the removal range at the beginning of a new coded frame group back to the coded frame group start time, the original buffer at time 1.0ms is removed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Minor comments. Also, I want to try to take a stab at explaining how part 7 (in your cl description) came to be a part of this change... for my own understanding and for public record, since this stuff is subtle. As I see, part 7 is really two separate changes 1) Addresses my comments in PS2 SouceBufferStream::RemoveInternal - basically fixing a bug where Remove could be called just after OnStartOfCodedFrameGroup without having done any appends for the new coded frame group. Using PotentialNextAppendTimestamp addresses my concerns :) 2) Allow for muxed streams to have jagged start times for coded frame groups without having the jaggedness cause gaps in the buffered ranges. Accomplished by adding new context of "new_buffers_group_start_timestamp" to SBR::AppendBuffersToEnd and SBR::CanAppendBuffersToEnd, thereby avoiding a DCHECK. 2a) Consider the jaggedness in SBS when PreparingRangesForNextAppend by deleting any existing overlappping range beginning with the coded_frame_group_start_ rather than trusting the potentially jagged new_buffers.front() timestamp. Now... I remember you saying that part 2 is in this CL because untangling media-segment vs coded-frame-group somehow made this existing jaggedness issue more pronounced... I forget exactly how. Can you remind me? https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_bu... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_bu... media/filters/source_buffer_range.cc:162: new_range_start_timestamp = timestamp; I might be missing something here, but this sounds like a recipe for making *this* range become an empty range. If so, I don't see any handling (removal) of the empty range in SourceBufferStream. If this is effectively creating a new range that has all the content of the existing range.. maybe the right thing to do there is just return null? ie - this range cannot be split. https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_bu... media/filters/source_buffer_range.cc:178: CHECK_GE(split_range->next_buffer_index_, 0) You might also add a check for next_buffer_index < split_range_->size() https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4657: AppendBuffers("2000K 2010"); On 2016/02/12 01:23:47, wolenetz wrote: > On 2016/02/09 01:42:31, wolenetz wrote: > > This line causes DCHECK boom. Future patch set will include more such tests > and > > associated fixes. > > This is a bit ugly: the DCHECK fails due to the range gap between the DTS of the > last buffer in the range being appended to (1080ms) and the actual first buffer > appended in the new coded frame group (2000). This is a bit extreme, but > demonstrates an edge case for which our GetFudgeRoom logic is insufficient. I > believe this issue predates keyframe relaxation. > > Patch set 5 includes fixes and more tests like this. Acknowledged. https://codereview.chromium.org/1670033002/diff/80001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1670033002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3208: EXPECT_EQ(last_timestamp.InMilliseconds(), 760); Whats this small adjustment about? https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... media/filters/source_buffer_range.cc:53: DCHECK(buffers_.empty() || DCHECK -> CHECK? https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... media/filters/source_buffer_range.cc:55: DCHECK(range_start_time_ == kNoDecodeTimestamp() || Can range start time really be kNoDecodeTimestamp? What scenario? https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... media/filters/source_buffer_range.cc:565: // group and the first buffer, then just pretend there is a keyframe at the Why do we pretend? Why not return the actual timestamp of the first key frame? https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:442: base::TimeDelta::FromInternalValue(1); What if the next timestamp is the same as the last timestamp? Should we not do the +1 micro
LGTM % my nit (and answers to less critical questions from last round of comments) https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:442: base::TimeDelta::FromInternalValue(1); On 2016/02/12 22:46:40, chcunningham wrote: > What if the next timestamp is the same as the last timestamp? Should we not do > the +1 micro We chatted. Please make a todo && crbug. This is at least no worse than it was previously and doesn't seem to be impacted by key frame relaxation.
On 2016/02/12 23:25:36, chcunningham wrote: > LGTM % my nit (and answers to less critical questions from last round of > comments) > > https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... > File media/filters/source_buffer_stream.cc (right): > > https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... > media/filters/source_buffer_stream.cc:442: > base::TimeDelta::FromInternalValue(1); > On 2016/02/12 22:46:40, chcunningham wrote: > > What if the next timestamp is the same as the last timestamp? Should we not do > > the +1 micro > > We chatted. Please make a todo && crbug. This is at least no worse than it was > previously and doesn't seem to be impacted by key frame relaxation. Thanks for review. I'll rebase and update this as we discussed offline, and also per your two sets of CR comments from Feb 12th once https://codereview.chromium.org/1692403002/ lands, since that other CL needs to be more easily mergeable than this one.
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/1670033002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1670033002/140001
On 2016/02/16 23:53:05, wolenetz wrote: > On 2016/02/12 23:25:36, chcunningham wrote: > > LGTM % my nit (and answers to less critical questions from last round of > > comments) > > > > > https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... > > File media/filters/source_buffer_stream.cc (right): > > > > > https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... > > media/filters/source_buffer_stream.cc:442: > > base::TimeDelta::FromInternalValue(1); > > On 2016/02/12 22:46:40, chcunningham wrote: > > > What if the next timestamp is the same as the last timestamp? Should we not > do > > > the +1 micro > > > > We chatted. Please make a todo && crbug. This is at least no worse than it was > > previously and doesn't seem to be impacted by key frame relaxation. > > Thanks for review. I'll rebase and update this as we discussed offline, and also > per your two sets of CR comments from Feb 12th once > https://codereview.chromium.org/1692403002/ lands, since that other CL needs to > be more easily mergeable than this one. Rebase is done. I'll continue with the rest of the updates and land this tomorrow morning.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/02/12 22:46:41, chcunningham wrote: > Minor comments. > > Also, I want to try to take a stab at explaining how part 7 (in your cl > description) came to be a part of this change... for my own understanding and > for public record, since this stuff is subtle. > > As I see, part 7 is really two separate changes > > 1) Addresses my comments in PS2 SouceBufferStream::RemoveInternal - basically > fixing a bug where Remove could be called just after OnStartOfCodedFrameGroup > without having done any appends for the new coded frame group. Using > PotentialNextAppendTimestamp addresses my concerns :) > > 2) Allow for muxed streams to have jagged start times for coded frame groups > without having the jaggedness cause gaps in the buffered ranges. Accomplished by > adding new context of "new_buffers_group_start_timestamp" to > SBR::AppendBuffersToEnd and SBR::CanAppendBuffersToEnd, thereby avoiding a > DCHECK. > > 2a) Consider the jaggedness in SBS when PreparingRangesForNextAppend by deleting > any existing overlappping range beginning with the coded_frame_group_start_ > rather than trusting the potentially jagged new_buffers.front() timestamp. > > > Now... I remember you saying that part 2 is in this CL because untangling > media-segment vs coded-frame-group somehow made this existing jaggedness issue > more pronounced... I forget exactly how. Can you remind me? I think we chatted about this last week, but for posterity: A muxed A/V coded frame group could perhaps be so jagged-started that perhaps not all A/V tracks have their first coded frame in the group for quite some time (late in the first media segment, or perhaps not until some much-later media segment). Parts 2+2a (in your comment) intends to preserve the nature of the muxed stream such that the stream has identified "A and V start at time X". If either the first A or V coded frame doesn't arrive until actual time X+delta, part 2+2a causes any overlap of previously buffered A/V within [X, X+delta) to be removed once (if ever) the frame at X+delta finally arrives. Prior to this keyframe relaxation CL, **delta was limited by media segment duration**, since every media segment reset X. Now that this CL trusts the coded frame processing algorithm to correctly identify and signal coded frame group starts irrespective of media segment boundaries, it makes sense to also buffer those groups (including removal of underlying overlaps in the [X, X+delta)) together across SourceBufferStreams (and note that delta *could* now be longer than the first media segment in the coded frame group, hence "more pronounced"). Further, it makes sense to retain knowledge of the coded frame group start such that range membership of the first frame for each stream in a coded frame group is continuous with whatever (if any) previously buffered range the coded frame group start is adjacent to or within. Note that I attempted to constrain delta to be within the first media segment of a new coded frame group in the MSE spec (see the discussion contained within https://github.com/w3c/media-source/pull/43), but that received enough concern of potentially regressing MSE API users, for instance, requiring low-latency mp4 muxed A/V playback via MSE.
Description was changed from ========== Reland: MSE: Relax the 'media segment must begin with keyframe' requirement The current version of the MSE spec no longer requires that media segments start with a keyframe. This change reworks the MSE implementation so that it now allows tracks within media segments to not begin with a keyframe. To accomplish this requirement's relaxation, this CL: 1) Separates media segment signaling from coded frame group signaling: MSE stream parsers need to report whether or not they are currently in the middle of parsing a media segment, such as a WebM cluster, to comply with spec disallowance of certain operations like changing SourceBuffer appendMode when appendState is PARSING_MEDIA_SEGMENT. However, SourceBufferStream only needs to be signaled when a new coded frame group is beginning, for example when buffers are appended following a discontinuity. 2) Since the FrameProcessor now can transitively signal SourceBufferStreams when a new coded frame group is beginning (which might span multiple media segments), the SourceBufferStream can rely upon the fact that the next buffer appended must be a keyframe if it's been told that a new coded frame group is starting. A CHECK() is included. 3) New ChunkDemuxerTests. 4) Various test cleanup to conform to the distinct "new coded frame group" and "new media segment" signaling separation, as well as removal of decode error verification when a media segment doesn't begin with a keyframe. 5) Fixes regression found in previous attempt, tracked in bug 581125. In short, relaxes the restriction on same timestamp sequences. A MediaLog is added where decode error would occur previously. 6) Fixes regression found in previous attempt, tracked in bug 581458. In short, prevents tiny gaps under the FrameProcessor's discontinuity threshold from triggering buffered range gaps in SourceBufferStream if there is an intervening removal of media from the range last appended to, but the last appended GOP survived the removal. 7) Uses coded frame group start time more intelligently to determine range adjacency on appends. This allows extremely jagged start coded frame group appends to overlap an existing range, fixing a DCHECK which could occur otherwise (and adhering more correctly to muxed buffered range expectations.) Improves handling of range removals that may occur between SBS::OnStartOfCodedFrameGroup() and SBS::Append(). Note that the FrameProcessor already drops non-keyframes prior to the first keyframe processed for a track following a discontinuity, and the SourceBufferStream already drops non-keyframes prior to the first keyframe processed for a track following a range removal that removed the last appended position in the current coded frame group. BUG=229412,459546,581125,581458 R=chcunningham@chromium.org TEST=Updated ChunkDemuxerTests, SourceBufferStreamTests, FrameProcessorTests. Also locally tested: bug 459546's original report no longer fails. A sample webm VP9 file from b/26524063 no longer fails. GPM webm vp9/opus test no longer fails. Bug 518854's original report's "flags_all.mp4" no longer fails. Bugs 581125 and 581458 no longer repro. ========== to ========== Reland: MSE: Relax the 'media segment must begin with keyframe' requirement The current version of the MSE spec no longer requires that media segments start with a keyframe. This change reworks the MSE implementation so that it now allows tracks within media segments to not begin with a keyframe. To accomplish this requirement's relaxation, this CL: 1) Separates media segment signaling from coded frame group signaling: MSE stream parsers need to report whether or not they are currently in the middle of parsing a media segment, such as a WebM cluster, to comply with spec disallowance of certain operations like changing SourceBuffer appendMode when appendState is PARSING_MEDIA_SEGMENT. However, SourceBufferStream only needs to be signaled when a new coded frame group is beginning, for example when buffers are appended following a discontinuity. 2) Since the FrameProcessor now can transitively signal SourceBufferStreams when a new coded frame group is beginning (which might span multiple media segments), the SourceBufferStream can rely upon the fact that the next buffer appended must be a keyframe if it's been told that a new coded frame group is starting. A CHECK() is included. 3) New ChunkDemuxerTests. 4) Various test cleanup to conform to the distinct "new coded frame group" and "new media segment" signaling separation, as well as removal of decode error verification when a media segment doesn't begin with a keyframe. 5) Fixes regression found in previous attempt, tracked in bug 581125. In short, relaxes the restriction on same timestamp sequences. A MediaLog is added where decode error would occur previously. 6) Fixes regression found in previous attempt, tracked in bug 581458. In short, prevents tiny gaps under the FrameProcessor's discontinuity threshold from triggering buffered range gaps in SourceBufferStream if there is an intervening removal of media from the range last appended to, but the last appended GOP survived the removal. 7) Uses coded frame group start time more intelligently to determine range adjacency on appends. This allows extremely jagged start coded frame group appends to overlap an existing range, fixing a DCHECK which could occur otherwise (and adhering more correctly to muxed buffered range expectations.) Improves handling of range removals that may occur between SBS::OnStartOfCodedFrameGroup() and SBS::Append(). Note that the FrameProcessor already drops non-keyframes prior to the first keyframe processed for a track following a discontinuity, and the SourceBufferStream already drops non-keyframes prior to the first keyframe processed for a track following a range removal that removed the last appended position in the current coded frame group. BUG=229412,459546,581125,581458,462575 R=chcunningham@chromium.org TEST=Updated ChunkDemuxerTests, SourceBufferStreamTests, FrameProcessorTests. Also locally tested: bug 459546's original report no longer fails. A sample webm VP9 file from b/26524063 no longer fails. GPM webm vp9/opus test no longer fails. Bug 518854's original report's "flags_all.mp4" no longer fails. Bugs 581125 and 581458 no longer repro. ==========
Addressed all current CR comments. Except: the new CHECK_LT in SBR is failing media_unittests. I'm investigating. It might require yet another prereq SBR fix land first with this rebasing on top of it. https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_bu... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_bu... media/filters/source_buffer_range.cc:162: new_range_start_timestamp = timestamp; On 2016/02/12 22:46:40, chcunningham wrote: > I might be missing something here, but this sounds like a recipe for making > *this* range become an empty range. If so, I don't see any handling (removal) of > the empty range in SourceBufferStream. > > If this is effectively creating a new range that has all the content of the > existing range.. maybe the right thing to do there is just return null? ie - > this range cannot be split. Hmm. This doesn't appear to be broken, though is a little confusing and out of scope of this CL: SourceBufferStream calls SBR::TruncateAt(timestamp, a boolean) shortly after SBR::SplitRange(timestamp). If the split caused |buffers_| to become empty, then the truncate range should also be empty and TruncateAt should return true (meaning, the range is empty and should be deleted), regardless of the TruncateAt boolean parameter value. I added a code comment at least to SplitRange to indicate that |this| might become empty. https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_bu... media/filters/source_buffer_range.cc:178: CHECK_GE(split_range->next_buffer_index_, 0) On 2016/02/12 22:46:40, chcunningham wrote: > You might also add a check for next_buffer_index < split_range_->size() Done.... BUT: the new CHECK_LT triggered a crash in SourceBufferStreamTest.RemoveShouldAlwaysExcludeEnd !! I'm investigating and will post a further patch set once I figure that out. https://codereview.chromium.org/1670033002/diff/80001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1670033002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3208: EXPECT_EQ(last_timestamp.InMilliseconds(), 760); On 2016/02/12 22:46:40, chcunningham wrote: > Whats this small adjustment about? Per l.909 and l.916, and per the new "remove previously buffered overlapped region between coded-frame-group-start and the start of the first frame for this track since the coded frame group started" logic, there is one less video coded frame buffered from where the 320x200 bear1 coded frame group (with start at time 779 (jagged start, audio: 779, video:801)) overlapped the 640x480 bear2 append. So there is an expected additional remove of [779,801) included in the remove [new video coded frames from 801 onwards) done as part of the append to the video SBS. This is "part 2a" from your CR comment breakdown of step 7. See also InitDemuxerWithConfigChangeData() for details on how the API is exercised to setup the SourceBufferStreams for these config change tests. https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... media/filters/source_buffer_range.cc:53: DCHECK(buffers_.empty() || On 2016/02/12 22:46:40, chcunningham wrote: > DCHECK -> CHECK? Done. https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... media/filters/source_buffer_range.cc:55: DCHECK(range_start_time_ == kNoDecodeTimestamp() || On 2016/02/12 22:46:40, chcunningham wrote: > Can range start time really be kNoDecodeTimestamp? What scenario? Yes. See the comment for |range_start_time_| in the header. See also SplitRange()'s "new_range_start_timestamp", DeleteGOPFromFront(), etc. tl;dr: it's kNoDecodeTimestamp when the front of a range is involved in a remove such that it may no longer be associated with a "coded frame group start time". In such case, GetStartTimestamp() just returns the DTS of the first buffer in the range (which must not be empty....) https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... media/filters/source_buffer_range.cc:565: // group and the first buffer, then just pretend there is a keyframe at the On 2016/02/12 22:46:40, chcunningham wrote: > Why do we pretend? Why not return the actual timestamp of the first key frame? I think it simplifies seeking by SBS. (See FindNewSelectedRangeSeekTimestamp(), for instance). It simplifies pruning of SBS |track_buffer_| in muxed SourceBuffer in the presence of a jagged start (see near the end of SBS::Append() for the pruning.) Perhaps we could "simplify" it better, but out of scope of this CL. https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:442: base::TimeDelta::FromInternalValue(1); On 2016/02/12 23:25:36, chcunningham wrote: > On 2016/02/12 22:46:40, chcunningham wrote: > > What if the next timestamp is the same as the last timestamp? Should we not do > > the +1 micro > > We chatted. Please make a todo && crbug. This is at least no worse than it was > previously and doesn't seem to be impacted by key frame relaxation. I think this +1us was useful when we previously had more strict disallowance of Same Timestamp sequences of various kinds (now more lenient) along with uncertainty about all buffers having nonzero duration (crbug 312836). Since SourceBufferRange::BelongsToRange() is now more lenient w.r.t. same timestamp sequences, I think we can relax this particular +1us. I'm on the fence about doing that in this CL at this point, so I've gone with our previous decision to TODO + crbug for this. https://codereview.chromium.org/1670033002/diff/100001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1670033002/diff/100001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:909: // bear-640x360.webm : [527-760) On 2016/02/12 01:23:47, wolenetz_slow_reviews_feb22-23 wrote: > This is intentional. As of patch set 4-5, the removal range for a new coded > frame group append extends earlier in time potentially than the first new frame > for the track in the group (it can go back to the coded frame group start time.) > Also, the test media includes audio discontinuities at every webm cluster > boundary, triggering frequent new coded frame groups (and removal logic that now > removes a little more). > ChunkDemuxerTest.ConfigChange_Video is also adjusted to match this. Note this earlier comment was meant to help explain the related ConfigChange_Video test change.
PS9 is just a test cleanup patch set. The same new CHECK_LT in SBR is failing (and I'll send a later patch set or perhaps rebase to address that failure). Failure snippet: [28933:28933:0223/171328:625044791838:VERBOSE1:source_buffer_stream.cc(235)] Append VIDEO: buffers dts/pts=[0.008;0.008(last frame dur=0.002)] [28933:28933:0223/171328:625044791891:VERBOSE2:source_buffer_stream.cc(461)] RemoveInternal VIDEO (0.008, 0.01, 1) [28933:28933:0223/171328:625044791920:VERBOSE3:source_buffer_stream.cc(464)] RemoveInternal VIDEO: before remove ranges_=[0.008;0.014(0.016)] [28933:28933:0223/171328:625044792014:FATAL:source_buffer_range.cc(186)] Check failed: static_cast<unsigned int>(split_range_next_buffer_index) < removed_buffers.size() (3 vs. 3) https://codereview.chromium.org/1670033002/diff/160001/media/filters/source_b... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1670033002/diff/160001/media/filters/source_b... media/filters/source_buffer_stream_unittest.cc:4342: // of the append (and any later dependent frames) to be removed. FrameProcessor will never emit a non-keyframe at the beginning of a coded frame group. Similarly, it would have detected discontinuity in sequence of "0D10K 0D10" and dropped the second frame since it wasn't a keyframe. I've updated this test to make the first frame a zero duration keyframe that is close enough to the subsequent range for our code to merge it into the same range and still conform to the test intent and also allow a same-timestamp nonkeyframe (not at beginning of coded frame group) to be appended next, to match what FrameProcessor would do.
Please take a look at patch set 10. Nothing should be surprising per our previous discussions IIUC except I included a cleanup of a related SBSTest in patch set 9 since your l*tm.
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/1670033002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1670033002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Per chat, please take a look at patch set 11. It should be even less surprising :) Thanks! https://codereview.chromium.org/1670033002/diff/160001/media/filters/source_b... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1670033002/diff/160001/media/filters/source_b... media/filters/source_buffer_stream_unittest.cc:4342: // of the append (and any later dependent frames) to be removed. On 2016/02/24 01:14:02, wolenetz_slow_reviews_feb22-23 wrote: > FrameProcessor will never emit a non-keyframe at the beginning of a coded frame > group. Similarly, it would have detected discontinuity in sequence of "0D10K > 0D10" and dropped the second frame since it wasn't a keyframe. I've updated this > test to make the first frame a zero duration keyframe that is close enough to > the subsequent range for our code to merge it into the same range and still > conform to the test intent and also allow a same-timestamp nonkeyframe (not at > beginning of coded frame group) to be appended next, to match what > FrameProcessor would do. I was incorrect. Coded frame processing algorithm allows "0D10K 0D10" sequence without indicating discontinuity between the two. I'll change this test back to what it was to lessen the scope of this CL.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1670033002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1670033002/220001
https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_bu... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_bu... media/filters/source_buffer_range.cc:162: new_range_start_timestamp = timestamp; On 2016/02/24 00:34:46, wolenetz_slow_reviews_feb22-23 wrote: > On 2016/02/12 22:46:40, chcunningham wrote: > > I might be missing something here, but this sounds like a recipe for making > > *this* range become an empty range. If so, I don't see any handling (removal) > of > > the empty range in SourceBufferStream. > > > > If this is effectively creating a new range that has all the content of the > > existing range.. maybe the right thing to do there is just return null? ie - > > this range cannot be split. > > Hmm. This doesn't appear to be broken, though is a little confusing and out of > scope of this CL: SourceBufferStream calls SBR::TruncateAt(timestamp, a boolean) > shortly after SBR::SplitRange(timestamp). If the split caused |buffers_| to > become empty, then the truncate range should also be empty and TruncateAt should > return true (meaning, the range is empty and should be deleted), regardless of > the TruncateAt boolean parameter value. I added a code comment at least to > SplitRange to indicate that |this| might become empty. Acknowledged. https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_bu... media/filters/source_buffer_range.cc:178: CHECK_GE(split_range->next_buffer_index_, 0) On 2016/02/24 00:34:46, wolenetz_slow_reviews_feb22-23 wrote: > On 2016/02/12 22:46:40, chcunningham wrote: > > You might also add a check for next_buffer_index < split_range_->size() > > Done.... BUT: the new CHECK_LT triggered a crash in > SourceBufferStreamTest.RemoveShouldAlwaysExcludeEnd !! > I'm investigating and will post a further patch set once I figure that out. Acknowledged. Will be interesting to see whats going on with that test https://codereview.chromium.org/1670033002/diff/80001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1670033002/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3208: EXPECT_EQ(last_timestamp.InMilliseconds(), 760); On 2016/02/24 00:34:46, wolenetz_slow_reviews_feb22-23 wrote: > On 2016/02/12 22:46:40, chcunningham wrote: > > Whats this small adjustment about? > > Per l.909 and l.916, and per the new "remove previously buffered overlapped > region between coded-frame-group-start and the start of the first frame for this > track since the coded frame group started" logic, there is one less video coded > frame buffered from where the 320x200 bear1 coded frame group (with start at > time 779 (jagged start, audio: 779, video:801)) overlapped the 640x480 bear2 > append. So there is an expected additional remove of [779,801) included in the > remove [new video coded frames from 801 onwards) done as part of the append to > the video SBS. This is "part 2a" from your CR comment breakdown of step 7. > See also InitDemuxerWithConfigChangeData() for details on how the API is > exercised to setup the SourceBufferStreams for these config change tests. Acknowledged. https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... media/filters/source_buffer_range.cc:55: DCHECK(range_start_time_ == kNoDecodeTimestamp() || On 2016/02/24 00:34:46, wolenetz_slow_reviews_feb22-23 wrote: > On 2016/02/12 22:46:40, chcunningham wrote: > > Can range start time really be kNoDecodeTimestamp? What scenario? > > Yes. See the comment for |range_start_time_| in the header. See also > SplitRange()'s "new_range_start_timestamp", DeleteGOPFromFront(), etc. tl;dr: > it's kNoDecodeTimestamp when the front of a range is involved in a remove such > that it may no longer be associated with a "coded frame group start time". In > such case, GetStartTimestamp() just returns the DTS of the first buffer in the > range (which must not be empty....) Acknowledged. https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... media/filters/source_buffer_range.cc:565: // group and the first buffer, then just pretend there is a keyframe at the On 2016/02/24 00:34:46, wolenetz_slow_reviews_feb22-23 wrote: > On 2016/02/12 22:46:40, chcunningham wrote: > > Why do we pretend? Why not return the actual timestamp of the first key frame? > > I think it simplifies seeking by SBS. (See FindNewSelectedRangeSeekTimestamp(), > for instance). It simplifies pruning of SBS |track_buffer_| in muxed > SourceBuffer in the presence of a jagged start (see near the end of > SBS::Append() for the pruning.) > Perhaps we could "simplify" it better, but out of scope of this CL. Ack. SBR and SBS seem annoyingly aware/reliant of/on each others internal workings. Not your fault... just whining about it. https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1670033002/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:442: base::TimeDelta::FromInternalValue(1); On 2016/02/24 00:34:46, wolenetz_slow_reviews_feb22-23 wrote: > On 2016/02/12 23:25:36, chcunningham wrote: > > On 2016/02/12 22:46:40, chcunningham wrote: > > > What if the next timestamp is the same as the last timestamp? Should we not > do > > > the +1 micro > > > > We chatted. Please make a todo && crbug. This is at least no worse than it was > > previously and doesn't seem to be impacted by key frame relaxation. > > I think this +1us was useful when we previously had more strict disallowance of > Same Timestamp sequences of various kinds (now more lenient) along with > uncertainty about all buffers having nonzero duration (crbug 312836). Since > SourceBufferRange::BelongsToRange() is now more lenient w.r.t. same timestamp > sequences, I think we can relax this particular +1us. I'm on the fence about > doing that in this CL at this point, so I've gone with our previous decision to > TODO + crbug for this. Acknowledged. https://codereview.chromium.org/1670033002/diff/100001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1670033002/diff/100001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:909: // bear-640x360.webm : [527-760) On 2016/02/24 00:34:46, wolenetz_slow_reviews_feb22-23 wrote: > On 2016/02/12 01:23:47, wolenetz_slow_reviews_feb22-23 wrote: > > This is intentional. As of patch set 4-5, the removal range for a new coded > > frame group append extends earlier in time potentially than the first new > frame > > for the track in the group (it can go back to the coded frame group start > time.) > > Also, the test media includes audio discontinuities at every webm cluster > > boundary, triggering frequent new coded frame groups (and removal logic that > now > > removes a little more). > > ChunkDemuxerTest.ConfigChange_Video is also adjusted to match this. > > Note this earlier comment was meant to help explain the related > ConfigChange_Video test change. Acknowledged. https://codereview.chromium.org/1670033002/diff/200001/media/filters/source_b... File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/1670033002/diff/200001/media/filters/source_b... media/filters/source_buffer_stream_unittest.cc:4357: // Start new segment, appending KF to abut the start of previous segment. We chatted. Can you revert this test to be as it was?
lgtm https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_bu... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_bu... media/filters/source_buffer_range.cc:178: CHECK_GE(split_range->next_buffer_index_, 0) On 2016/02/24 19:06:26, chcunningham wrote: > On 2016/02/24 00:34:46, wolenetz_slow_reviews_feb22-23 wrote: > > On 2016/02/12 22:46:40, chcunningham wrote: > > > You might also add a check for next_buffer_index < split_range_->size() > > > > Done.... BUT: the new CHECK_LT triggered a crash in > > SourceBufferStreamTest.RemoveShouldAlwaysExcludeEnd !! > > I'm investigating and will post a further patch set once I figure that out. > > Acknowledged. Will be interesting to see whats going on with that test We chatted. For posterity: You made needed to make the CHECK_LE rather than strictly _LT because SBR has subtle behavior where the next_buffer_index_ will be increased until it equals size. https://codereview.chromium.org/1670033002/diff/200001/media/filters/source_b... File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/1670033002/diff/200001/media/filters/source_b... media/filters/source_buffer_stream_unittest.cc:4357: // Start new segment, appending KF to abut the start of previous segment. On 2016/02/24 19:06:26, chcunningham wrote: > We chatted. Can you revert this test to be as it was? You beat me to it in PS11. LGTM
The CQ bit was unchecked by wolenetz@chromium.org
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/1670033002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1670033002/220001
Message was sent while issue was closed.
Description was changed from ========== Reland: MSE: Relax the 'media segment must begin with keyframe' requirement The current version of the MSE spec no longer requires that media segments start with a keyframe. This change reworks the MSE implementation so that it now allows tracks within media segments to not begin with a keyframe. To accomplish this requirement's relaxation, this CL: 1) Separates media segment signaling from coded frame group signaling: MSE stream parsers need to report whether or not they are currently in the middle of parsing a media segment, such as a WebM cluster, to comply with spec disallowance of certain operations like changing SourceBuffer appendMode when appendState is PARSING_MEDIA_SEGMENT. However, SourceBufferStream only needs to be signaled when a new coded frame group is beginning, for example when buffers are appended following a discontinuity. 2) Since the FrameProcessor now can transitively signal SourceBufferStreams when a new coded frame group is beginning (which might span multiple media segments), the SourceBufferStream can rely upon the fact that the next buffer appended must be a keyframe if it's been told that a new coded frame group is starting. A CHECK() is included. 3) New ChunkDemuxerTests. 4) Various test cleanup to conform to the distinct "new coded frame group" and "new media segment" signaling separation, as well as removal of decode error verification when a media segment doesn't begin with a keyframe. 5) Fixes regression found in previous attempt, tracked in bug 581125. In short, relaxes the restriction on same timestamp sequences. A MediaLog is added where decode error would occur previously. 6) Fixes regression found in previous attempt, tracked in bug 581458. In short, prevents tiny gaps under the FrameProcessor's discontinuity threshold from triggering buffered range gaps in SourceBufferStream if there is an intervening removal of media from the range last appended to, but the last appended GOP survived the removal. 7) Uses coded frame group start time more intelligently to determine range adjacency on appends. This allows extremely jagged start coded frame group appends to overlap an existing range, fixing a DCHECK which could occur otherwise (and adhering more correctly to muxed buffered range expectations.) Improves handling of range removals that may occur between SBS::OnStartOfCodedFrameGroup() and SBS::Append(). Note that the FrameProcessor already drops non-keyframes prior to the first keyframe processed for a track following a discontinuity, and the SourceBufferStream already drops non-keyframes prior to the first keyframe processed for a track following a range removal that removed the last appended position in the current coded frame group. BUG=229412,459546,581125,581458,462575 R=chcunningham@chromium.org TEST=Updated ChunkDemuxerTests, SourceBufferStreamTests, FrameProcessorTests. Also locally tested: bug 459546's original report no longer fails. A sample webm VP9 file from b/26524063 no longer fails. GPM webm vp9/opus test no longer fails. Bug 518854's original report's "flags_all.mp4" no longer fails. Bugs 581125 and 581458 no longer repro. ========== to ========== Reland: MSE: Relax the 'media segment must begin with keyframe' requirement The current version of the MSE spec no longer requires that media segments start with a keyframe. This change reworks the MSE implementation so that it now allows tracks within media segments to not begin with a keyframe. To accomplish this requirement's relaxation, this CL: 1) Separates media segment signaling from coded frame group signaling: MSE stream parsers need to report whether or not they are currently in the middle of parsing a media segment, such as a WebM cluster, to comply with spec disallowance of certain operations like changing SourceBuffer appendMode when appendState is PARSING_MEDIA_SEGMENT. However, SourceBufferStream only needs to be signaled when a new coded frame group is beginning, for example when buffers are appended following a discontinuity. 2) Since the FrameProcessor now can transitively signal SourceBufferStreams when a new coded frame group is beginning (which might span multiple media segments), the SourceBufferStream can rely upon the fact that the next buffer appended must be a keyframe if it's been told that a new coded frame group is starting. A CHECK() is included. 3) New ChunkDemuxerTests. 4) Various test cleanup to conform to the distinct "new coded frame group" and "new media segment" signaling separation, as well as removal of decode error verification when a media segment doesn't begin with a keyframe. 5) Fixes regression found in previous attempt, tracked in bug 581125. In short, relaxes the restriction on same timestamp sequences. A MediaLog is added where decode error would occur previously. 6) Fixes regression found in previous attempt, tracked in bug 581458. In short, prevents tiny gaps under the FrameProcessor's discontinuity threshold from triggering buffered range gaps in SourceBufferStream if there is an intervening removal of media from the range last appended to, but the last appended GOP survived the removal. 7) Uses coded frame group start time more intelligently to determine range adjacency on appends. This allows extremely jagged start coded frame group appends to overlap an existing range, fixing a DCHECK which could occur otherwise (and adhering more correctly to muxed buffered range expectations.) Improves handling of range removals that may occur between SBS::OnStartOfCodedFrameGroup() and SBS::Append(). Note that the FrameProcessor already drops non-keyframes prior to the first keyframe processed for a track following a discontinuity, and the SourceBufferStream already drops non-keyframes prior to the first keyframe processed for a track following a range removal that removed the last appended position in the current coded frame group. BUG=229412,459546,581125,581458,462575 R=chcunningham@chromium.org TEST=Updated ChunkDemuxerTests, SourceBufferStreamTests, FrameProcessorTests. Also locally tested: bug 459546's original report no longer fails. A sample webm VP9 file from b/26524063 no longer fails. GPM webm vp9/opus test no longer fails. Bug 518854's original report's "flags_all.mp4" no longer fails. Bugs 581125 and 581458 no longer repro. ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Reland: MSE: Relax the 'media segment must begin with keyframe' requirement The current version of the MSE spec no longer requires that media segments start with a keyframe. This change reworks the MSE implementation so that it now allows tracks within media segments to not begin with a keyframe. To accomplish this requirement's relaxation, this CL: 1) Separates media segment signaling from coded frame group signaling: MSE stream parsers need to report whether or not they are currently in the middle of parsing a media segment, such as a WebM cluster, to comply with spec disallowance of certain operations like changing SourceBuffer appendMode when appendState is PARSING_MEDIA_SEGMENT. However, SourceBufferStream only needs to be signaled when a new coded frame group is beginning, for example when buffers are appended following a discontinuity. 2) Since the FrameProcessor now can transitively signal SourceBufferStreams when a new coded frame group is beginning (which might span multiple media segments), the SourceBufferStream can rely upon the fact that the next buffer appended must be a keyframe if it's been told that a new coded frame group is starting. A CHECK() is included. 3) New ChunkDemuxerTests. 4) Various test cleanup to conform to the distinct "new coded frame group" and "new media segment" signaling separation, as well as removal of decode error verification when a media segment doesn't begin with a keyframe. 5) Fixes regression found in previous attempt, tracked in bug 581125. In short, relaxes the restriction on same timestamp sequences. A MediaLog is added where decode error would occur previously. 6) Fixes regression found in previous attempt, tracked in bug 581458. In short, prevents tiny gaps under the FrameProcessor's discontinuity threshold from triggering buffered range gaps in SourceBufferStream if there is an intervening removal of media from the range last appended to, but the last appended GOP survived the removal. 7) Uses coded frame group start time more intelligently to determine range adjacency on appends. This allows extremely jagged start coded frame group appends to overlap an existing range, fixing a DCHECK which could occur otherwise (and adhering more correctly to muxed buffered range expectations.) Improves handling of range removals that may occur between SBS::OnStartOfCodedFrameGroup() and SBS::Append(). Note that the FrameProcessor already drops non-keyframes prior to the first keyframe processed for a track following a discontinuity, and the SourceBufferStream already drops non-keyframes prior to the first keyframe processed for a track following a range removal that removed the last appended position in the current coded frame group. BUG=229412,459546,581125,581458,462575 R=chcunningham@chromium.org TEST=Updated ChunkDemuxerTests, SourceBufferStreamTests, FrameProcessorTests. Also locally tested: bug 459546's original report no longer fails. A sample webm VP9 file from b/26524063 no longer fails. GPM webm vp9/opus test no longer fails. Bug 518854's original report's "flags_all.mp4" no longer fails. Bugs 581125 and 581458 no longer repro. ========== to ========== Reland: MSE: Relax the 'media segment must begin with keyframe' requirement The current version of the MSE spec no longer requires that media segments start with a keyframe. This change reworks the MSE implementation so that it now allows tracks within media segments to not begin with a keyframe. To accomplish this requirement's relaxation, this CL: 1) Separates media segment signaling from coded frame group signaling: MSE stream parsers need to report whether or not they are currently in the middle of parsing a media segment, such as a WebM cluster, to comply with spec disallowance of certain operations like changing SourceBuffer appendMode when appendState is PARSING_MEDIA_SEGMENT. However, SourceBufferStream only needs to be signaled when a new coded frame group is beginning, for example when buffers are appended following a discontinuity. 2) Since the FrameProcessor now can transitively signal SourceBufferStreams when a new coded frame group is beginning (which might span multiple media segments), the SourceBufferStream can rely upon the fact that the next buffer appended must be a keyframe if it's been told that a new coded frame group is starting. A CHECK() is included. 3) New ChunkDemuxerTests. 4) Various test cleanup to conform to the distinct "new coded frame group" and "new media segment" signaling separation, as well as removal of decode error verification when a media segment doesn't begin with a keyframe. 5) Fixes regression found in previous attempt, tracked in bug 581125. In short, relaxes the restriction on same timestamp sequences. A MediaLog is added where decode error would occur previously. 6) Fixes regression found in previous attempt, tracked in bug 581458. In short, prevents tiny gaps under the FrameProcessor's discontinuity threshold from triggering buffered range gaps in SourceBufferStream if there is an intervening removal of media from the range last appended to, but the last appended GOP survived the removal. 7) Uses coded frame group start time more intelligently to determine range adjacency on appends. This allows extremely jagged start coded frame group appends to overlap an existing range, fixing a DCHECK which could occur otherwise (and adhering more correctly to muxed buffered range expectations.) Improves handling of range removals that may occur between SBS::OnStartOfCodedFrameGroup() and SBS::Append(). Note that the FrameProcessor already drops non-keyframes prior to the first keyframe processed for a track following a discontinuity, and the SourceBufferStream already drops non-keyframes prior to the first keyframe processed for a track following a range removal that removed the last appended position in the current coded frame group. BUG=229412,459546,581125,581458,462575 R=chcunningham@chromium.org TEST=Updated ChunkDemuxerTests, SourceBufferStreamTests, FrameProcessorTests. Also locally tested: bug 459546's original report no longer fails. A sample webm VP9 file from b/26524063 no longer fails. GPM webm vp9/opus test no longer fails. Bug 518854's original report's "flags_all.mp4" no longer fails. Bugs 581125 and 581458 no longer repro. Committed: https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb Cr-Commit-Position: refs/heads/master@{#377368} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb Cr-Commit-Position: refs/heads/master@{#377368} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
