Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(12)

Issue 1670033002: Reland: MSE: Relax the 'media segment must begin with keyframe' requirement (Closed)

Created:
3 years, 1 month ago by wolenetz
Modified:
3 years 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.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1146 lines, -758 lines) Patch
M media/filters/chunk_demuxer.h View 1 chunk +2 lines, -2 lines 0 comments Download
M media/filters/chunk_demuxer.cc View 1 2 3 4 5 6 7 1 chunk +4 lines, -3 lines 0 comments Download
M media/filters/chunk_demuxer_unittest.cc View 1 2 3 5 chunks +216 lines, -4 lines 0 comments Download
M media/filters/frame_processor.h View 5 chunks +12 lines, -9 lines 0 comments Download
M media/filters/frame_processor.cc View 9 chunks +33 lines, -29 lines 0 comments Download
M media/filters/frame_processor_unittest.cc View 29 chunks +32 lines, -50 lines 0 comments Download
M media/filters/media_source_state.h View 1 chunk +0 lines, -10 lines 0 comments Download
M media/filters/media_source_state.cc View 4 chunks +4 lines, -7 lines 0 comments Download
M media/filters/source_buffer_range.h View 1 2 3 4 5 6 7 4 chunks +47 lines, -25 lines 0 comments Download
M media/filters/source_buffer_range.cc View 1 2 3 4 5 6 7 8 9 8 chunks +50 lines, -43 lines 0 comments Download
M media/filters/source_buffer_stream.h View 1 2 3 6 chunks +25 lines, -18 lines 0 comments Download
M media/filters/source_buffer_stream.cc View 1 2 3 4 5 6 7 17 chunks +126 lines, -82 lines 0 comments Download
M media/filters/source_buffer_stream_unittest.cc View 1 2 3 4 5 6 9 10 208 chunks +592 lines, -475 lines 0 comments Download
M media/formats/mp2t/mp2t_stream_parser.cc View 1 chunk +1 line, -0 lines 0 comments Download
M media/formats/mp4/mp4_stream_parser.cc View 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 54 (20 generated)
wolenetz
Not quite ready for review yet. I need to do some cleanup of various log ...
3 years, 1 month ago (2016-02-05 01:43:29 UTC) #1
commit-bot: I haz the power
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
3 years, 1 month ago (2016-02-05 01:44:16 UTC) #3
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
3 years, 1 month ago (2016-02-05 02:59:52 UTC) #5
DaleCurtis
To make this easier to review, PS#1 should be the old patch and PS#2 the ...
3 years, 1 month ago (2016-02-05 18:43:41 UTC) #6
wolenetz
On 2016/02/05 18:43:41, DaleCurtis wrote: > To make this easier to review, PS#1 should be ...
3 years, 1 month ago (2016-02-05 23:01:54 UTC) #7
wolenetz
Please take a look at patch set #2. Note that PS#1 is a rebased-onto-ToT version ...
3 years, 1 month ago (2016-02-05 23:27:24 UTC) #10
commit-bot: I haz the power
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
3 years, 1 month ago (2016-02-05 23:28:45 UTC) #11
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
3 years, 1 month ago (2016-02-06 01:10:36 UTC) #13
chcunningham
https://codereview.chromium.org/1670033002/diff/20001/media/filters/source_buffer_stream.cc File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1670033002/diff/20001/media/filters/source_buffer_stream.cc#newcode564 media/filters/source_buffer_stream.cc:564: new_coded_frame_group_ || Great find! This is *exactly* the sort ...
3 years, 1 month ago (2016-02-09 00:21:08 UTC) #14
wolenetz
Just comment replies. https://codereview.chromium.org/1670033002/diff/20001/media/filters/source_buffer_stream.cc File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1670033002/diff/20001/media/filters/source_buffer_stream.cc#newcode564 media/filters/source_buffer_stream.cc:564: new_coded_frame_group_ || On 2016/02/09 00:21:08, chcunningham ...
3 years, 1 month ago (2016-02-09 00:47:19 UTC) #15
wolenetz
more comment replies. https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_buffer_stream.cc File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1670033002/diff/40001/media/filters/source_buffer_stream.cc#newcode444 media/filters/source_buffer_stream.cc:444: last_appended_buffer_timestamp_ != kNoDecodeTimestamp()) { On 2016/02/09 ...
3 years, 1 month ago (2016-02-09 01:36:41 UTC) #16
wolenetz
Not ready for review yet. Just demos newly found bad stuff demonstrated in patch set ...
3 years, 1 month ago (2016-02-09 01:42:31 UTC) #17
commit-bot: I haz the power
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
3 years, 1 month ago (2016-02-12 01:23:43 UTC) #22
wolenetz
Please take a look at patch set 5. The marginal change to include the new ...
3 years, 1 month ago (2016-02-12 01:23:47 UTC) #23
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
3 years, 1 month ago (2016-02-12 02:45:02 UTC) #25
chcunningham
Minor comments. Also, I want to try to take a stab at explaining how part ...
3 years, 1 month ago (2016-02-12 22:46:41 UTC) #26
chcunningham
LGTM % my nit (and answers to less critical questions from last round of comments) ...
3 years, 1 month ago (2016-02-12 23:25:36 UTC) #27
wolenetz
On 2016/02/12 23:25:36, chcunningham wrote: > LGTM % my nit (and answers to less critical ...
3 years, 1 month ago (2016-02-16 23:53:05 UTC) #28
commit-bot: I haz the power
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
3 years, 1 month ago (2016-02-17 01:10:52 UTC) #30
wolenetz
On 2016/02/16 23:53:05, wolenetz wrote: > On 2016/02/12 23:25:36, chcunningham wrote: > > LGTM % ...
3 years, 1 month ago (2016-02-17 01:11:51 UTC) #31
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
3 years, 1 month ago (2016-02-17 02:22:49 UTC) #33
wolenetz
On 2016/02/12 22:46:41, chcunningham wrote: > Minor comments. > > Also, I want to try ...
3 years ago (2016-02-23 21:42:45 UTC) #34
wolenetz
Addressed all current CR comments. Except: the new CHECK_LT in SBR is failing media_unittests. I'm ...
3 years ago (2016-02-24 00:34:47 UTC) #36
wolenetz
PS9 is just a test cleanup patch set. The same new CHECK_LT in SBR is ...
3 years ago (2016-02-24 01:14:02 UTC) #37
wolenetz
Please take a look at patch set 10. Nothing should be surprising per our previous ...
3 years ago (2016-02-24 01:32:13 UTC) #38
commit-bot: I haz the power
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
3 years ago (2016-02-24 01:39:52 UTC) #40
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
3 years ago (2016-02-24 04:22:06 UTC) #42
wolenetz
Per chat, please take a look at patch set 11. It should be even less ...
3 years ago (2016-02-24 18:57:46 UTC) #44
commit-bot: I haz the power
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
3 years ago (2016-02-24 18:59:06 UTC) #45
chcunningham
https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_buffer_range.cc File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_buffer_range.cc#newcode162 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: > ...
3 years ago (2016-02-24 19:06:26 UTC) #46
chcunningham
lgtm https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_buffer_range.cc File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1670033002/diff/60001/media/filters/source_buffer_range.cc#newcode178 media/filters/source_buffer_range.cc:178: CHECK_GE(split_range->next_buffer_index_, 0) On 2016/02/24 19:06:26, chcunningham wrote: > ...
3 years ago (2016-02-24 19:12:54 UTC) #47
commit-bot: I haz the power
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
3 years ago (2016-02-24 19:14:56 UTC) #50
commit-bot: I haz the power
Committed patchset #11 (id:220001)
3 years ago (2016-02-24 20:27:20 UTC) #52
commit-bot: I haz the power
3 years ago (2016-02-24 20:28:24 UTC) #54
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/250b8204cef19bf2bf6b61b6d9c82d03f20e9ceb
Cr-Commit-Position: refs/heads/master@{#377368}

Powered by Google App Engine
This is Rietveld 408576698