Bug 211623 – [GStreamer] Playbin3 track switch rework

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alicia Boya García]

This patch reworks how track selection and reporting of selected
tracks is done in the player.

The following found limitations and assumptions in current GStreamer
have informed this patch:

a) Although the API for playbin3 is designed to be able to accept any
number of tracks of any kind, this is not supported in practice.

b) The first track of each type is always selected. Even in playbin3
mode, looking for GST_STREAM_FLAG_SELECT is not a reliable method, as
in most cases the demuxer does not set it at all. [qtdemux never sets
it at all, and matroskademux only sets it in certain cases.]

c) Sending GST_EVENT_SELECT_STREAMS is only safe at certain moments.
It's not safe before pre-roll, after EOS or during the handling of
another SELECT_STREAMS.

d) Selecting text tracks with playbin APIs is not relevant. All text
tracks are already being picked by WebKitTextCombiner, unaffected by
playbin track selection.

e) Tracks requested in a GST_EVENT_SELECT_STREAMS are eventually
selected. On the other hand,  looking at
GST_MESSAGE_STREAMS_SELECTED's content is not reliable, as this has
been seen to miss tracks depending on thread luck.

This patch takes the points above into account to rework how track
selection is handled in MediaPlayerPrivateGStreamer and fix the
following issues:

1) In playbin3 mode, no track was marked as selected initially,
because of reliance on GST_STREAM_FLAG_SELECT.

2) In playbin2 mode, sometimes tracks would not be initially marked as
selected. This occurred because of reliance on the "selected" property
in inputselector sinkpads, whose initialization is racy -- it can
occur after the track has been added and picked up by WebKit.

3) In playbin3 mode, the limitations explained before has been honored
to make track selection stable, delaying SELECT_STREAMS events until
they are safe to send.

This patch doesn't introduce significative behavior changes, rather
aiming for improving the stabilitity of the player. Existing tests
should provide enough coverage.

[Alicia Boya García]

Created attachment 398859 [details]
Patch

[Philippe Normand]

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()?

[Alicia Boya García]

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.

[Philippe Normand]

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.

[Alicia Boya García]

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

[Philippe Normand]

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!

[Alicia Boya García]

Created attachment 399125 [details]
Patch

[Xabier Rodríguez Calvar]

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?

[Philippe Normand]

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

[Alicia Boya García]

Created attachment 399342 [details]
Patch for landing

[EWS]

Committed r261683: <https://trac.webkit.org/changeset/261683>

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