[webkit-reviews] review denied: [Bug 182530] [GStreamer] Playbin3 support : [Attachment 333910] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 182530: [GStreamer] Playbin3 support
https://bugs.webkit.org/show_bug.cgi?id=182530

Attachment 333910: Patch

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




--- Comment #5 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 333910
  --> https://bugs.webkit.org/attachment.cgi?id=333910
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<MediaPlayerPriv
ateGStreamer> 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:21
3
> +	   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.