Summary: | [GStreamer] Playbin3 track switch rework | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alicia Boya García <aboya> | ||||||||
Component: | WebKitGTK | Assignee: | Alicia Boya García <aboya> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bugs-noreply, calvaris, cgarcia, eric.carlson, ews-watchlist, glenn, gustavo, jer.noble, menard, philipj, pnormand, sergio, vjaquez | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Alicia Boya García
2020-05-08 07:21:15 PDT
Created attachment 398859 [details]
Patch
Comment on attachment 398859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398859&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1595 > + GST_DEBUG_OBJECT(pipeline(), "Received STREAM_COLLECTION message with upstream id \"%s\" defining the following streams:", gst_stream_collection_get_upstream_id(collection.get())); > + unsigned numStreams = gst_stream_collection_get_size(collection.get()); > + for (unsigned i = 0; i < numStreams; i++) { > + GstStream* stream = gst_stream_collection_get_stream(collection.get(), i); > + GST_DEBUG_OBJECT(pipeline(), "#%u %s %s", i, gst_stream_type_get_name(gst_stream_get_stream_type(stream)), gst_stream_get_stream_id(stream)); > + } This is used for logging only, perhaps wrap it around a #ifndef GST_DISABLE_DEBUG? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1608 > + GST_DEBUG_OBJECT(pipeline(), "Received STREAMS_SELECTED message selecting the following streams:"); > + unsigned numStreams = gst_message_streams_selected_get_size(message); > + for (unsigned i = 0; i < numStreams; i++) { > + GstStream* stream = gst_message_streams_selected_get_stream(message, i); > + GST_DEBUG_OBJECT(pipeline(), "#%u %s %s", i, gst_stream_type_get_name(gst_stream_get_stream_type(stream)), gst_stream_get_stream_id(stream)); > + } Ditto > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:224 > + void updateEnabledAudioTrack(); no updateEnabledTextTrack()? Comment on attachment 398859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398859&action=review >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:224 >> + void updateEnabledAudioTrack(); > > no updateEnabledTextTrack()? This is intentional. Text track selection is handled on another level and it's unaffected by whatever we select in playbin. Comment on attachment 398859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398859&action=review >>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:224 >>> + void updateEnabledAudioTrack(); >> >> no updateEnabledTextTrack()? > > This is intentional. Text track selection is handled on another level and it's unaffected by whatever we select in playbin. Can you elaborate? Playbin still has code to manage text tracks, last time I checked. (In reply to Philippe Normand from comment #4) > Comment on attachment 398859 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=398859&action=review > > >>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:224 > >>> + void updateEnabledAudioTrack(); > >> > >> no updateEnabledTextTrack()? > > > > This is intentional. Text track selection is handled on another level and it's unaffected by whatever we select in playbin. > > Can you elaborate? Playbin still has code to manage text tracks, last time I > checked. All text tracks are connected to WebKitTextCombiner. You can remove all this track selection code and it will still work. WebKit will still detect all the same text tracks and let you select any number of them. This is true even when using playbin2 which wouldn't support that by itself. Comment on attachment 398859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398859&action=review >>>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:224 >>>>> + void updateEnabledAudioTrack(); >>>> >>>> no updateEnabledTextTrack()? >>> >>> This is intentional. Text track selection is handled on another level and it's unaffected by whatever we select in playbin. >> >> Can you elaborate? Playbin still has code to manage text tracks, last time I checked. > > All text tracks are connected to WebKitTextCombiner. You can remove all this track selection code and it will still work. WebKit will still detect all the same text tracks and let you select any number of them. This is true even when using playbin2 which wouldn't support that by itself. OK, thanks. I totally forgot we had that combiner! Created attachment 399125 [details]
Patch
Comment on attachment 399125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399125&action=review LGTM > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1605 > + } else if (GST_MESSAGE_TYPE(message) == GST_MESSAGE_STREAMS_SELECTED && !m_isLegacyPlaybin) { > +#ifndef GST_DISABLE_DEBUG Doesn't it make sense to swap this lines? (In reply to Xabier Rodríguez Calvar from comment #8) > Comment on attachment 399125 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=399125&action=review > > LGTM > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1605 > > + } else if (GST_MESSAGE_TYPE(message) == GST_MESSAGE_STREAMS_SELECTED && !m_isLegacyPlaybin) { > > +#ifndef GST_DISABLE_DEBUG > > Doesn't it make sense to swap this lines? Yes I think it would prevent build warnings, when debug is disabled. Created attachment 399342 [details]
Patch for landing
Committed r261683: <https://trac.webkit.org/changeset/261683> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399342 [details]. |