Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2171133002:
Implement opus seek preroll for MediaSourceExtensions. (Closed)
|
Created:
4 years, 5 months ago by chcunningham Modified:
4 years ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement opus seek preroll for MediaSourceExtensions.
Opus requires that for seeks to time T, we should actually seek to
T-preroll to prime the decoder. Decoded samples before time T are
discarded by the audio renderer. Src= playbacks already implement this
preroll logic in FFmpegDemuxer.
BUG=509894
Committed: https://crrev.com/c5bf0429542b3314b0259b278aeeb9d6b308d652
Cr-Commit-Position: refs/heads/master@{#437289}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Rebase & Feedback #Patch Set 3 : Add dummy fixed point hashes #
Total comments: 7
Patch Set 4 : Fix opus hashes and propogate OPUS_FIXED_POINT define #
Total comments: 2
Patch Set 5 : Feedback #
Messages
Total messages: 36 (16 generated)
chcunningham@chromium.org changed reviewers: + wolenetz@chromium.org
PTAL. I can write some additional tests for the new source_buffer_range functions, but I want to first input on these functions. FYI, this needs to land after the following fix is cherrypicked from ffmpeg: http://ffmpeg.org/pipermail/ffmpeg-devel/2016-July/196916.html
https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_range.h (right): https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... media/filters/source_buffer_range.h:40: int GetConfigIdAtTime(DecodeTimestamp timestamp); TODO: document these functions https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream.cc:1113: if ((*itr)->CanSeekTo(preroll_dts) && FYI: I've taken a conservative approach here. If we cant seek back far enough to do the proll, or if the configs don't match, we don't preroll (same as today). https://codereview.chromium.org/2171133002/diff/1/media/test/pipeline_integra... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2171133002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:158: // Hash for a full playthrough of "bear-opus.webm". TODO - impl fixed point hashes too.
I'm wondering if the config ID verification helpers, since used just once, need not be put into SBR, but rather inlined right where they're needed. WDYT? https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... media/filters/source_buffer_range.cc:129: return true; What if the buffer at this time contains splice buffers with differing config IDs? Also note that the splice buffers' decode comes before this time, in reality, in part to support preroll of cross-fade IIUC. Please document intent for these methods -- would really help review. https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... media/filters/source_buffer_range.cc:140: if (buffers_[buffer_index]->GetSpliceBufferConfigId(0) != start_config) Ditto: if there is a splice buffer, it/they could have different config ID than the buffer at buffers_[buffer_index] https://codereview.chromium.org/2171133002/diff/1/media/test/pipeline_integra... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2171133002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:161: // The above hash, plus an additional plathrough starting from T=1.414s. nit: 1.414 --> put in a constant? https://codereview.chromium.org/2171133002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:1118: // Verify that this file's preroll is not eclipsed by the codec delay so we nit: please do the same verification in the MSE version, since different demuxer paths...
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/v2/patch-status/codereview.chromium.or...
On 2016/07/25 21:49:02, wolenetz wrote: > I'm wondering if the config ID verification helpers, since used just once, need > not be put into SBR, but rather inlined right where they're needed. WDYT? > > https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... > File media/filters/source_buffer_range.cc (right): > > https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... > media/filters/source_buffer_range.cc:129: return true; > What if the buffer at this time contains splice buffers with differing config > IDs? Also note that the splice buffers' decode comes before this time, in > reality, in part to support preroll of cross-fade IIUC. Please document intent > for these methods -- would really help review. > > https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... > media/filters/source_buffer_range.cc:140: if > (buffers_[buffer_index]->GetSpliceBufferConfigId(0) != start_config) > Ditto: if there is a splice buffer, it/they could have different config ID than > the buffer at buffers_[buffer_index] > > https://codereview.chromium.org/2171133002/diff/1/media/test/pipeline_integra... > File media/test/pipeline_integration_test.cc (right): > > https://codereview.chromium.org/2171133002/diff/1/media/test/pipeline_integra... > media/test/pipeline_integration_test.cc:161: // The above hash, plus an > additional plathrough starting from T=1.414s. > nit: 1.414 --> put in a constant? > > https://codereview.chromium.org/2171133002/diff/1/media/test/pipeline_integra... > media/test/pipeline_integration_test.cc:1118: // Verify that this file's preroll > is not eclipsed by the codec delay so we > nit: please do the same verification in the MSE version, since different demuxer > paths... I started a CQ dry run just to see the current results, with the acknowledgement that the cherry-pick or full ffmpeg roll needs to occur to include http://ffmpeg.org/pipermail/ffmpeg-devel/2016-July/196916.html
Also, we chatted offline w.r.t. Opus might need large codec delay (multiple frames) to preroll correctly at a gapless splice point - do we allow/do this correctly (if not, addressing that sounds like something for a separate crbug/CL).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
>I'm wondering if the config ID verification helpers, since used just once, need >not be put into SBR, but rather inlined right where they're needed. WDYT? I can't easily implement these methods in SBS. I would need to add some APIs to SBR to iterate over buffers outside of the typical GetNextBuffer API. I'm not sure theres much benefit to making that change. https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... media/filters/source_buffer_range.cc:129: return true; On 2016/07/25 21:49:02, wolenetz_slow_nov29 wrote: > What if the buffer at this time contains splice buffers with differing config > IDs? Also note that the splice buffers' decode comes before this time, in > reality, in part to support preroll of cross-fade IIUC. Please document intent > for these methods -- would really help review. Splicing is gone. Now just checking the config ID on the buffer itself. https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... media/filters/source_buffer_range.cc:140: if (buffers_[buffer_index]->GetSpliceBufferConfigId(0) != start_config) On 2016/07/25 21:49:02, wolenetz_slow_nov29 wrote: > Ditto: if there is a splice buffer, it/they could have different config ID than > the buffer at buffers_[buffer_index] Ditto. https://codereview.chromium.org/2171133002/diff/1/media/test/pipeline_integra... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2171133002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:158: // Hash for a full playthrough of "bear-opus.webm". Will do fixed point in a following PS. For now I've added dummy hashes so it will compile and hopefully the bots will expose what the real hashes should be. https://codereview.chromium.org/2171133002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:161: // The above hash, plus an additional plathrough starting from T=1.414s. On 2016/07/25 21:49:02, wolenetz_slow_nov29 wrote: > nit: 1.414 --> put in a constant? Eh. Its the same pattern used with kOpusEndTrimmingHash_3 above. I can make it a constant, but then the comment should refer to the constant and it will probably have some long horrible name like kOpusEndTrimmingHash_3_seekPoint. I say lets keep it simple. https://codereview.chromium.org/2171133002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:1118: // Verify that this file's preroll is not eclipsed by the codec delay so we On 2016/07/25 21:49:02, wolenetz_slow_nov29 wrote: > nit: please do the same verification in the MSE version, since different demuxer > paths... Done.
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2171133002/diff/40001/media/test/pipeline_int... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2171133002/diff/40001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:145: "-0.47,-0.09,1.28,1.07,1.55,-0.22,"; surprisingly the arm bots didn't fail. not sure if they just don't run this test. looks like i'll have to build arm and calculate myself .... SELF: don't forget.
Looking good % the tests' arm hash peculiarity and 1 comment nit. https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... media/filters/source_buffer_range.cc:129: return true; On 2016/12/01 17:07:16, chcunningham wrote: > On 2016/07/25 21:49:02, wolenetz_slow_nov29 wrote: > > What if the buffer at this time contains splice buffers with differing config > > IDs? Also note that the splice buffers' decode comes before this time, in > > reality, in part to support preroll of cross-fade IIUC. Please document intent > > for these methods -- would really help review. > > Splicing is gone. Now just checking the config ID on the buffer itself. Acknowledged. https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... media/filters/source_buffer_range.cc:140: if (buffers_[buffer_index]->GetSpliceBufferConfigId(0) != start_config) On 2016/12/01 17:07:16, chcunningham wrote: > On 2016/07/25 21:49:02, wolenetz_slow_nov29 wrote: > > Ditto: if there is a splice buffer, it/they could have different config ID > than > > the buffer at buffers_[buffer_index] > > Ditto. Acknowledged. https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_range.h (right): https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... media/filters/source_buffer_range.h:40: int GetConfigIdAtTime(DecodeTimestamp timestamp); On 2016/07/22 01:50:28, chcunningham wrote: > TODO: document these functions (Done) https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2171133002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream.cc:1113: if ((*itr)->CanSeekTo(preroll_dts) && On 2016/07/22 01:50:28, chcunningham wrote: > FYI: I've taken a conservative approach here. If we cant seek back far enough to > do the proll, or if the configs don't match, we don't preroll (same as today). Acknowledged. https://codereview.chromium.org/2171133002/diff/1/media/test/pipeline_integra... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2171133002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:158: // Hash for a full playthrough of "bear-opus.webm". On 2016/12/01 17:07:16, chcunningham wrote: > Will do fixed point in a following PS. For now I've added dummy hashes so it > will compile and hopefully the bots will expose what the real hashes should be. Acknowledged. https://codereview.chromium.org/2171133002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:161: // The above hash, plus an additional plathrough starting from T=1.414s. On 2016/12/01 17:07:16, chcunningham wrote: > On 2016/07/25 21:49:02, wolenetz_slow_nov29 wrote: > > nit: 1.414 --> put in a constant? > > Eh. Its the same pattern used with kOpusEndTrimmingHash_3 above. I can make it a > constant, but then the comment should refer to the constant and it will probably > have some long horrible name like kOpusEndTrimmingHash_3_seekPoint. I say lets > keep it simple. Acknowledged. https://codereview.chromium.org/2171133002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/2171133002/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:114: CHECK(result != keyframe_map_.end()); aside: This is a good addition. https://codereview.chromium.org/2171133002/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:133: CHECK(result != keyframe_map_.end()); ditto aside https://codereview.chromium.org/2171133002/diff/40001/media/test/pipeline_int... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2171133002/diff/40001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:145: "-0.47,-0.09,1.28,1.07,1.55,-0.22,"; On 2016/12/01 21:12:24, chcunningham wrote: > surprisingly the arm bots didn't fail. not sure if they just don't run this > test. looks like i'll have to build arm and calculate myself .... SELF: don't > forget. I wonder if this is an artifact of OpusAudioDecoder's removal and usage of FFmpegAudioDecoder + ffmpeg + third_party/opus to accomplish the decodes (ffmpeg M56 roll + https://codereview.chromium.org/2435603009/): You might check to see if OPUS_FIXED_POINT is never defined here now on any arm build. https://codereview.chromium.org/2171133002/diff/40001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:161: // The above hash, plus an additional plathrough starting from T=1.414s. nit: plathrough
https://codereview.chromium.org/2171133002/diff/40001/media/test/pipeline_int... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2171133002/diff/40001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:145: "-0.47,-0.09,1.28,1.07,1.55,-0.22,"; On 2016/12/02 00:18:49, wolenetz wrote: > On 2016/12/01 21:12:24, chcunningham wrote: > > surprisingly the arm bots didn't fail. not sure if they just don't run this > > test. looks like i'll have to build arm and calculate myself .... SELF: don't > > forget. > > I wonder if this is an artifact of OpusAudioDecoder's removal and usage of > FFmpegAudioDecoder + ffmpeg + third_party/opus to accomplish the decodes (ffmpeg > M56 roll + https://codereview.chromium.org/2435603009/): You might check to see > if OPUS_FIXED_POINT is never defined here now on any arm build. Oh - android doesn't run pipeline_integration_tests, apparently (at least, without mojo, and with mojo, clockless is disabled so the new tests using these hashes are disabled). See https://cs.chromium.org/chromium/src/media/test/BUILD.gn?rcl=0&l=57 (and watk@, I guess). Maybe an arm cros run might help. lack of pipeline_integration_tests on android is a bit concerning though.
wolenetz@chromium.org changed reviewers: + watk@chromium.org
R+=watk@ for the fixed-point/lack of pipeline_integration_tests on android arm questions. cros arm seems like the best bet to get updated fixed point hashes.
On 2016/12/02 00:47:56, wolenetz wrote: > R+=watk@ for the fixed-point/lack of pipeline_integration_tests on android arm > questions. > > cros arm seems like the best bet to get updated fixed point hashes. You could try audio_decoder_unittests on Android to get them too maybe.
On 2016/12/02 00:58:38, watk wrote: > On 2016/12/02 00:47:56, wolenetz wrote: > > R+=watk@ for the fixed-point/lack of pipeline_integration_tests on android arm > > questions. > > > > cros arm seems like the best bet to get updated fixed point hashes. > > You could try audio_decoder_unittests on Android to get them too maybe. Unfortunately, those test just the decode, not the streaming/buffering mechanism which this CL changes (MSE) and wishes to test :)
Updated the hashes. Fastest way is to just force usage of opus_fixed_point in the opus BUILD.gn. This revealed that OPUS_FIXED_POINT is actually never defined in media_unittests. The define is set in opus:opus_config, which is a public_config of the opus target. Media directly depends on opus, so it gets the define. But media_unittests only has the indirect dependency, so we need to explicitly bring in the opus config to see the define there. This also confirms the fixed point tests have no waterfall / bot coverage. https://codereview.chromium.org/2171133002/diff/40001/media/test/pipeline_int... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2171133002/diff/40001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:161: // The above hash, plus an additional plathrough starting from T=1.414s. On 2016/12/02 00:18:50, wolenetz wrote: > nit: plathrough Done.
LGTM % 0) fixing public_configs (see below), And filing/updating bug(s) to: 1) get arm pipeline_integration_test coverage on bots (the crbug watk@'s TODO from media/BUILD.gn perhaps?) 2) you mentioned there's a problem with buffered range reporting and Opus https://codereview.chromium.org/2171133002/diff/60001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/2171133002/diff/60001/media/BUILD.gn#newcode290 media/BUILD.gn:290: public_configs += [ "//third_party/opus:opus_config" ] only the tests need this, not everyone using media. move to media_unittests?
Thanks Matt > And filing/updating bug(s) to: > 1) get arm pipeline_integration_test coverage on bots (the crbug watk@'s TODO > from media/BUILD.gn perhaps?) http://crbug.com/672291 > 2) you mentioned there's a problem with buffered range reporting and Opus http://crbug.com/672339 and http://crbug.com/672342 https://codereview.chromium.org/2171133002/diff/60001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/2171133002/diff/60001/media/BUILD.gn#newcode290 media/BUILD.gn:290: public_configs += [ "//third_party/opus:opus_config" ] On 2016/12/03 00:48:36, wolenetz_ooo_dec5 wrote: > only the tests need this, not everyone using media. move to media_unittests? Done.
The CQ bit was checked by chcunningham@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/2171133002/#ps80001 (title: "Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by chcunningham@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481219017353990,
"parent_rev": "c14efbb49963372e532539489cb1c04069fa25df", "commit_rev":
"27f2e9cc0990ce45fd284649d23b0d7cee6184cb"}
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Implement opus seek preroll for MediaSourceExtensions. Opus requires that for seeks to time T, we should actually seek to T-preroll to prime the decoder. Decoded samples before time T are discarded by the audio renderer. Src= playbacks already implement this preroll logic in FFmpegDemuxer. BUG=509894 ========== to ========== Implement opus seek preroll for MediaSourceExtensions. Opus requires that for seeks to time T, we should actually seek to T-preroll to prime the decoder. Decoded samples before time T are discarded by the audio renderer. Src= playbacks already implement this preroll logic in FFmpegDemuxer. BUG=509894 Committed: https://crrev.com/c5bf0429542b3314b0259b278aeeb9d6b308d652 Cr-Commit-Position: refs/heads/master@{#437289} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c5bf0429542b3314b0259b278aeeb9d6b308d652 Cr-Commit-Position: refs/heads/master@{#437289} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
