Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1658033002:
Add SourceBuffer implementations of Audio/VideoTracks (Closed)
|
Created:
4 years, 10 months ago by servolk Modified:
4 years, 8 months ago CC:
fs, blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, gasubic, mlamouri+watch-blink_chromium.org, philipj_slow, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@pass-media-tracks-to-blink Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd SourceBuffer implementations of Audio/VideoTracks
This CL provides SourceBuffer extensions described in MSE spec:
http://www.w3.org/TR/media-source/#audio-track-extensions
http://www.w3.org/TR/media-source/#video-track-extensions
The only difference between those extensions and the existing audio/
video track is that these have the .sourceBuffer property, which is
only going to be non-null for a/v tracks created by a SourceBuffer
and will return the parent SourceBuffer object.
I've looked at how other extension interfaces are implemented in blink,
e.g. NavigatorGeolocation extension property, and tried to do the same.
BUG=249427, 249428
Committed: https://crrev.com/1b3dc5a8202a332891b6aed5d4d4521db0b26261
Cr-Commit-Position: refs/heads/master@{#385816}
Patch Set 1 #Patch Set 2 : ps2 #Patch Set 3 : rebase #Patch Set 4 : rebase #
Total comments: 20
Patch Set 5 : Fixes #Patch Set 6 : Fixed copyright headers #Patch Set 7 : Make Audio/VideoTrackSB non-finalizable #Patch Set 8 : Make Audio/VideoTrackSB finalizable again #
Total comments: 9
Patch Set 9 : rebase #Patch Set 10 : Fixed incorrect conflict resolution #Patch Set 11 : Make m_sourceBuffer a strong Member of A/V tracks #Patch Set 12 : rebase #Patch Set 13 : Better formatting in idl files #Patch Set 14 : Moved supplement functionality to a base class #Patch Set 15 : rebase #Patch Set 16 : Add A/V track lists to SourceBuffer and use A/V extensions #Patch Set 17 : Updated test #Patch Set 18 : Mark SourceBuffer A/V tracks as GarbageCollected in .idl #Patch Set 19 : Make A/V track lists plain members in SourceBuffer #
Total comments: 8
Patch Set 20 : Remove redundant classes/files #Patch Set 21 : Fix ChromeOS builds #
Total comments: 5
Patch Set 22 : rebase #Patch Set 23 : rebase #Patch Set 24 : rebase #Patch Set 25 : rebase #Patch Set 26 : rebase #Patch Set 27 : rebase #Patch Set 28 : rebase #Patch Set 29 : rebase #
Total comments: 2
Patch Set 30 : Improved test + test coverage for addtrack events #Patch Set 31 : rebase #
Total comments: 2
Patch Set 32 : rebase #Patch Set 33 : rebase #Patch Set 34 : rebase #Patch Set 35 : Added SourceBufferTrackBaseSupplement::from #
Total comments: 4
Patch Set 36 : Added SourceBufferTrackBaseSupplement::fromIfExists #Dependent Patchsets: Messages
Total messages: 43 (8 generated)
Description was changed from ========== Add SourceBuffer implementations of Audio/VideoTracks ========== to ========== Add SourceBuffer implementations of Audio/VideoTracks This CL provides SourceBuffer extensions described in MSE spec: http://www.w3.org/TR/media-source/#audio-track-extensions http://www.w3.org/TR/media-source/#video-track-extensions The only difference between those extensions and the existing audio/ video track is that these have the .sourceBuffer property, which is only going to be non-null for a/v tracks created by a SourceBuffer and will return the parent SourceBuffer object. I've looked at how other extension interfaces are implemented in blink, e.g. NavigatorGeolocation extension property, and tried to do the same. BUG=249427 ==========
servolk@chromium.org changed reviewers: + philipj@opera.com, wolenetz@chromium.org
A bit context: I'm working on a few CLs to allow us to get full media track support and eventually move the MSE 'init segment received' algorithm completely to blink (currently it lives in Chromium media pipeline). This CL is part of that work, it introduces a Audio/VideoTrack interface extensions for SourceBuffer. Other related CLs include Basic media track functionality on Chromium media pipeline level: https://codereview.chromium.org/1716503002/ Passing media track info from Chromium to blink: https://codereview.chromium.org/1659653002/ First stab at implementing the MSE 'init segment received' algorithm in blink: https://codereview.chromium.org/1678523003/
OK, so this is the context I was missing, and an important difference between AudioTrack with MSE and for plain HTTP playback is the sourceBuffer attribute.
This scaffolding looks good, but there isn't anything that will actually populate audioTrackList/videoTrackList, and the new sourceBuffer attribute will always return null. That means there isn't much to test, but *some* tests of at least audioTrackList/videoTrackList should be possible. Of the CLs that follow I don't see anything that will make this code really do anything, is that a yet-to-be-written part? I guess that the invariant should be that any track that is in a SourceBuffer's audioTrackList/videoTrackList should also have its sourceBuffer attribute return that value. The best place to maintain that is probably in the code to add/remove tracks from SourceBuffer, can that be written at this point? https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/AudioTrack.idl (right): https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/AudioTrack.idl:2: * Copyright (C) 2016 Google Inc. All rights reserved. Use the new smaller copyright header, here and in other new files. https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/AudioTrack.idl:31: // http://www.w3.org/TR/media-source/#audio-track-extensions Please use the most up-to-date spec available: https://w3c.github.io/media-source/#audio-track-extensions As implementors, we should always look at the most recent version of any spec, to not risk implementing bugs that have already been fixed, etc. https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/AudioTrack.idl:35: ImplementedAs=SourceBufferAudioTrack, If you rename this file to AudioTrackSourceBuffer.idl and do the same with the implementation files, you can drop ImplementedAs. BaseInterfaceExtension is the usual pattern, see e.g. DocumentFullscreen.idl and the many HTMLMediaElement*.idl https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:233: return *m_audioTracks; Has m_audioTracks actually been created, won't this crash? This looks like HTMLMediaElement, but if it works I don't understand why, since Member<T> can be null. Right? https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.h (right): https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBuffer.h:152: Member<AudioTrackList> m_audioTracks; I think this will work with Oilpan disabled, but before landing can you make sure it builds and all tests pass without Oilpan? https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBufferAudioTrack.h (right): https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBufferAudioTrack.h:42: class SourceBufferAudioTrack final : public GarbageCollectedFinalized<SourceBufferAudioTrack>, public HeapSupplement<AudioTrack> { Does it need to be finalized?
https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/AudioTrack.idl (right): https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/AudioTrack.idl:2: * Copyright (C) 2016 Google Inc. All rights reserved. On 2016/02/23 04:35:07, philipj_UTC7 wrote: > Use the new smaller copyright header, here and in other new files. Done. https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/AudioTrack.idl:31: // http://www.w3.org/TR/media-source/#audio-track-extensions On 2016/02/23 04:35:07, philipj_UTC7 wrote: > Please use the most up-to-date spec available: > https://w3c.github.io/media-source/#audio-track-extensions > > As implementors, we should always look at the most recent version of any spec, > to not risk implementing bugs that have already been fixed, etc. Done. https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/AudioTrack.idl:35: ImplementedAs=SourceBufferAudioTrack, On 2016/02/23 04:35:07, philipj_UTC7 wrote: > If you rename this file to AudioTrackSourceBuffer.idl and do the same with the > implementation files, you can drop ImplementedAs. BaseInterfaceExtension is the > usual pattern, see e.g. DocumentFullscreen.idl and the many > HTMLMediaElement*.idl Done. https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:233: return *m_audioTracks; On 2016/02/23 04:35:07, philipj_UTC7 wrote: > Has m_audioTracks actually been created, won't this crash? This looks like > HTMLMediaElement, but if it works I don't understand why, since Member<T> can be > null. Right? Yeah, that's right. I need to figure out a good place where to init the m_audio/videoTracks. IIUC we can't do this directly in the constructor because at that point the media source might be not yet attached to the HTMLMediaElement and so m_source->getElement might be null. We'll probably need to notify the SB when MediaSource gets attached to an HTMLMediaElement and create the track collections at that point. https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBufferAudioTrack.h (right): https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBufferAudioTrack.h:42: class SourceBufferAudioTrack final : public GarbageCollectedFinalized<SourceBufferAudioTrack>, public HeapSupplement<AudioTrack> { On 2016/02/23 04:35:07, philipj_UTC7 wrote: > Does it need to be finalized? I guess not, so I've changed to a regulat GarbageCollected for now. Although for some reason the current implementations of regular Audio/VideoTracks are also GarbageCollectedFinalized, even though they have trivial destructors. Is that because we want to ensure that we always check that assert in blink ~TrackBase ?
https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBufferAudioTrack.h (right): https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBufferAudioTrack.h:42: class SourceBufferAudioTrack final : public GarbageCollectedFinalized<SourceBufferAudioTrack>, public HeapSupplement<AudioTrack> { On 2016/02/24 00:11:36, servolk wrote: > On 2016/02/23 04:35:07, philipj_UTC7 wrote: > > Does it need to be finalized? > > I guess not, so I've changed to a regulat GarbageCollected for now. Although for > some reason the current implementations of regular Audio/VideoTracks are also > GarbageCollectedFinalized, even though they have trivial destructors. Is that > because we want to ensure that we always check that assert in blink ~TrackBase ? Actually, turns out this needs to be finalized, since the PS #7 caused build failures, so I'm making them finalizable again.
https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:233: return *m_audioTracks; On 2016/02/24 00:11:36, servolk wrote: > On 2016/02/23 04:35:07, philipj_UTC7 wrote: > > Has m_audioTracks actually been created, won't this crash? This looks like > > HTMLMediaElement, but if it works I don't understand why, since Member<T> can > be > > null. Right? > > Yeah, that's right. I need to figure out a good place where to init the > m_audio/videoTracks. IIUC we can't do this directly in the constructor because > at that point the media source might be not yet attached to the HTMLMediaElement > and so m_source->getElement might be null. We'll probably need to notify the SB > when MediaSource gets attached to an HTMLMediaElement and create the track > collections at that point. Until this works and doesn't crash, can you instead ASSERT_NOT_REACHED here? If it can be reached, I guess remove things until it's unreachable, we ought not have things that can crash even behind a flag I think. https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBufferAudioTrack.h (right): https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBufferAudioTrack.h:42: class SourceBufferAudioTrack final : public GarbageCollectedFinalized<SourceBufferAudioTrack>, public HeapSupplement<AudioTrack> { On 2016/02/24 02:37:15, servolk wrote: > On 2016/02/24 00:11:36, servolk wrote: > > On 2016/02/23 04:35:07, philipj_UTC7 wrote: > > > Does it need to be finalized? > > > > I guess not, so I've changed to a regulat GarbageCollected for now. Although > for > > some reason the current implementations of regular Audio/VideoTracks are also > > GarbageCollectedFinalized, even though they have trivial destructors. Is that > > because we want to ensure that we always check that assert in blink ~TrackBase > ? > > Actually, turns out this needs to be finalized, since the PS #7 caused build > failures, so I'm making them finalizable again. The build error was: In file included from ../../third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.cpp:5: ../../third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h:16:1: error: [blink-gc] Class 'AudioTrackSourceBuffer' requires finalization. class AudioTrackSourceBuffer final : public GarbageCollected<AudioTrackSourceBuffer>, public HeapSupplement<AudioTrack> { ^ ../../third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.cpp:17:1: note: [blink-gc] User-declared destructor declared here: AudioTrackSourceBuffer::~AudioTrackSourceBuffer() ^ 1 error generated. If you remote the empty destructor, do you still need finalized? https://codereview.chromium.org/1658033002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h (right): https://codereview.chromium.org/1658033002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h:16: class AudioTrackSourceBuffer final : public GarbageCollectedFinalized<AudioTrackSourceBuffer>, public HeapSupplement<AudioTrack> { Did you check if it's possible to have only a single HeapSupplement<TrackBase>? Nothing is both an AudioTrack and a VideoTrack, so it ought to work? It could be that you'll need to have almost-empty AudioTrackSourceBuffer and VideoTrackSourceBuffer that merely forward to that shared implementation. https://codereview.chromium.org/1658033002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h:30: WeakMember<SourceBuffer> m_sourceBuffer; Should this really be weak? If a script has a reference to an AudioTrack but has never looked at the sourceBuffer attribute (so it doesn't have a wrapper or another reference), the SourceBuffer should still be kept alive for as long as its possible that the script will peek at the sourceBuffer attribute. In other words, Member<SourceBuffer>? https://codereview.chromium.org/1658033002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.idl (right): https://codereview.chromium.org/1658033002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.idl:6: [RuntimeEnabled=AudioVideoTracks] partial interface AudioTrack { Please format like this: [ RuntimeEnabled=AudioVideoTracks, ] partial interface ... That's the norm, probably because it makes diffs when adding/removing extended attributes smaller. Also a blank line after the URL, at least I've done that almost everywhere.
https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBufferAudioTrack.h (right): https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBufferAudioTrack.h:42: class SourceBufferAudioTrack final : public GarbageCollectedFinalized<SourceBufferAudioTrack>, public HeapSupplement<AudioTrack> { On 2016/02/26 13:36:54, philipj_UTC7 wrote: > On 2016/02/24 02:37:15, servolk wrote: > > On 2016/02/24 00:11:36, servolk wrote: > > > On 2016/02/23 04:35:07, philipj_UTC7 wrote: > > > > Does it need to be finalized? > > > > > > I guess not, so I've changed to a regulat GarbageCollected for now. Although > > for > > > some reason the current implementations of regular Audio/VideoTracks are > also > > > GarbageCollectedFinalized, even though they have trivial destructors. Is > that > > > because we want to ensure that we always check that assert in blink > ~TrackBase > > ? > > > > Actually, turns out this needs to be finalized, since the PS #7 caused build > > failures, so I'm making them finalizable again. > > The build error was: > In file included from > ../../third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.cpp:5: > ../../third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h:16:1: > error: [blink-gc] Class 'AudioTrackSourceBuffer' requires finalization. > class AudioTrackSourceBuffer final : public > GarbageCollected<AudioTrackSourceBuffer>, public HeapSupplement<AudioTrack> { > ^ > ../../third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.cpp:17:1: > note: [blink-gc] User-declared destructor declared here: > AudioTrackSourceBuffer::~AudioTrackSourceBuffer() > ^ > 1 error generated. > > If you remote the empty destructor, do you still need finalized? Yes, I tried completely removing the destructor first, that produced a compile error saying a user-type with non-trivial members must have an explicit destructor. And after adding an explicit destructor I got this error about mandatory finalization. https://codereview.chromium.org/1658033002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h (right): https://codereview.chromium.org/1658033002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h:16: class AudioTrackSourceBuffer final : public GarbageCollectedFinalized<AudioTrackSourceBuffer>, public HeapSupplement<AudioTrack> { On 2016/02/26 13:36:54, philipj_UTC7 wrote: > Did you check if it's possible to have only a single HeapSupplement<TrackBase>? > Nothing is both an AudioTrack and a VideoTrack, so it ought to work? It could be > that you'll need to have almost-empty AudioTrackSourceBuffer and > VideoTrackSourceBuffer that merely forward to that shared implementation. Good idea, I haven't tried that yet, I'll give it a try. https://codereview.chromium.org/1658033002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h:30: WeakMember<SourceBuffer> m_sourceBuffer; On 2016/02/26 13:36:54, philipj_UTC7 wrote: > Should this really be weak? If a script has a reference to an AudioTrack but has > never looked at the sourceBuffer attribute (so it doesn't have a wrapper or > another reference), the SourceBuffer should still be kept alive for as long as > its possible that the script will peek at the sourceBuffer attribute. In other > words, Member<SourceBuffer>? I don't know much about oilpan/GC being used by blink, so I was worried that if the GC is using refcounting then using a strong Member reference here would create a loop (since SourceBuffer already had strong refs to its tracks). Can oilpan handle reference loops?
https://codereview.chromium.org/1658033002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h (right): https://codereview.chromium.org/1658033002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h:30: WeakMember<SourceBuffer> m_sourceBuffer; On 2016/02/27 at 01:03:45, servolk wrote: > On 2016/02/26 13:36:54, philipj_UTC7 wrote: > > Should this really be weak? If a script has a reference to an AudioTrack but has > > never looked at the sourceBuffer attribute (so it doesn't have a wrapper or > > another reference), the SourceBuffer should still be kept alive for as long as > > its possible that the script will peek at the sourceBuffer attribute. In other > > words, Member<SourceBuffer>? > > I don't know much about oilpan/GC being used by blink, so I was worried that if the GC is using refcounting then using a strong Member reference here would create a loop (since SourceBuffer already had strong refs to its tracks). Can oilpan handle reference loops? Oilpan handles reference-loops (it is one of the higher-ranking reasons to have it I'd say.)
https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBufferAudioTrack.h (right): https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBufferAudioTrack.h:42: class SourceBufferAudioTrack final : public GarbageCollectedFinalized<SourceBufferAudioTrack>, public HeapSupplement<AudioTrack> { On 2016/02/27 01:03:45, servolk wrote: > On 2016/02/26 13:36:54, philipj_UTC7 wrote: > > On 2016/02/24 02:37:15, servolk wrote: > > > On 2016/02/24 00:11:36, servolk wrote: > > > > On 2016/02/23 04:35:07, philipj_UTC7 wrote: > > > > > Does it need to be finalized? > > > > > > > > I guess not, so I've changed to a regulat GarbageCollected for now. > Although > > > for > > > > some reason the current implementations of regular Audio/VideoTracks are > > also > > > > GarbageCollectedFinalized, even though they have trivial destructors. Is > > that > > > > because we want to ensure that we always check that assert in blink > > ~TrackBase > > > ? > > > > > > Actually, turns out this needs to be finalized, since the PS #7 caused build > > > failures, so I'm making them finalizable again. > > > > The build error was: > > In file included from > > > ../../third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.cpp:5: > > > ../../third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h:16:1: > > error: [blink-gc] Class 'AudioTrackSourceBuffer' requires finalization. > > class AudioTrackSourceBuffer final : public > > GarbageCollected<AudioTrackSourceBuffer>, public HeapSupplement<AudioTrack> { > > ^ > > > ../../third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.cpp:17:1: > > note: [blink-gc] User-declared destructor declared here: > > AudioTrackSourceBuffer::~AudioTrackSourceBuffer() > > ^ > > 1 error generated. > > > > If you remote the empty destructor, do you still need finalized? > > Yes, I tried completely removing the destructor first, that produced a compile > error saying a user-type with non-trivial members must have an explicit > destructor. And after adding an explicit destructor I got this error about > mandatory finalization. I'm sure this is right then, but which are the non-trivial members? If the only member was a Member<T>, I'd expect that no finalization is required, but maybe it's something that comes via inheritance, from HeapSupplement<AudioTrack>?
On 2016/03/02 11:20:44, philipj_UTC7 wrote: > https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/mediasource/SourceBufferAudioTrack.h > (right): > > https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediasource/SourceBufferAudioTrack.h:42: class > SourceBufferAudioTrack final : public > GarbageCollectedFinalized<SourceBufferAudioTrack>, public > HeapSupplement<AudioTrack> { > On 2016/02/27 01:03:45, servolk wrote: > > On 2016/02/26 13:36:54, philipj_UTC7 wrote: > > > On 2016/02/24 02:37:15, servolk wrote: > > > > On 2016/02/24 00:11:36, servolk wrote: > > > > > On 2016/02/23 04:35:07, philipj_UTC7 wrote: > > > > > > Does it need to be finalized? > > > > > > > > > > I guess not, so I've changed to a regulat GarbageCollected for now. > > Although > > > > for > > > > > some reason the current implementations of regular Audio/VideoTracks are > > > also > > > > > GarbageCollectedFinalized, even though they have trivial destructors. Is > > > that > > > > > because we want to ensure that we always check that assert in blink > > > ~TrackBase > > > > ? > > > > > > > > Actually, turns out this needs to be finalized, since the PS #7 caused > build > > > > failures, so I'm making them finalizable again. > > > > > > The build error was: > > > In file included from > > > > > > ../../third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.cpp:5: > > > > > > ../../third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h:16:1: > > > error: [blink-gc] Class 'AudioTrackSourceBuffer' requires finalization. > > > class AudioTrackSourceBuffer final : public > > > GarbageCollected<AudioTrackSourceBuffer>, public HeapSupplement<AudioTrack> > { > > > ^ > > > > > > ../../third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.cpp:17:1: > > > note: [blink-gc] User-declared destructor declared here: > > > AudioTrackSourceBuffer::~AudioTrackSourceBuffer() > > > ^ > > > 1 error generated. > > > > > > If you remote the empty destructor, do you still need finalized? > > > > Yes, I tried completely removing the destructor first, that produced a compile > > error saying a user-type with non-trivial members must have an explicit > > destructor. And after adding an explicit destructor I got this error about > > mandatory finalization. > > I'm sure this is right then, but which are the non-trivial members? If the only > member was a Member<T>, I'd expect that no finalization is required, but maybe > it's something that comes via inheritance, from HeapSupplement<AudioTrack>? Ok, I have made some more progress on this today. I have reworked this to move as much functionality into base classes (by making TrackBase supplementable). It has removed a lot of duplication in the earlier patchsets. Also after reworking this I've tried removing the explicit destructor in the base class, and that was successful, so the supplement classes are no longer finalizable.
https://codereview.chromium.org/1658033002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h (right): https://codereview.chromium.org/1658033002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h:16: class AudioTrackSourceBuffer final : public GarbageCollectedFinalized<AudioTrackSourceBuffer>, public HeapSupplement<AudioTrack> { On 2016/02/27 01:03:45, servolk wrote: > On 2016/02/26 13:36:54, philipj_UTC7 wrote: > > Did you check if it's possible to have only a single > HeapSupplement<TrackBase>? > > Nothing is both an AudioTrack and a VideoTrack, so it ought to work? It could > be > > that you'll need to have almost-empty AudioTrackSourceBuffer and > > VideoTrackSourceBuffer that merely forward to that shared implementation. > > Good idea, I haven't tried that yet, I'll give it a try. Done. https://codereview.chromium.org/1658033002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h:30: WeakMember<SourceBuffer> m_sourceBuffer; On 2016/02/29 11:11:11, fs wrote: > On 2016/02/27 at 01:03:45, servolk wrote: > > On 2016/02/26 13:36:54, philipj_UTC7 wrote: > > > Should this really be weak? If a script has a reference to an AudioTrack but > has > > > never looked at the sourceBuffer attribute (so it doesn't have a wrapper or > > > another reference), the SourceBuffer should still be kept alive for as long > as > > > its possible that the script will peek at the sourceBuffer attribute. In > other > > > words, Member<SourceBuffer>? > > > > I don't know much about oilpan/GC being used by blink, so I was worried that > if the GC is using refcounting then using a strong Member reference here would > create a loop (since SourceBuffer already had strong refs to its tracks). Can > oilpan handle reference loops? > > Oilpan handles reference-loops (it is one of the higher-ranking reasons to have > it I'd say.) Done. https://codereview.chromium.org/1658033002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.idl (right): https://codereview.chromium.org/1658033002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.idl:6: [RuntimeEnabled=AudioVideoTracks] partial interface AudioTrack { On 2016/02/26 13:36:54, philipj_UTC7 wrote: > Please format like this: > [ > RuntimeEnabled=AudioVideoTracks, > ] partial interface ... > > That's the norm, probably because it makes diffs when adding/removing extended > attributes smaller. Also a blank line after the URL, at least I've done that > almost everywhere. Done.
Sorry for being slower than a dead horse. This all looks pretty good, but here I'm also keen to hear from wolenetz@. https://codereview.chromium.org/1658033002/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h (right): https://codereview.chromium.org/1658033002/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h:15: class AudioTrackSourceBuffer final : public SourceBufferTrackBaseSupplement { Can this be simplified even further? It ought to be possible to have class AudioTrackSourceBuffer { static SourceBuffer* sourceBuffer(AudioTrack&); }; where the implementation cast AudioTrack& to TrackBase& and calls into a shared "static SourceBuffer* TrackBase::sourceBuffer(TrackBase&)" that's used for both audio and video tracks. I tried to apply the patch locally to try it out myself first, but the CL doesn't apply any longer. Can you rebase? https://codereview.chromium.org/1658033002/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1658033002/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:530: audioTracks().remove(trackId); This makes it clear that the order in which the track is removed from both track lists will affect the order of the ends. This doesn't seem to be spelled out by https://w3c.github.io/media-source/#audio-track-extensions but I could be looking in the wrong place. This would also need tests. https://codereview.chromium.org/1658033002/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:545: audioTracks().add(audioTrack); Order tests needed for adding tracks too. https://codereview.chromium.org/1658033002/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:569: if (audioTrack) { It can't be null, can it? On OOM we should have crashed already?
https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:233: return *m_audioTracks; On 2016/02/26 13:36:54, philipj_UTC7 wrote: > On 2016/02/24 00:11:36, servolk wrote: > > On 2016/02/23 04:35:07, philipj_UTC7 wrote: > > > Has m_audioTracks actually been created, won't this crash? This looks like > > > HTMLMediaElement, but if it works I don't understand why, since Member<T> > can > > be > > > null. Right? > > > > Yeah, that's right. I need to figure out a good place where to init the > > m_audio/videoTracks. IIUC we can't do this directly in the constructor because > > at that point the media source might be not yet attached to the > HTMLMediaElement > > and so m_source->getElement might be null. We'll probably need to notify the > SB > > when MediaSource gets attached to an HTMLMediaElement and create the track > > collections at that point. > > Until this works and doesn't crash, can you instead ASSERT_NOT_REACHED here? If > it can be reached, I guess remove things until it's unreachable, we ought not > have things that can crash even behind a flag I think. Ping.
https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:233: return *m_audioTracks; On 2016/03/10 13:13:48, philipj_UTC7 wrote: > On 2016/02/26 13:36:54, philipj_UTC7 wrote: > > On 2016/02/24 00:11:36, servolk wrote: > > > On 2016/02/23 04:35:07, philipj_UTC7 wrote: > > > > Has m_audioTracks actually been created, won't this crash? This looks like > > > > HTMLMediaElement, but if it works I don't understand why, since Member<T> > > can > > > be > > > > null. Right? > > > > > > Yeah, that's right. I need to figure out a good place where to init the > > > m_audio/videoTracks. IIUC we can't do this directly in the constructor > because > > > at that point the media source might be not yet attached to the > > HTMLMediaElement > > > and so m_source->getElement might be null. We'll probably need to notify the > > SB > > > when MediaSource gets attached to an HTMLMediaElement and create the track > > > collections at that point. > > > > Until this works and doesn't crash, can you instead ASSERT_NOT_REACHED here? > If > > it can be reached, I guess remove things until it's unreachable, we ought not > > have things that can crash even behind a flag I think. > > Ping. How strongly do you feel about this? One reason I haven't added an ASSERT_NOT_REACHED here yet, is that these collections actually do get created/populated after SourceBuffer::initializationSegmentReceived runs, and being able to actually access these makes it easier to test (e.g. I can access these lists in developer console after loading some video, since the initializationSegmentReceived is guaranteed to run at least once before playback starts). Plus, you are right, these are currently hidden behind the experimental feature flag anyway, so they are not easily accessible and shouldn't cause any accidental crashes. https://codereview.chromium.org/1658033002/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h (right): https://codereview.chromium.org/1658033002/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.h:15: class AudioTrackSourceBuffer final : public SourceBufferTrackBaseSupplement { On 2016/03/10 13:12:17, philipj_UTC7 wrote: > Can this be simplified even further? It ought to be possible to have > > class AudioTrackSourceBuffer { > static SourceBuffer* sourceBuffer(AudioTrack&); > }; > > where the implementation cast AudioTrack& to TrackBase& and calls into a shared > "static SourceBuffer* TrackBase::sourceBuffer(TrackBase&)" that's used for both > audio and video tracks. > > I tried to apply the patch locally to try it out myself first, but the CL > doesn't apply any longer. Can you rebase? Yep. In fact we don't even need the sourceBuffer method here, since it's exactly the same for A and V cases. And we can remove the constructors that do the implicit casting from A/V tracks to TrackBase. Then we are left with just two empty classes that are descendants of SourceBufferTrackBaseSupplement, and we can just get rid of these two classes completely and instead just use ImplementedAs=SourceBufferTrackBaseSupplement in the .idl files. https://codereview.chromium.org/1658033002/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1658033002/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:530: audioTracks().remove(trackId); On 2016/03/10 13:12:17, philipj_UTC7 wrote: > This makes it clear that the order in which the track is removed from both track > lists will affect the order of the ends. This doesn't seem to be spelled out by > https://w3c.github.io/media-source/#audio-track-extensions but I could be > looking in the wrong place. This would also need tests. see below https://codereview.chromium.org/1658033002/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:545: audioTracks().add(audioTrack); On 2016/03/10 13:12:17, philipj_UTC7 wrote: > Order tests needed for adding tracks too. I think we are better off postponing these tests until a separate CL that will implement a fully spec-compliant MSE init segment received algorithm. The current code removes all the old tracks, but the spec compliant algorithm will need to check the new track ids and match them with existing tracks, verify that track properties haven't changed and leave those track in the lists. The MSE spec does specify the order here, the track must be added to the SB track list first and then to the HTMLMediaElement (see for example for audio tracks items 5.2.8.8 and 5.2.8.10 at https://w3c.github.io/media-source/#sourcebuffer-init-segment-received). https://codereview.chromium.org/1658033002/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:569: if (audioTrack) { On 2016/03/10 13:12:17, philipj_UTC7 wrote: > It can't be null, can it? On OOM we should have crashed already? Done.
https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:233: return *m_audioTracks; On 2016/03/11 02:00:57, servolk wrote: > On 2016/03/10 13:13:48, philipj_UTC7 wrote: > > On 2016/02/26 13:36:54, philipj_UTC7 wrote: > > > On 2016/02/24 00:11:36, servolk wrote: > > > > On 2016/02/23 04:35:07, philipj_UTC7 wrote: > > > > > Has m_audioTracks actually been created, won't this crash? This looks > like > > > > > HTMLMediaElement, but if it works I don't understand why, since > Member<T> > > > can > > > > be > > > > > null. Right? > > > > > > > > Yeah, that's right. I need to figure out a good place where to init the > > > > m_audio/videoTracks. IIUC we can't do this directly in the constructor > > because > > > > at that point the media source might be not yet attached to the > > > HTMLMediaElement > > > > and so m_source->getElement might be null. We'll probably need to notify > the > > > SB > > > > when MediaSource gets attached to an HTMLMediaElement and create the track > > > > collections at that point. > > > > > > Until this works and doesn't crash, can you instead ASSERT_NOT_REACHED here? > > If > > > it can be reached, I guess remove things until it's unreachable, we ought > not > > > have things that can crash even behind a flag I think. > > > > Ping. > > How strongly do you feel about this? One reason I haven't added an > ASSERT_NOT_REACHED here yet, is that these collections actually do get > created/populated after SourceBuffer::initializationSegmentReceived runs, and > being able to actually access these makes it easier to test (e.g. I can access > these lists in developer console after loading some video, since the > initializationSegmentReceived is guaranteed to run at least once before playback > starts). Plus, you are right, these are currently hidden behind the experimental > feature flag anyway, so they are not easily accessible and shouldn't cause any > accidental crashes. Oh, yes, and also these new collections are also accessed by the newly added code in the layout test mediasource-avtracks.html. If we add ASSERT_NOT_REACHED here, the test will crash.
MSE part looking pretty good. non-nit comment summary: 1) I'm less clear on memory mgmt of the SupplementTrackBase part: is it always managed by oilpan? If not, will something leak (and if now, I'm unclear how that's taken care of) 2) (see below): move sourcebuffer's tracklist creations into its ctor? 3) what happens when a sourcebuffer is removed from a MediaSource as part of MediaSource detachment from a mediaElement: 3a) should the tracks also be removed from the mediaElement (this might be done already in HTMLMediaElement.cpp, I recommend testing++ around this please) 3b) should the tracks also be removed from the sourcebuffer? and if so, should their 'sourcebuffer' attribute value become null? (and testing++ please around this) details and nits: https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:239: return *m_audioTracks; Prior to attachment of MediaSource to a MediaElement, it hasn't yet reached open state (and sourcebuffers cannot be created for the MediaSource). Once it's attached, (setWebMediaSourceAndOpen()), then AddId() can create SourceBuffers. So, at creation time, SourceBuffers should know they are part of an 'open' MediaSource that is attached to a mediaElement. So why not create these in ctor? (The SourceBuffer ctor is only available via ::create, which is only available via MediaSource.AddId()). Later, if MediaSource is detached from the element, the spec is a quite a bit less clear, unfortunately, around what happens to the SB's tracks (and associated mediaElement tracks). I think the idea would be to do something similar to the "removeSourceBuffer()" steps as part of detaching from the element. WDYT? This also needs some testing. https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.idl (right): https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.idl:73: [RuntimeEnabled=AudioVideoTracks] readonly attribute AudioTrackList audioTracks; nit: please put these new interface members in order that matches the spec's idl order
https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1658033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:233: return *m_audioTracks; On 2016/03/11 02:03:37, servolk wrote: > On 2016/03/11 02:00:57, servolk wrote: > > On 2016/03/10 13:13:48, philipj_UTC7 wrote: > > > On 2016/02/26 13:36:54, philipj_UTC7 wrote: > > > > On 2016/02/24 00:11:36, servolk wrote: > > > > > On 2016/02/23 04:35:07, philipj_UTC7 wrote: > > > > > > Has m_audioTracks actually been created, won't this crash? This looks > > like > > > > > > HTMLMediaElement, but if it works I don't understand why, since > > Member<T> > > > > can > > > > > be > > > > > > null. Right? > > > > > > > > > > Yeah, that's right. I need to figure out a good place where to init the > > > > > m_audio/videoTracks. IIUC we can't do this directly in the constructor > > > because > > > > > at that point the media source might be not yet attached to the > > > > HTMLMediaElement > > > > > and so m_source->getElement might be null. We'll probably need to notify > > the > > > > SB > > > > > when MediaSource gets attached to an HTMLMediaElement and create the > track > > > > > collections at that point. > > > > > > > > Until this works and doesn't crash, can you instead ASSERT_NOT_REACHED > here? > > > If > > > > it can be reached, I guess remove things until it's unreachable, we ought > > not > > > > have things that can crash even behind a flag I think. > > > > > > Ping. > > > > How strongly do you feel about this? One reason I haven't added an > > ASSERT_NOT_REACHED here yet, is that these collections actually do get > > created/populated after SourceBuffer::initializationSegmentReceived runs, and > > being able to actually access these makes it easier to test (e.g. I can access > > these lists in developer console after loading some video, since the > > initializationSegmentReceived is guaranteed to run at least once before > playback > > starts). Plus, you are right, these are currently hidden behind the > experimental > > feature flag anyway, so they are not easily accessible and shouldn't cause any > > accidental crashes. > > Oh, yes, and also these new collections are also accessed by the newly added > code in the layout test mediasource-avtracks.html. If we add ASSERT_NOT_REACHED > here, the test will crash. OK, so if it's used for testing it surely shouldn't ASSERT_NOT_REACHED, but can you ensure that never crashes by just creating the objects in the constructor, as for HTMLMediaElement? That they're only populated later isn't a problem. Creating the lazily on access here is also an option, of course.
I've been reminded of an open HTML spec bug related to this: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26461 I don't know if there's anything to be done there, but if while implementing this there is a single tiny thing that isn't covered by the HTML and MSE spec together, like event ordering, please let me know what it is :)
Description was changed from ========== Add SourceBuffer implementations of Audio/VideoTracks This CL provides SourceBuffer extensions described in MSE spec: http://www.w3.org/TR/media-source/#audio-track-extensions http://www.w3.org/TR/media-source/#video-track-extensions The only difference between those extensions and the existing audio/ video track is that these have the .sourceBuffer property, which is only going to be non-null for a/v tracks created by a SourceBuffer and will return the parent SourceBuffer object. I've looked at how other extension interfaces are implemented in blink, e.g. NavigatorGeolocation extension property, and tried to do the same. BUG=249427 ========== to ========== Add SourceBuffer implementations of Audio/VideoTracks This CL provides SourceBuffer extensions described in MSE spec: http://www.w3.org/TR/media-source/#audio-track-extensions http://www.w3.org/TR/media-source/#video-track-extensions The only difference between those extensions and the existing audio/ video track is that these have the .sourceBuffer property, which is only going to be non-null for a/v tracks created by a SourceBuffer and will return the parent SourceBuffer object. I've looked at how other extension interfaces are implemented in blink, e.g. NavigatorGeolocation extension property, and tried to do the same. BUG=249427,249428 ==========
Description was changed from ========== Add SourceBuffer implementations of Audio/VideoTracks This CL provides SourceBuffer extensions described in MSE spec: http://www.w3.org/TR/media-source/#audio-track-extensions http://www.w3.org/TR/media-source/#video-track-extensions The only difference between those extensions and the existing audio/ video track is that these have the .sourceBuffer property, which is only going to be non-null for a/v tracks created by a SourceBuffer and will return the parent SourceBuffer object. I've looked at how other extension interfaces are implemented in blink, e.g. NavigatorGeolocation extension property, and tried to do the same. BUG=249427 ========== to ========== Add SourceBuffer implementations of Audio/VideoTracks This CL provides SourceBuffer extensions described in MSE spec: http://www.w3.org/TR/media-source/#audio-track-extensions http://www.w3.org/TR/media-source/#video-track-extensions The only difference between those extensions and the existing audio/ video track is that these have the .sourceBuffer property, which is only going to be non-null for a/v tracks created by a SourceBuffer and will return the parent SourceBuffer object. I've looked at how other extension interfaces are implemented in blink, e.g. NavigatorGeolocation extension property, and tried to do the same. BUG=249427,249428 ==========
On 2016/03/23 20:18:53, wolenetz wrote: > MSE part looking pretty good. > > non-nit comment summary: > 1) I'm less clear on memory mgmt of the SupplementTrackBase part: is it always > managed by oilpan? If not, will something leak (and if now, I'm unclear how > that's taken care of) > 2) (see below): move sourcebuffer's tracklist creations into its ctor? > 3) what happens when a sourcebuffer is removed from a MediaSource as part of > MediaSource detachment from a mediaElement: > 3a) should the tracks also be removed from the mediaElement (this might be done > already in HTMLMediaElement.cpp, I recommend testing++ around this please) > 3b) should the tracks also be removed from the sourcebuffer? and if so, should > their 'sourcebuffer' attribute value become null? (and testing++ please around > this) > > details and nits: > > https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): > > https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:239: return > *m_audioTracks; > Prior to attachment of MediaSource to a MediaElement, it hasn't yet reached open > state (and sourcebuffers cannot be created for the MediaSource). Once it's > attached, (setWebMediaSourceAndOpen()), then AddId() can create SourceBuffers. > > So, at creation time, SourceBuffers should know they are part of an 'open' > MediaSource that is attached to a mediaElement. > So why not create these in ctor? (The SourceBuffer ctor is only available via > ::create, which is only available via MediaSource.AddId()). > > Later, if MediaSource is detached from the element, the spec is a quite a bit > less clear, unfortunately, around what happens to the SB's tracks (and > associated mediaElement tracks). I think the idea would be to do something > similar to the "removeSourceBuffer()" steps as part of detaching from the > element. WDYT? This also needs some testing. > > https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/mediasource/SourceBuffer.idl (right): > > https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediasource/SourceBuffer.idl:73: > [RuntimeEnabled=AudioVideoTracks] readonly attribute AudioTrackList audioTracks; > nit: please put these new interface members in order that matches the spec's idl > order Ok, now that the base CL has landed, getting back to this. 1) Yes, IIUC the SupplementTrackBase is always managed by oilpan. As far as I can see all HeapSupplement classes do it this way. In fact the SupplementBase::provideTo method, which we also invoke for our SupplementTrackBase objects requires that its input parameter has an isGarbageCollected trait. So I think this should be fine. 2) Done in a previous CL 3a,b) Yes, you are right, after looking more closely at this I've noticed that MSE spec prescribes removal of SB media tracks from parent HTMLMediaElement (steps 3-8 at https://w3c.github.io/media-source/#widl-MediaSource-removeSourceBuffer-void-...). There is also a related FIXME in MediaSource::removeSourceBuffer for that. I'm planning to fix this and add tests, but probably in a separate CL, to ensure that this CL stays focused on adding supplements and .sourceBuffer property for SB media tracks.
https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:239: return *m_audioTracks; On 2016/03/23 20:18:52, wolenetz wrote: > Prior to attachment of MediaSource to a MediaElement, it hasn't yet reached open > state (and sourcebuffers cannot be created for the MediaSource). Once it's > attached, (setWebMediaSourceAndOpen()), then AddId() can create SourceBuffers. > > So, at creation time, SourceBuffers should know they are part of an 'open' > MediaSource that is attached to a mediaElement. > So why not create these in ctor? (The SourceBuffer ctor is only available via > ::create, which is only available via MediaSource.AddId()). > > Later, if MediaSource is detached from the element, the spec is a quite a bit > less clear, unfortunately, around what happens to the SB's tracks (and > associated mediaElement tracks). I think the idea would be to do something > similar to the "removeSourceBuffer()" steps as part of detaching from the > element. WDYT? This also needs some testing. Yes, I've changed this already in the previous CL. The track lists are now going to be created in SB constructor. As for MediaSource being detached from the element, I think it's covered in MSE spec at https://w3c.github.io/media-source/#mediasource-detach. Steps 3 and 5 say that all sourcebuffers should be removed, so yes we'll need to run removeSourceBuffer and that will take care of media tracks. But you are right in that there seems to be no test coverage for this atm. I'll try to fix it in a separate CL. https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.idl (right): https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.idl:73: [RuntimeEnabled=AudioVideoTracks] readonly attribute AudioTrackList audioTracks; On 2016/03/23 20:18:52, wolenetz wrote: > nit: please put these new interface members in order that matches the spec's idl > order Done.
On 2016/03/31 00:03:40, servolk wrote: > https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): > > https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:239: return > *m_audioTracks; > On 2016/03/23 20:18:52, wolenetz wrote: > > Prior to attachment of MediaSource to a MediaElement, it hasn't yet reached > open > > state (and sourcebuffers cannot be created for the MediaSource). Once it's > > attached, (setWebMediaSourceAndOpen()), then AddId() can create SourceBuffers. > > > > So, at creation time, SourceBuffers should know they are part of an 'open' > > MediaSource that is attached to a mediaElement. > > So why not create these in ctor? (The SourceBuffer ctor is only available via > > ::create, which is only available via MediaSource.AddId()). > > > > Later, if MediaSource is detached from the element, the spec is a quite a bit > > less clear, unfortunately, around what happens to the SB's tracks (and > > associated mediaElement tracks). I think the idea would be to do something > > similar to the "removeSourceBuffer()" steps as part of detaching from the > > element. WDYT? This also needs some testing. > > Yes, I've changed this already in the previous CL. The track lists are now going > to be created in SB constructor. As for MediaSource being detached from the > element, I think it's covered in MSE spec at > https://w3c.github.io/media-source/#mediasource-detach. Steps 3 and 5 say that > all sourcebuffers should be removed, so yes we'll need to run removeSourceBuffer > and that will take care of media tracks. But you are right in that there seems > to be no test coverage for this atm. I'll try to fix it in a separate CL. > > https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/mediasource/SourceBuffer.idl (right): > > https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediasource/SourceBuffer.idl:73: > [RuntimeEnabled=AudioVideoTracks] readonly attribute AudioTrackList audioTracks; > On 2016/03/23 20:18:52, wolenetz wrote: > > nit: please put these new interface members in order that matches the spec's > idl > > order > > Done. This CL should take care of removing SB media tracks when detaching from media element: https://codereview.chromium.org/1846863002/
On 2016/03/31 00:01:43, servolk wrote: > On 2016/03/23 20:18:53, wolenetz wrote: > > MSE part looking pretty good. > > > > non-nit comment summary: > > 1) I'm less clear on memory mgmt of the SupplementTrackBase part: is it always > > managed by oilpan? If not, will something leak (and if now, I'm unclear how > > that's taken care of) > > 2) (see below): move sourcebuffer's tracklist creations into its ctor? > > 3) what happens when a sourcebuffer is removed from a MediaSource as part of > > MediaSource detachment from a mediaElement: > > 3a) should the tracks also be removed from the mediaElement (this might be > done > > already in HTMLMediaElement.cpp, I recommend testing++ around this please) > > 3b) should the tracks also be removed from the sourcebuffer? and if so, should > > their 'sourcebuffer' attribute value become null? (and testing++ please around > > this) > > > > details and nits: > > > > > https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): > > > > > https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:239: return > > *m_audioTracks; > > Prior to attachment of MediaSource to a MediaElement, it hasn't yet reached > open > > state (and sourcebuffers cannot be created for the MediaSource). Once it's > > attached, (setWebMediaSourceAndOpen()), then AddId() can create SourceBuffers. > > > > So, at creation time, SourceBuffers should know they are part of an 'open' > > MediaSource that is attached to a mediaElement. > > So why not create these in ctor? (The SourceBuffer ctor is only available via > > ::create, which is only available via MediaSource.AddId()). > > > > Later, if MediaSource is detached from the element, the spec is a quite a bit > > less clear, unfortunately, around what happens to the SB's tracks (and > > associated mediaElement tracks). I think the idea would be to do something > > similar to the "removeSourceBuffer()" steps as part of detaching from the > > element. WDYT? This also needs some testing. > > > > > https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/modules/mediasource/SourceBuffer.idl (right): > > > > > https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/mediasource/SourceBuffer.idl:73: > > [RuntimeEnabled=AudioVideoTracks] readonly attribute AudioTrackList > audioTracks; > > nit: please put these new interface members in order that matches the spec's > idl > > order > > Ok, now that the base CL has landed, getting back to this. > 1) Yes, IIUC the SupplementTrackBase is always managed by oilpan. As far as I > can see all HeapSupplement classes do it this way. In fact the > SupplementBase::provideTo method, which we also invoke for our > SupplementTrackBase objects requires that its input parameter has an > isGarbageCollected trait. So I think this should be fine. > 2) Done in a previous CL > 3a,b) Yes, you are right, after looking more closely at this I've noticed that > MSE spec prescribes removal of SB media tracks from parent HTMLMediaElement > (steps 3-8 at > https://w3c.github.io/media-source/#widl-MediaSource-removeSourceBuffer-void-...). > There is also a related FIXME in MediaSource::removeSourceBuffer for that. I'm > planning to fix this and add tests, but probably in a separate CL, to ensure > that this CL stays focused on adding supplements and .sourceBuffer property for > SB media tracks. Acknowledged.
lgtm % adding 'addtrack' event expectations' testing to the layout test. Sorry I missed that before :) Also, please await philipj@'s final review before landing. https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1658033002/diff/390001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:239: return *m_audioTracks; On 2016/03/31 00:03:40, servolk wrote: > On 2016/03/23 20:18:52, wolenetz wrote: > > Prior to attachment of MediaSource to a MediaElement, it hasn't yet reached > open > > state (and sourcebuffers cannot be created for the MediaSource). Once it's > > attached, (setWebMediaSourceAndOpen()), then AddId() can create SourceBuffers. > > > > So, at creation time, SourceBuffers should know they are part of an 'open' > > MediaSource that is attached to a mediaElement. > > So why not create these in ctor? (The SourceBuffer ctor is only available via > > ::create, which is only available via MediaSource.AddId()). > > > > Later, if MediaSource is detached from the element, the spec is a quite a bit > > less clear, unfortunately, around what happens to the SB's tracks (and > > associated mediaElement tracks). I think the idea would be to do something > > similar to the "removeSourceBuffer()" steps as part of detaching from the > > element. WDYT? This also needs some testing. > > Yes, I've changed this already in the previous CL. The track lists are now going > to be created in SB constructor. As for MediaSource being detached from the > element, I think it's covered in MSE spec at > https://w3c.github.io/media-source/#mediasource-detach. Steps 3 and 5 say that > all sourcebuffers should be removed, so yes we'll need to run removeSourceBuffer > and that will take care of media tracks. But you are right in that there seems > to be no test coverage for this atm. I'll try to fix it in a separate CL. Acknowledged. https://codereview.chromium.org/1658033002/diff/550001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html (right): https://codereview.chromium.org/1658033002/diff/550001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:16: test.expectEvent(sourceBuffer, 'updateend', 'initSegment append ended.'); Please also add the SourceBuffer.{audio,video}Tracks 'addtrack' event expectations. Example from spec (video is similar): "Queue a task to fire a trusted event named addtrack, that does not bubble and is not cancelable, and that uses the TrackEvent interface, at the AudioTrackList object referenced by the audioTracks attribute on this SourceBuffer object. Add new audio track to the audioTracks attribute on the HTMLMediaElement. Queue a task to fire a trusted event named addtrack, that does not bubble and is not cancelable, and that uses the TrackEvent interface, at the AudioTrackList object referenced by the audioTracks attribute on the HTMLMediaElement."
https://codereview.chromium.org/1658033002/diff/550001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html (right): https://codereview.chromium.org/1658033002/diff/550001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:16: test.expectEvent(sourceBuffer, 'updateend', 'initSegment append ended.'); On 2016/03/31 18:57:59, wolenetz wrote: > Please also add the SourceBuffer.{audio,video}Tracks 'addtrack' event > expectations. Example from spec (video is similar): > "Queue a task to fire a trusted event named addtrack, that does not bubble and > is not cancelable, and that uses the TrackEvent interface, at the AudioTrackList > object referenced by the audioTracks attribute on this SourceBuffer object. > Add new audio track to the audioTracks attribute on the HTMLMediaElement. > Queue a task to fire a trusted event named addtrack, that does not bubble and is > not cancelable, and that uses the TrackEvent interface, at the AudioTrackList > object referenced by the audioTracks attribute on the HTMLMediaElement." Done.
(PS30 lgtm)
On 2016/03/31 20:43:30, wolenetz_Sick_Apr1 wrote: > (PS30 lgtm) Ok, thanks Matt. Philip any further comments on this? (keeping in mind that proper track removal is going to be done as a separate CL https://codereview.chromium.org/1846863002/).
https://codereview.chromium.org/1658033002/diff/590001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/SourceBufferTrackBaseSupplement.h (right): https://codereview.chromium.org/1658033002/diff/590001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBufferTrackBaseSupplement.h:19: explicit SourceBufferTrackBaseSupplement(TrackBase&, SourceBuffer*); Can you make this constructor private and have a static from(TrackBase&) that's idempotent? Then instead of a bare new SourceBufferTrackBaseSupplement you'd have a from(trackList).setSourceBuffer(...) or similar. The current setup looks a bit fragile in that the constructor can be called twice.
https://codereview.chromium.org/1658033002/diff/590001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.idl (right): https://codereview.chromium.org/1658033002/diff/590001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.idl:8: ImplementedAs=SourceBufferTrackBaseSupplement, If you make the other suggested change, I think you could also consider dropping this, and instead just having AudioTrackSourceBuffer.h with a single static method that does return from(someContext).sourceBuffer() instead.
On 2016/04/05 14:56:11, philipj wrote: > https://codereview.chromium.org/1658033002/diff/590001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.idl > (right): > > https://codereview.chromium.org/1658033002/diff/590001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.idl:8: > ImplementedAs=SourceBufferTrackBaseSupplement, > If you make the other suggested change, I think you could also consider dropping > this, and instead just having AudioTrackSourceBuffer.h with a single static > method that does return from(someContext).sourceBuffer() instead. Ok, yeah, I guess we can do this (see the latest patchset). Although I've kept the .sourceProperty getter unchanged for now. The newly added from method always creates a supplement for trackBase if one doesn't currently exist. This means that if we used from(someContext).sourceBuffer() in the property getter, then accessing .sourceBuffer property on audio/video tracks created by HTMLMediaElement (and not SourceBuffer) would create a supplement with m_sourceBuffer=nullptr. I think it's a little more efficient to avoid creating the supplement if we can help it.
On 2016/04/06 22:24:06, servolk wrote: > On 2016/04/05 14:56:11, philipj wrote: > > > https://codereview.chromium.org/1658033002/diff/590001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.idl > > (right): > > > > > https://codereview.chromium.org/1658033002/diff/590001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/mediasource/AudioTrackSourceBuffer.idl:8: > > ImplementedAs=SourceBufferTrackBaseSupplement, > > If you make the other suggested change, I think you could also consider > dropping > > this, and instead just having AudioTrackSourceBuffer.h with a single static > > method that does return from(someContext).sourceBuffer() instead. > > Ok, yeah, I guess we can do this (see the latest patchset). Although I've kept > the .sourceProperty getter unchanged for now. The newly added from method always > creates a supplement for trackBase if one doesn't currently exist. This means > that if we used from(someContext).sourceBuffer() in the property getter, then > accessing .sourceBuffer property on audio/video tracks created by > HTMLMediaElement (and not SourceBuffer) would create a supplement with > m_sourceBuffer=nullptr. I think it's a little more efficient to avoid creating > the supplement if we can help it. I've also added a comment explaining that in SourceBufferTrackBaseSupplement::sourceBuffer
LGTM % nits https://codereview.chromium.org/1658033002/diff/670001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/SourceBufferTrackBaseSupplement.cpp (right): https://codereview.chromium.org/1658033002/diff/670001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBufferTrackBaseSupplement.cpp:35: SourceBufferTrackBaseSupplement* supplement = static_cast<SourceBufferTrackBaseSupplement*>(Supplement<TrackBase>::from(track, SupplementName)); You're only doing this in one place, but CSSSelectorWatch and Fullscreen have fromIfExists, which can then also be used in the implementation of from. Makes it obvious what's going on even without a comment. https://codereview.chromium.org/1658033002/diff/670001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/SourceBufferTrackBaseSupplement.h (right): https://codereview.chromium.org/1658033002/diff/670001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBufferTrackBaseSupplement.h:22: void setSourceBuffer(SourceBuffer*); Hmm, it doesn't matter much, but I think it'd look a bit tidier if this were also static, then clients of the class will only interact with this getter/setter pair, while from/fromIfExists is internal.
https://codereview.chromium.org/1658033002/diff/670001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/SourceBufferTrackBaseSupplement.cpp (right): https://codereview.chromium.org/1658033002/diff/670001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBufferTrackBaseSupplement.cpp:35: SourceBufferTrackBaseSupplement* supplement = static_cast<SourceBufferTrackBaseSupplement*>(Supplement<TrackBase>::from(track, SupplementName)); On 2016/04/07 08:59:35, philipj wrote: > You're only doing this in one place, but CSSSelectorWatch and Fullscreen have > fromIfExists, which can then also be used in the implementation of from. Makes > it obvious what's going on even without a comment. Ah, ok, thanks for pointing that out. If just wasn't aware about this. But if we already have those other fromIfExists precedents, we can use the same pattern here for consistency. https://codereview.chromium.org/1658033002/diff/670001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/SourceBufferTrackBaseSupplement.h (right): https://codereview.chromium.org/1658033002/diff/670001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBufferTrackBaseSupplement.h:22: void setSourceBuffer(SourceBuffer*); On 2016/04/07 08:59:35, philipj wrote: > Hmm, it doesn't matter much, but I think it'd look a bit tidier if this were > also static, then clients of the class will only interact with this > getter/setter pair, while from/fromIfExists is internal. 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, philipj@opera.com Link to the patchset: https://codereview.chromium.org/1658033002/#ps690001 (title: "Added SourceBufferTrackBaseSupplement::fromIfExists")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1658033002/690001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658033002/690001
Message was sent while issue was closed.
Description was changed from ========== Add SourceBuffer implementations of Audio/VideoTracks This CL provides SourceBuffer extensions described in MSE spec: http://www.w3.org/TR/media-source/#audio-track-extensions http://www.w3.org/TR/media-source/#video-track-extensions The only difference between those extensions and the existing audio/ video track is that these have the .sourceBuffer property, which is only going to be non-null for a/v tracks created by a SourceBuffer and will return the parent SourceBuffer object. I've looked at how other extension interfaces are implemented in blink, e.g. NavigatorGeolocation extension property, and tried to do the same. BUG=249427,249428 ========== to ========== Add SourceBuffer implementations of Audio/VideoTracks This CL provides SourceBuffer extensions described in MSE spec: http://www.w3.org/TR/media-source/#audio-track-extensions http://www.w3.org/TR/media-source/#video-track-extensions The only difference between those extensions and the existing audio/ video track is that these have the .sourceBuffer property, which is only going to be non-null for a/v tracks created by a SourceBuffer and will return the parent SourceBuffer object. I've looked at how other extension interfaces are implemented in blink, e.g. NavigatorGeolocation extension property, and tried to do the same. BUG=249427,249428 ==========
Message was sent while issue was closed.
Committed patchset #36 (id:690001)
Message was sent while issue was closed.
Description was changed from ========== Add SourceBuffer implementations of Audio/VideoTracks This CL provides SourceBuffer extensions described in MSE spec: http://www.w3.org/TR/media-source/#audio-track-extensions http://www.w3.org/TR/media-source/#video-track-extensions The only difference between those extensions and the existing audio/ video track is that these have the .sourceBuffer property, which is only going to be non-null for a/v tracks created by a SourceBuffer and will return the parent SourceBuffer object. I've looked at how other extension interfaces are implemented in blink, e.g. NavigatorGeolocation extension property, and tried to do the same. BUG=249427,249428 ========== to ========== Add SourceBuffer implementations of Audio/VideoTracks This CL provides SourceBuffer extensions described in MSE spec: http://www.w3.org/TR/media-source/#audio-track-extensions http://www.w3.org/TR/media-source/#video-track-extensions The only difference between those extensions and the existing audio/ video track is that these have the .sourceBuffer property, which is only going to be non-null for a/v tracks created by a SourceBuffer and will return the parent SourceBuffer object. I've looked at how other extension interfaces are implemented in blink, e.g. NavigatorGeolocation extension property, and tried to do the same. BUG=249427,249428 Committed: https://crrev.com/1b3dc5a8202a332891b6aed5d4d4521db0b26261 Cr-Commit-Position: refs/heads/master@{#385816} ==========
Message was sent while issue was closed.
Patchset 36 (id:??) landed as https://crrev.com/1b3dc5a8202a332891b6aed5d4d4521db0b26261 Cr-Commit-Position: refs/heads/master@{#385816} |
