Bug 109857 – Factor MediaSource methods out of MediaPlayer & MediaPlayerPrivate and into a new MediaSourcePrivate interface.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Aaron Colwell]

Factor MediaSource methods out of MediaPlayer & MediaPlayerPrivate and into a new MediaSourcePrivate interface.

[Aaron Colwell]

Created attachment 188422 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Aaron Colwell]

Created attachment 188604 [details]
Moved WebMediaPlayerClient::getMediaSource() to WebMediaSource::getMediaSourceForURL().

[WebKit Review Bot]

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 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]

Created attachment 188605 [details]
Add missing file

[Aaron Colwell]

Created attachment 188894 [details]
Alternate implementation that makes MediaSource load more explicit.

[Early Warning System Bot]

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 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]

Created attachment 188897 [details]
Alternate implementation that makes MediaSource load more explicit.

[Aaron Colwell]

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 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 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 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]

Created attachment 189370 [details]
Addressing CR comments

[Build Bot]

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 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 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]

Created attachment 189572 [details]
Patch

[Build Bot]

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 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]

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]

(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]

> 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]

Created attachment 189816 [details]
Patch

[Aaron Colwell]

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]

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 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]

(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]

Thanks.

[Aaron Colwell]

Created attachment 189833 [details]
Removed SourceBufferPrivate changes and removed acolwell from FIXMEs

[Adam Barth]

Comment on attachment 189833 [details]
Removed SourceBufferPrivate changes and removed acolwell from FIXMEs

Thanks.

[WebKit Review Bot]

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]

All reviewed patches have been landed.  Closing bug.