Bug 204686 – [GStreamer] Refactor MediaPlayerPrivateGStreamer::notifyPlayerOf*

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Charlie Turner]

There is a lot of unnecessary duplication in these methods that can be cleaned up.

[Enrique Ocaña]

Created attachment 431418 [details]
Patch

[Philippe Normand]

Comment on attachment 431418 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:966
> +static void addTrackToPlayer(MediaPlayer&player, AudioTrackPrivate& track)

Missing space after &, also twice below in add methods.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1012
> +        if (*hasType)
> +            m_player->sizeChanged();

This should be done only for video

[Enrique Ocaña]

Created attachment 431448 [details]
Patch

[Philippe Normand]

Comment on attachment 431448 [details]
Patch

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

Nice clean-up!

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1050
> +        addTrackToPlayer(*m_player, track.get());

Nit: I wonder if instead of static funcs we could use a Variant on the enum value here?

[Enrique Ocaña]

Comment on attachment 431448 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1050
>> +        addTrackToPlayer(*m_player, track.get());
> 
> Nit: I wonder if instead of static funcs we could use a Variant on the enum value here?

Thanks, you've just given me the missing piece I needed to achieve all the needed functionality without function parameters!

[Enrique Ocaña]

Created attachment 431534 [details]
Patch

[Philippe Normand]

Comment on attachment 431534 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1052
> +        case 0: m_player->addAudioTrack(get<AudioTrackPrivate&>(variantTrack)); break;
> +        case 1: m_player->addVideoTrack(get<VideoTrackPrivate&>(variantTrack)); break;
> +        case 2: m_player->addTextTrack(get<InbandTextTrackPrivate&>(variantTrack)); break;

Instead of hard-coding numbers, can you re-use the TrackType enum?

[Enrique Ocaña]

Comment on attachment 431534 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1052
>> +        case 2: m_player->addTextTrack(get<InbandTextTrackPrivate&>(variantTrack)); break;
> 
> Instead of hard-coding numbers, can you re-use the TrackType enum?

Sure. I'm also renaming tracksVariant (at the begining of the method) as variantTracks, to make it consistent with the variantTrack here.

[Enrique Ocaña]

Created attachment 431550 [details]
Patch

[EWS]

Committed r278978 (238904@main): <https://commits.webkit.org/238904@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431550 [details].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 227115

[Enrique Ocaña]

I had to revert this patch because the g++ 8 compiler used in the Debian and Ubuntu bots doesn't like the array-based initializations:

../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp: In instantiation of ‘void WebCore::MediaPlayerPrivateGStreamer::notifyPlayerOfTrack() [with TrackPrivateType = WebCore::AudioTrackPrivateGStreamer]’:
../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1097:65:   required from here
../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:977:65: error: taking address of temporary array
     auto typeName = (const char*[]) { "audio", "video", "text" }[type];
                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:978:68: error: taking address of temporary array
     auto* hasType = (bool*[]) { &m_hasAudio, &m_hasVideo, nullptr }[type];
                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^

Also, this warning:

In file included from WTF/Headers/wtf/JSONValues.h:35,
                 from ../../Source/WebCore/platform/graphics/IntSize.h:29,
                 from ../../Source/WebCore/platform/graphics/IntPoint.h:28,
                 from ../../Source/WebCore/platform/graphics/FloatSize.h:30,
                 from ../../Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:23,
                 from ../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:29,
                 from ../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:27:
WTF/Headers/wtf/HashMap.h:480:13: error: ‘bool WTF::HashMap<KeyArg, MappedArg, <template-parameter-1-3>, <template-parameter-1-4>, <template-parameter-1-5>, <template-parameter-1-6> >::removeIf(Functor&&) [with Functor = WebCore::MediaPlayerPrivateGStreamer::notifyPlayerOfTrack() [with TrackPrivateType = WebCore::AudioTrackPrivateGStreamer]::<lambda(auto:16&)>; KeyArg = WTF::AtomString; MappedArg = WTF::RefPtr<WebCore::AudioTrackPrivateGStreamer>; HashArg = WTF::DefaultHash<WTF::AtomString>; KeyTraitsArg = WTF::HashTraits<WTF::AtomString>; MappedTraitsArg = WTF::HashTraits<WTF::RefPtr<WebCore::AudioTrackPrivateGStreamer> >; TableTraitsArg = WTF::HashTableTraits]’, declared using local type ‘WebCore::MediaPlayerPrivateGStreamer::notifyPlayerOfTrack() [with TrackPrivateType = WebCore::AudioTrackPrivateGStreamer]::<lambda(auto:16&)>’, is used but never defined [-fpermissive]
 inline bool HashMap<T, U, V, W, X, Y>::removeIf(Functor&& functor)
             ^~~~~~~~~~~~~~~~~~~~~~~~~
WTF/Headers/wtf/HashMap.h:480:13: error: ‘bool WTF::HashMap<KeyArg, MappedArg, <template-parameter-1-3>, <template-parameter-1-4>, <template-parameter-1-5>, <template-parameter-1-6> >::removeIf(Functor&&) [with Functor = WebCore::MediaPlayerPrivateGStreamer::notifyPlayerOfTrack() [with TrackPrivateType = WebCore::InbandTextTrackPrivateGStreamer]::<lambda(auto:16&)>; KeyArg = WTF::AtomString; MappedArg = WTF::RefPtr<WebCore::InbandTextTrackPrivateGStreamer>; HashArg = WTF::DefaultHash<WTF::AtomString>; KeyTraitsArg = WTF::HashTraits<WTF::AtomString>; MappedTraitsArg = WTF::HashTraits<WTF::RefPtr<WebCore::InbandTextTrackPrivateGStreamer> >; TableTraitsArg = WTF::HashTableTraits]’, declared using local type ‘WebCore::MediaPlayerPrivateGStreamer::notifyPlayerOfTrack() [with TrackPrivateType = WebCore::InbandTextTrackPrivateGStreamer]::<lambda(auto:16&)>’, is used but never defined [-fpermissive]
WTF/Headers/wtf/HashMap.h:480:13: error: ‘bool WTF::HashMap<KeyArg, MappedArg, <template-parameter-1-3>, <template-parameter-1-4>, <template-parameter-1-5>, <template-parameter-1-6> >::removeIf(Functor&&) [with Functor = WebCore::MediaPlayerPrivateGStreamer::notifyPlayerOfTrack() [with TrackPrivateType = WebCore::VideoTrackPrivateGStreamer]::<lambda(auto:16&)>; KeyArg = WTF::AtomString; MappedArg = WTF::RefPtr<WebCore::VideoTrackPrivateGStreamer>; HashArg = WTF::DefaultHash<WTF::AtomString>; KeyTraitsArg = WTF::HashTraits<WTF::AtomString>; MappedTraitsArg = WTF::HashTraits<WTF::RefPtr<WebCore::VideoTrackPrivateGStreamer> >; TableTraitsArg = WTF::HashTableTraits]’, declared using local type ‘WebCore::MediaPlayerPrivateGStreamer::notifyPlayerOfTrack() [with TrackPrivateType = WebCore::VideoTrackPrivateGStreamer]::<lambda(auto:16&)>’, is used but never defined [-fpermissive]

[Enrique Ocaña]

Created attachment 431652 [details]
Patch

[EWS]

Committed r278981 (238907@main): <https://commits.webkit.org/238907@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431652 [details].

[Enrique Ocaña]

Reopening to attach new patch.

[Enrique Ocaña]

Created attachment 431655 [details]
Patch

[EWS]

Committed r278983 (238909@main): <https://commits.webkit.org/238909@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431655 [details].