Summary: | Add interfaces and stubs for audio and video tracks | ||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brendan Long <b.long> | ||||||||||||||||||||||||||||||||||||||||||
Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, b.long, buildbot, commit-queue, david.corvoysier, eric.carlson, esprehn+autocc, glenn, gtk-ews, gyuyoung.kim, jer.noble, ojan.autocc, philn, pnormand, rakuco, rego+ews, rniwa, stephanepublic, webkit.review.bot, xan.lopez, zan | ||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | 79822, 113522, 114330 | ||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 113514 | ||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Brendan Long
2013-04-04 14:57:05 PDT
(In reply to comment #0) > Add interfaces and stubs for audio and video tracks What does that mean? Sorry, I meant to upload a patch immediately after the bug was created, but I noticed something when I was looking through the diff and now I'm waiting for the build to finish again. I meant for this bug to be an AudioTrack/VideoTrack version of bug #60379. I'm also working on a GStreamer implementation, but it seemed like it would be easier to give you the generic (non-port specific) part first, then the GStreamer implementation. I can do it as one giant patch if you'd prefer. Basically, this patch will: * Add AudioTrack, AudioTrackPrivate, AudioTrackList, VideoTrack, VideoTrackPrivate, and VideoTrackList classes (with various clients). * Add IDL for the tracks and tracklists. * Add an AudioTrackList and VideoTrackList to HTMLMediaElment. The classes are based on TextTrack, InbandTextTrack, TextTrackPrivate and TextTrackList. All audio and video tracks are in-band, so there's no need for an AudioTrack + InbandAudioTrack -- it's just called AudioTrack. I hid it behind ENABLE_VIDEO_TRACK, like TextTracks are. I could create another feature define if that seems useful. (In reply to comment #2) > Sorry, I meant to upload a patch immediately after the bug was created, but I noticed something when I was looking through the diff and now I'm waiting for the build to finish again. > > I meant for this bug to be an AudioTrack/VideoTrack version of bug #60379. Do you have a class diagram and design doc similar to what was referenced at [1]? [1] https://bugs.webkit.org/show_bug.cgi?id=60379#c3 > > I'm also working on a GStreamer implementation, but it seemed like it would be easier to give you the generic (non-port specific) part first, then the GStreamer implementation. I can do it as one giant patch if you'd prefer. The larger the patch the less likely it will get reviewed or reviewed in a timely manner. I would suggest you use a meta bug and then a number of blocking bugs that introduce small well defined patches. (In reply to comment #3) > Basically, this patch will: > > * Add AudioTrack, AudioTrackPrivate, AudioTrackList, VideoTrack, VideoTrackPrivate, and VideoTrackList classes (with various clients). > * Add IDL for the tracks and tracklists. > * Add an AudioTrackList and VideoTrackList to HTMLMediaElment. > > The classes are based on TextTrack, InbandTextTrack, TextTrackPrivate and TextTrackList. All audio and video tracks are in-band, so there's no need for an AudioTrack + InbandAudioTrack -- it's just called AudioTrack. Are you sure that MSE won't introduce/entail a need for out-of-band audio tracks in order to do late binding? > > I hid it behind ENABLE_VIDEO_TRACK, like TextTracks are. I could create another feature define if that seems useful. (In reply to comment #5) > Are you sure that MSE won't introduce/entail a need for out-of-band audio tracks in order to do late binding? What is MSE? We will need to be able to dynamically add in-band audio and video tracks, but they will all come from the MediaPlayer. There's no concept of out-of-band audio and video tracks in the spec. (In reply to comment #4) > Do you have a class diagram and design doc similar to what was referenced at [1]? > > [1] https://bugs.webkit.org/show_bug.cgi?id=60379#c3 I'll look into making those. The changes I'm making are basically identical to the ones for text tracks (but they're simpler since I don't need to deal with cues or parsing WebVTT). (In reply to comment #6) > (In reply to comment #5) > > Are you sure that MSE won't introduce/entail a need for out-of-band audio tracks in order to do late binding? > > What is MSE? > > We will need to be able to dynamically add in-band audio and video tracks, but they will all come from the MediaPlayer. There's no concept of out-of-band audio and video tracks in the spec. Please review MSE [1] to make certain. [1] https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html (In reply to comment #5) > > Are you sure that MSE won't introduce/entail a need for out-of-band audio tracks in order to do late binding? > No, this is to implement an existing part of the spec, media.audioTracks and media.videoTracks [1]. Anything required for MSE will have to be added to that spec, and will be part of that feature. [1] http://dev.w3.org/html5/spec-author-view/video.html#media-resources-with-multiple-media-tracks (In reply to comment #9) > (In reply to comment #5) > > > > Are you sure that MSE won't introduce/entail a need for out-of-band audio tracks in order to do late binding? > > > No, this is to implement an existing part of the spec, media.audioTracks and media.videoTracks [1]. Anything required for MSE will have to be added to that spec, and will be part of that feature. > > [1] http://dev.w3.org/html5/spec-author-view/video.html#media-resources-with-multiple-media-tracks OK. I just wanted to make sure we were not making an assumption (about always in-band) that was going to be reverted in short order. Created attachment 196560 [details]
Patch
This patch won't build in QtWebKit without the changes in bug #113522. Should I just include them in this patch for review purposes? It's just basic things like adding TextTrack.cpp to Target.pri and making the runtimeEnabledFeature able to be enabled in Qt. Comment on attachment 196560 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196560&action=review > Source/WebCore/ChangeLog:9 How about test(s) for presence of the JS signatures introduced here? > Source/WebCore/ChangeLog:12 > + * Target.pri: Where is build support for other platforms? Comment on attachment 196560 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196560&action=review > Source/WebCore/ChangeLog:9 How about test(s) for presence of the JS signatures introduced here? > Source/WebCore/ChangeLog:12 > + * Target.pri: Where is build support for other platforms? (In reply to comment #14) > (From update of attachment 196560 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=196560&action=review > > > Source/WebCore/ChangeLog:9 > > > How about test(s) for presence of the JS signatures introduced here? > > > Source/WebCore/ChangeLog:12 > > + * Target.pri: > > Where is build support for other platforms? Ignore this duplicate set of comments. (In reply to comment #12) > This patch won't build in QtWebKit without the changes in bug #113522. Should I just include them in this patch for review purposes? It's just basic things like adding TextTrack.cpp to Target.pri and making the runtimeEnabledFeature able to be enabled in Qt. No, you should just reattach the patch once bug #113522 has landed so the bots can use it. (In reply to comment #12) > This patch won't build in QtWebKit without the changes in bug #113522. Should I just include them in this patch for review purposes? It's just basic things like adding TextTrack.cpp to Target.pri and making the runtimeEnabledFeature able to be enabled in Qt. For record keeping, also add 113522 to the "Depend on" field. Also, when you upload a patch, you might want to use: webkit-patch upload --no-review And then add the r? flag only after the patch is building/testing properly on EWS. I won't have a chance to look at this in detail before next week, but a high level comment is that it makes me sad to see so much boilerplate code duplicated in all three *Track class. Would it be possible to have all three Track types share a base class? If so, you may also be able to have a single mediaPlayerDidAdd*Track, mediaPlayerDidRemove*Track, etc. It may also be possible to have the *TrackPrivate classes share a base class. (In reply to comment #19) > Would it be possible to have all three Track types share a base class? The track classes already share a TrackBase class. I think it has most of their shared code. Looking into this more closely made me realize that TrackBase extends EventTarget, but AudioTrack and VideoTrack aren't EventTargets, so fixing that should simplify things a little bit. I'm also looking at making a TrackListBase since their interfaces are extremely similar. I might be able to make a TrackList<T> class that has almost all of the code in it. > If so, you may also be able to have a single mediaPlayerDidAdd*Track, mediaPlayerDidRemove*Track, etc. Do you mean just having a mediaPlayerDidAddTrack(TrackBase)? I could look into that. > It may also be possible to have the *TrackPrivate classes share a base class. I'll look into that too. When making the template for TrackListBase<T>, should I: * Include templated member functions in TrackListBase.h Or * Define TrackListBase<AudioTrack>, TrackListBase<TextTrack> and TrackListBase<VideoTrack> at the end of TrackListBase.cpp There's not a lot of templated classes in WebKit so I'm not sure what the preferred version is. (In reply to comment #21) > When making the template for TrackListBase<T>, should I: > > * Include templated member functions in TrackListBase.h > > Or > > * Define TrackListBase<AudioTrack>, TrackListBase<TextTrack> and TrackListBase<VideoTrack> at the end of TrackListBase.cpp > > There's not a lot of templated classes in WebKit so I'm not sure what the preferred version is. Please don't use a templated base class. (In reply to comment #21) > When making the template for TrackListBase<T>, should I: > > * Include templated member functions in TrackListBase.h > > Or > > * Define TrackListBase<AudioTrack>, TrackListBase<TextTrack> and TrackListBase<VideoTrack> at the end of TrackListBase.cpp > > There's not a lot of templated classes in WebKit so I'm not sure what the preferred version is. While I haven't seen your proposed templated base class yet, it seems like it will be much more complicated than it has to be. Why not just make a (non templated) TrackListBase, where all the methods take TrackBase* arguments, add specialized {Video,Audio,Text}TrackList classes with non-virtual index accessors, and add "ASSERT(type() == VideoTrack)" checks where necessary? (In reply to comment #22) > Please don't use a templated base class. Let me quickly expand on this: templated container classes make sense when the template arguments are unrelated, e.g. Vector<int> and Vector<String>. But when the set of possible arguments are all similar types (e.g., VideoTrack, AudioTrack, and TextTrack all derive from TrackBase), using a templated container is usually the wrong solution. (In reply to comment #22) > While I haven't seen your proposed templated base class yet, it seems like it will be much more complicated than it has to be. Why not just make a (non templated) TrackListBase, where all the methods take TrackBase* arguments, add specialized {Video,Audio,Text}TrackList classes with non-virtual index accessors, and add "ASSERT(type() == VideoTrack)" checks where necessary? Ok, I'll do it that way. Created attachment 196979 [details]
Patch
This patch: * Moved as much code as I could into TrackListBase (not templated). This makes TextTrackList slightly more complicated, but audio and video track lists are much simpler. * Made TextTrack extend EventTarget instead of TrackBase. AudioTrack and VideoTrack are not EventTargets. I looked at the *TrackPrivate classes, but they don't really share any code. Would it be worth it to make them inherit from something just so we could have fewer callbacks? In the callback we'd need to immediately cast them back to their real type. One thing worth looking at is the JS*Custom files, since I don't really understand how these work. I based these on JSTextTrackCustom and JSTextTrackListCustom, but I'm not sure if they're necessary. I'll at at adding this to other ports now. Comment on attachment 196979 [details] Patch Attachment 196979 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17690003 Comment on attachment 196979 [details] Patch Attachment 196979 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17616007 Comment on attachment 196979 [details] Patch Attachment 196979 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17686006 Created attachment 196990 [details]
Patch
This should theoretically make EFL and GTK builds work, but I can't get EFL to build at all on Fedora, and I got started on GTK too late. I'll try to figure out the Mac version tomorrow. I don't have time to do a proper review tonight, but this looks like it is definitely headed in the right direction! I think it would be useful to split the refactoring out into a separate patch from the new functionality. This will make it much easier to review, but I suspect it will make it easier to figure out build issues on the various bots as well. Comment on attachment 196990 [details] Patch Attachment 196990 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17603014 Comment on attachment 196990 [details] Patch Attachment 196990 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17643015 Comment on attachment 196990 [details] Patch Attachment 196990 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17700016 Comment on attachment 196979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196979&action=review I found some time to do a review. Overall it looks really good, but I'd like to see more functionality moved into TrackBase, leaving only those things which are truly different across Track types in each subclass. > Source/WebCore/html/HTMLMediaElement.cpp:3022 > + m_audioTracks->remove(track); What happens when someone calls removeAudioTrack() before calling addAudioTrack()? Should that be an ASSERT(), or should it be an early return? > Source/WebCore/html/HTMLMediaElement.cpp:3038 > + m_videoTracks->remove(track); Ditto. > Source/WebCore/html/HTMLMediaElement.cpp:3046 > + for (int i = m_audioTracks->length() - 1; i >= 0; --i) { I see why you're doing it this way, but it still seems awkward. How about: while (m_audioTracks->length()) removeAudioTrack(m_audioTracks->itemAt(0)); It would be even easier if there were a convenience "lastItem()" or "firstItem()" method on TrackListBase. > Source/WebCore/html/HTMLMediaElement.cpp:3073 > + for (int i = m_videoTracks->length() - 1; i >= 0; --i) { > + VideoTrack* track = m_videoTracks->item(i); > + removeVideoTrack(track); Ditto. > Source/WebCore/html/HTMLMediaElement.h:56 > +class AudioTrackList; Shouldn't this be in the #if ENABLE(VIDEO_TRACK)? > Source/WebCore/html/HTMLMediaElement.h:65 > +class VideoTrackList; Ditto. > Source/WebCore/html/track/AudioTrack.cpp:152 > +void AudioTrack::setKind(const AtomicString& kind) > +{ > + String oldKind = m_kind; > + > + if (isValidKindKeyword(kind)) > + m_kind = kind; > + else > + m_kind = ""; > +} It seems like this can be moved into TrackBase, since it's identical to the code in VideoTrack.cpp. > Source/WebCore/html/track/AudioTrack.cpp:169 > +int AudioTrack::inbandTrackIndex() > +{ > + ASSERT(m_private); > + return m_private->audioTrackIndex(); > +} This too can get moved into TrackBase. Just move the m_trackIndex member variable from TextTrack to TrackBase and set the value in the AudioTrack's constructor. > Source/WebCore/html/track/AudioTrack.h:58 > + AtomicString id() const { return m_id; } > + void setId(const AtomicString& id) { m_id = id; } This is common across Audio and VideoTracks, but not TextTracks. I'd move it up into TrackBase anyway. > Source/WebCore/html/track/AudioTrack.h:61 > + AtomicString kind() const { return m_kind; } > + void setKind(const AtomicString&); Move up into TrackBase(). > Source/WebCore/html/track/AudioTrack.h:69 > + static bool isValidKindKeyword(const AtomicString&); Turn this into a virtual method, which overrides a pure virtual in TrackBase. > Source/WebCore/html/track/AudioTrack.h:75 > + AtomicString label() const { return m_label; } > + void setLabel(const AtomicString& label) { m_label = label; } > + > + AtomicString language() const { return m_language; } > + void setLanguage(const AtomicString& language) { m_language = language; } These are all common across track types, so move this up. > Source/WebCore/html/track/AudioTrack.h:83 > + int inbandTrackIndex(); Ditto. > Source/WebCore/html/track/AudioTrack.h:92 > + AtomicString m_id; > + AtomicString m_kind; > + AtomicString m_label; > + AtomicString m_language; Ditto. > Source/WebCore/html/track/VideoTrack.cpp:152 > +void VideoTrack::setKind(const AtomicString& kind) > +{ > + String oldKind = m_kind; > + > + if (isValidKindKeyword(kind)) > + m_kind = kind; > + else > + m_kind = ""; > +} Move into TrackBase. > Source/WebCore/html/track/VideoTrack.cpp:169 > +int VideoTrack::inbandTrackIndex() > +{ > + ASSERT(m_private); > + return m_private->videoTrackIndex(); > +} Ditto. > Source/WebCore/html/track/VideoTrack.h:61 > + AtomicString id() const { return m_id; } > + void setId(const AtomicString& id) { m_id = id; } > + > + AtomicString kind() const { return m_kind; } > + void setKind(const AtomicString&); Ditto. > Source/WebCore/html/track/VideoTrack.h:75 > + AtomicString label() const { return m_label; } > + void setLabel(const AtomicString& label) { m_label = label; } > + > + AtomicString language() const { return m_language; } > + void setLanguage(const AtomicString& language) { m_language = language; } Ditto. > Source/WebCore/html/track/VideoTrack.h:83 > + int inbandTrackIndex(); Ditto. > Source/WebCore/html/track/VideoTrack.h:92 > + AtomicString m_id; > + AtomicString m_kind; > + AtomicString m_label; > + AtomicString m_language; Ditto. Created attachment 197143 [details]
Patch
If I didn't comment on part of this, it means I did it and have nothing to say besides "done". (In reply to comment #36) > I found some time to do a review. Overall it looks really good, but I'd like to see more functionality moved into TrackBase, leaving only those things which are truly different across Track types in each subclass. > > > Source/WebCore/html/HTMLMediaElement.cpp:3022 > > + m_audioTracks->remove(track); > > What happens when someone calls removeAudioTrack() before calling addAudioTrack()? Should that be an ASSERT(), or should it be an early return? I'm not really sure. I'm not familiar with WebKit's style when handling this yet. Currently, there's no way for JavaScript to remove any kind of track (per the spec, no remove method, even for added tracks), so right now an ASSERT would probably be fine. I assume someone will eventually realize that it should be possible to remove tracks though, but there probably won't be a way to remove audio and video tracks. > > Source/WebCore/html/HTMLMediaElement.cpp:3046 > > + for (int i = m_audioTracks->length() - 1; i >= 0; --i) { > > I see why you're doing it this way, but it still seems awkward. How about: > > while (m_audioTracks->length()) > removeAudioTrack(m_audioTracks->itemAt(0)); I changed it to: while (unsigned length = m_audioTracks->length()) removeAudioTrack(m_audioTracks->item(length - 1)); Does that work? Since the removeAll*Tracks() methods are all called at the same time (in clearMediaPlayer(), I'm considering if it would be better to just make it removeAllInbandTracks() again and have it remove all three types of tracks. > > Source/WebCore/html/track/AudioTrack.cpp:169 > > +int AudioTrack::inbandTrackIndex() > > +{ > > + ASSERT(m_private); > > + return m_private->audioTrackIndex(); > > +} > > This too can get moved into TrackBase. Just move the m_trackIndex member variable from TextTrack to TrackBase and set the value in the AudioTrack's constructor. This is actually different in TextTracks vs Audio/VideoTrack, since TextTracks aren't necessarily in-band. I could create an InbandTrackBase for InbandTextTrack, AudioTrack and VideoTrack to extend, but I'm not sure if you want me to go that far. TextTrack's trackIndex() does something different (to be honest, I don't know why the text tracks know their own index in the TextTrackList, but existing code uses it so I assume it's there for a reason). > > Source/WebCore/html/track/AudioTrack.h:58 > > + AtomicString id() const { return m_id; } > > + void setId(const AtomicString& id) { m_id = id; } > > This is common across Audio and VideoTracks, but not TextTracks. I'd move it up into TrackBase anyway. I'd really prefer not to. I think it would be confusing in the future for TextTrack to have an id() function when it's not supposed to. I can do this though if you think it's necessary. > > Source/WebCore/html/track/AudioTrack.h:69 > > + static bool isValidKindKeyword(const AtomicString&); > > Turn this into a virtual method, which overrides a pure virtual in TrackBase. I did this, but it caused some complication because in TextTrack, it's used as a static method in HTMLMediaElement: // 1. If kind is not one of the following strings, then throw a SyntaxError exception and abort these steps if (!TextTrack::isValidKindKeyword(kind)) { ec = SYNTAX_ERR; return 0; } Audio and video tracks won't need to do this though, so it worked fine. I had to name the virtual method isValidKindKeywordProtected() because it couldn't have the same name as the static function. I doubt that's what I should name it though. Any suggestions? > I think it would be useful to split the refactoring out into a separate patch from the new functionality. This will make it much easier to review, but I suspect it will make it easier to figure out build issues on the various bots as well. I'll look into it. There's a big list of commits at this point so it'll take time to rearrange it. --- I also want to mention the TextTrackPrivate/AudioTrackPrivate/VideoTrackPrivate classes. I could create a TrackPrivateBase class, but those classes don't actually share any functionality except the label() and language() functions. Comment on attachment 197143 [details] Patch Attachment 197143 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17711022 Created attachment 197147 [details]
Patch
This should fix the CMake build (I missed some cpp files)
(In reply to comment #38) > If I didn't comment on part of this, it means I did it and have nothing to say besides "done". > > (In reply to comment #36) > > I found some time to do a review. Overall it looks really good, but I'd like to see more functionality moved into TrackBase, leaving only those things which are truly different across Track types in each subclass. > > > > > Source/WebCore/html/HTMLMediaElement.cpp:3022 > > > + m_audioTracks->remove(track); > > > > What happens when someone calls removeAudioTrack() before calling addAudioTrack()? Should that be an ASSERT(), or should it be an early return? > > I'm not really sure. I'm not familiar with WebKit's style when handling this yet. Currently, there's no way for JavaScript to remove any kind of track (per the spec, no remove method, even for added tracks), so right now an ASSERT would probably be fine. I assume someone will eventually realize that it should be possible to remove tracks though, but there probably won't be a way to remove audio and video tracks. Sounds like you need an ASSERT here then. > > > Source/WebCore/html/HTMLMediaElement.cpp:3046 > > > + for (int i = m_audioTracks->length() - 1; i >= 0; --i) { > > > > I see why you're doing it this way, but it still seems awkward. How about: > > > > while (m_audioTracks->length()) > > removeAudioTrack(m_audioTracks->itemAt(0)); > > I changed it to: > > while (unsigned length = m_audioTracks->length()) > removeAudioTrack(m_audioTracks->item(length - 1)); > > Does that work? > > Since the removeAll*Tracks() methods are all called at the same time (in clearMediaPlayer(), I'm considering if it would be better to just make it removeAllInbandTracks() again and have it remove all three types of tracks. That does seem better. :) > > > Source/WebCore/html/track/AudioTrack.cpp:169 > > > +int AudioTrack::inbandTrackIndex() > > > +{ > > > + ASSERT(m_private); > > > + return m_private->audioTrackIndex(); > > > +} > > > > This too can get moved into TrackBase. Just move the m_trackIndex member variable from TextTrack to TrackBase and set the value in the AudioTrack's constructor. > > This is actually different in TextTracks vs Audio/VideoTrack, since TextTracks aren't necessarily in-band. I could create an InbandTrackBase for InbandTextTrack, AudioTrack and VideoTrack to extend, but I'm not sure if you want me to go that far. > > TextTrack's trackIndex() does something different (to be honest, I don't know why the text tracks know their own index in the TextTrackList, but existing code uses it so I assume it's there for a reason). Right, but you could move the member into TrackBase and leave the code which modifies it in TextTrack. > > > Source/WebCore/html/track/AudioTrack.h:58 > > > + AtomicString id() const { return m_id; } > > > + void setId(const AtomicString& id) { m_id = id; } > > > > This is common across Audio and VideoTracks, but not TextTracks. I'd move it up into TrackBase anyway. > > I'd really prefer not to. I think it would be confusing in the future for TextTrack to have an id() function when it's not supposed to. I can do this though if you think it's necessary. Eh, that's fine. > > > Source/WebCore/html/track/AudioTrack.h:69 > > > + static bool isValidKindKeyword(const AtomicString&); > > > > Turn this into a virtual method, which overrides a pure virtual in TrackBase. > > I did this, but it caused some complication because in TextTrack, it's used as a static method in HTMLMediaElement: > > // 1. If kind is not one of the following strings, then throw a SyntaxError exception and abort these steps > if (!TextTrack::isValidKindKeyword(kind)) { > ec = SYNTAX_ERR; > return 0; > } > > Audio and video tracks won't need to do this though, so it worked fine. I had to name the virtual method isValidKindKeywordProtected() because it couldn't have the same name as the static function. I doubt that's what I should name it though. Any suggestions? Ah, that's interesting. I'd just call it "isValidKind()" and put it in protected: in TextBase and private: in all the subclasses. > > I think it would be useful to split the refactoring out into a separate patch from the new functionality. This will make it much easier to review, but I suspect it will make it easier to figure out build issues on the various bots as well. > > I'll look into it. There's a big list of commits at this point so it'll take time to rearrange it. > > --- > > I also want to mention the TextTrackPrivate/AudioTrackPrivate/VideoTrackPrivate classes. I could create a TrackPrivateBase class, but those classes don't actually share any functionality except the label() and language() functions. Yeah, I guess there's an opportunity to make the graph more complicated and introduce an AudioVisualTrackBase. Seems like the increase in boilerplate overwhelm the reduction in duplication though. :-/ Comment on attachment 197147 [details] Patch Attachment 197147 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17676037 Comment on attachment 197147 [details] Patch Attachment 197147 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17711031 Created attachment 197154 [details]
Patch
Fix unused variable warning in EFL build
(In reply to comment #32) > I think it would be useful to split the refactoring out into a separate patch from the new functionality. This will make it much easier to review, but I suspect it will make it easier to figure out build issues on the various bots as well. I still think it would be helpful to split this into two patches, one for the refactoring of the existing code and another to add the new track types. Comment on attachment 197154 [details] Patch Attachment 197154 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17717029 Comment on attachment 197154 [details] Patch Attachment 197154 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17673059 Comment on attachment 197154 [details] Patch Attachment 197154 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17673052 Created attachment 197158 [details]
Patch
Fix some review issues and add to DerivedSources.make/cpp, JSBindingsAllInOne.cpp and UseJSC.make
Alright, I have no clue how to get WebKitGTK to generate the JS*.h files. It generates the .cpp ones now, so why not the .h's? Comment on attachment 197158 [details] Patch Attachment 197158 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17713073 Comment on attachment 197158 [details] Patch Attachment 197158 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17722081 I created bug #114330 so we can do the refactoring as one patch and the audio and video tracks as another. Created attachment 197206 [details]
Patch
This new patch is much simpler, but it won't build until bug #114330 is resolved. Created attachment 197313 [details]
Patch
Created attachment 197896 [details]
Patch
I updated this patch based on comments in #114330: * Add toAudioTrack() and toVideoTrack() * Added some comments in the ChangeLog There are only two things that concern me about this patch: * I'm still not sure how to get the GTK and Mac builds to pick up the new .idl files. * I basically just copied the JS*Custom files and have no idea why they're necessary (but the build won't work without them). Comment on attachment 197896 [details] Patch Attachment 197896 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/37192 Comment on attachment 197896 [details] Patch Attachment 197896 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/9038 Comment on attachment 197896 [details] Patch Attachment 197896 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2203 If you rebase the patch against trunk I can test it and fix the GTK build... Created attachment 197940 [details]
Patch
I rebased it again. Sorry, I didn't notice that the GTK build failed because the patch didn't apply. I'm also working on some tests, but almost all of them will fail at first because there's no way to add audio and video tracks by JavaScript. Comment on attachment 197940 [details] Patch Attachment 197940 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/97171 Comment on attachment 197940 [details] Patch Attachment 197940 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/102202 Comment on attachment 197940 [details] Patch Attachment 197940 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/148155 Comment on attachment 197940 [details] Patch Attachment 197940 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/143187 Add this in the webcore_built_sources variable of GNUmakefile.list.am DerivedSources/WebCore/JSAudioTrack.cpp \ DerivedSources/WebCore/JSAudioTrack.h \ DerivedSources/WebCore/JSVideoTrack.cpp \ DerivedSources/WebCore/JSVideoTrack.h \ DerivedSources/WebCore/JSVideoTrackList.cpp \ DerivedSources/WebCore/JSVideoTrackList.h \ And: DerivedSources/WebCore/JSAudioTrackList.cpp \ DerivedSources/WebCore/JSAudioTrackList.h \ Created attachment 198010 [details]
Patch
(In reply to comment #69) > Add this in the webcore_built_sources variable of GNUmakefile.list.am > > DerivedSources/WebCore/JSAudioTrack.cpp \ > DerivedSources/WebCore/JSAudioTrack.h \ > DerivedSources/WebCore/JSVideoTrack.cpp \ > DerivedSources/WebCore/JSVideoTrack.h \ > DerivedSources/WebCore/JSVideoTrackList.cpp \ > DerivedSources/WebCore/JSVideoTrackList.h \ Thanks! Comment on attachment 198010 [details] Patch Attachment 198010 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/74216 Comment on attachment 198010 [details] Patch Attachment 198010 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/28247 Created attachment 198174 [details]
Patch
Comment on attachment 198174 [details] Patch Attachment 198174 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/42437 Created attachment 198185 [details]
Patch
Created attachment 198191 [details]
Patch
Somehow I'm still missing somewhere where AudioTrackPrivate.h and TextTrackPrivate.h are supposed to be in the Xcode project.. Comment on attachment 198191 [details] Patch Attachment 198191 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/154437 Comment on attachment 198191 [details] Patch Attachment 198191 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/22257 Comment on attachment 198191 [details] Patch Attachment 198191 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/132048 I will fix the Mac build problems and update the patch. Created attachment 198336 [details]
Updated to fix Mac builds.
Comment on attachment 198336 [details] Updated to fix Mac builds. Attachment 198336 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/71129 Comment on attachment 198336 [details] Updated to fix Mac builds. Attachment 198336 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/21297 Comment on attachment 198336 [details] Updated to fix Mac builds. Attachment 198336 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/169088 Created attachment 198390 [details]
Export VideoTrackPrivate.h in private framework headers.
JSCustomIsReachable needs to be changed to CustomIsReachable after r148638 Comment on attachment 198390 [details] Export VideoTrackPrivate.h in private framework headers. View in context: https://bugs.webkit.org/attachment.cgi?id=198390&action=review > Source/WebCore/bindings/js/JSAudioTrackCustom.cpp:47 > +bool JSAudioTrackOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor) > +{ > + JSAudioTrack* jsAudioTrack = jsCast<JSAudioTrack*>(handle.get().asCell()); > + AudioTrack* audioTrack = static_cast<AudioTrack*>(jsAudioTrack->impl());\ > + > + // If the track has no custom properties, it is not reachable. > + if (!jsAudioTrack->hasCustomProperties()) > + return false; > + > + return visitor.containsOpaqueRoot(root(audioTrack)); > +} It's unnecessary to do this with a custom function. Adding "GenerateIsReachable" in the IDL file will do the right thing here. > Source/WebCore/bindings/js/JSAudioTrackListCustom.cpp:54 > +bool JSAudioTrackListOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor) > +{ > + JSAudioTrackList* jsAudioTrackList = jsCast<JSAudioTrackList*>(handle.get().asCell()); > + AudioTrackList* audioTrackList = static_cast<AudioTrackList*>(jsAudioTrackList->impl()); > + > + // If the list is firing event listeners, its wrapper is reachable because > + // the wrapper is responsible for marking those event listeners. > + if (audioTrackList->isFiringEventListeners()) > + return true; > + > + // If the list has no event listeners and has no custom properties, it is not reachable. > + if (!audioTrackList->hasEventListeners() && !jsAudioTrackList->hasCustomProperties()) > + return false; > + > + // It is reachable if the media element parent is reachable. > + return visitor.containsOpaqueRoot(root(audioTrackList->owner())); > +} This could be generated with "GenerateIsReachable", if the generator was updated to add the "isFiringEventListeners()" check for classes which inherited from EventTarget. :-D > Source/WebCore/bindings/js/JSVideoTrackCustom.cpp:47 > +bool JSVideoTrackOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor) > +{ > + JSVideoTrack* jsVideoTrack = jsCast<JSVideoTrack*>(handle.get().asCell()); > + VideoTrack* videoTrack = static_cast<VideoTrack*>(jsVideoTrack->impl()); > + > + // If the track has no custom properties, it is not reachable. > + if (!jsVideoTrack->hasCustomProperties()) > + return false; > + > + return visitor.containsOpaqueRoot(root(videoTrack)); > +} Ditto: "GenerateIsReachable". > Source/WebCore/bindings/js/JSVideoTrackListCustom.cpp:54 > +bool JSVideoTrackListOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor) > +{ > + JSVideoTrackList* jsVideoTrackList = jsCast<JSVideoTrackList*>(handle.get().asCell()); > + VideoTrackList* videoTrackList = static_cast<VideoTrackList*>(jsVideoTrackList->impl()); > + > + // If the list is firing event listeners, its wrapper is reachable because > + // the wrapper is responsible for marking those event listeners. > + if (videoTrackList->isFiringEventListeners()) > + return true; > + > + // If the list has no event listeners and has no custom properties, it is not reachable. > + if (!videoTrackList->hasEventListeners() && !jsVideoTrackList->hasCustomProperties()) > + return false; > + > + // It is reachable if the media element parent is reachable. > + return visitor.containsOpaqueRoot(root(videoTrackList->owner())); > +} Ditto with the generator change. > Source/WebCore/html/HTMLMediaElement.cpp:2828 > + RefPtr<AudioTrack> track = AudioTrack::create(this, prpTrack); > + > + addAudioTrack(track.get()); This seems unfortunate. If addAudioTrack took a PassRefPtr<AudioTrack>, this could be: addAudioTrack(AudioTrack::create(this, prpTrack)); > Source/WebCore/html/HTMLMediaElement.cpp:2873 > + RefPtr<VideoTrack> track = VideoTrack::create(this, prpTrack); > + > + addVideoTrack(track.get()); Ditto. > Source/WebCore/html/HTMLMediaElement.cpp:2888 > + // This cast is safe because we created the AudioTrack with the AudioTrackPrivate > + // passed to mediaPlayerDidAddAudioTrack. > + RefPtr<AudioTrack> track = static_cast<AudioTrack*>(prpTrack->client()); > + if (!track) > + return; I still don't like this. There should be a better way to get from an AudioTrackPrivate to an AudioTrack. Perhaps the notification needs to come through the AudioTrackPrivate, like so: void AudioTrackPrivate::willBeRemoved() { client()->willRemoveAudioTrackPrivate(this); } void AudioTrack::willRemoveAudioTrackPrivate(AudioTrackPrivate* trackPrivate) { ASSERT(trackPrivate == m_private); mediaElement()->removeAudioTrack(this); } > Source/WebCore/html/HTMLMediaElement.cpp:2922 > + // This cast is safe because we created the VideoTrack with the VideoTrackPrivate > + // passed to mediaPlayerDidAddVideoTrack. > + RefPtr<VideoTrack> track = static_cast<VideoTrack*>(prpTrack->client()); > + if (!track) > + return; Ditto. > Source/WebCore/html/HTMLMediaElement.cpp:3003 > +void HTMLMediaElement::addAudioTrack(AudioTrack* track) Since this is taking ownership, this could take a PassRefPtr<AudioTrack>. > Source/WebCore/html/HTMLMediaElement.cpp:3005 > + audioTracks()->append(track); Add a null check: audioTracks() can return null if tracks are disabled at runtime. > Source/WebCore/html/HTMLMediaElement.cpp:3015 > +void HTMLMediaElement::addVideoTrack(VideoTrack* track) Ditto w.r.t. PassRefPtr. > Source/WebCore/html/HTMLMediaElement.cpp:3017 > + videoTracks()->append(track); Ditto w.r.t null check. > Source/WebCore/html/HTMLMediaElement.cpp:3022 > + m_audioTracks->remove(track); Ditto w.r.t. null check. > Source/WebCore/html/HTMLMediaElement.cpp:3039 > + m_videoTracks->remove(track); Ditto w.r.t. null check. > Source/WebCore/html/HTMLMediaElement.cpp:3046 > + while (unsigned length = m_audioTracks->length()) > + removeAudioTrack(m_audioTracks->item(length - 1)); This would be a lot less confusing if AudioTrackList had a "lastItem()" method. And the "if" could be rolled into the while: while (m_audioTracks && m_audioTracks->length()) removeAudioTrack(m_audioTracks->lastItem()); > Source/WebCore/html/HTMLMediaElement.cpp:3060 > + if (m_videoTracks) > + while (unsigned length = m_videoTracks->length()) > + removeVideoTrack(m_videoTracks->item(length - 1)); Ditto. > Source/WebCore/html/HTMLMediaElement.h:245 > + void addAudioTrack(AudioTrack*); > void addTextTrack(TextTrack*); > + void addVideoTrack(VideoTrack*); As before, these can be PassRefPtrs. > Source/WebCore/html/track/AudioTrack.idl:29 > + JSCustomIsReachable As before, this can be GenerateIsReachable. > Source/WebCore/html/track/AudioTrackList.idl:31 > + JSCustomIsReachable With a change to the generator, this could be GenerateIsReachable. > Source/WebCore/html/track/VideoTrack.idl:29 > + JSCustomIsReachable Ditto. > Source/WebCore/html/track/VideoTrackList.idl:31 > + JSCustomIsReachable Ditto. If I switch to GenerateIsReachable, I get these errors: generated/JSAudioTrack.cpp: In member function 'virtual bool WebCore::JSAudioTrackOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown>, void*, JSC::SlotVisitor&)': generated/JSAudioTrack.cpp:233:18: error: 'root' is not a member of 'WebCore' generated/JSVideoTrack.cpp: In member function 'virtual bool WebCore::JSVideoTrackOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown>, void*, JSC::SlotVisitor&)': generated/JSVideoTrack.cpp:233:18: error: 'root' is not a member of 'WebCore' Even though root(AudioTrack*) and root(VideoTrack*) are unchanged and still in JS*TrackCustom.h (In reply to comment #91) > If I switch to GenerateIsReachable, I get these errors: > > generated/JSAudioTrack.cpp: In member function 'virtual bool WebCore::JSAudioTrackOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown>, void*, JSC::SlotVisitor&)': > generated/JSAudioTrack.cpp:233:18: error: 'root' is not a member of 'WebCore' > generated/JSVideoTrack.cpp: In member function 'virtual bool WebCore::JSVideoTrackOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown>, void*, JSC::SlotVisitor&)': > generated/JSVideoTrack.cpp:233:18: error: 'root' is not a member of 'WebCore' > > Even though root(AudioTrack*) and root(VideoTrack*) are unchanged and still in JS*TrackCustom.h Ah, yes, the generator won't add a #include "JSAudioTrackCustom.h" directive. It will add a #include "AudioTrack.h", so you could add the WebCore::root() declaration there, instead. The relevant code in the generator is in Source/WebCore/bindings/js/CodeGeneratorJS.pm, line ~2648. It shows what the various directive parameters will generate. One option that might make sense would be to have an element() accessor (in addition to mediaElement()) in TrackBase, and use "GenerateIsReachable(ImplElementRoot)". The reason I'm pushing for getting rid of the JS*TrackCustom files is that when improvements are made to the generator, classes with custom directives don't automatically benefit; they need to be improved manually, and they may just get forgotten about. So it's best not to have custom directives unless strictly necessary. Filed https://bugs.webkit.org/show_bug.cgi?id=114784 to track adding a generated "isFiringEventListeners()" check. (In reply to comment #90) > > Source/WebCore/html/HTMLMediaElement.cpp:2888 > > + // This cast is safe because we created the AudioTrack with the AudioTrackPrivate > > + // passed to mediaPlayerDidAddAudioTrack. > > + RefPtr<AudioTrack> track = static_cast<AudioTrack*>(prpTrack->client()); > > + if (!track) > > + return; > > I still don't like this. There should be a better way to get from an AudioTrackPrivate to an AudioTrack. Perhaps the notification needs to come through the AudioTrackPrivate, like so: > > void AudioTrackPrivate::willBeRemoved() > { > client()->willRemoveAudioTrackPrivate(this); > } > > void AudioTrack::willRemoveAudioTrackPrivate(AudioTrackPrivate* trackPrivate) > { > ASSERT(trackPrivate == m_private); > mediaElement()->removeAudioTrack(this); > } Is there any reason that the AudioTrackPrivate shouldn't know its parent AudioTrack? I could add AudioTrack* m_track (basically identical to m_client), then: removeAudioTrack(privateTrack->audioTrack()); The only weird thing I can think of for this is that m_client and m_track would be the same object, so it's kind of weird. Created attachment 198774 [details]
Updated based on review
I'm not able to obsolete the previous patch (I incorporated the changes to WebCore.xcodeproj). Making root() work without the TrackCustom.h files was surprisingly difficult, but I was able to combine everything into root(TrackBase*), so that's nice. I didn't notice that bug #114784 was finished, so I'm including the changes related to that now. Comment on attachment 198774 [details] Updated based on review Attachment 198774 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/116089 To do the GenerateIsReachable change for the TrackList classes, I need bug #79822 to be commited. It has a reviewed change, so I'm not sure why it's not. Comment on attachment 198774 [details] Updated based on review Attachment 198774 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/96380 (In reply to comment #98) > To do the GenerateIsReachable change for the TrackList classes, I need bug #79822 to be commited. It has a reviewed change, so I'm not sure why it's not. I notice that bug 79822 failed to build on any EWS bot. In any case, someone (such as the reviewer) would need to set cq+ before it would be landed by the cq. (In reply to comment #100) > (In reply to comment #98) > > To do the GenerateIsReachable change for the TrackList classes, I need bug #79822 to be commited. It has a reviewed change, so I'm not sure why it's not. > > I notice that bug 79822 failed to build on any EWS bot. In any case, someone (such as the reviewer) would need to set cq+ before it would be landed by the cq. It will need to be rebaselined and run past the EWS bots again. I'll take that on. Created attachment 198891 [details]
Add UNUSED_PARAM to willRemove*TrackPrivate
Comment on attachment 198891 [details] Add UNUSED_PARAM to willRemove*TrackPrivate Attachment 198891 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/95389 (In reply to comment #103) > (From update of attachment 198891 [details]) > Attachment 198891 [details] did not pass mac-ews (mac): > Output: http://webkit-queues.appspot.com/results/95389 InbandTextTrackPrivateClient.h needs to be add to the framework's private interfaces so it is visible to WebKit. Created attachment 198913 [details]
Generate isReachableFromOpaqueRoots() and fix mac build again
Comment on attachment 198913 [details] Generate isReachableFromOpaqueRoots() and fix mac build again View in context: https://bugs.webkit.org/attachment.cgi?id=198913&action=review > Source/WebCore/html/HTMLMediaElement.cpp:2889 > +void HTMLMediaElement::mediaPlayerDidRemoveAudioTrack(PassRefPtr<AudioTrackPrivate> prpTrack) > +{ > + prpTrack->willBeRemoved(); > +} > + > +void HTMLMediaElement::mediaPlayerDidRemoveTextTrack(PassRefPtr<InbandTextTrackPrivate> prpTrack) > +{ > + prpTrack->willBeRemoved(); > +} > + > +void HTMLMediaElement::mediaPlayerDidRemoveVideoTrack(PassRefPtr<VideoTrackPrivate> prpTrack) > +{ > + prpTrack->willBeRemoved(); > } This looks much better. It seems like, in theory, MediaPlayerPrivates could remove the middle-man and just call VideoTrackPrivate::willBeRemoved() directly, but there's nothing keeping both approaches from working with this patch. (In reply to comment #106) It looks like I forgot to mark this as --request-commit. Should I mark it to be committed now? I assume review+ means it's good to go but I don't want to accidentally commit it if that wasn't the intention. (In reply to comment #107) > (In reply to comment #106) > It looks like I forgot to mark this as --request-commit. Should I mark it to be committed now? I assume review+ means it's good to go but I don't want to accidentally commit it if that wasn't the intention. Yes, r+ means that any committer is free to commit. You can mark it cq? if you don't have commit access and someone else can change it to cq+. (In reply to comment #108) > (In reply to comment #107) > > (In reply to comment #106) > > It looks like I forgot to mark this as --request-commit. Should I mark it to be committed now? I assume review+ means it's good to go but I don't want to accidentally commit it if that wasn't the intention. > > Yes, r+ means that any committer is free to commit. You can mark it cq? if you don't have commit access and someone else can change it to cq+. I see. I had been assuming the commit bot was commiting anything that was r+ if it was cq?, but I see now that the reviewers set the cq+ also. Comment on attachment 198913 [details] Generate isReachableFromOpaqueRoots() and fix mac build again Clearing flags on attachment: 198913 Committed r148785: <http://trac.webkit.org/changeset/148785> |