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

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

Created:
4 years ago by chcunningham
Modified:
3 years, 10 months 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 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 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 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 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 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 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 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 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 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: > ...
3 years, 12 months ago (2016-09-19 22:51:37 UTC) #17
chcunningham
Friendly ping.
3 years, 12 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.
3 years, 12 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 ...
3 years, 12 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 ...
3 years, 12 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: ...
3 years, 12 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): ...
3 years, 11 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) ...
3 years, 11 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) ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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: ...
3 years, 10 months 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
3 years, 10 months ago (2016-11-02 20:40:07 UTC) #44
commit-bot: I haz the power
Committed patchset #7 (id:120001)
3 years, 10 months ago (2016-11-02 20:47:42 UTC) #46
commit-bot: I haz the power
3 years, 10 months 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