WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Moved WebMediaPlayerClient::getMediaSource() to WebMediaSource::getMediaSourceForURL().
(54.44 KB, patch)
2013-02-15 11:13 PST
,
Aaron Colwell
no flags
Details
Formatted Diff
Diff
Add missing file
(57.06 KB, patch)
2013-02-15 11:26 PST
,
Aaron Colwell
no flags
Details
Formatted Diff
Diff
Alternate implementation that makes MediaSource load more explicit.
(64.12 KB, patch)
2013-02-18 08:39 PST
,
Aaron Colwell
webkit-ews
: commit-queue-
Details
Formatted Diff
Diff
Alternate implementation that makes MediaSource load more explicit.
(64.15 KB, patch)
2013-02-18 08:53 PST
,
Aaron Colwell
no flags
Details
Formatted Diff
Diff
Addressing CR comments
(64.40 KB, patch)
2013-02-20 13:57 PST
,
Aaron Colwell
no flags
Details
Formatted Diff
Diff
Patch
(62.31 KB, patch)
2013-02-21 12:12 PST
,
Aaron Colwell
no flags
Details
Formatted Diff
Diff
Patch
(87.18 KB, patch)
2013-02-22 13:46 PST
,
Aaron Colwell
no flags
Details
Formatted Diff
Diff
Removed SourceBufferPrivate changes and removed acolwell from FIXMEs
(62.25 KB, patch)
2013-02-22 14:40 PST
,
Aaron Colwell
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Aaron Colwell
Comment 1
2013-02-14 14:09:51 PST
Created
attachment 188422
[details]
Patch
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
Created
attachment 189572
[details]
Patch
Build Bot
Comment 20
2013-02-22 08:54:04 PST
Comment on
attachment 189572
[details]
Patch
Attachment 189572
[details]
did not pass win-ews (win): Output:
http://queues.webkit.org/results/16694996
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
Created
attachment 189816
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug