Summary: | [GStreamer] Refactor MediaPlayerPrivateGStreamer::notifyPlayerOf* | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Charlie Turner <cturner> | ||||||||||||||
Component: | WebKitGTK | Assignee: | Enrique Ocaña <eocanha> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | bugs-noreply, calvaris, cgarcia, commit-queue, eocanha, eric.carlson, ews-watchlist, glenn, gustavo, jer.noble, menard, philipj, pnormand, sergio, vjaquez | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=229123 | ||||||||||||||||
Bug Depends on: | 227115 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Charlie Turner
2019-11-28 10:02:10 PST
Created attachment 431418 [details]
Patch
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 Created attachment 431448 [details]
Patch
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? 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! Created attachment 431534 [details]
Patch
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? 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. Created attachment 431550 [details]
Patch
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]. Re-opened since this is blocked by bug 227115 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] Created attachment 431652 [details]
Patch
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]. Reopening to attach new patch. Created attachment 431655 [details]
Patch
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]. |