Bug 182530 - [GStreamer] Playbin3 support
Summary: [GStreamer] Playbin3 support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords: InRadar
Depends on:
Blocks: 182531
  Show dependency treegraph
 
Reported: 2018-02-06 03:13 PST by Philippe Normand
Modified: 2018-02-19 01:00 PST (History)
4 users (show)

See Also:


Attachments
Patch (61.98 KB, patch)
2018-02-15 09:52 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (61.98 KB, patch)
2018-02-15 09:57 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (62.75 KB, patch)
2018-02-16 02:07 PST, Philippe Normand
calvaris: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2018-02-06 03:13:09 PST
The player should react to the stream collection messages emitted by playbin3 when the USE_PLAYBIN3 env var is set.
Comment 1 Philippe Normand 2018-02-06 04:22:50 PST
This will imply almost a rewrite of the Track support in the backend... fun
Comment 2 Philippe Normand 2018-02-15 09:52:01 PST
Created attachment 333908 [details]
Patch
Comment 3 Philippe Normand 2018-02-15 09:57:07 PST
Created attachment 333910 [details]
Patch
Comment 4 Michael Catanzaro 2018-02-15 10:41:36 PST
GTK and WPE EWS are both red.
Comment 5 Xabier Rodríguez Calvar 2018-02-16 00:47:24 PST
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.
Comment 6 Philippe Normand 2018-02-16 02:07:12 PST
Created attachment 334026 [details]
Patch
Comment 7 Philippe Normand 2018-02-16 02:07:40 PST
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
Comment 8 Xabier Rodríguez Calvar 2018-02-19 00:11:13 PST
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.
Comment 9 Philippe Normand 2018-02-19 00:58:29 PST
Committed r228617: <https://trac.webkit.org/changeset/228617>
Comment 10 Radar WebKit Bug Importer 2018-02-19 01:00:11 PST
<rdar://problem/37663471>