Bug 182530 – [GStreamer] Playbin3 support

• Component: Platform
• Status: RESOLVED FIXED

[Philippe Normand]

The player should react to the stream collection messages emitted by playbin3 when the USE_PLAYBIN3 env var is set.

[Philippe Normand]

This will imply almost a rewrite of the Track support in the backend... fun

[Philippe Normand]

Created attachment 333908 [details]
Patch

[Philippe Normand]

Created attachment 333910 [details]
Patch

[Michael Catanzaro]

GTK and WPE EWS are both red.

[Xabier Rodríguez Calvar]

Comment on attachment 333910 [details]
Patch

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

Unless I am mistaken, there are some leaks that need to be plugged.

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.cpp:37
> +AudioTrackPrivateGStreamer::AudioTrackPrivateGStreamer(WeakPtr<MediaPlayerPrivateGStreamer> player, gint index, GRefPtr<GstPad> pad)

We should consider using move semantics for the pad and stream in the overload in these methods, specially because of the way they are used, Though I agree this can be done in a new bug.

> Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:356
> +#if USE(GSTREAMER_PLAYBIN3)

I have doubts about this. These are needed now only because of the playbin3 code but I think they could end up being useful for regular code too, so I would unflag them for playbin3 but I leave the final decision to you.

> Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h:46
> +#if USE(GSTREAMER_PLAYBIN3)

ditto

> Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h:135
> +#if USE(GSTREAMER_PLAYBIN3)

ditto

> Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:62
> +    : InbandTextTrackPrivate(WebVTT), TrackPrivateBaseGStreamer(this, index, stream)

You can move , TrackPrivate... to a new line

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:680
> +    GST_INFO("Media has %u video tracks, %u audio tracks and %u text tracks", validVideoStreams.size(),
> +        validAudioStreams.size(), validTextStreams.size());

You can use a single line

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:715
> +    case TrackPrivateBaseGStreamer::TrackType::Audio: {

You don't need variable block allocation, you can remove the { }

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:769
> +    if (selectedStreams)
> +        g_list_free(selectedStreams);

I think we're leaking the strings inside the GList, aren't we? I think we can avoid the g_strdup in the g_list_append.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1298
> +            GRefPtr<GstStream> stream = gst_message_streams_selected_get_stream(message, i);

gst_message_streams_selected_get_stream is transfer full, you need to adopt here.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:213
> +        StreamCollectionChanged = 1 << 7

If looks like you can flag this for playbin3 only

> Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp:52
> +#if USE(GSTREAMER_PLAYBIN3)
> +    , m_stream(nullptr)
> +#endif

not needed

> Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp:69
> +    , m_pad(nullptr)

not needed

> Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp:76
> +    // We can't call notifyTrackOfTagsChanged() directly, because we need tagsChanged()
> +    // to setup m_tags.

Single line.

[Philippe Normand]

Created attachment 334026 [details]
Patch

[Philippe Normand]

Comment on attachment 333910 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.cpp:37
>> +AudioTrackPrivateGStreamer::AudioTrackPrivateGStreamer(WeakPtr<MediaPlayerPrivateGStreamer> player, gint index, GRefPtr<GstPad> pad)
> 
> We should consider using move semantics for the pad and stream in the overload in these methods, specially because of the way they are used, Though I agree this can be done in a new bug.

I can try to apply this in this patch.

>> Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:356
>> +#if USE(GSTREAMER_PLAYBIN3)
> 
> I have doubts about this. These are needed now only because of the playbin3 code but I think they could end up being useful for regular code too, so I would unflag them for playbin3 but I leave the final decision to you.

I actually tried to enable those only for gst 1.10.x but I was getting crashes in gst-mpegts (wtf?) that were fixed in git already. Since we use GstStream* only for playbin3 I thought I'd guard this code accordingly.

>> Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:62
>> +    : InbandTextTrackPrivate(WebVTT), TrackPrivateBaseGStreamer(this, index, stream)
> 
> You can move , TrackPrivate... to a new line

True, I just copied the other constructor :)

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:680
>> +        validAudioStreams.size(), validTextStreams.size());
> 
> You can use a single line

Ok

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:715
>> +    case TrackPrivateBaseGStreamer::TrackType::Audio: {
> 
> You don't need variable block allocation, you can remove the { }

Sure

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:769
>> +        g_list_free(selectedStreams);
> 
> I think we're leaking the strings inside the GList, aren't we? I think we can avoid the g_strdup in the g_list_append.

Avoiding the strdup was my first attempt yes but: error: invalid conversion from ‘const void*’ to ‘gpointer {aka void*}’ [-fpermissive]
             selectedStreams = g_list_append(selectedStreams, m_currentVideoStreamId.utf8().data());
                                                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1298
>> +            GRefPtr<GstStream> stream = gst_message_streams_selected_get_stream(message, i);
> 
> gst_message_streams_selected_get_stream is transfer full, you need to adopt here.

Ooops

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:213
>> +        StreamCollectionChanged = 1 << 7
> 
> If looks like you can flag this for playbin3 only

Indeed.

>> Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp:52
>> +#endif
> 
> not needed

Sure

[Xabier Rodríguez Calvar]

Comment on attachment 334026 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:738
> +    case TrackPrivateBaseGStreamer::TrackType::Unknown:

You could event default: here.

[Philippe Normand]

Committed r228617: <https://trac.webkit.org/changeset/228617>

[Radar WebKit Bug Importer]

<rdar://problem/37663471>