RESOLVED FIXED 109857
Factor MediaSource methods out of MediaPlayer & MediaPlayerPrivate and into a new MediaSourcePrivate interface.
https://bugs.webkit.org/show_bug.cgi?id=109857
Summary Factor MediaSource methods out of MediaPlayer & MediaPlayerPrivate and into a...
Aaron Colwell
Reported 2013-02-14 13:44:41 PST
Factor MediaSource methods out of MediaPlayer & MediaPlayerPrivate and into a new MediaSourcePrivate interface.
Attachments
Patch (56.32 KB, patch)
2013-02-14 14:09 PST, Aaron Colwell
no flags
Moved WebMediaPlayerClient::getMediaSource() to WebMediaSource::getMediaSourceForURL(). (54.44 KB, patch)
2013-02-15 11:13 PST, Aaron Colwell
no flags
Add missing file (57.06 KB, patch)
2013-02-15 11:26 PST, Aaron Colwell
no flags
Alternate implementation that makes MediaSource load more explicit. (64.12 KB, patch)
2013-02-18 08:39 PST, Aaron Colwell
webkit-ews: commit-queue-
Alternate implementation that makes MediaSource load more explicit. (64.15 KB, patch)
2013-02-18 08:53 PST, Aaron Colwell
no flags
Addressing CR comments (64.40 KB, patch)
2013-02-20 13:57 PST, Aaron Colwell
no flags
Patch (62.31 KB, patch)
2013-02-21 12:12 PST, Aaron Colwell
no flags
Patch (87.18 KB, patch)
2013-02-22 13:46 PST, Aaron Colwell
no flags
Removed SourceBufferPrivate changes and removed acolwell from FIXMEs (62.25 KB, patch)
2013-02-22 14:40 PST, Aaron Colwell
no flags
Aaron Colwell
Comment 1 2013-02-14 14:09:51 PST
WebKit Review Bot
Comment 2 2013-02-14 14:13:55 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Aaron Colwell
Comment 3 2013-02-15 11:13:35 PST
Created attachment 188604 [details] Moved WebMediaPlayerClient::getMediaSource() to WebMediaSource::getMediaSourceForURL().
WebKit Review Bot
Comment 4 2013-02-15 11:21:28 PST
Comment on attachment 188604 [details] Moved WebMediaPlayerClient::getMediaSource() to WebMediaSource::getMediaSourceForURL(). Attachment 188604 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16525056
WebKit Review Bot
Comment 5 2013-02-15 11:21:42 PST
Comment on attachment 188604 [details] Moved WebMediaPlayerClient::getMediaSource() to WebMediaSource::getMediaSourceForURL(). Attachment 188604 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16598151
Aaron Colwell
Comment 6 2013-02-15 11:26:37 PST
Created attachment 188605 [details] Add missing file
Aaron Colwell
Comment 7 2013-02-18 08:39:49 PST
Created attachment 188894 [details] Alternate implementation that makes MediaSource load more explicit.
Early Warning System Bot
Comment 8 2013-02-18 08:44:59 PST
Comment on attachment 188894 [details] Alternate implementation that makes MediaSource load more explicit. Attachment 188894 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16603938
Early Warning System Bot
Comment 9 2013-02-18 08:45:13 PST
Comment on attachment 188894 [details] Alternate implementation that makes MediaSource load more explicit. Attachment 188894 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16626089
Aaron Colwell
Comment 10 2013-02-18 08:53:04 PST
Created attachment 188897 [details] Alternate implementation that makes MediaSource load more explicit.
Aaron Colwell
Comment 11 2013-02-18 08:54:19 PST
I've added an alternate form of this change that makes the MediaSource load more explicit. This feels a little better than the implicit coordination that HTMLMediaElement & WebMediaSource do through the MediaSourceRegistry in the original patch. I'm looking for some guidance on which path is preferred. abarth@ or fishd@ : Could you please provide feedback on the Chromium specific changes. eric.carlson@ : Could you please provide feedback on the WebCore side of things. Thank you.
Jer Noble
Comment 12 2013-02-18 10:18:12 PST
Comment on attachment 188897 [details] Alternate implementation that makes MediaSource load more explicit. View in context: https://bugs.webkit.org/attachment.cgi?id=188897&action=review > Source/WebCore/html/HTMLMediaElement.cpp:1053 > + } else if (!m_player->load(url, contentType, keySystem)) > + mediaLoadingFailed(MediaPlayer::FormatError); > +#else > if (!m_player->load(url, contentType, keySystem)) > mediaLoadingFailed(MediaPlayer::FormatError); > +#endif This could be cleaned up so as not to duplicate code between the #if and #else. Something like: #if .... } else #endif if (!m_player->load(url, contentType, keySystem)) mediaLoadingFailed(MediaPlayer::FormatError); > Source/WebCore/html/HTMLMediaElement.cpp:4478 > - if (m_mediaSource) > - m_mediaSource->setMediaPlayer(m_player.get()); > + if (m_mediaSource && m_mediaSource->readyState() != MediaSource::closedKeyword()) > + m_mediaSource->setReadyState(MediaSource::closedKeyword()); MediaSource is already checking whether the passed in state == the current state. Is this strictly necessary here? > Source/WebCore/platform/graphics/MediaPlayer.cpp:445 > + else > + m_private->load(m_url.string()); > +#else > m_private->load(m_url.string()); This could also be cleaned up to avoid duplicating code. > Source/WebCore/platform/graphics/MediaPlayer.h:41 > +#if ENABLE(MEDIA_SOURCE) > +#include "MediaSource.h" > +#endif Why does this need to be included (rather than forward declared)? > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:56 > +// TODO(acolwell): Remove this class once the Chromium code starts calling > +// WebMediaSource::getMediaSourceForURL() and opening the WebMediaSource object > +// itself. Nit: ideally TODO should be FIXME with an associated bug number / link. > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:302 > +// TODO(acolwell): Remove this and sourceURL() once the Chromium code starts > +// calling WebMediaSource::getMediaSourceForURL() and opening the WebMediaSource > +// itself. Ditto. > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:456 > + // TODO(acolwell): Replace with m_webMediaPlayer->load(new WebMediaSourceImpl(m_mediaSource), corsMode) > + // once the Chromium code has access to the WebMediaSource declaration. Ditto.
Build Bot
Comment 13 2013-02-19 14:10:53 PST
Comment on attachment 188897 [details] Alternate implementation that makes MediaSource load more explicit. Attachment 188897 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16636043
Aaron Colwell
Comment 14 2013-02-20 13:25:55 PST
Comment on attachment 188897 [details] Alternate implementation that makes MediaSource load more explicit. View in context: https://bugs.webkit.org/attachment.cgi?id=188897&action=review >> Source/WebCore/html/HTMLMediaElement.cpp:1053 >> +#endif > > This could be cleaned up so as not to duplicate code between the #if and #else. Something like: > > #if .... > } else > #endif > if (!m_player->load(url, contentType, keySystem)) > mediaLoadingFailed(MediaPlayer::FormatError); Done. I was wary of introducing an #if construct like this because I figured the dangling else would be frowned upon. :) >> Source/WebCore/html/HTMLMediaElement.cpp:4478 >> + m_mediaSource->setReadyState(MediaSource::closedKeyword()); > > MediaSource is already checking whether the passed in state == the current state. Is this strictly necessary here? Nope. Check removed. >> Source/WebCore/platform/graphics/MediaPlayer.cpp:445 >> m_private->load(m_url.string()); > > This could also be cleaned up to avoid duplicating code. Done. >> Source/WebCore/platform/graphics/MediaPlayer.h:41 >> +#endif > > Why does this need to be included (rather than forward declared)? It doesn't. I had it here because of m_mediaSource. I've changed the code to forward declare in this file and then #include "MediaSource.h" in MediaPlayer.cpp where the include is actually needed. >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:56 >> +// itself. > > Nit: ideally TODO should be FIXME with an associated bug number / link. Done. >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:302 >> +// itself. > > Ditto. Done. >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:456 >> + // once the Chromium code has access to the WebMediaSource declaration. > > Ditto. Done
Aaron Colwell
Comment 15 2013-02-20 13:57:35 PST
Created attachment 189370 [details] Addressing CR comments
Build Bot
Comment 16 2013-02-21 09:32:14 PST
Comment on attachment 189370 [details] Addressing CR comments Attachment 189370 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16695145
Adam Barth
Comment 17 2013-02-21 11:22:32 PST
Comment on attachment 189370 [details] Addressing CR comments View in context: https://bugs.webkit.org/attachment.cgi?id=189370&action=review > Source/WebKit/chromium/public/WebMediaSource.h:43 > + WEBKIT_EXPORT static WebMediaSource* getMediaSourceForURL(const WebURL&); getMediaSourceForURL -> create We use the word "create" in functions that allocate memory so that their callers remember to delete them. > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:59 > + WebMediaSourceClientImpl(WebMediaPlayer*); please use the "explicit" keyword for one-argument constructors. > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:74 > + WebMediaPlayer* m_webMediaPlayer; What's the relationship between the lifetime of this class and m_webMediaPlayer? > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:309 > + mediaSource->open(new WebMediaSourceClientImpl(m_webMediaPlayer.get())); This "naked new" is concerning. Where is the object deleted? > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:318 > + return KURL(ParsedURLString, m_url); Should we store m_url as a KURL instead? > Source/WebKit/chromium/src/WebMediaSource.cpp:47 > + return new WebMediaSourceImpl(mediaSource); mediaSource.release() > Source/WebKit/chromium/src/WebMediaSourceImpl.cpp:43 > + MediaSourcePrivateImpl(PassOwnPtr<WebMediaSourceClient>); Please use the "explicit" keyword for one-argument constructors.
Aaron Colwell
Comment 18 2013-02-21 12:11:39 PST
Comment on attachment 189370 [details] Addressing CR comments View in context: https://bugs.webkit.org/attachment.cgi?id=189370&action=review >> Source/WebKit/chromium/public/WebMediaSource.h:43 >> + WEBKIT_EXPORT static WebMediaSource* getMediaSourceForURL(const WebURL&); > > getMediaSourceForURL -> create > > We use the word "create" in functions that allocate memory so that their callers remember to delete them. Oops. This was left over from the original version. Removed. >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:59 >> + WebMediaSourceClientImpl(WebMediaPlayer*); > > please use the "explicit" keyword for one-argument constructors. Done. >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:74 >> + WebMediaPlayer* m_webMediaPlayer; > > What's the relationship between the lifetime of this class and m_webMediaPlayer? This class has a shorter lifetime than WebMediaPlayer. It isn't clear from this context, but HTMLMediaElement always makes sure the MediaSource is closed before it destroys its MediaPlayer & WebMediaPlayer instances. The MediaSource destroys its instance of this class when it gets closed. >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:309 >> + mediaSource->open(new WebMediaSourceClientImpl(m_webMediaPlayer.get())); > > This "naked new" is concerning. Where is the object deleted? The mediaSource object takes ownership of this object in the open() call. It wasn't clear to me whether PassOwnPtr<> could be used in WebXXX interfaces so I just pass the raw pointer here. If you look in WebMediaSourceImpl::open() you will see that this pointer is immetiately adopted into an OwnPtr<WebMediaSourceClient> and passed to MediaSourcePrivateImpl. >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:318 >> + return KURL(ParsedURLString, m_url); > > Should we store m_url as a KURL instead? Sure. Done. >> Source/WebKit/chromium/src/WebMediaSource.cpp:47 >> + return new WebMediaSourceImpl(mediaSource); > > mediaSource.release() Oops. This method left over from the old implementation. Code removed. >> Source/WebKit/chromium/src/WebMediaSourceImpl.cpp:43 >> + MediaSourcePrivateImpl(PassOwnPtr<WebMediaSourceClient>); > > Please use the "explicit" keyword for one-argument constructors. Done.
Aaron Colwell
Comment 19 2013-02-21 12:12:44 PST
Build Bot
Comment 20 2013-02-22 08:54:04 PST
Adam Barth
Comment 21 2013-02-22 11:34:05 PST
Comment on attachment 189572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189572&action=review I don't feel super confident in this review, but this patch seems plausible to me. It looks like the apple-win bubble is red, which might indicate an issue that we should resolve before landing this patch. > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:54 > +// FIXME(acolwell): Remove this class once the Chromium code implements its own (acolwell) <--- We usually skip the name annotations in WebKit. > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:309 > + mediaSource->open(new WebMediaSourceClientImpl(m_webMediaPlayer.get())); It's ok that mediaSource will get deleted immediately after this line?
Adam Barth
Comment 22 2013-02-22 11:35:03 PST
10>..\platform\graphics\win\MediaPlayerPrivateQuickTimeVisualContext.cpp(154) : error C2259: 'WebCore::MediaPlayerPrivateQuickTimeVisualContext' : cannot instantiate abstract class 10> due to following members: 10> 'void WebCore::MediaPlayerPrivateInterface::load(const WTF::String &,WTF::PassRefPtr<T>)' : is abstract 10> with 10> [ 10> T=WebCore::MediaSource 10> ] 10> c:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\graphics\MediaPlayerPrivate.h(48) : see declaration of 'WebCore::MediaPlayerPrivateInterface::load'
Aaron Colwell
Comment 23 2013-02-22 12:43:58 PST
(In reply to comment #22) > 10>..\platform\graphics\win\MediaPlayerPrivateQuickTimeVisualContext.cpp(154) : error C2259: 'WebCore::MediaPlayerPrivateQuickTimeVisualContext' : cannot instantiate abstract class > 10> due to following members: > 10> 'void WebCore::MediaPlayerPrivateInterface::load(const WTF::String &,WTF::PassRefPtr<T>)' : is abstract > 10> with > 10> [ > 10> T=WebCore::MediaSource > 10> ] > 10> c:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\graphics\MediaPlayerPrivate.h(48) : see declaration of 'WebCore::MediaPlayerPrivateInterface::load' Thanks for the review. I'm trying to address the Windows issue with Bug 110494. Is there a way to force the win-ews bot to run quicker. It took almost a whole day to find out the patch failed to apply on Windows.
Adam Barth
Comment 24 2013-02-22 12:50:54 PST
> Is there a way to force the win-ews bot to run quicker. There isn't. > It took almost a whole day to find out the patch failed to apply on Windows. You can either ask someone from apple to try the build for you, or you can make your best effort. You shouldn't let the EWS bots slow you down.
Aaron Colwell
Comment 25 2013-02-22 13:46:36 PST
Aaron Colwell
Comment 26 2013-02-22 13:51:22 PST
Please take another look. I was working on a follow up patch for this one and realized that adding SourceBuffer functionality would be much easier if the SourceBuffer specific methods were in their own SourceBufferPrivate interface. Without this MediaSourcePrivate would have to be modified everytime we wanted to add SourceBuffer functionality which is a similar problem to what was happening with MediaPlayer.
Adam Barth
Comment 27 2013-02-22 13:54:28 PST
62.31 KB -> 87.18 KB Can we make this second change in a separate patch on a separate bug? This patch is getting too big to review effectively.
Aaron Colwell
Comment 28 2013-02-22 14:01:03 PST
Comment on attachment 189572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189572&action=review >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:54 >> +// FIXME(acolwell): Remove this class once the Chromium code implements its own > > (acolwell) <--- We usually skip the name annotations in WebKit. Done >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:309 >> + mediaSource->open(new WebMediaSourceClientImpl(m_webMediaPlayer.get())); > > It's ok that mediaSource will get deleted immediately after this line? Yes. The WebMediaSource only needs to stay alive long enough to be opened. open() is simply used to set the MediaSourcePrivate interface on the MediaSource object. For now WebMediaSource isn't used for anything other than that so it's ok if it gets destroyed here.
Aaron Colwell
Comment 29 2013-02-22 14:02:25 PST
(In reply to comment #27) > 62.31 KB -> 87.18 KB > > Can we make this second change in a separate patch on a separate bug? This patch is getting too big to review effectively. Sure. Sorry about that.
Adam Barth
Comment 30 2013-02-22 14:04:55 PST
Thanks.
Aaron Colwell
Comment 31 2013-02-22 14:40:07 PST
Created attachment 189833 [details] Removed SourceBufferPrivate changes and removed acolwell from FIXMEs
Adam Barth
Comment 32 2013-02-22 14:41:09 PST
Comment on attachment 189833 [details] Removed SourceBufferPrivate changes and removed acolwell from FIXMEs Thanks.
WebKit Review Bot
Comment 33 2013-02-22 18:50:45 PST
Comment on attachment 189833 [details] Removed SourceBufferPrivate changes and removed acolwell from FIXMEs Clearing flags on attachment: 189833 Committed r143826: <http://trac.webkit.org/changeset/143826>
WebKit Review Bot
Comment 34 2013-02-22 18:50:50 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.