Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1846863002:
Remove SourceBuffer media tracks on detach from media element (Closed)
|
Created:
4 years, 8 months ago by servolk Modified:
4 years, 7 months ago Reviewers:
wolenetz CC:
chromium-reviews, blink-reviews, feature-media-reviews_chromium.org, philipj_slow, mlamouri+watch-blink_chromium.org, eric.carlson_apple.com Base URL:
https://chromium.googlesource.com/chromium/src.git@blink-sb-tracks6 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove SourceBuffer media tracks on detach from media element
Also added a layout test for this.
BUG=249428
Committed: https://crrev.com/2f5b9a25cf9fe0ed9fbb9fe855d7bf143d14e5b4
Cr-Commit-Position: refs/heads/master@{#390460}
Patch Set 1 #
Total comments: 6
Patch Set 2 : rebase #Patch Set 3 : Updated test #Patch Set 4 : Set .sourceBuffer to null when removing tracks #Patch Set 5 : rebase #Patch Set 6 : rebase #Patch Set 7 : rebase #Patch Set 8 : rebase #Patch Set 9 : rebase #Patch Set 10 : rebase #Patch Set 11 : Ensure that m_attachedElement is still valid while SourceBuffers are removed from MediaSource #Patch Set 12 : better fix #Patch Set 13 : rebase #Patch Set 14 : buildfix #Patch Set 15 : Don't attempt to remove anything if there's no tracks #
Total comments: 27
Patch Set 16 : CR feedback #
Total comments: 33
Patch Set 17 : CR feedback #
Total comments: 12
Patch Set 18 : nits #
Messages
Total messages: 21 (8 generated)
Description was changed from ========== Remove SourceBuffer media tracks on detach from media element Also added a layout test for this. ========== to ========== Remove SourceBuffer media tracks on detach from media element Also added a layout test for this. ==========
servolk@chromium.org changed reviewers: + philipj@opera.com, wolenetz@chromium.org
Description was changed from ========== Remove SourceBuffer media tracks on detach from media element Also added a layout test for this. ========== to ========== Remove SourceBuffer media tracks on detach from media element Also added a layout test for this. BUG=249428 ==========
Thanks for adding this functionality. Looking pretty good. https://codereview.chromium.org/1846863002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html (right): https://codereview.chromium.org/1846863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:61: // Removing enabled audio track and selected video track should fire 'change' events on mediaElement track lists. Please also check for all of the HTMLME.{audio,video}Tracks and SB.{audio,video}Tracks getting the required 'removetrack' events with the correct track IDs. https://codereview.chromium.org/1846863002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1846863002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:515: // TODO(servolk): 4.3.1 Set the sourceBuffer attribute on the AudioTrack object to null. Is this not doable once https://codereview.chromium.org/1658033002/ lands (that's currently the CL that this CL depends on in Rietveld)? https://codereview.chromium.org/1846863002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:541: // TODO(servolk): 6.3.1 Set the sourceBuffer attribute on the VideoTrack object to null. ditto
https://codereview.chromium.org/1846863002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html (right): https://codereview.chromium.org/1846863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:61: // Removing enabled audio track and selected video track should fire 'change' events on mediaElement track lists. On 2016/03/31 19:06:15, wolenetz wrote: > Please also check for all of the HTMLME.{audio,video}Tracks and > SB.{audio,video}Tracks getting the required 'removetrack' events with the > correct track IDs. Done. https://codereview.chromium.org/1846863002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1846863002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:515: // TODO(servolk): 4.3.1 Set the sourceBuffer attribute on the AudioTrack object to null. On 2016/03/31 19:06:15, wolenetz wrote: > Is this not doable once https://codereview.chromium.org/1658033002/ lands > (that's currently the CL that this CL depends on in Rietveld)? Done. https://codereview.chromium.org/1846863002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:541: // TODO(servolk): 6.3.1 Set the sourceBuffer attribute on the VideoTrack object to null. On 2016/03/31 19:06:15, wolenetz wrote: > ditto Done.
servolk@chromium.org changed reviewers: - philipj@opera.com
On 2016/03/31 22:39:51, servolk wrote: > https://codereview.chromium.org/1846863002/diff/1/third_party/WebKit/LayoutTe... > File > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html > (right): > > https://codereview.chromium.org/1846863002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:61: > // Removing enabled audio track and selected video track should fire 'change' > events on mediaElement track lists. > On 2016/03/31 19:06:15, wolenetz wrote: > > Please also check for all of the HTMLME.{audio,video}Tracks and > > SB.{audio,video}Tracks getting the required 'removetrack' events with the > > correct track IDs. > > Done. > > https://codereview.chromium.org/1846863002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): > > https://codereview.chromium.org/1846863002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:515: // > TODO(servolk): 4.3.1 Set the sourceBuffer attribute on the AudioTrack object to > null. > On 2016/03/31 19:06:15, wolenetz wrote: > > Is this not doable once https://codereview.chromium.org/1658033002/ lands > > (that's currently the CL that this CL depends on in Rietveld)? > > Done. > > https://codereview.chromium.org/1846863002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:541: // > TODO(servolk): 6.3.1 Set the sourceBuffer attribute on the VideoTrack object to > null. > On 2016/03/31 19:06:15, wolenetz wrote: > > ditto > > Done. ping
Sorry for delay -- this review now benefits from my recent experience with updated style/etc when I added mediasource-preload.html layout test as well as the recent clarification in the MSE spec around detachment. Mostly nits remain, though I've included requests to include additional detach-related layout testing: https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html (right): https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:8: <link rel='stylesheet' href='/w3c/resources/testharness.css'> nit: remove this line. css is added by testharness.js if necessary https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:11: <div id="log"></div> nit: remove this line. unnecessary now.. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:13: mediasource_testafterdataloaded(function(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData) nit indent everything 2 more columns between <script></script> here. (4-space indent) https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:16: test.expectEvent(sourceBuffer.audioTracks, 'addtrack', 'sourceBuffer.videoTracks addtrack event'); nit: use either ' or ", not mixed, to contain strings within <script>...</script>. Let's go with ". https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:16: test.expectEvent(sourceBuffer.audioTracks, 'addtrack', 'sourceBuffer.videoTracks addtrack event'); nit: s/video/audio/ in message. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:18: test.expectEvent(mediaElement.audioTracks, 'addtrack', 'mediaElement.videoTracks addtrack event'); nit ditto here and below as appropriate. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:25: assert_equals(sourceBuffer.videoTracks.length, 1, "videoTracks.length"); nit:indentation https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:56: test.expectEvent(sourceBuffer.audioTracks, 'addtrack', 'sourceBuffer.videoTracks addtrack event'); nit ditto (audio <-> video) here and below as appropriate. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:66: test.waitForExpectedEvents(function() Since these first few lines are common across these two tests. I suggest putting them in a utility function that takes a callback that is invoked on this first test.waitForExpectedEvents() in each test. Or even subsume most of the first test in this utility function, since much of the property checking after that first waitForExpectedEvents is the same in both tests. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:68: assert_equals(mediaElement.videoTracks.length, 1, "videoTracks.length"); nit:indentation https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:82: test.expectEvent(sourceBuffer.audioTracks, 'removetrack', 'sourceBuffer.videoTracks removetrack event'); nit ditto (audio <-> video) here and below as appropriate... https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:95: assert_equals(mediaSource.sourceBuffers.length, 0, "mediaSource.sourceBuffers.length"); nit:indentation https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:106: </script> Please add a test specifically for this removal also happening on detach. IIUC, mediaElement.load() should trigger detach(). Please reference the new non-normative note in http://w3c.github.io/media-source/#mediasource-detach A more interoperable test (than load() triggering detach) might be to just set src="". Perhaps testing both would be best, and when this is upstreamed to w3c web-platform-tests, I'll include only the src="" version, since that's guaranteed to be spec compliant/interoperable IIUC. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:479: void SourceBuffer::removedFromMediaSource() It looks like https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... can be fixed in this CL now, too. IIUC, MediaSource::removeSourceBuffer() should call buffer->removedFromMediaSource() now to perform its steps 2-8, and step 11 is a no-op since removedFromMediaSource and oilpan does that. (Media track) layout tests for MediaSource.removeSourceBuffer() would be good, too.
https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html (right): https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:8: <link rel='stylesheet' href='/w3c/resources/testharness.css'> On 2016/04/27 21:39:55, wolenetz wrote: > nit: remove this line. css is added by testharness.js if necessary Done. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:11: <div id="log"></div> On 2016/04/27 21:39:55, wolenetz wrote: > nit: remove this line. unnecessary now.. Done. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:13: mediasource_testafterdataloaded(function(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData) On 2016/04/27 21:39:54, wolenetz wrote: > nit indent everything 2 more columns between <script></script> here. (4-space > indent) Done. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:16: test.expectEvent(sourceBuffer.audioTracks, 'addtrack', 'sourceBuffer.videoTracks addtrack event'); On 2016/04/27 21:39:54, wolenetz wrote: > nit: use either ' or ", not mixed, to contain strings within > <script>...</script>. Let's go with ". Done. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:16: test.expectEvent(sourceBuffer.audioTracks, 'addtrack', 'sourceBuffer.videoTracks addtrack event'); On 2016/04/27 21:39:55, wolenetz wrote: > nit: s/video/audio/ in message. Done. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:18: test.expectEvent(mediaElement.audioTracks, 'addtrack', 'mediaElement.videoTracks addtrack event'); On 2016/04/27 21:39:55, wolenetz wrote: > nit ditto here and below as appropriate. Done. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:25: assert_equals(sourceBuffer.videoTracks.length, 1, "videoTracks.length"); On 2016/04/27 21:39:55, wolenetz wrote: > nit:indentation Done. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:56: test.expectEvent(sourceBuffer.audioTracks, 'addtrack', 'sourceBuffer.videoTracks addtrack event'); On 2016/04/27 21:39:55, wolenetz wrote: > nit ditto (audio <-> video) here and below as appropriate. Done. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:66: test.waitForExpectedEvents(function() On 2016/04/27 21:39:55, wolenetz wrote: > Since these first few lines are common across these two tests. I suggest putting > them in a utility function that takes a callback that is invoked on this first > test.waitForExpectedEvents() in each test. Or even subsume most of the first > test in this utility function, since much of the property checking after that > first waitForExpectedEvents is the same in both tests. Done. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:68: assert_equals(mediaElement.videoTracks.length, 1, "videoTracks.length"); On 2016/04/27 21:39:54, wolenetz wrote: > nit:indentation Done. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:82: test.expectEvent(sourceBuffer.audioTracks, 'removetrack', 'sourceBuffer.videoTracks removetrack event'); On 2016/04/27 21:39:55, wolenetz wrote: > nit ditto (audio <-> video) here and below as appropriate... Done. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:95: assert_equals(mediaSource.sourceBuffers.length, 0, "mediaSource.sourceBuffers.length"); On 2016/04/27 21:39:55, wolenetz wrote: > nit:indentation Done. https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:106: </script> On 2016/04/27 21:39:55, wolenetz wrote: > Please add a test specifically for this removal also happening on detach. IIUC, > mediaElement.load() should trigger detach(). > Please reference the new non-normative note in > http://w3c.github.io/media-source/#mediasource-detach > A more interoperable test (than load() triggering detach) might be to just set > src="". Perhaps testing both would be best, and when this is upstreamed to w3c > web-platform-tests, I'll include only the src="" version, since that's > guaranteed to be spec compliant/interoperable IIUC. > Done.
Almost there :) https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html (right): https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:10: function loadMediaAndVerifyAddedTracks(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData, successCallback) nit: mediaSource is unused param. remove please. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:23: assert_equals(sourceBuffer.videoTracks[0].id, "1", "videoTrack.id"); nit: to help upstream this, all that's needed is for these id's to be unique. Please change to verify just the uniqueness, since other user agents might generate completely different unique IDs here. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:32: assert_equals(sourceBuffer.audioTracks[0].id, "2", "audioTrack.id"); nit ditto https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:55: function verifyTrackRemoval(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData, trackRemovalAction, successCallback) { nit: mediaSource, segmentInfo, mediaData are unused params. remove please. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:92: loadMediaAndVerifyAddedTracks(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData, function() { nit: s/function()/test.step_func()/ https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:93: verifyTrackRemoval(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData, function removeSB() { nit: s/function removeSB()/test.step_func()/ https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:95: }, function successCallback() { nit: s/function successCallback(){...}/test.step_func_done()/ https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:103: loadMediaAndVerifyAddedTracks(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData, function() { nit ditto https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:104: verifyTrackRemoval(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData, function removeSB() { nit ditto https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:106: }, function successCallback() { nit ditto https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:110: }, "Media tracks must be removed when the HTMLMediaElement is reset"); nit:s/is reset/src is changed/ https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:114: loadMediaAndVerifyAddedTracks(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData, function() { nit ditto https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:115: verifyTrackRemoval(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData, function removeSB() { nit ditto https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:118: test.done(); nit ditto https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:121: }, "Media tracks must be removed when the HTMLMediaElement is reset"); nit:s/is reset/load() is called/ https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:183: // Steps 3-8 are related to updating audioTracks, videoTracks, and textTracks which aren't implmented yet. This is fixed by this CL (except text), right? Please move l.194 to here. Note that buffer->abortIfUpdating() is redundant with what buffer->removedFromMediaSource() already does, so drop l.181 and update the comment to say Steps 2-8 are done by (buffer->removedFromMediaSource). This was what I was trying to indicate in my earlier CR comment at https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Sou...
https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html (right): https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:10: function loadMediaAndVerifyAddedTracks(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData, successCallback) On 2016/04/28 00:46:47, wolenetz wrote: > nit: mediaSource is unused param. remove please. Done. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:23: assert_equals(sourceBuffer.videoTracks[0].id, "1", "videoTrack.id"); On 2016/04/28 00:46:46, wolenetz wrote: > nit: to help upstream this, all that's needed is for these id's to be unique. > Please change to verify just the uniqueness, since other user agents might > generate completely different unique IDs here. Done. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:32: assert_equals(sourceBuffer.audioTracks[0].id, "2", "audioTrack.id"); On 2016/04/28 00:46:47, wolenetz wrote: > nit ditto Done. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:55: function verifyTrackRemoval(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData, trackRemovalAction, successCallback) { On 2016/04/28 00:46:46, wolenetz wrote: > nit: mediaSource, segmentInfo, mediaData are unused params. remove please. mediaSource is used to verify that sourceBuffers collection is empty. Removed segmentInfo/mediaData. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:92: loadMediaAndVerifyAddedTracks(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData, function() { On 2016/04/28 00:46:47, wolenetz wrote: > nit: s/function()/test.step_func()/ Done. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:93: verifyTrackRemoval(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData, function removeSB() { On 2016/04/28 00:46:46, wolenetz wrote: > nit: s/function removeSB()/test.step_func()/ Done. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:95: }, function successCallback() { On 2016/04/28 00:46:46, wolenetz wrote: > nit: s/function successCallback(){...}/test.step_func_done()/ Done. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:103: loadMediaAndVerifyAddedTracks(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData, function() { On 2016/04/28 00:46:47, wolenetz wrote: > nit ditto Done. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:104: verifyTrackRemoval(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData, function removeSB() { On 2016/04/28 00:46:47, wolenetz wrote: > nit ditto Done. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:106: }, function successCallback() { On 2016/04/28 00:46:47, wolenetz wrote: > nit ditto Done. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:110: }, "Media tracks must be removed when the HTMLMediaElement is reset"); On 2016/04/28 00:46:46, wolenetz wrote: > nit:s/is reset/src is changed/ Done. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:114: loadMediaAndVerifyAddedTracks(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData, function() { On 2016/04/28 00:46:47, wolenetz wrote: > nit ditto Done. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:115: verifyTrackRemoval(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData, function removeSB() { On 2016/04/28 00:46:46, wolenetz wrote: > nit ditto Done. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:118: test.done(); On 2016/04/28 00:46:46, wolenetz wrote: > nit ditto Done. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:121: }, "Media tracks must be removed when the HTMLMediaElement is reset"); On 2016/04/28 00:46:46, wolenetz wrote: > nit:s/is reset/load() is called/ Done. https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:183: // Steps 3-8 are related to updating audioTracks, videoTracks, and textTracks which aren't implmented yet. On 2016/04/28 00:46:47, wolenetz wrote: > This is fixed by this CL (except text), right? Please move l.194 to here. Note > that buffer->abortIfUpdating() is redundant with what > buffer->removedFromMediaSource() already does, so drop l.181 and update the > comment to say Steps 2-8 are done by (buffer->removedFromMediaSource). This was > what I was trying to indicate in my earlier CR comment at > https://codereview.chromium.org/1846863002/diff/280001/third_party/WebKit/Sou... Done.
lgtm % some tiny remaining nits. Thanks again for adding this functionality! https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html (right): https://codereview.chromium.org/1846863002/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:55: function verifyTrackRemoval(test, mediaElement, mediaSource, segmentInfo, sourceBuffer, mediaData, trackRemovalAction, successCallback) { On 2016/04/28 18:37:53, servolk wrote: > On 2016/04/28 00:46:46, wolenetz wrote: > > nit: mediaSource, segmentInfo, mediaData are unused params. remove please. > > mediaSource is used to verify that sourceBuffers collection is empty. Removed > segmentInfo/mediaData. Acknowledged. https://codereview.chromium.org/1846863002/diff/320001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html (right): https://codereview.chromium.org/1846863002/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:92: loadMediaAndVerifyAddedTracks(test, mediaElement, segmentInfo, sourceBuffer, mediaData, test.step_func(function (event) nit: s/function (event)/function()/ https://codereview.chromium.org/1846863002/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:94: verifyTrackRemoval(test, mediaElement, mediaSource, sourceBuffer, test.step_func(function (event) nit ditto https://codereview.chromium.org/1846863002/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:103: loadMediaAndVerifyAddedTracks(test, mediaElement, segmentInfo, sourceBuffer, mediaData, test.step_func(function (event) nit ditto https://codereview.chromium.org/1846863002/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:105: verifyTrackRemoval(test, mediaElement, mediaSource, sourceBuffer, test.step_func(function (event) nit ditto https://codereview.chromium.org/1846863002/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:114: loadMediaAndVerifyAddedTracks(test, mediaElement, segmentInfo, sourceBuffer, mediaData, test.step_func(function (event) nit ditto https://codereview.chromium.org/1846863002/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:116: verifyTrackRemoval(test, mediaElement, mediaSource, sourceBuffer, test.step_func(function (event) nit ditto
https://codereview.chromium.org/1846863002/diff/320001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html (right): https://codereview.chromium.org/1846863002/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:92: loadMediaAndVerifyAddedTracks(test, mediaElement, segmentInfo, sourceBuffer, mediaData, test.step_func(function (event) On 2016/04/28 18:48:29, wolenetz wrote: > nit: s/function (event)/function()/ Done. https://codereview.chromium.org/1846863002/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:94: verifyTrackRemoval(test, mediaElement, mediaSource, sourceBuffer, test.step_func(function (event) On 2016/04/28 18:48:29, wolenetz wrote: > nit ditto Done. https://codereview.chromium.org/1846863002/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:103: loadMediaAndVerifyAddedTracks(test, mediaElement, segmentInfo, sourceBuffer, mediaData, test.step_func(function (event) On 2016/04/28 18:48:29, wolenetz wrote: > nit ditto Done. https://codereview.chromium.org/1846863002/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:105: verifyTrackRemoval(test, mediaElement, mediaSource, sourceBuffer, test.step_func(function (event) On 2016/04/28 18:48:29, wolenetz wrote: > nit ditto Done. https://codereview.chromium.org/1846863002/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:114: loadMediaAndVerifyAddedTracks(test, mediaElement, segmentInfo, sourceBuffer, mediaData, test.step_func(function (event) On 2016/04/28 18:48:29, wolenetz wrote: > nit ditto Done. https://codereview.chromium.org/1846863002/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:116: verifyTrackRemoval(test, mediaElement, mediaSource, sourceBuffer, test.step_func(function (event) On 2016/04/28 18:48:29, wolenetz wrote: > nit ditto Done.
The CQ bit was checked by servolk@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/1846863002/#ps340001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1846863002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846863002/340001
Message was sent while issue was closed.
Description was changed from ========== Remove SourceBuffer media tracks on detach from media element Also added a layout test for this. BUG=249428 ========== to ========== Remove SourceBuffer media tracks on detach from media element Also added a layout test for this. BUG=249428 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/2f5b9a25cf9fe0ed9fbb9fe855d7bf143d14e5b4 Cr-Commit-Position: refs/heads/master@{#390460}
Message was sent while issue was closed.
Description was changed from ========== Remove SourceBuffer media tracks on detach from media element Also added a layout test for this. BUG=249428 ========== to ========== Remove SourceBuffer media tracks on detach from media element Also added a layout test for this. BUG=249428 Committed: https://crrev.com/2f5b9a25cf9fe0ed9fbb9fe855d7bf143d14e5b4 Cr-Commit-Position: refs/heads/master@{#390460} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
