Bug 124986 – Nix Upstream: Adding Nix MediaPlayer related files

• Status: RESOLVED
• Resolution: INVALID
• Component: WebCore Misc.

[Thiago de Barros Lacerda]

Nix Upstream: Adding Nix MediaPlayer related files

[Thiago de Barros Lacerda]

Created attachment 218018 [details]
Patch

[Eric Carlson]

Comment on attachment 218018 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218018&action=review

> Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:50
> +void MediaPlayerPrivateNix::getSupportedTypes(HashSet<String>&)
> +{
> +}

You don't support any MIME types?

> Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:54
> +    // MediaPlayer calls load even if the engine says it doesn't support the format :-/

":-/" ??

> Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:130
> +    // TODO: Add cancelLoad to Nix::MediaPlayer

"TODO" -> "FIXME" + a bug number.

> Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:135
> +void MediaPlayerPrivateNix::prepareToPlay()
> +{
> +}

Is there nothing to do here, or does this need to be implemented later? If the later, a bug number would be good.

> Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:140
> +    m_nixPlayer->play();
> +    m_paused = false;

Just an observation, but it might be good to have a callback from the platform player when the rate changes and set "m_paused" there. Without this, how will you handle playback stopping when currentTime == duration?

> Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:231
> +    // Add this to Nix::MediaPlayer

Bug number please.

> Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:237
> +    return true;

Really, loading always progresses?

[Benjamin Poulain]

Comment on attachment 218018 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218018&action=review

Let's see a second round after Eric's comments.

> Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:38
> +    auto nixPlayer = adoptPtr(Nix::Platform::current()->createMediaPlayer(nullptr));

Please don't use auto here. The type is not obvious at all when reading the line.

[Hugo Parente Lima]

(In reply to comment #2)
> (From update of attachment 218018 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218018&action=review
> 
> > Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:50
> > +void MediaPlayerPrivateNix::getSupportedTypes(HashSet<String>&)
> > +{
> > +}
> 
> You don't support any MIME types?

No matter what this function returns WebCore always ask MediaPlayer to play the stream (Nix has only one MediaPlayer). So our approach was to let the backend try to load the stream and rise a "FormatError" if the stream isn't supported by the backend.
 
> > Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:54
> > +    // MediaPlayer calls load even if the engine says it doesn't support the format :-/
> 
> ":-/" ??

To be removed :-).
 
> > Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:130
> > +    // TODO: Add cancelLoad to Nix::MediaPlayer
> 
> "TODO" -> "FIXME" + a bug number.

Right.
 
> > Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:135
> > +void MediaPlayerPrivateNix::prepareToPlay()
> > +{
> > +}
> 
> Is there nothing to do here, or does this need to be implemented later? If the later, a bug number would be good.

Nothing to do here until some backend used by Nix prove to need this.
 
> > Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:140
> > +    m_nixPlayer->play();
> > +    m_paused = false;
> 
> Just an observation, but it might be good to have a callback from the platform player when the rate changes and set "m_paused" there. Without this, how will you handle playback stopping when currentTime == duration?

You are right, this need a function on Nix::MediaPlayerClient or the UI will never get updated after the playback stop due to end of stream.
 
> > Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:231
> > +    // Add this to Nix::MediaPlayer
> 
> Bug number please.

For sure.
 
> > Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:237
> > +    return true;
> 
> Really, loading always progresses?

This also need to be forwarded to the backend, this one is easy to do.

[Eric Carlson]

Comment on attachment 218018 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218018&action=review

>>> Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:50
>>> +}
>> 
>> You don't support any MIME types?
> 
> No matter what this function returns WebCore always ask MediaPlayer to play the stream (Nix has only one MediaPlayer). So our approach was to let the backend try to load the stream and rise a "FormatError" if the stream isn't supported by the backend.

WebCore asks the MediaPlayer because MIME type is frequently inaccurate. In any case, this method is not used by MediaPlayer directly, it is used by MIMETypeRegistry to populate the list of media MIME types.

>>> Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:54
>>> +    // MediaPlayer calls load even if the engine says it doesn't support the format :-/
>> 
>> ":-/" ??
> 
> To be removed :-).

Always returning false is not a great idea because this is exposed to the web as video.canPlayType(type).

[Hugo Parente Lima]

(In reply to comment #5)
> (From update of attachment 218018 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218018&action=review
> 
> >>> Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:50
> >>> +}
> >> 
> >> You don't support any MIME types?
> > 
> > No matter what this function returns WebCore always ask MediaPlayer to play the stream (Nix has only one MediaPlayer). So our approach was to let the backend try to load the stream and rise a "FormatError" if the stream isn't supported by the backend.
> 
> WebCore asks the MediaPlayer because MIME type is frequently inaccurate. In any case, this method is not used by MediaPlayer directly, it is used by MIMETypeRegistry to populate the list of media MIME types.

So would be useful to ask the backend.
 
> >>> Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:54
> >>> +    // MediaPlayer calls load even if the engine says it doesn't support the format :-/
> >> 
> >> ":-/" ??
> > 
> > To be removed :-).
> 
> Always returning false is not a great idea because this is exposed to the web as video.canPlayType(type).

Thanks for the explanation, video tag still not supported but after what you said this also needs to be changed, or just checked against the list returned by getSupportedTypes.

Those things were not implemented mostly as a effort to have only audio tag working and with a minimalist platform API, so here and there things start to show why they exists, thanks.

[Eric Carlson]

Comment on attachment 218018 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218018&action=review

>>>>> Source/WebCore/platform/graphics/nix/MediaPlayerPrivateNix.cpp:54
>>>>> +    // MediaPlayer calls load even if the engine says it doesn't support the format :-/
>>>> 
>>>> ":-/" ??
>>> 
>>> To be removed :-).
>> 
>> Always returning false is not a great idea because this is exposed to the web as video.canPlayType(type).
> 
> Thanks for the explanation, video tag still not supported but after what you said this also needs to be changed, or just checked against the list returned by getSupportedTypes.
> 
> Those things were not implemented mostly as a effort to have only audio tag working and with a minimalist platform API, so here and there things start to show why they exists, thanks.

FWIW, this is called by HTMLMediaElement::canPlayType so audio.canPlayType(type) will also end up here.