|
|
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. |
DescriptionMSE: 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 #
Messages
Total messages: 44 (19 generated)
PS1 is not ready for review (it is just a checkpoint). I need to fix a couple test failures (PipelineIntegrationTest.ChunkDemuxerAbortRead_AudioVideo fails and ChunkDemuxerTest.GetBufferedRanges_AudioVideo crashes), re-proof especially the coverage of SourceBufferStreamTests, and try layout tests.
On 2015/04/25 02:16:54, wolenetz wrote: > PS1 is not ready for review (it is just a checkpoint). > I need to fix a couple test failures > (PipelineIntegrationTest.ChunkDemuxerAbortRead_AudioVideo fails and > ChunkDemuxerTest.GetBufferedRanges_AudioVideo crashes), re-proof especially the > coverage of SourceBufferStreamTests, and try layout tests. Likewise, PS2 isn't ready for review. Though all MSE layout tests now pass (including the abort-in-middle-of-media-segment portion of mediasource-append-buffer.html), and all PipelineIntegrationTests now pass, a couple tests still crash: ChunkDemuxerTest.GetBufferedRanges_AudioVideo SequenceMode/FrameProcessorTest.AudioVideo_Discontinuity/0 I suspect I might need to make FrameProcessor::Reset() condition based on: appendMode (don't reset in_coded_frame_group_ if mode is Sequence) I suspect the ChunkDemuxerTest failure is due to our relaxed enforcement that ALL tracks exist in each cluster. Previously, every cluster signaled a possible new range (and when we appended single stream clusters for each of audio and video starting near the same timestamp, each triggered discontinuity logic within SBS). PS2 doesn't detect such discontinuity and re-uses the first single-stream-cluster's start timestamp. This is bad if the second single-stream-cluster has buffers below that timestamp (since we've relaxed the enforcement, at least this latter case should trigger a new coded frame group. I think the simplest solution is to expand FrameProcessor's discontinuity detection to check the current frame's DTS against *all* track buffers' last_decode_timestamps, to catch the case where a new coded frame group must begin on DTS going backwards. The other case (second single-stream cluster's first buffer is WAY in the future) is questionable. ISTM that re-using the (lower) coded frame group start time from the first single-stream cluster of the other track is possibly OK, and that the expectation in ChunkDemuxerTest.GetBufferedRanges_AudioVideo will simply need to be updated. This other case would affect only badly muxed content appended to the same SourceBuffer, so hopefully doesn't break existing MSE users.
On 2015/04/25 03:19:53, wolenetz wrote: > On 2015/04/25 02:16:54, wolenetz wrote: > > PS1 is not ready for review (it is just a checkpoint). > > I need to fix a couple test failures > > (PipelineIntegrationTest.ChunkDemuxerAbortRead_AudioVideo fails and > > ChunkDemuxerTest.GetBufferedRanges_AudioVideo crashes), re-proof especially > the > > coverage of SourceBufferStreamTests, and try layout tests. > > Likewise, PS2 isn't ready for review. > Though all MSE layout tests now pass (including the > abort-in-middle-of-media-segment portion of mediasource-append-buffer.html), and > all PipelineIntegrationTests now pass, a couple tests still crash: > ChunkDemuxerTest.GetBufferedRanges_AudioVideo > SequenceMode/FrameProcessorTest.AudioVideo_Discontinuity/0 > > I suspect I might need to make FrameProcessor::Reset() condition based on: > appendMode (don't reset in_coded_frame_group_ if mode is Sequence) > > I suspect the ChunkDemuxerTest failure is due to our relaxed enforcement that > ALL tracks exist in each cluster. Previously, every cluster signaled a possible > new range (and when we appended single stream clusters for each of audio and > video starting near the same timestamp, each triggered discontinuity logic > within SBS). PS2 doesn't detect such discontinuity and re-uses the first > single-stream-cluster's start timestamp. This is bad if the second > single-stream-cluster has buffers below that timestamp (since we've relaxed the > enforcement, at least this latter case should trigger a new coded frame group. > > I think the simplest solution is to expand FrameProcessor's discontinuity > detection to check the current frame's DTS against *all* track buffers' > last_decode_timestamps, to catch the case where a new coded frame group must > begin on DTS going backwards. > > The other case (second single-stream cluster's first buffer is WAY in the > future) is questionable. ISTM that re-using the (lower) coded frame group start > time from the first single-stream cluster of the other track is possibly OK, and > that the expectation in ChunkDemuxerTest.GetBufferedRanges_AudioVideo will > simply need to be updated. This other case would affect only badly muxed content > appended to the same SourceBuffer, so hopefully doesn't break existing MSE > users. PS4 is just a (big) rebase. Same underlying questions exist from ^^^.
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/1091293005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1091293005/60001
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Description was changed from ========== WIP MSE: Separate 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. This change separates the signaling of new media segments from the signaling of new coded frame groups. Subsequent changes to handle discontinuities around range removal algorithm execution and further loosen keyframe requirement at the beginning of media segments will depend on this separation of signaling. BUG=229412 ========== to ========== WIP MSE: Separate 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. This change separates the signaling of new media segments from the signaling of new coded frame groups. It is incomplete while work is in progress to: * handle discontinuities around range removal algorithm execution * further loosen keyframe requirement at the beginning of media segments BUG=229412 ==========
Patch set 7 is a further large rebase. Assuming https://codereview.chromium.org/1564983003/ lands with code functionally similar to its patch set 6, further work on this current CL is needed beyond the patch 7 rebase to: * Fix FrameProcessorTest.PartialAppendWindowFilterNoNewMediaSegment (using the assumption resulting from https://codereview.chromium.org/1564983003/ ps6 that the spec's coded frame processing algorithm discontinuity detection logic is ok as-is) * Include signalling of coded frame range removals into frame processor such that discontinuity detection can be informed by it. * Add tests for this bullet. * Consider layout tests
Description was changed from ========== WIP MSE: Separate 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. This change separates the signaling of new media segments from the signaling of new coded frame groups. It is incomplete while work is in progress to: * handle discontinuities around range removal algorithm execution * further loosen keyframe requirement at the beginning of media segments BUG=229412 ========== to ========== 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 Chrome's 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. 3) New ChunkDemuxer tests 4) Various test cleanup to conform to the distinct "new coded frame group" and "new media segment" signalling separation, as well as removal of decode error verification when a media segment doesn't begin with a keyframe. BUG=229412 R=chcunningham@chromium.org ==========
Description was changed from ========== 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 Chrome's 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. 3) New ChunkDemuxer tests 4) Various test cleanup to conform to the distinct "new coded frame group" and "new media segment" signalling separation, as well as removal of decode error verification when a media segment doesn't begin with a keyframe. BUG=229412 R=chcunningham@chromium.org ========== to ========== 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 Chrome's 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 R=chcunningham@chromium.org ==========
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/1091293005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1091293005/200001
Description was changed from ========== 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 Chrome's 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 R=chcunningham@chromium.org ========== to ========== 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 Chrome's 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. ==========
wolenetz@chromium.org changed reviewers: + chcunningham@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
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_androi...)
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/1091293005/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1091293005/220001
Patch set 12 is ready for review. Please take a look. Thanks! https://codereview.chromium.org/1091293005/diff/220001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4573: msi[1] = MuxedStreamInfo(kVideoTrackNum, "31 41 51 61 71K 81", 10); Note: this change to +1 is to try to prevent bot flakes that were seen on previous patch set. If further flakes occur, the problem may be in the 'holding back buffer missing duration' logic, or elsewhere, instead of possibly mixed ordering of a std::priority_queue<BlockInfo> on various platforms when the comparator (<) sees no difference. This potential issue with comparator is what the current patch set tries to fix with these +1's here and below.
Description was changed from ========== 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 Chrome's 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. ========== to ========== 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. ==========
The CQ bit was unchecked by commit-bot@chromium.org
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_androi...)
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/1091293005/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1091293005/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Just a comment update. This CL (patch set 12) is still awaiting CR. https://codereview.chromium.org/1091293005/diff/220001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4573: msi[1] = MuxedStreamInfo(kVideoTrackNum, "31 41 51 61 71K 81", 10); On 2016/01/20 05:20:53, wolenetz wrote: > Note: this change to +1 is to try to prevent bot flakes that were seen on > previous patch set. If further flakes occur, the problem may be in the 'holding > back buffer missing duration' logic, or elsewhere, instead of possibly mixed > ordering of a std::priority_queue<BlockInfo> on various platforms when the > comparator (<) sees no difference. This potential issue with comparator is what > the current patch set tries to fix with these +1's here and below. Update: It looks like this strict ordering (included in patch set 12) fixed the CQ dry run bot flakiness seen in previous patch set.
Just a further nit-to-self that I'll fix before landing. This CL (patch set 12) is still awaiting CR. https://codereview.chromium.org/1091293005/diff/220001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4577: // video simpleblock. The result should be jst 3 audio blocks and no video nit to self: s/jst 3/just 4/, and note that the 4th block is 'held back pending duration' by the parser, so only the first 3 audio blocks are expected to be buffered after this first part is appended.
ping
Fantastic, esp the unit tests in ChunkDemuxerTests. My comments are mostly minor stuff and just documenting for the future. https://codereview.chromium.org/1091293005/diff/220001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4573: msi[1] = MuxedStreamInfo(kVideoTrackNum, "31 41 51 61 71K 81", 10); On 2016/01/20 21:12:12, wolenetz wrote: > On 2016/01/20 05:20:53, wolenetz wrote: > > Note: this change to +1 is to try to prevent bot flakes that were seen on > > previous patch set. If further flakes occur, the problem may be in the > 'holding > > back buffer missing duration' logic, or elsewhere, instead of possibly mixed > > ordering of a std::priority_queue<BlockInfo> on various platforms when the > > comparator (<) sees no difference. This potential issue with comparator is > what > > the current patch set tries to fix with these +1's here and below. > > Update: It looks like this strict ordering (included in patch set 12) fixed the > CQ dry run bot flakiness seen in previous patch set. Acknowledged. https://codereview.chromium.org/1091293005/diff/220001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4577: // video simpleblock. The result should be jst 3 audio blocks and no video On 2016/01/21 00:35:29, wolenetz wrote: > nit to self: s/jst 3/just 4/, and note that the 4th block is 'held back pending > duration' by the parser, so only the first 3 audio blocks are expected to be > buffered after this first part is appended. Acknowledged. https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... media/filters/frame_processor.cc:299: in_coded_frame_group_ = false; Do you not reset this in sequence mode? (return a few lines up) https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... File media/filters/frame_processor_unittest.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... media/filters/frame_processor_unittest.cc:247: bool in_coded_frame_group() { I feel like this is testing implementation details rather than the public interface. Can you instead replace this with checks that the ChunkDemuxerStream (possibly mocked) receives the notification of a new coded frame group at the points when it should and not when it shouldn't? Unless I'm missing it, I think a test thats definitely worth adding would be: 1. Append media segment that doesn't start with a keyframe and has none 2. verify no notification to ChunkDemuxerStream 3. Append media segment that still doesn't start with KF, but has one in middle 4. verify notification to ChunkDemuxerStrea, but careful to verify the timestamp notifies matches that of the KF in middle of segment https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... media/filters/frame_processor_unittest.cc:247: bool in_coded_frame_group() { Edit: I that the test I described (and many more) live in chunk demuxer unit tests... I'm fine with that. I still think these tests would be better off using the mocked stream and checking for the notify call instead of checking this private flag though. https://codereview.chromium.org/1091293005/diff/220001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/source_b... media/filters/source_buffer_stream.cc:228: CHECK(!new_coded_frame_group_ || buffers.front()->is_key_frame()); Just to confirm/document: this is a CHECK because FrameProcessor should always prevent such frames from making it to SourceBufferStream. Users will never hit this if they simply append bad media. https://codereview.chromium.org/1091293005/diff/220001/media/filters/source_b... File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/1091293005/diff/220001/media/filters/source_b... media/filters/source_buffer_stream_unittest.cc:783: TEST_F(SourceBufferStreamTest, Append_DoesNotBeginWithKeyframe) { Just to confirm, this is ripped out here because we expect this to be handled at frame processor (and passing this stuff to SBS is now going to yeild a check).
Replies to comments (including a couple "WDYT" queries). Patch set update will be forthcoming early tomorrow morning. Thanks for the hefty review :) https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... media/filters/frame_processor.cc:299: in_coded_frame_group_ = false; On 2016/01/22 01:26:37, chcunningham wrote: > Do you not reset this in sequence mode? (return a few lines up) Correct. Sequence mode means everything appended is made to be in sequence, i.e. in the same coded frame group. The "made to be" part includes dropping any non-keyframes following a detected discontinuity, etc, and aligning buffer timestamps to be in sequence, etc. Therefore, the result should be a continuous sequence of coded frames within each track. Note that the MseTrackBuffer::Reset() calls earlier in this FrameProcessor::Reset() still allow ProcessFrame() to identify discontinuity (and also enforces that next "passing" frame emitted by ProcessFrame() for each track *must* be a keyframe). I suppose I could add a comment to the early return like "While in sequence mode, maintain current state of in_coded_frame_group_ across this Reset()" wdyt? https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... File media/filters/frame_processor_unittest.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... media/filters/frame_processor_unittest.cc:247: bool in_coded_frame_group() { On 2016/01/22 01:26:38, chcunningham wrote: > I feel like this is testing implementation details rather than the public > interface. Can you instead replace this with checks that the ChunkDemuxerStream > (possibly mocked) receives the notification of a new coded frame group at the > points when it should and not when it shouldn't? > > Unless I'm missing it, I think a test thats definitely worth adding would be: > > 1. Append media segment that doesn't start with a keyframe and has none > 2. verify no notification to ChunkDemuxerStream > 3. Append media segment that still doesn't start with KF, but has one in middle > 4. verify notification to ChunkDemuxerStrea, but careful to verify the timestamp > notifies matches that of the KF in middle of segment > > SGTM. Good idea (around mocked stream; checking for notify call/lack thereof). I'll see if such a mock implementation is reasonably workable for inclusion in this CL (IOW, not too complex for a potential merge-to-m49.) If complex, would you be ok with a TODO and crbug to follow-up with the recommended rework? (I think it'll fit within this CL, just getting an idea to how strong you're feeling about this is.) https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... media/filters/frame_processor_unittest.cc:247: bool in_coded_frame_group() { On 2016/01/22 01:26:37, chcunningham wrote: > Edit: > > I that the test I described (and many more) live in chunk demuxer unit tests... > I'm fine with that. > > I still think these tests would be better off using the mocked stream and > checking for the notify call instead of checking this private flag though. Acknowledged (your edit). https://codereview.chromium.org/1091293005/diff/220001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/source_b... media/filters/source_buffer_stream.cc:228: CHECK(!new_coded_frame_group_ || buffers.front()->is_key_frame()); On 2016/01/22 01:26:38, chcunningham wrote: > Just to confirm/document: this is a CHECK because FrameProcessor should always > prevent such frames from making it to SourceBufferStream. Users will never hit > this if they simply append bad media. Correct. Normally I'd use a DCHECK for something like this, but the consequences of this condition not being met merit us getting data earlier, faster. I don't think it would be a security vuln, but per the style guide, CHECK can also be used (temporarily): "you can also temporarily use CHECK() instead of DCHECK() when trying to force crashes in release builds to sniff out which of your assertions is failing." I think this also merits me adding a TODO + crbug to change this to a MEDIA_LOG(ERROR) (+ return false to cause decode error in release builds, or + a DCHECK otherwise) once the CHECK has been confirmed not to be triggered by this change initially in the wild in release builds. wdyt? https://codereview.chromium.org/1091293005/diff/220001/media/filters/source_b... File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/1091293005/diff/220001/media/filters/source_b... media/filters/source_buffer_stream_unittest.cc:783: TEST_F(SourceBufferStreamTest, Append_DoesNotBeginWithKeyframe) { On 2016/01/22 01:26:38, chcunningham wrote: > Just to confirm, this is ripped out here because we expect this to be handled at > frame processor (and passing this stuff to SBS is now going to yeild a check). Correct.
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Please take a look at patch set 13. If lgty, please send to CQ if I'm OoO already this afternoon by the time you review. https://codereview.chromium.org/1091293005/diff/220001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4577: // video simpleblock. The result should be jst 3 audio blocks and no video On 2016/01/21 00:35:29, wolenetz wrote: > nit to self: s/jst 3/just 4/, and note that the 4th block is 'held back pending > duration' by the parser, so only the first 3 audio blocks are expected to be > buffered after this first part is appended. Done. https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... media/filters/frame_processor.cc:299: in_coded_frame_group_ = false; On 2016/01/22 01:50:03, wolenetz wrote: > On 2016/01/22 01:26:37, chcunningham wrote: > > Do you not reset this in sequence mode? (return a few lines up) > > Correct. Sequence mode means everything appended is made to be in sequence, i.e. > in the same coded frame group. The "made to be" part includes dropping any > non-keyframes following a detected discontinuity, etc, and aligning buffer > timestamps to be in sequence, etc. Therefore, the result should be a continuous > sequence of coded frames within each track. Note that the > MseTrackBuffer::Reset() calls earlier in this FrameProcessor::Reset() still > allow ProcessFrame() to identify discontinuity (and also enforces that next > "passing" frame emitted by ProcessFrame() for each track *must* be a keyframe). > > I suppose I could add a comment to the early return like "While in sequence > mode, maintain current state of in_coded_frame_group_ across this Reset()" > > wdyt? Done. https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... File media/filters/frame_processor_unittest.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... media/filters/frame_processor_unittest.cc:247: bool in_coded_frame_group() { On 2016/01/22 01:50:03, wolenetz_OoO_Jan_22PM_PST wrote: > On 2016/01/22 01:26:38, chcunningham wrote: > > I feel like this is testing implementation details rather than the public > > interface. Can you instead replace this with checks that the > ChunkDemuxerStream > > (possibly mocked) receives the notification of a new coded frame group at the > > points when it should and not when it shouldn't? > > > > Unless I'm missing it, I think a test thats definitely worth adding would be: > > > > 1. Append media segment that doesn't start with a keyframe and has none > > 2. verify no notification to ChunkDemuxerStream > > 3. Append media segment that still doesn't start with KF, but has one in > middle > > 4. verify notification to ChunkDemuxerStrea, but careful to verify the > timestamp > > notifies matches that of the KF in middle of segment > > > > > > SGTM. Good idea (around mocked stream; checking for notify call/lack thereof). > I'll see if such a mock implementation is reasonably workable for inclusion in > this CL (IOW, not too complex for a potential merge-to-m49.) If complex, would > you be ok with a TODO and crbug to follow-up with the recommended rework? (I > think it'll fit within this CL, just getting an idea to how strong you're > feeling about this is.) From my investigation and offline chat w/chcunningham@, it's not a simple mock method addition since FrameProcessorTests rely on ChunkDemuxerStream to do all the things like buffering and reading/stalling reads, etc. Rather, probably one of at least two more complex alternatives would be needed: 1) Determine a (or create a new) subset of FrameProcessorTests that verify expected new-coded-frame-group calls on a mocked ChunkDemuxerStream, with limited scope on the mock otherwise (no buffered ranges, reading back of buffers, etc). 2) Make the ChunkDemuxerStream new-coded-frame-group callback method virtual and subclass it specifically for these tests. 3) Find some other way to mock/verify the signal. Note that at least the "media-segmented-ness" is no longer the responsibility at all of FrameProcessor (solely owned now by StreamParsers and their interaction with MediaSourceState callbacks, and verified by ChunkDemuxerTests). Only the "new coded frame group signalling" verification needs some cleanup. IIUC, chcunningham@ was ok with such verification cleanup in a follow-up CL to ease the merging of this CL (and since we have mostly equivalent coverage currently, but on the internal state of FPTest's coded-frame-group flag but not the signalling thereof.) I've added a TODO and crbug to clean this up. https://codereview.chromium.org/1091293005/diff/220001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/source_b... media/filters/source_buffer_stream.cc:228: CHECK(!new_coded_frame_group_ || buffers.front()->is_key_frame()); On 2016/01/22 01:50:03, wolenetz_OoO_Jan_22PM_PST wrote: > On 2016/01/22 01:26:38, chcunningham wrote: > > Just to confirm/document: this is a CHECK because FrameProcessor should always > > prevent such frames from making it to SourceBufferStream. Users will never hit > > this if they simply append bad media. > > Correct. Normally I'd use a DCHECK for something like this, but the consequences > of this condition not being met merit us getting data earlier, faster. I don't > think it would be a security vuln, but per the style guide, CHECK can also be > used (temporarily): "you can also temporarily use CHECK() instead of DCHECK() > when trying to force crashes in release builds to sniff out which of your > assertions is failing." > > I think this also merits me adding a TODO + crbug to change this to a > MEDIA_LOG(ERROR) (+ return false to cause decode error in release builds, or + a > DCHECK otherwise) once the CHECK has been confirmed not to be triggered by this > change initially in the wild in release builds. > > wdyt? From offline chat w/chcunningham@, temporary CHECK + TODO+[crbug to change to DCHECK+MEDIA_LOG(ERROR..)+decode error] is best. (Done)
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
lgtm https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... media/filters/frame_processor.cc:299: in_coded_frame_group_ = false; On 2016/01/22 18:59:31, wolenetz_OoO_Jan_22PM_PST wrote: > On 2016/01/22 01:50:03, wolenetz wrote: > > On 2016/01/22 01:26:37, chcunningham wrote: > > > Do you not reset this in sequence mode? (return a few lines up) > > > > Correct. Sequence mode means everything appended is made to be in sequence, > i.e. > > in the same coded frame group. The "made to be" part includes dropping any > > non-keyframes following a detected discontinuity, etc, and aligning buffer > > timestamps to be in sequence, etc. Therefore, the result should be a > continuous > > sequence of coded frames within each track. Note that the > > MseTrackBuffer::Reset() calls earlier in this FrameProcessor::Reset() still > > allow ProcessFrame() to identify discontinuity (and also enforces that next > > "passing" frame emitted by ProcessFrame() for each track *must* be a > keyframe). > > > > I suppose I could add a comment to the early return like "While in sequence > > mode, maintain current state of in_coded_frame_group_ across this Reset()" > > > > wdyt? > > Done. Acknowledged. https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... File media/filters/frame_processor_unittest.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/frame_pr... media/filters/frame_processor_unittest.cc:247: bool in_coded_frame_group() { On 2016/01/22 18:59:31, wolenetz_OoO_Jan_22PM_PST wrote: > On 2016/01/22 01:50:03, wolenetz_OoO_Jan_22PM_PST wrote: > > On 2016/01/22 01:26:38, chcunningham wrote: > > > I feel like this is testing implementation details rather than the public > > > interface. Can you instead replace this with checks that the > > ChunkDemuxerStream > > > (possibly mocked) receives the notification of a new coded frame group at > the > > > points when it should and not when it shouldn't? > > > > > > Unless I'm missing it, I think a test thats definitely worth adding would > be: > > > > > > 1. Append media segment that doesn't start with a keyframe and has none > > > 2. verify no notification to ChunkDemuxerStream > > > 3. Append media segment that still doesn't start with KF, but has one in > > middle > > > 4. verify notification to ChunkDemuxerStrea, but careful to verify the > > timestamp > > > notifies matches that of the KF in middle of segment > > > > > > > > > > SGTM. Good idea (around mocked stream; checking for notify call/lack thereof). > > I'll see if such a mock implementation is reasonably workable for inclusion in > > this CL (IOW, not too complex for a potential merge-to-m49.) If complex, would > > you be ok with a TODO and crbug to follow-up with the recommended rework? (I > > think it'll fit within this CL, just getting an idea to how strong you're > > feeling about this is.) > > From my investigation and offline chat w/chcunningham@, it's not a simple mock > method addition since FrameProcessorTests rely on ChunkDemuxerStream to do all > the things like buffering and reading/stalling reads, etc. Rather, probably one > of at least two more complex alternatives would be needed: > 1) Determine a (or create a new) subset of FrameProcessorTests that verify > expected new-coded-frame-group calls on a mocked ChunkDemuxerStream, with > limited scope on the mock otherwise (no buffered ranges, reading back of > buffers, etc). > 2) Make the ChunkDemuxerStream new-coded-frame-group callback method virtual and > subclass it specifically for these tests. > 3) Find some other way to mock/verify the signal. > Note that at least the "media-segmented-ness" is no longer the responsibility at > all of FrameProcessor (solely owned now by StreamParsers and their interaction > with MediaSourceState callbacks, and verified by ChunkDemuxerTests). Only the > "new coded frame group signalling" verification needs some cleanup. > > IIUC, chcunningham@ was ok with such verification cleanup in a follow-up CL to > ease the merging of this CL (and since we have mostly equivalent coverage > currently, but on the internal state of FPTest's coded-frame-group flag but not > the signalling thereof.) > > I've added a TODO and crbug to clean this up. Acknowledged. https://codereview.chromium.org/1091293005/diff/220001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1091293005/diff/220001/media/filters/source_b... media/filters/source_buffer_stream.cc:228: CHECK(!new_coded_frame_group_ || buffers.front()->is_key_frame()); On 2016/01/22 18:59:31, wolenetz_OoO_Jan_22PM_PST wrote: > On 2016/01/22 01:50:03, wolenetz_OoO_Jan_22PM_PST wrote: > > On 2016/01/22 01:26:38, chcunningham wrote: > > > Just to confirm/document: this is a CHECK because FrameProcessor should > always > > > prevent such frames from making it to SourceBufferStream. Users will never > hit > > > this if they simply append bad media. > > > > Correct. Normally I'd use a DCHECK for something like this, but the > consequences > > of this condition not being met merit us getting data earlier, faster. I don't > > think it would be a security vuln, but per the style guide, CHECK can also be > > used (temporarily): "you can also temporarily use CHECK() instead of DCHECK() > > when trying to force crashes in release builds to sniff out which of your > > assertions is failing." > > > > I think this also merits me adding a TODO + crbug to change this to a > > MEDIA_LOG(ERROR) (+ return false to cause decode error in release builds, or + > a > > DCHECK otherwise) once the CHECK has been confirmed not to be triggered by > this > > change initially in the wild in release builds. > > > > wdyt? > > From offline chat w/chcunningham@, temporary CHECK + TODO+[crbug to change to > DCHECK+MEDIA_LOG(ERROR..)+decode error] is best. > (Done) Acknowledged.
The CQ bit was unchecked by chcunningham@chromium.org
The CQ bit was checked by chcunningham@chromium.org
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
Message was sent while issue was closed.
Description was changed from ========== 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. ========== to ========== 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. ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== 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. ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/3d152e528e97024860822d3e75109c0c597bf4de Cr-Commit-Position: refs/heads/master@{#371018}
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).. |