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

Issue 2343543002: MSE: Replace crossfade splicing overlap trimming. (Closed)

Created:
4 years, 3 months ago by chcunningham
Modified:
4 years, 1 month ago
Reviewers:
wolenetz
CC:
chromium-reviews, feature-media-reviews_chromium.org, posciak+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

MSE: Replace crossfade splicing overlap trimming. Crossfading is quite complex and error prone (source of many crashes). It is also not necessary for the gapless playback scenario (handled entirely by partial append window trimming). The simpler approach is to just trim the existing buffer, eliminating the overlap. See MSE spec proposal here: https://github.com/w3c/media-source/issues/165 BUG=588351, 577438 TEST=new unit tests Committed: https://crrev.com/967db2f72de7e17e8c3326438e29c90375e3a1be Cr-Commit-Position: refs/heads/master@{#429394}

Patch Set 1 #

Total comments: 4

Patch Set 2 : No more silence, just trim the overlap. #

Total comments: 2

Patch Set 3 : No tiny splices #

Total comments: 6

Patch Set 4 : feedback: add new and old tests #

Patch Set 5 : fix compile error #

Total comments: 58

Patch Set 6 : Feedback #

Total comments: 14

Patch Set 7 : Merge and feedback. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+397 lines, -623 lines) Patch
M media/base/test_helpers.h View 1 2 3 4 5 6 2 chunks +12 lines, -7 lines 0 comments Download
M media/blink/webmediaplayer_impl.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M media/filters/chunk_demuxer.h View 1 2 3 4 5 4 chunks +2 lines, -12 lines 0 comments Download
M media/filters/chunk_demuxer.cc View 1 2 3 4 5 7 chunks +11 lines, -22 lines 0 comments Download
M media/filters/chunk_demuxer_unittest.cc View 1 2 3 4 5 6 10 chunks +50 lines, -22 lines 0 comments Download
M media/filters/frame_processor_unittest.cc View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M media/filters/source_buffer_state_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M media/filters/source_buffer_stream.h View 1 2 3 4 5 6 chunks +14 lines, -33 lines 0 comments Download
M media/filters/source_buffer_stream.cc View 1 2 3 4 5 6 14 chunks +132 lines, -232 lines 0 comments Download
M media/filters/source_buffer_stream_unittest.cc View 1 2 3 4 5 6 18 chunks +171 lines, -289 lines 0 comments Download
M media/test/pipeline_integration_test.cc View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download

Messages

Total messages: 48 (23 generated)
chcunningham
Hey Matt, PTAL Will do a follow up CL that deletes all of the splicing ...
4 years, 3 months ago (2016-09-14 03:44:33 UTC) #5
wolenetz
On 2016/09/14 03:44:33, chcunningham wrote: > Hey Matt, PTAL > > Will do a follow ...
4 years, 3 months ago (2016-09-14 19:02:18 UTC) #8
wolenetz
On 2016/09/14 19:02:18, wolenetz wrote: > On 2016/09/14 03:44:33, chcunningham wrote: > > Hey Matt, ...
4 years, 3 months ago (2016-09-14 19:08:08 UTC) #9
wolenetz
I'll review this tomorrow morning - my review isn't wholly blocked, but chcunningham@ is working ...
4 years, 3 months ago (2016-09-14 23:47:17 UTC) #10
wolenetz
Just chatted with chcunningham@ who indicated that the dependent CL is going to change to ...
4 years, 3 months ago (2016-09-15 23:47:52 UTC) #11
chcunningham
Reworked as discussed. Re: config changes, the config change code for splicing is now entirely ...
4 years, 3 months ago (2016-09-16 20:42:44 UTC) #12
DaleCurtis
https://codereview.chromium.org/2343543002/diff/20001/media/filters/source_buffer_stream.cc File media/filters/source_buffer_stream.cc (left): https://codereview.chromium.org/2343543002/diff/20001/media/filters/source_buffer_stream.cc#oldcode1766 media/filters/source_buffer_stream.cc:1766: // Don't generate splice frames which represent less than ...
4 years, 3 months ago (2016-09-16 21:01:04 UTC) #14
chcunningham
https://codereview.chromium.org/2343543002/diff/20001/media/filters/source_buffer_stream.cc File media/filters/source_buffer_stream.cc (left): https://codereview.chromium.org/2343543002/diff/20001/media/filters/source_buffer_stream.cc#oldcode1766 media/filters/source_buffer_stream.cc:1766: // Don't generate splice frames which represent less than ...
4 years, 3 months ago (2016-09-16 22:25:24 UTC) #15
DaleCurtis
Should have left this comment the first time too, sorry. https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_buffer_stream_unittest.cc File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_buffer_stream_unittest.cc#oldcode4298 ...
4 years, 3 months ago (2016-09-16 22:28:29 UTC) #16
chcunningham
https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_buffer_stream_unittest.cc File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_buffer_stream_unittest.cc#oldcode4298 media/filters/source_buffer_stream_unittest.cc:4298: TEST_F(SourceBufferStreamTest, Audio_SpliceFrame_NoMillisecondSplices) { On 2016/09/16 22:28:29, DaleCurtis wrote: > ...
4 years, 3 months ago (2016-09-19 22:51:37 UTC) #17
chcunningham
Friendly ping.
4 years, 3 months ago (2016-09-22 17:50:15 UTC) #26
wolenetz
On 2016/09/22 17:50:15, chcunningham wrote: > Friendly ping. Was OoO sick. Looking at this now.
4 years, 3 months ago (2016-09-22 18:24:38 UTC) #27
wolenetz
Please update the CL description to reference the MSE VNext spec issue I filed to ...
4 years, 3 months ago (2016-09-22 20:57:10 UTC) #28
chcunningham
On 2016/09/22 20:57:10, wolenetz wrote: > Please update the CL description to reference the MSE ...
4 years, 3 months ago (2016-09-22 21:21:20 UTC) #30
wolenetz
partial CR. Lots of interesting spec side-effects and investigations are delaying this CR a little: ...
4 years, 3 months ago (2016-09-22 23:48:37 UTC) #31
wolenetz
Looking pretty good, though I have unfortunately a lot of comments: https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_demuxer_unittest.cc File media/filters/chunk_demuxer_unittest.cc (left): ...
4 years, 2 months ago (2016-09-26 23:52:38 UTC) #32
wolenetz
Updates from interactive review of comments w/chcunningham@: https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_demuxer_unittest.cc File media/filters/chunk_demuxer_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_demuxer_unittest.cc#oldcode2903 media/filters/chunk_demuxer_unittest.cc:2903: "{ [0,23) ...
4 years, 2 months ago (2016-09-27 00:35:36 UTC) #33
chcunningham
All comments addressed (finally). https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_demuxer_unittest.cc File media/filters/chunk_demuxer_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/80001/media/filters/chunk_demuxer_unittest.cc#oldcode2903 media/filters/chunk_demuxer_unittest.cc:2903: "{ [0,23) [300,400) [520,590) [720,750) ...
4 years, 1 month ago (2016-10-27 23:36:07 UTC) #34
wolenetz
Looking almost ready to land: https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_buffer_stream_unittest.cc File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_buffer_stream_unittest.cc#oldcode4377 media/filters/source_buffer_stream_unittest.cc:4377: CheckExpectedRangesByTimestamp("{ [0,16) }"); On ...
4 years, 1 month ago (2016-10-28 23:08:19 UTC) #35
wolenetz
Though I'm out tomorrow (Monday Oct 31) I'll try to take a look in the ...
4 years, 1 month ago (2016-10-31 02:27:41 UTC) #36
chcunningham
Thanks Matt. I think I got them all this time https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_buffer_stream_unittest.cc File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_buffer_stream_unittest.cc#oldcode4377 ...
4 years, 1 month ago (2016-11-02 01:28:42 UTC) #37
wolenetz
LGTM https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_buffer_stream_unittest.cc File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/2343543002/diff/40001/media/filters/source_buffer_stream_unittest.cc#oldcode4377 media/filters/source_buffer_stream_unittest.cc:4377: CheckExpectedRangesByTimestamp("{ [0,16) }"); On 2016/11/02 01:28:42, chcunningham wrote: ...
4 years, 1 month ago (2016-11-02 20:39:34 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2343543002/120001
4 years, 1 month ago (2016-11-02 20:40:07 UTC) #44
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 1 month ago (2016-11-02 20:47:42 UTC) #46
commit-bot: I haz the power
4 years, 1 month ago (2016-11-02 20:51:15 UTC) #48
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/967db2f72de7e17e8c3326438e29c90375e3a1be
Cr-Commit-Position: refs/heads/master@{#429394}

Powered by Google App Engine
This is Rietveld 408576698