Factor MediaSource methods out of MediaPlayer & MediaPlayerPrivate and into a new MediaSourcePrivate interface.
Created attachment 188422 [details] Patch
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.
Created attachment 188604 [details] Moved WebMediaPlayerClient::getMediaSource() to WebMediaSource::getMediaSourceForURL().
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
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
Created attachment 188605 [details] Add missing file
Created attachment 188894 [details] Alternate implementation that makes MediaSource load more explicit.
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
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
Created attachment 188897 [details] Alternate implementation that makes MediaSource load more explicit.
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.
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.
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
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
Created attachment 189370 [details] Addressing CR comments
Comment on attachment 189370 [details] Addressing CR comments Attachment 189370 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16695145
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.
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.
Created attachment 189572 [details] Patch
Comment on attachment 189572 [details] Patch Attachment 189572 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16694996
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?
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'
(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.
> 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.
Created attachment 189816 [details] Patch
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.
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.
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.
(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.
Thanks.
Created attachment 189833 [details] Removed SourceBufferPrivate changes and removed acolwell from FIXMEs
Comment on attachment 189833 [details] Removed SourceBufferPrivate changes and removed acolwell from FIXMEs Thanks.
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>
All reviewed patches have been landed. Closing bug.