Bug 213777

Created attachment 403182 [details] Patch

Comment on attachment 403182 [details] 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@igalia.com> > * Author: Alejandro G. Castro <alex@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:183 > + 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:184 > + 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:187 > + 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:200 > + if (!m_src) > + return; Remove as per comment above. > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:212 > + m_src = nullptr; Not needed. > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:215 > + 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:219 > + ASSERT(m_src); Remove as per comment above. > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:268 > + 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:289 > +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:369 > + 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:441 > + static Atomic<uint32_t> padId; nextPadId > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:503 > + 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:514 > + source.addAudioSampleObserver(*priv->mediaStreamTrackObserver.get()); can remove the .get() > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:517 > + source.addVideoSampleObserver(*priv->mediaStreamTrackObserver.get()); ditto. > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:562 > + self->priv->audioSrc = WTF::nullopt; You can use .clear() > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:564 > + self->priv->videoSrc = WTF::nullopt; You can use .clear()

Comment on attachment 403182 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403182&action=review >> 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. There is no release() in GRefPtr. However there is a leakRef(). Is this what you mean? Anyway I'm not sure this is worth the ref-churn here, to be honest. >> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:268 >> + GUniquePtr<GstFlowCombiner> flowCombiner; > > I think it is safe to move this to GRefPtr (and maybe moving all appearances of this as well). Why? I think we can avoid useless ref-churn here. And elsewhere we use this type as a GUniquePtr as well. >> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:562 >> + self->priv->audioSrc = WTF::nullopt; > > You can use .clear() this is a private method in Optional, there's a reset() though :)

(In reply to Philippe Normand from comment #3) > Comment on attachment 403182 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403182&action=review > > >> 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. > > There is no release() in GRefPtr. However there is a leakRef(). Is this what > you mean? Yes. > Anyway I'm not sure this is worth the ref-churn here, to be honest. I have no strong opinion about it. It's only that that it gives me the creeps leaking return variables in big chunks of code as this one. Anyway, even we don't add it, we should definitely add WARN_UNUSED_RETURN to this kind of methods. > >> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:268 > >> + GUniquePtr<GstFlowCombiner> flowCombiner; > > > > I think it is safe to move this to GRefPtr (and maybe moving all appearances of this as well). > > Why? I think we can avoid useless ref-churn here. And elsewhere we use this > type as a GUniquePtr as well. By using _new and _free you're not avoiding ref and unref, actually the implementation is as follows: void gst_flow_combiner_free (GstFlowCombiner * combiner) { gst_flow_combiner_unref (combiner); } I am not strongly against using GUniquePtr instead of GRefPtr (if we go for the latter we should not forget to add GstFlowCombiner to GRefPtrGStreamer.h). So far the code is not using them but it gives the me creeps that we can do something weird. _free is clearly legacy and this should be used with ref and unref. I think we should to GRefPtr here and move the other GUniquePtr instances to GRefPtr but this could be for another bug. > >> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:562 > >> + self->priv->audioSrc = WTF::nullopt; > > > > You can use .clear() > > this is a private method in Optional, there's a reset() though :) Yes.

Comment on attachment 403182 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403182&action=review >>>> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:268 >>>> + GUniquePtr<GstFlowCombiner> flowCombiner; >>> >>> I think it is safe to move this to GRefPtr (and maybe moving all appearances of this as well). >> >> Why? I think we can avoid useless ref-churn here. And elsewhere we use this type as a GUniquePtr as well. > > By using _new and _free you're not avoiding ref and unref, actually the implementation is as follows: > > void > gst_flow_combiner_free (GstFlowCombiner * combiner) > { > gst_flow_combiner_unref (combiner); > } > > I am not strongly against using GUniquePtr instead of GRefPtr (if we go for the latter we should not forget to add GstFlowCombiner to GRefPtrGStreamer.h). So far the code is not using them but it gives the me creeps that we can do something weird. _free is clearly legacy and this should be used with ref and unref. I think we should to GRefPtr here and move the other GUniquePtr instances to GRefPtr but this could be for another bug. I think I did a bad job at explaining myself again... My apologies. Let's try again. Here this flowCombiner has a clear, unique, owner. Switching it to GRefPtr would add one extra ref and one extra unref call, IIUC the magics of GRefPtr. I don't think this is needed.

Committed r263836: <https://trac.webkit.org/changeset/263836>

<rdar://problem/65024038>