[webkit-reviews] review granted: [Bug 213777] [GStreamer] Rewrite mediastreamsrc element : [Attachment 403182] Patch

[bugzilla-daemon at webkit.org]

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

Attachment 403182: Patch

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




--- Comment #2 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 403182
  --> https://bugs.webkit.org/attachment.cgi?id=403182
Patch

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

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:5
> + * Copyright (C) 2020 Igalia S.L.
>   * Author: Thibault Saunier <tsaunier at igalia.com>
>   * Author: Alejandro G. Castro <alex at igalia.com>

Though I think authors are not needed here, your rewrite is substantial and I
think you can add yourself here.

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:53
>  static GstTagList* mediaStreamTrackPrivateGetTags(MediaStreamTrackPrivate*
track)

Now that you're reworking smart pointers, it might be a good and wild idea to
make this function return a GRefPtr and when you need to return it, you can
just call .release() on the returned value of this function. I think that way
we are making explicit that the tags are allocated and transfer the ownership
to the smart pointer. Then you have to make the release explicit but in case
you don't do it and they are accidentally kept till the end of the function,
they are released automatically. I am not completely sure of this but let me
know what you think about it. Obviously, when you adopt this below, you would
need to remove the adopt.

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:18
3
> +    InternalSource(bool isCaptureTrack)
> +    {
> +	   ASSERT(!m_src);
> +	   m_src = gst_element_factory_make("appsrc", nullptr);

Considering this is a constructor, I think this assertion does not make sense.

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:18
4
> +	   if (!GST_IS_APP_SRC(m_src.get()))

I think this is a huge GStreamer configuration problem that would make
InternalSource completely useless and faulty. I think the proper think to do
here is a RELEASE_ASSERT_WITH_MESSAGE and let it crash by giving the hint of
why we can't continue running. I say this because below we have the src->get
method that is not checking if we're returning a valid source (which could lead
to nullptr deferencing) and we're also ASSERTing in other methods. I think
doing this here is the right thing to do and we can also forget about the
validity of the source in all the other methods.

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:18
7
> +	   g_object_set(m_src.get(), "is-live", true, "format",
GST_FORMAT_TIME, "emit-signals", true, "min-percent", 100,

I think you should use TRUE and TRUE here.

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:20
0
> +	   if (!m_src)
> +	       return;

Remove as per comment above.

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:21
2
> +	   m_src = nullptr;

Not needed.

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:21
5
> +    GstElement* src() const { return m_src.get(); }

I think, for coherence with the rest of the code, we should call this method
get().

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:21
9
> +	   ASSERT(m_src);

Remove as per comment above.

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:26
8
> +    GUniquePtr<GstFlowCombiner> flowCombiner;

I think it is safe to move this to GRefPtr (and maybe moving all appearances of
this as well).

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:28
9
> +static char* webkitMediaStreamSrcUriGetUri(GstURIHandler* handler)

I don't really know why the design of GstURIHandlerInterface returns here a new
string instead of a const char*. You could let people copy it in case they need
it.

Nothing we can do anyway, sigh.

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:36
9
> +	   priv->track->removeObserver(*priv->mediaStreamTrackObserver.get());

I think you don't need the .get() here as std::unique_ptr implements the
operator*

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:44
1
> +    static Atomic<uint32_t> padId;

nextPadId

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:50
3
> +	   auto data = makeUnique<ProbeData>(GST_ELEMENT_CAST(self),
padTemplate, track);
> +	   gst_pad_add_probe(pad.get(), GST_PAD_PROBE_TYPE_EVENT_DOWNSTREAM,
reinterpret_cast<GstPadProbeCallback>(webkitMediaStreamSrcPadProbeCb),
data.release(), nullptr);

I think this is wrong. You're creating a pointer, releasing the pointer info
the gst_pad_add_probe, which makes it a leaked pointer (even if it was created
as a unique_ptr, it is released and ownership does not belong to the unique_ptr
anymore) because you are not proviving a GDestroyNotify function which should
delete the probe data. Summing up, creating a unique_ptr to release it here
does not give you magical automatic deletion, you need to do it manually and
for that I would avoind the unique_ptr, just new the struct and provide
GDestroyNotify function (you could even make a static method of the struct
called destroyNotifyCb, for example, and pass it instead of the nullptr).

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:51
4
> +	  
source.addAudioSampleObserver(*priv->mediaStreamTrackObserver.get());

can remove the .get()

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:51
7
> +	  
source.addVideoSampleObserver(*priv->mediaStreamTrackObserver.get());

ditto.

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:56
2
> +	   self->priv->audioSrc = WTF::nullopt;

You can use .clear()

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:56
4
> +	   self->priv->videoSrc = WTF::nullopt;

You can use .clear()