Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(108)

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

Created:
5 years, 8 months ago by wolenetz
Modified:
4 years, 11 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.

Description

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. 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 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. Committed: https://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de Cr-Commit-Position: refs/heads/master@{#371018}

Patch Set 1 #

Patch Set 2 : Checkpoint after layout tests fixed. CDT.GetBufferedRanges_AudioVideo, Seq/FPT.AudioVideo_Discontin… #

Patch Set 3 : Checkpoint. Not ready for review just yet. #

Patch Set 4 : Rebased. Checkpoint. Not ready for review yet. #

Patch Set 5 : WIP, not review-ready. Just a rebase #

Patch Set 6 : Minor WIP update (hit another problem). Not review-ready. #

Patch Set 7 : Rebase onto https://codereview.chromium.org/1564983003/, cleanup, need to fix further #

Patch Set 8 : Still WIP. Fixed FrameProcessorTests by undoing FP last_decode_timestamp hack. #

Patch Set 9 : Add relaxed keyframe tests, clean-up. Nix the range-removal SBS->FrameProcessor idea per investigat… #

Patch Set 10 : Corrected base URL to have this CL depend on https://codereview.chromium.org/1564983003/ PS 6 #

Patch Set 11 : Cleanup, new tests, CHECK() in SBS instead of decode err, and rebase to ToT since prereq landed #

Patch Set 12 : Try to fix bot flakes by strictly ordering the blocks in muxed test 3 #

Total comments: 22

Patch Set 13 : Rebased(noop) + addressed comments from PS12 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+773 lines, -623 lines) Patch
M media/filters/chunk_demuxer.h View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M media/filters/chunk_demuxer.cc View 1 2 3 4 5 6 1 chunk +4 lines, -3 lines 0 comments Download
M media/filters/chunk_demuxer_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +214 lines, -2 lines 0 comments Download
M media/filters/frame_processor.h View 1 2 3 4 5 6 7 5 chunks +12 lines, -9 lines 0 comments Download
M media/filters/frame_processor.cc View 1 2 3 4 5 6 7 8 9 10 11 12 9 chunks +33 lines, -29 lines 0 comments Download
M media/filters/frame_processor_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 29 chunks +32 lines, -50 lines 0 comments Download
M media/filters/media_source_state.h View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -10 lines 0 comments Download
M media/filters/media_source_state.cc View 1 2 3 4 5 6 7 8 9 4 chunks +4 lines, -7 lines 0 comments Download
M media/filters/source_buffer_stream.h View 1 2 3 4 5 6 2 chunks +8 lines, -7 lines 0 comments Download
M media/filters/source_buffer_stream.cc View 1 2 3 4 5 6 7 8 9 10 11 12 8 chunks +35 lines, -38 lines 0 comments Download
M media/filters/source_buffer_stream_unittest.cc View 1 2 3 4 5 6 7 8 9 10 208 chunks +426 lines, -465 lines 0 comments Download
M media/formats/mp2t/mp2t_stream_parser.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M media/formats/mp4/mp4_stream_parser.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 44 (19 generated)
wolenetz
PS1 is not ready for review (it is just a checkpoint). I need to fix ...
5 years, 8 months ago (2015-04-25 02:16:54 UTC) #1
wolenetz
On 2015/04/25 02:16:54, wolenetz wrote: > PS1 is not ready for review (it is just ...
5 years, 8 months ago (2015-04-25 03:19:53 UTC) #2
wolenetz
On 2015/04/25 03:19:53, wolenetz wrote: > On 2015/04/25 02:16:54, wolenetz wrote: > > PS1 is ...
5 years, 3 months ago (2015-09-09 22:47:50 UTC) #3
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1091293005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1091293005/60001
5 years, 3 months ago (2015-09-09 22:48:35 UTC) #5
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/60867)
5 years, 3 months ago (2015-09-09 23:13:40 UTC) #7
wolenetz
Patch set 7 is a further large rebase. Assuming https://codereview.chromium.org/1564983003/ lands with code functionally similar ...
4 years, 11 months ago (2016-01-19 20:56:34 UTC) #9
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1091293005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1091293005/200001
4 years, 11 months ago (2016-01-20 03:40:14 UTC) #13
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/10729)
4 years, 11 months ago (2016-01-20 04:50:10 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/1091293005/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1091293005/220001
4 years, 11 months ago (2016-01-20 05:20:51 UTC) #19
wolenetz
Patch set 12 is ready for review. Please take a look. Thanks! https://codereview.chromium.org/1091293005/diff/220001/media/filters/chunk_demuxer_unittest.cc File media/filters/chunk_demuxer_unittest.cc ...
4 years, 11 months ago (2016-01-20 05:20:53 UTC) #20
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/10772)
4 years, 11 months ago (2016-01-20 05:49:16 UTC) #23
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1091293005/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1091293005/220001
4 years, 11 months ago (2016-01-20 07:05:23 UTC) #25
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 11 months ago (2016-01-20 08:43:07 UTC) #27
wolenetz
Just a comment update. This CL (patch set 12) is still awaiting CR. https://codereview.chromium.org/1091293005/diff/220001/media/filters/chunk_demuxer_unittest.cc File ...
4 years, 11 months ago (2016-01-20 21:12:12 UTC) #28
wolenetz
Just a further nit-to-self that I'll fix before landing. This CL (patch set 12) is ...
4 years, 11 months ago (2016-01-21 00:35:29 UTC) #29
wolenetz
ping
4 years, 11 months ago (2016-01-21 19:06:38 UTC) #30
chcunningham
Fantastic, esp the unit tests in ChunkDemuxerTests. My comments are mostly minor stuff and just ...
4 years, 11 months ago (2016-01-22 01:26:38 UTC) #31
wolenetz
Replies to comments (including a couple "WDYT" queries). Patch set update will be forthcoming early ...
4 years, 11 months ago (2016-01-22 01:50:03 UTC) #32
wolenetz
Please take a look at patch set 13. If lgty, please send to CQ if ...
4 years, 11 months ago (2016-01-22 18:59:31 UTC) #34
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1091293005/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1091293005/240001
4 years, 11 months ago (2016-01-22 18:59:50 UTC) #35
chcunningham
lgtm https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_processor.cc File media/filters/frame_processor.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_processor.cc#newcode299 media/filters/frame_processor.cc:299: in_coded_frame_group_ = false; On 2016/01/22 18:59:31, wolenetz_OoO_Jan_22PM_PST wrote: ...
4 years, 11 months ago (2016-01-22 19:26:03 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1091293005/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1091293005/240001
4 years, 11 months ago (2016-01-22 19:26:58 UTC) #39
commit-bot: I haz the power
Committed patchset #13 (id:240001)
4 years, 11 months ago (2016-01-22 20:10:21 UTC) #41
commit-bot: I haz the power
Patchset 13 (id:??) landed as https://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de Cr-Commit-Position: refs/heads/master@{#371018}
4 years, 11 months ago (2016-01-22 20:11:30 UTC) #43
wolenetz
4 years, 11 months ago (2016-01-27 00:39:46 UTC) #44
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/1637213002/ by wolenetz@chromium.org.

The reason for reverting is: Two regressions:
B=581125,581458

The latter causes NFLX playback stall. Reverting to fix canary, even if the
latter's eventual fix might be external (e.g. NFLX web app)..

Powered by Google App Engine
This is Rietveld 408576698