Bug 236540

Created attachment 451786 [details] Patch

EWS doesn't look happy. I'll check it...

Created attachment 451811 [details] Patch

Created attachment 451816 [details] Patch

Comment on attachment 451816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451816&action=review > Source/WTF/Scripts/Preferences/WebPreferences.yaml:1726 > + "USE(GSTREAMER_WEBRTC)": true Perhaps a function here to check, at least, webrtcbin and webrtcdsp elements are available. > Source/WebCore/Modules/mediastream/MediaDevices.cpp:43 > +#include "Logging.h" Is this required? > Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:141 > + // g_object_set(m_src.get(), "io-mode", 2, nullptr); crumbs left behind? > Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoEncoder.cpp:300 > + "video/x-h264,alignment=au,stream-format=byte-stream,profile=constrained-baseline", this is a to-do: we ought use what SDP negotiated (and also define what is possible to negotiate depending on the used decoder).

Comment on attachment 451816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451816&action=review >> Source/WTF/Scripts/Preferences/WebPreferences.yaml:1726 >> + "USE(GSTREAMER_WEBRTC)": true > > Perhaps a function here to check, at least, webrtcbin and webrtcdsp elements are available. Agreed, but I don't plan to work on a GstWebRTCProvider for this patch, which is already quite big :( Can we keep this task as one of the many follow-ups? >> Source/WebCore/Modules/mediastream/MediaDevices.cpp:43 >> +#include "Logging.h" > > Is this required? Yes >> Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:141 >> + // g_object_set(m_src.get(), "io-mode", 2, nullptr); > > crumbs left behind? oops :) >> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoEncoder.cpp:300 >> + "video/x-h264,alignment=au,stream-format=byte-stream,profile=constrained-baseline", > > this is a to-do: we ought use what SDP negotiated (and also define what is possible to negotiate depending on the used decoder). s/decoder/encoder? Yes anyway, this needs to be revised, I'll revert this part.

Comment on attachment 451816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451816&action=review It's a very big patch. Took a long time and I would have to have been more thorough. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:231 > + postTask([client = m_client, data, size] { I'm worried about the lifetime of this data. Are we guaranteeing that those GBytes will be alive by the moment this is run in the MT? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:241 > + CString buffer(string, strlen(string)); Why don't you create the String from the beginning and pass that around? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:265 > + postTask([client = m_client, error] { Again, lifetime of this error. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.h:58 > + // RTCDataChannelHandler API Period at the end. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDtlsTransportBackend.h:41 > + // RTCDtlsTransportBackend . > Source/WebCore/Modules/mediastream/gstreamer/GStreamerIceTransportBackend.h:41 > + // RTCIceTransportBackend . > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:305 > + pendingRemoteDescriptionSdp = description; not moving here? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:402 > + auto cleanup = makeScopeExit([&] { > + gst_promise_unref(promise); > + }); Can't we add GstPromise to GRefPtrGStreamer? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:623 > + // flushPendingSources(true); Left behind? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:644 > + auto cleanup = makeScopeExit([&] { GRefPtrGStreamer? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:1056 > + callOnMainThread([protectedThis = Ref(*this), description = description.release()] { Leaking the description? Maybe move the smart ptr inside? Besides, I recall GUniqueOutPtrs should be used just locally. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:1070 > + callOnMainThread([protectedThis = Ref(*this), error = error.release()] { ditto. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:1114 > + auto cleanup = makeScopeExit([&] { GRefPtrGStreamer > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:1182 > + callOnMainThread([protectedThis = Ref(*this), this, stats = statsCopy.release()] { leak? I might have missed this pattern elsewhere? I would recommend you check all uses of release in lambda creation. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerPeerConnectionBackend.cpp:51 > + WTFLogAlways("GstWebRTC plugin not found. Make sure to install gst-plugins-bad >= 1.20 with the webrtc plugin enabled."); Maybe ASSERT_NOT_REACHED as well? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerRtpReceiverTransformBackend.h:39 > + // RTCRtpTransformBackend . > Source/WebCore/Modules/mediastream/gstreamer/GStreamerRtpSenderBackend.h:59 > + return WTF::switchOn(m_source, For this easy variant handlings I prefer to use holds_alternative + get > Source/WebCore/Modules/mediastream/gstreamer/GStreamerRtpSenderBackend.h:67 > + return WTF::switchOn(m_source, ditto > Source/WebCore/Modules/mediastream/gstreamer/GStreamerRtpSenderBackend.h:75 > + return WTF::switchOn(m_source, ditto > Source/WebCore/Modules/mediastream/gstreamer/GStreamerRtpSenderBackend.h:100 > + GRefPtr<GstElement> stopSource(); Might be interesting to warn for unused return > Source/WebCore/Modules/mediastream/gstreamer/GStreamerRtpSenderTransformBackend.h:42 > + // RTCRtpTransformBackend . > Source/WebCore/Modules/mediastream/gstreamer/GStreamerRtpTransceiverBackend.cpp:97 > +static inline ExceptionOr<GstCaps*> toRtpCodecCapability(const RTCRtpCodecCapability& codec) Might be interesting to mark this as warning on unused return to prevent leaks. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerRtpTransformBackend.h:38 > + // RTCRtpTransformBackend . > Source/WebCore/Modules/mediastream/gstreamer/GStreamerSctpTransportBackend.h:42 > + // RTCSctpTransportBackend . > Source/WebCore/Modules/mediastream/gstreamer/GStreamerStatsCollector.cpp:247 > + gst_promise_unref(promise); GRefPtr and move into the lambdas. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerWebRTCUtils.cpp:49 > + if (!protocol || protocol.isEmpty()) redundant, isEmpty implies the other > Source/WebCore/Modules/mediastream/gstreamer/GStreamerWebRTCUtils.cpp:59 > + if (!type || type.isEmpty()) ditto > Source/WebCore/Modules/mediastream/gstreamer/GStreamerWebRTCUtils.cpp:71 > + if (!type || type.isEmpty()) ditto > Source/WebCore/Modules/mediastream/gstreamer/GStreamerWebRTCUtils.cpp:136 > + if (priority < 0.7) > + return RTCPriorityType::VeryLow; > + if (priority < 1.5) > + return RTCPriorityType::Low; > + if (priority < 2.5) It could be a good idea to make defines for these values above. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerWebRTCUtils.cpp:300 > + auto scopeExit = WTF::makeScopeExit([&] { I wonder if it could make sense to make GUniquePtr definition for this, even inside this file. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerWebRTCUtils.h:97 > +static inline RTCSdpType fromSessionDescriptionType(GstWebRTCSessionDescription* description) const * or even const & > Source/WebCore/Modules/mediastream/gstreamer/GStreamerWebRTCUtils.h:208 > +static inline GstWebRTCBundlePolicy bundlePolicyFromConfiguration(MediaEndpointConfiguration& configuration) const & > Source/WebCore/Modules/mediastream/gstreamer/GStreamerWebRTCUtils.h:223 > +static inline GstWebRTCICETransportPolicy iceTransportPolicyFromConfiguration(MediaEndpointConfiguration& configuration) const & > Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:141 > + // g_object_set(m_src.get(), "io-mode", 2, nullptr); leftovers > Source/WebCore/platform/mediastream/gstreamer/GStreamerDTMFSenderBackend.h:37 > + // RTCDTMFSenderBackend . > Source/WebCore/platform/mediastream/gstreamer/RealtimeIncomingAudioSourceGStreamer.h:38 > + // RealtimeMediaSource API . > Source/WebCore/platform/mediastream/gstreamer/RealtimeIncomingAudioSourceGStreamer.h:40 > + void stopProducingData() final; extra space before final > Source/WebCore/platform/mediastream/gstreamer/RealtimeIncomingAudioSourceGStreamer.h:45 > + // RealtimeIncomingSourceGStreamer API . > Source/WebCore/platform/mediastream/gstreamer/RealtimeIncomingSourceGStreamer.h:37 > + void lockValve() const; > + void releaseValve() const; Maybe it's a cultural thing, but for me it makes more sense open and close. > Source/WebCore/platform/mediastream/gstreamer/RealtimeIncomingVideoSourceGStreamer.h:38 > + // RealtimeMediaSource API . > Source/WebCore/platform/mediastream/gstreamer/RealtimeIncomingVideoSourceGStreamer.h:46 > + // RealtimeIncomingSourceGStreamer API . > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingAudioSourceGStreamer.cpp:59 > +bool RealtimeOutgoingAudioSourceGStreamer::setPayloadType(GRefPtr<GstCaps>& caps) const & > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingMediaSourceGStreamer.cpp:144 > + g_object_get(transceiver.get(), "sender", &m_sender.outPtr(), nullptr); Can this be called more than once? outPtr is not going to release an outPtr for you and it will ASSERT if there was one already. > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingMediaSourceGStreamer.h:39 > + GRefPtr<GstCaps> allowedCaps() const { return m_allowedCaps; } const & > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingMediaSourceGStreamer.h:76 > + // MediaStreamTrackPrivate::Observer API . > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingVideoSourceGStreamer.cpp:61 > +bool RealtimeOutgoingVideoSourceGStreamer::setPayloadType(GRefPtr<GstCaps>& caps) const &

Comment on attachment 451816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451816&action=review >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.h:58 >> + // RTCDataChannelHandler API > > Period at the end. We don't put a . for this kind of comment, they're not sentences, they are preamble to virtual method overrides. :) Otherwise there's a lot of full stops to put elsewhere in WebCore. And what about "namespace WebCore" comments?

Comment on attachment 451816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451816&action=review >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:402 >> + }); > > Can't we add GstPromise to GRefPtrGStreamer? Well yeah I thought about it I think, but wouldn't that be awkward because the change_func obviously passes a raw pointer, so what would we do here? >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:1056 >> + callOnMainThread([protectedThis = Ref(*this), description = description.release()] { > > Leaking the description? Maybe move the smart ptr inside? Besides, I recall GUniqueOutPtrs should be used just locally. Right, need to revisit this, passing the OutPtr around is not great >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerRtpSenderBackend.h:59 >> + return WTF::switchOn(m_source, > > For this easy variant handlings I prefer to use holds_alternative + get I didn't know about this. Generally I used the same patterns as in the libwebrtc backend :) >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerWebRTCUtils.cpp:300 >> + auto scopeExit = WTF::makeScopeExit([&] { > > I wonder if it could make sense to make GUniquePtr definition for this, even inside this file. I thought about reusing the ones already defined in the crypto backend, but didn't have time... >> Source/WebCore/platform/mediastream/gstreamer/RealtimeIncomingSourceGStreamer.h:37 >> + void releaseValve() const; > > Maybe it's a cultural thing, but for me it makes more sense open and close. Sounds better! >> Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingMediaSourceGStreamer.cpp:144 >> + g_object_get(transceiver.get(), "sender", &m_sender.outPtr(), nullptr); > > Can this be called more than once? outPtr is not going to release an outPtr for you and it will ASSERT if there was one already. No, it's meant to be called once, maybe I can rework this a bit though....

Comment on attachment 451816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451816&action=review > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:131 > + m_hasClient = true; > + ASSERT(!m_client); > + m_client = &client; We can probably use an optional here for the m_client. nullopt means it doesn't have a client and nullptr value that it has a null client. >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:231 >> + postTask([client = m_client, data, size] { > > I'm worried about the lifetime of this data. Are we guaranteeing that those GBytes will be alive by the moment this is run in the MT? We can either pass a ref to the GBytes or create a SharedBuffer and pass that. Same happens with the client, it's a WeakPtr but we are not checking it's still valid before using it. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDtlsTransportBackend.cpp:68 > + callOnMainThreadAndWait([this] { > + if (!m_client) > + return; What ensures this is till alive when the lambda is called? You should make weak ref to this, I think > Source/WebCore/Modules/mediastream/gstreamer/GStreamerIceTransportBackend.cpp:82 > + if (!protectedThis->m_client) > + return; You should check first if (protectedThis). In this case protected is not a good name, because WeakPtr doesn't protect this from being destroyed, use weakThis instead. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerIceTransportBackend.cpp:102 > + if (!protectedThis->m_client) Same here. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerIceTransportBackend.cpp:116 > + if (!protectedThis->m_client) And here. >>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:402 >>> + }); >> >> Can't we add GstPromise to GRefPtrGStreamer? > > Well yeah I thought about it I think, but wouldn't that be awkward because the change_func obviously passes a raw pointer, so what would we do here? Adopt it >>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:1056 >>> + callOnMainThread([protectedThis = Ref(*this), description = description.release()] { >> >> Leaking the description? Maybe move the smart ptr inside? Besides, I recall GUniqueOutPtrs should be used just locally. > > Right, need to revisit this, passing the OutPtr around is not great Yes, I would convert the GUniqueOutPtr to GUniquePtr and make this function receive a GUnqiuePtr instead. >>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerRtpSenderBackend.h:59 >>> + return WTF::switchOn(m_source, >> >> For this easy variant handlings I prefer to use holds_alternative + get > > I didn't know about this. Generally I used the same patterns as in the libwebrtc backend :) I like switchOn, but I guess it's a matter of taste, no strong opinion anyway. >>> Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingMediaSourceGStreamer.cpp:144 >>> + g_object_get(transceiver.get(), "sender", &m_sender.outPtr(), nullptr); >> >> Can this be called more than once? outPtr is not going to release an outPtr for you and it will ASSERT if there was one already. > > No, it's meant to be called once, maybe I can rework this a bit though.... Maybe we should change that for consistency with GUniqueOtrPtr() that calls reset() on outPtr() to be able to use it multiple times safely.

Comment on attachment 451816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451816&action=review >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerPeerConnectionBackend.cpp:51 >> + WTFLogAlways("GstWebRTC plugin not found. Make sure to install gst-plugins-bad >= 1.20 with the webrtc plugin enabled."); > > Maybe ASSERT_NOT_REACHED as well? Why? It's entirely possible to have the gstwebrtc lib available at runtime, but not webrtcbin. I agree it's unlikely, but I'm not sure it warrants an ASSERT :)

Created attachment 452541 [details] Patch

Created attachment 452544 [details] Patch

Created attachment 452632 [details] Patch

Created attachment 452634 [details] Patch

Comment on attachment 452634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452634&action=review > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:93 > + auto channel = RTCDataChannel::create(document, WTFMove(handler), String(label.get()), WTFMove(init)); String::fromUTF8() > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:107 > + auto bytes = adoptGRef(rawBytes); > + handler->onMessageData(WTFMove(bytes)); Is the GBytes ownership transferred? I guess it's not, so we shouldn't adoptGRef. I would not even create a smart pointer since we don't own it and this is sync call from message callback, just pass the pointer to onMessageData() and if there we need to take a ref take it there when needed. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:109 > + g_signal_connect_swapped(m_channel.get(), "on-message-string", G_CALLBACK(+[](GStreamerDataChannelHandler* handler, char* message) { const char* message > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:112 > + CString buffer(message, strlen(message)); > + auto string = String::fromUTF8(buffer); > + handler->onMessageString(WTFMove(string)); I don't think you need the CString this could be just: handler->onMessageString(String::fromUTF8(message)); However, I would pass the char* here for the same reasons as GBytes > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:116 > + GUniquePtr<GError> error(rawError); > + handler->onError(WTFMove(error)); And the same here, we are taking the ownership of a GError, but I don't think it's transferred. Let's just pass the GError

Comment on attachment 452634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452634&action=review >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:107 >> + handler->onMessageData(WTFMove(bytes)); > > Is the GBytes ownership transferred? I guess it's not, so we shouldn't adoptGRef. I would not even create a smart pointer since we don't own it and this is sync call from message callback, just pass the pointer to onMessageData() and if there we need to take a ref take it there when needed. Ah yeah it's not transferred. Maybe that's why the first version of this patch wasn't using smart ptrs :)

Comment on attachment 452634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452634&action=review > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.h:50 > +protected: Why protected if this class is final? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.h:77 > + std::optional<WeakPtr<RTCDataChannelHandlerClient>> m_client WTF_GUARDED_BY_LOCK(m_clientLock); > + bool m_hasClient WTF_GUARDED_BY_LOCK(m_clientLock) { false }; Why do we need m_hasClient if m_client is optional? nullopt -> no client, nullptr -> has a null client > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.h:80 > + PendingMessages m_bufferedMessages WTF_GUARDED_BY_LOCK(m_clientLock); m_pendingMessages? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDtlsTransportBackend.cpp:64 > +void GStreamerDtlsTransportBackend::stateChanged() Could this be const? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDtlsTransportBackend.cpp:70 > + if (!weakThis->m_client) > + return; Here you are checking m_client is not nullopt, but it can be nullptr, since it's a WeakPtr. I wonder why it's optional here, we only care if we have a valid client or not. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDtlsTransportBackend.cpp:83 > + certificates.append(WTFMove(jsRemoteCertificate)); unchedkedAppend() since you reserved the capacity > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDtlsTransportBackend.cpp:87 > + certificates.append(WTFMove(jsCertificate)); Ditto. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDtlsTransportBackend.h:38 > + void stateChanged(); Why is this public? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerIceTransportBackend.cpp:85 > + // We start observing a bit late and might miss the checking state. Synthesize it as needed. > + if (transportState > GST_WEBRTC_ICE_CONNECTION_STATE_CHECKING && transportState != GST_WEBRTC_ICE_CONNECTION_STATE_CLOSED) { > + callOnMainThread([weakThis = WeakPtr { *this }] { > + if (!weakThis || !weakThis->m_client) > + return; > + weakThis->m_client->onStateChanged(RTCIceTransportState::Checking); > + }); > + } > + callOnMainThread([weakThis = WeakPtr { *this }, transportState, gatheringState] { > + if (!weakThis || !weakThis->m_client) > + return; > + weakThis->m_client->onStateChanged(toRTCIceTransportState(transportState)); > + weakThis->m_client->onGatheringStateChanged(toRTCIceGatheringState(gatheringState)); Do we really need two calls to callOnMainThread? Can we just move the if to the lambda and synthetize the Checking if needed there? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerIceTransportBackend.cpp:95 > +void GStreamerIceTransportBackend::stateChanged() Could this be const? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerIceTransportBackend.cpp:109 > +void GStreamerIceTransportBackend::gatheringStateChanged() Ditto > Source/WebCore/Modules/mediastream/gstreamer/GStreamerIceTransportBackend.h:38 > + void stateChanged(); > + void gatheringStateChanged(); Why are these public? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:925 > + auto init = GStreamerDataChannelHandler::fromRTCDataChannelInit(options); > + GST_DEBUG_OBJECT(m_pipeline.get(), "Creating data channel for init options %" GST_PTR_FORMAT, init.get()); > + GRefPtr<GstWebRTCDataChannel> channel; > + g_signal_emit_by_name(m_webrtcBin.get(), "create-data-channel", label.utf8().data(), init.get(), &channel.outPtr()); > + if (!channel) > + return nullptr; GStreamerDataChannelHandler::fromRTCDataChannelInit is a bit confusing, could we add something like: GRefPtr<GstWebRTCDataChannel> GstWebRTCDataChannel::create(GstElement* webrtcBin, const String& label, const RTCDataChannelInit& init); And move all this code to the create including the converstion from RTCDataChannelInit to GstStructure

Comment on attachment 452634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452634&action=review > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:74 > +Ref<RTCDataChannelEvent> GStreamerDataChannelHandler::channelEvent(Document& document, GRefPtr<GstWebRTCDataChannel>&& dataChannel) This sounds like a getter too, I would just calls this createEvent() > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:81 > + static int nPipeline = 0; Could this be unsigned or even uint32_t? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:116 > + protectedThis->addRemoteStream(pad); Why this needs to run in the main thread and all others don't? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:143 > +void GStreamerMediaEndpoint::teardownPipeline() This is expected to be called with a valid m_pipeline, so I would either add an early return here and remove the check from the callers, or add an assert here to ensure callers do the check. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:150 > + g_signal_handlers_disconnect_by_data(m_pipeline.get(), this); Should this be m_webrtcBin.get() instead of m_pipeline.get()? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:190 > + gst_element_set_locked_state(m_pipeline.get(), true); TRUE > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:195 > + gst_element_set_locked_state(m_pipeline.get(), false); FALSE > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:637 > + g_signal_emit_by_name(m_webrtcBin.get(), signalName.ascii().data(), options.release(), gst_promise_new_with_change_func([](GstPromise* rawPromise, gpointer userData) { Since we are not taking the ownership of the options, I think it's better to receive a raw pointer.

Comment on attachment 452634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452634&action=review >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:116 >> + protectedThis->addRemoteStream(pad); > > Why this needs to run in the main thread and all others don't? Because otherwise RealtimeMediaSource::forEachObserver() ASSERTs in Debug builds.

Created attachment 453288 [details] Patch

Comment on attachment 453288 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453288&action=review > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.h:54 > + void onMessageData(GBytes*); > + void onMessageString(const char*); > + void onError(GError*); > + void onBufferedAmountLow(); > + void readyStateChanged(); I think these could be private too. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:372 > + GStreamerMediaEndpoint& endPoint; Shouldn't this be a Ref<>? When the gst promise change func is called, the endPoint might be destriyed already, no? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:401 > + std::unique_ptr<SetDescriptionCallData> data(reinterpret_cast<SetDescriptionCallData*>(userData)); I think static_cast works here. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:411 > + callOnMainThread([error = error.get(), protectedThis = Ref(data->endPoint), failureCallback = WTFMove(data->failureCallback)] { error needs to be copied here. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:640 > + auto* endpoint = reinterpret_cast<GStreamerMediaEndpoint*>(userData); Can endPoint be destroyed at this point? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:929 > + auto init = GStreamerDataChannelHandler::fromRTCDataChannelInit(options); > + GST_DEBUG_OBJECT(m_pipeline.get(), "Creating data channel for init options %" GST_PTR_FORMAT, init.get()); > + GRefPtr<GstWebRTCDataChannel> channel; > + g_signal_emit_by_name(m_webrtcBin.get(), "create-data-channel", label.utf8().data(), init.get(), &channel.outPtr()); > + if (!channel) > + return nullptr; > + > + return WTF::makeUnique<GStreamerDataChannelHandler>(WTFMove(channel)); GStreamerDataChannelHandler::fromRTCDataChannelInit is a bit confusing, could we add something like: GRefPtr<GstWebRTCDataChannel> GstWebRTCDataChannel::create(GstElement* webrtcBin, const String& label, const RTCDataChannelInit& init); And move all this code to the create including the converstion from RTCDataChannelInit to GstStructure > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:1061 > + if (protectedThis->isStopped()) > + return; > + > + GUniquePtr<char> sdp(gst_sdp_message_as_text(description->sdp)); > + if (protectedThis->m_isInitiator) > + protectedThis->m_peerConnectionBackend.createOfferSucceeded(sdp.get()); > + else > + protectedThis->m_peerConnectionBackend.createAnswerSucceeded(sdp.get()); In general, I like capturing both the protectedThis and this, so that you don't have to use protectedfThis-> everywhere. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:1100 > + if (!receiver) > + continue; > + > + if (!mid) > + continue; if (!receiver || !mid) ? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:1120 > + auto* endPoint = reinterpret_cast<GStreamerMediaEndpoint*>(userData); Can endPoint be destroyed already at this point? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerRtpTransceiverBackend.cpp:40 > + g_object_set(m_rtcTransceiver.get(), "do-nack", true, "fec-type", GST_WEBRTC_FEC_TYPE_ULP_RED, nullptr); TRUE > Source/WebCore/Modules/mediastream/gstreamer/GStreamerSctpTransportBackend.cpp:87 > + callOnMainThread([this, transportState, maxChannels, maxMessageSize] { Should we pass a WeakPtr for this? Or not capture this, but m_client which is already a WeakPtr > Source/WebCore/Modules/mediastream/gstreamer/GStreamerStatsCollector.cpp:146 > + // FIXME Add a comment here to know what's left > Source/WebCore/Modules/mediastream/gstreamer/GStreamerStatsCollector.cpp:228 > + CallbackHolder(GStreamerStatsCollector::CollectorCallback&& collectorCallback) explicit > Source/WebCore/Modules/mediastream/gstreamer/GStreamerStatsCollector.cpp:263 > + }, holder.release(), nullptr); I wonder if if would be clearer to pass a destructor for user_data instead of release and adopt in the callback. Is the change func ensured to be always called before the promise is freed? We have WEBKIT_DEFINE_ASYNC_DATA_STRUCT macro in WTFGType.h for this use case, which provides create and free functions

Comment on attachment 453288 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453288&action=review >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:929 >> + return WTF::makeUnique<GStreamerDataChannelHandler>(WTFMove(channel)); > > GStreamerDataChannelHandler::fromRTCDataChannelInit is a bit confusing, could we add something like: > > GRefPtr<GstWebRTCDataChannel> GstWebRTCDataChannel::create(GstElement* webrtcBin, const String& label, const RTCDataChannelInit& init); > > And move all this code to the create including the converstion from RTCDataChannelInit to GstStructure I used the same pattern as in the libwebrtc backend. I'm not sure introducing too many changes would help much with future maintenance of both backends :( >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerStatsCollector.cpp:263 >> + }, holder.release(), nullptr); > > I wonder if if would be clearer to pass a destructor for user_data instead of release and adopt in the callback. Is the change func ensured to be always called before the promise is freed? We have WEBKIT_DEFINE_ASYNC_DATA_STRUCT macro in WTFGType.h for this use case, which provides create and free functions Yes, the change func is called from gst_promise_free().

(In reply to Philippe Normand from comment #23) > Comment on attachment 453288 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453288&action=review > > >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:929 > >> + return WTF::makeUnique<GStreamerDataChannelHandler>(WTFMove(channel)); > > > > GStreamerDataChannelHandler::fromRTCDataChannelInit is a bit confusing, could we add something like: > > > > GRefPtr<GstWebRTCDataChannel> GstWebRTCDataChannel::create(GstElement* webrtcBin, const String& label, const RTCDataChannelInit& init); > > > > And move all this code to the create including the converstion from RTCDataChannelInit to GstStructure > > I used the same pattern as in the libwebrtc backend. I'm not sure > introducing too many changes would help much with future maintenance of both > backends :( Ok. > >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerStatsCollector.cpp:263 > >> + }, holder.release(), nullptr); > > > > I wonder if if would be clearer to pass a destructor for user_data instead of release and adopt in the callback. Is the change func ensured to be always called before the promise is freed? We have WEBKIT_DEFINE_ASYNC_DATA_STRUCT macro in WTFGType.h for this use case, which provides create and free functions > > Yes, the change func is called from gst_promise_free(). I see the change notify being called there, but no the change func.

Comment on attachment 453288 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453288&action=review >>>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerStatsCollector.cpp:263 >>>> + }, holder.release(), nullptr); >>> >>> I wonder if if would be clearer to pass a destructor for user_data instead of release and adopt in the callback. Is the change func ensured to be always called before the promise is freed? We have WEBKIT_DEFINE_ASYNC_DATA_STRUCT macro in WTFGType.h for this use case, which provides create and free functions >> >> Yes, the change func is called from gst_promise_free(). > > I see the change notify being called there, but no the change func. Gosh yes, sorry I got confused there... But anyway, why were you asking this? I have trouble understanding the point here.

(In reply to Philippe Normand from comment #25) > Comment on attachment 453288 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453288&action=review > > >>>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerStatsCollector.cpp:263 > >>>> + }, holder.release(), nullptr); > >>> > >>> I wonder if if would be clearer to pass a destructor for user_data instead of release and adopt in the callback. Is the change func ensured to be always called before the promise is freed? We have WEBKIT_DEFINE_ASYNC_DATA_STRUCT macro in WTFGType.h for this use case, which provides create and free functions > >> > >> Yes, the change func is called from gst_promise_free(). > > > > I see the change notify being called there, but no the change func. > > Gosh yes, sorry I got confused there... But anyway, why were you asking > this? I have trouble understanding the point here. It looks weird to me to free the user data in the update func, when there's a destroy notify func for that, and we are sure it will be called when the data is no longer needed.

Created attachment 453930 [details] Patch

Comment on attachment 453930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453930&action=review > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:192 > + return gst_structure_get_string(structure, "media"); I had to change this line, otherwise I got crashes in tests, like if (gst_structure_has_field(structure, "media")) return gst_structure_get_string(structure, "media") return "unknown-rtp-media-type" The crash was in strlen-avx2.s:65 because it receives NULL

Comment on attachment 453930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453930&action=review >> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:192 >> + return gst_structure_get_string(structure, "media"); > > I had to change this line, otherwise I got crashes in tests, like > > if (gst_structure_has_field(structure, "media")) > return gst_structure_get_string(structure, "media") > return "unknown-rtp-media-type" > > The crash was in strlen-avx2.s:65 because it receives NULL That's with gst main branch, right? I also see this here, I suspect it's some new regression but I haven't debugged yet (help is welcome). With 1.20 from the SDK you shouldn't see a crash, the EWS didn't report unexpected crashes either.

(In reply to Philippe Normand from comment #29) > > That's with gst main branch, right? I also see this here, I suspect it's > some new regression but I haven't debugged yet (help is welcome). With 1.20 > from the SDK you shouldn't see a crash, the EWS didn't report unexpected > crashes either. bisected: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1762

I was wondering if this is really a regression or if we're supposed to be notified of the webrtcbin src pad caps differently now...

Comment on attachment 453930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453930&action=review > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:706 > + auto caps = adoptGRef(gst_pad_query_caps(pad, nullptr)); Ok. I found out how to deal with the issue by looking at the example: auto caps = adoptGRef(gst_pad_get_current_caps(pad)); if (!caps) caps = adoptGRef(gst_pad_query_caps(pad, nullptr)); Iiuc, it should avoid another pad query.

(In reply to Víctor M. Jáquez L. from comment #32) > Comment on attachment 453930 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453930&action=review > > > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:706 > > + auto caps = adoptGRef(gst_pad_query_caps(pad, nullptr)); > > Ok. I found out how to deal with the issue by looking at the example: > > auto caps = adoptGRef(gst_pad_get_current_caps(pad)); > if (!caps) > caps = adoptGRef(gst_pad_query_caps(pad, nullptr)); > > Iiuc, it should avoid another pad query. Yeah, right. But still answering the query with incomplete RTP caps seems a bit odd.

Created attachment 454531 [details] Patch

Comment on attachment 454531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454531&action=review There are a couple of memory issues in this patch that need to be addressed. After this, I'll give a lgtm and I think Carlos should give the r+ as he did most of this review iterations. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:140 > + if (stateChange.error) { Question, is letting this out of here to change the ready state on error legit or should we return? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.h:54 > + bool sendStringData(const CString&) final; > + bool sendRawData(const uint8_t*, size_t) final; Not strong opinion, you could rename this to sendData and trust the language to do the overload for you. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.h:58 > + void onMessageData(GBytes*); > + void onMessageString(const char*); Ditto, onMessage. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerIceTransportBackend.cpp:54 > + g_object_get(m_backend.get(), "transport", &m_iceTransport.outPtr(), nullptr); I guess there is a possibility that m_iceTransport has a value. Calling outPtr() will get it overwritten without releasing the former value, hence leaking. Please issue a .clear() before this. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:226 > + bool result = false; gboolean? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:412 > + errorHolder.emplace(g_error_copy(error.get())); I would try something like: errorHolder = GUniquePtr<GError>(error.release()); That way you transform GUniqueOutPtr into GUniquePtr, then the r-value logic should put that into the optional without copying the error. In fact, it would be nice to have something like releaseIntoGUniquePtr in GUniqueOutPtr to avoid this convolution. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:545 > + RELEASE_ASSERT(!gst_pad_is_linked(sinkPad.get())); Wouldn't it be better in this case to let it fail with just an ASSERT and leave the assertion for debug mode? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:665 > + GUniquePtr<GError> error(promiseError.release()); > + data->endPoint->createSessionDescriptionFailed(WTFMove(error)); See? Nicely done here. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:756 > + RELEASE_ASSERT_NOT_REACHED(); Just ASSERT? > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:837 > + GValue value = G_VALUE_INIT; > + g_value_init(&value, G_TYPE_STRING); > + g_value_set_string(&value, stream->id().utf8().data()); > + gst_value_list_append_value(&streamIdsValue, &value); > + g_value_unset(&value); It might be interesting to create the GValue in the heap and use gst_value_list_append_and_take_value instead. That way you don't need to unset later and you can save some copy. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:839 > + gst_structure_take_value(initData.get(), "stream-ids", &streamIdsValue); This seems to be wrong, since you are telling the structure to take that GValue*, which is stack memory. I hope I didn't miss this pattern anywhere else, please ensure it. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:846 > + GValue value = G_VALUE_INIT; Ditto > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:855 > + GValue value = G_VALUE_INIT; Ditto > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:862 > + gst_structure_take_value(initData.get(), "encodings", &encodingsValue); Ditto. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:1144 > + std::unique_ptr<GStreamerMediaEndpointHolder> data(static_cast<GStreamerMediaEndpointHolder*>(userData)); You need to do this at the beginning of the function or you'll be leaking the userData. Another thing is that this will only work once because the user data will be destroyed the second time it gets called or you'll be leaking if the signal is never emitted. I think the best is passing the GDestroyNotify. I hope I did not miss this pattern anywhere else, please ensure it. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerWebRTCUtils.cpp:406 > + Vector<uint8_t> buffer; > + buffer.reserveInitialCapacity(8); > + WTF::cryptographicallyRandomValues(buffer.data(), 8); I don't think this is ok. After you do this, the data is populated and it uses legit memory but the size is not updated, only the capacity, which is what you do when you reserve it initially. explicit Vector(size_t size) might be what you need. This said, since you don't use the size, just as a holder for data that does not need anything special (like calling destructors later), it won't cause trouble. > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingMediaSourceGStreamer.cpp:40 > + Extra line > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingMediaSourceGStreamer.cpp:148 > + g_object_get(transceiver.get(), "sender", &m_sender.outPtr(), nullptr); I guess there is a possibility that m_sender has a value. Calling outPtr() will get it overwritten without releasing the former value, hence leaking. Please issue a .clear() before this.

Comment on attachment 454531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454531&action=review >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerIceTransportBackend.cpp:54 >> + g_object_get(m_backend.get(), "transport", &m_iceTransport.outPtr(), nullptr); > > I guess there is a possibility that m_iceTransport has a value. Calling outPtr() will get it overwritten without releasing the former value, hence leaking. Please issue a .clear() before this. Yes, it's indeed disconnecting signals from previous one. I think we could call clear() in outPtr(), we do that in GUniqueOutPtr >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:226 >> + bool result = false; > > gboolean? Yes! >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:412 >> + errorHolder.emplace(g_error_copy(error.get())); > > I would try something like: > > errorHolder = GUniquePtr<GError>(error.release()); > > That way you transform GUniqueOutPtr into GUniquePtr, then the r-value logic should put that into the optional without copying the error. > > In fact, it would be nice to have something like releaseIntoGUniquePtr in GUniqueOutPtr to avoid this convolution. When I implemented GUniqueOutPtr, release() returned a GUniquePtr, and it was convenient for these cases, but then we changed to return a raw pointer in bug #199579 because it was not convenient when we want to release the raw pointer. I think GUniquePtr<Foo>(foo.release()) is easy enough and explicit >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:665 >> + data->endPoint->createSessionDescriptionFailed(WTFMove(error)); > > See? Nicely done here. We don't need the local variable + move, we can just pass promiseError.release() I think >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:839 >> + gst_structure_take_value(initData.get(), "stream-ids", &streamIdsValue); > > This seems to be wrong, since you are telling the structure to take that GValue*, which is stack memory. > > I hope I didn't miss this pattern anywhere else, please ensure it. I think this is ok, the GValue struct is copied, what it takes is the string, not the GValue pointer >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:1144 >> + std::unique_ptr<GStreamerMediaEndpointHolder> data(static_cast<GStreamerMediaEndpointHolder*>(userData)); > > You need to do this at the beginning of the function or you'll be leaking the userData. > > Another thing is that this will only work once because the user data will be destroyed the second time it gets called or you'll be leaking if the signal is never emitted. I think the best is passing the GDestroyNotify. > > I hope I did not miss this pattern anywhere else, please ensure it. I agree, I suggested to use the GDestroyNotify too

Comment on attachment 454531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454531&action=review >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:140 >> + if (stateChange.error) { > > Question, is letting this out of here to change the ready state on error legit or should we return? I can't parse this question :) Can you re-phrase? >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.h:54 >> + bool sendRawData(const uint8_t*, size_t) final; > > Not strong opinion, you could rename this to sendData and trust the language to do the overload for you. I'd rather not change the RTCDataChannelHandler API which is used by the libwebrtc backend too. >>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:1144 >>> + std::unique_ptr<GStreamerMediaEndpointHolder> data(static_cast<GStreamerMediaEndpointHolder*>(userData)); >> >> You need to do this at the beginning of the function or you'll be leaking the userData. >> >> Another thing is that this will only work once because the user data will be destroyed the second time it gets called or you'll be leaking if the signal is never emitted. I think the best is passing the GDestroyNotify. >> >> I hope I did not miss this pattern anywhere else, please ensure it. > > I agree, I suggested to use the GDestroyNotify too I already tried the GDestroyNotify approach and IIRC it was triggering crashes in tests, but sure, I can try again.

Comment on attachment 454531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454531&action=review > Source/cmake/GStreamerChecks.cmake:75 > + find_package(OpenSSL REQUIRED) Instead of REQUIRED we should check FOUND and show a fatal message saying that OpenSSL is required when building with USE_GSTREAMER_WEBRTC

Comment on attachment 454531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454531&action=review >>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:140 >>> + if (stateChange.error) { >> >> Question, is letting this out of here to change the ready state on error legit or should we return? > > I can't parse this question :) Can you re-phrase? My question is that if letting the error if fall through to dispatching the state change is ok or you forgot to return. >>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerIceTransportBackend.cpp:54 >>> + g_object_get(m_backend.get(), "transport", &m_iceTransport.outPtr(), nullptr); >> >> I guess there is a possibility that m_iceTransport has a value. Calling outPtr() will get it overwritten without releasing the former value, hence leaking. Please issue a .clear() before this. > > Yes, it's indeed disconnecting signals from previous one. I think we could call clear() in outPtr(), we do that in GUniqueOutPtr So we should do call clear before landing this then. >>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:665 >>> + data->endPoint->createSessionDescriptionFailed(WTFMove(error)); >> >> See? Nicely done here. > > We don't need the local variable + move, we can just pass promiseError.release() I think Carlos is right, yes. >>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:839 >>> + gst_structure_take_value(initData.get(), "stream-ids", &streamIdsValue); >> >> This seems to be wrong, since you are telling the structure to take that GValue*, which is stack memory. >> >> I hope I didn't miss this pattern anywhere else, please ensure it. > > I think this is ok, the GValue struct is copied, what it takes is the string, not the GValue pointer Gosh, Carlos, you're right. You need to actually check the code to see it because the doc is deceiving: Sets the field with the given name field to value . If the field does not exist, it is created. If the field exists, the previous value is replaced and freed. The function will take ownership of value . Parameters structure a GstStructure fieldname the name of the field to set value the new value of the field. [transfer full] I will have to check some EME code where I do what I ask here, which means that I am leaking. >> Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingMediaSourceGStreamer.cpp:148 >> + g_object_get(transceiver.get(), "sender", &m_sender.outPtr(), nullptr); > > I guess there is a possibility that m_sender has a value. Calling outPtr() will get it overwritten without releasing the former value, hence leaking. Please issue a .clear() before this. If we call clear in GRefPtr, then this won't be needed. And IIRC, there are other places where clear is called before outPtr so we could avoid the clear in those cases as well.

Comment on attachment 454531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454531&action=review >>>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:839 >>>> + gst_structure_take_value(initData.get(), "stream-ids", &streamIdsValue); >>> >>> This seems to be wrong, since you are telling the structure to take that GValue*, which is stack memory. >>> >>> I hope I didn't miss this pattern anywhere else, please ensure it. >> >> I think this is ok, the GValue struct is copied, what it takes is the string, not the GValue pointer > > Gosh, Carlos, you're right. You need to actually check the code to see it because the doc is deceiving: > > Sets the field with the given name field to value . If the field does not exist, it is created. If the field exists, the previous value is replaced and freed. The function will take ownership of value . > > Parameters > > structure > a GstStructure > > fieldname > the name of the field to set > > value > the new value of the field. > [transfer full] > > I will have to check some EME code where I do what I ask here, which means that I am leaking. Fortunately, I found no other code with this mistake, which is good as we are not leaking anywhere else.

Comment on attachment 454531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454531&action=review >>>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:140 >>>> + if (stateChange.error) { >>> >>> Question, is letting this out of here to change the ready state on error legit or should we return? >> >> I can't parse this question :) Can you re-phrase? > > My question is that if letting the error if fall through to dispatching the state change is ok or you forgot to return. It is ok as it is. >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:545 >> + RELEASE_ASSERT(!gst_pad_is_linked(sinkPad.get())); > > Wouldn't it be better in this case to let it fail with just an ASSERT and leave the assertion for debug mode? I don't understand, why is a RELEASE_ASSERT a problem here? >>>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:665 >>>> + data->endPoint->createSessionDescriptionFailed(WTFMove(error)); >>> >>> See? Nicely done here. >> >> We don't need the local variable + move, we can just pass promiseError.release() I think > > Carlos is right, yes. My compiler disagrees though: /app/webkit/Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:664:60: error: reference to type 'GUniquePtr<GError>' (aka 'unique_ptr<_GError, GPtrDeleter<_GError>>') could not bind to an rvalue of type '_GError *' data->endPoint->createSessionDescriptionFailed(promiseError.release()); ^~~~~~~~~~~~~~~~~~~~~~ /app/webkit/Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.h:96:61: note: passing argument to parameter here void createSessionDescriptionFailed(GUniquePtr<GError>&&); ^ 1 error generated.

(In reply to Philippe Normand from comment #41) > Comment on attachment 454531 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=454531&action=review > > >>>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:140 > >>>> + if (stateChange.error) { > >>> > >>> Question, is letting this out of here to change the ready state on error legit or should we return? > >> > >> I can't parse this question :) Can you re-phrase? > > > > My question is that if letting the error if fall through to dispatching the state change is ok or you forgot to return. > > It is ok as it is. > > >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:545 > >> + RELEASE_ASSERT(!gst_pad_is_linked(sinkPad.get())); > > > > Wouldn't it be better in this case to let it fail with just an ASSERT and leave the assertion for debug mode? > > I don't understand, why is a RELEASE_ASSERT a problem here? > > >>>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:665 > >>>> + data->endPoint->createSessionDescriptionFailed(WTFMove(error)); > >>> > >>> See? Nicely done here. > >> > >> We don't need the local variable + move, we can just pass promiseError.release() I think > > > > Carlos is right, yes. > > My compiler disagrees though: > /app/webkit/Source/WebCore/Modules/mediastream/gstreamer/ > GStreamerMediaEndpoint.cpp:664:60: error: reference to type > 'GUniquePtr<GError>' (aka 'unique_ptr<_GError, GPtrDeleter<_GError>>') could > not bind to an rvalue of type '_GError *' > > data->endPoint->createSessionDescriptionFailed(promiseError.release()); > > ^~~~~~~~~~~~~~~~~~~~~~ > /app/webkit/Source/WebCore/Modules/mediastream/gstreamer/ > GStreamerMediaEndpoint.h:96:61: note: passing argument to parameter here > void createSessionDescriptionFailed(GUniquePtr<GError>&&); > ^ > 1 error generated. Pass GUniquePtr<GError>(promiseError.release()) then

Comment on attachment 454531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454531&action=review >>>>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:665 >>>>> + data->endPoint->createSessionDescriptionFailed(WTFMove(error)); >>>> >>>> See? Nicely done here. >>> >>> We don't need the local variable + move, we can just pass promiseError.release() I think >> >> Carlos is right, yes. > > My compiler disagrees though: > /app/webkit/Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:664:60: error: reference to type 'GUniquePtr<GError>' (aka 'unique_ptr<_GError, GPtrDeleter<_GError>>') could not bind to an rvalue of type '_GError *' > data->endPoint->createSessionDescriptionFailed(promiseError.release()); > ^~~~~~~~~~~~~~~~~~~~~~ > /app/webkit/Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.h:96:61: note: passing argument to parameter here > void createSessionDescriptionFailed(GUniquePtr<GError>&&); > ^ > 1 error generated. This is because promiseError.release() returns a GError* here. Why can't you call `data->endPoint->createSessionDescriptionFailed(WTFMove(promiseError))` and get rid of the error local variable?

Comment on attachment 454531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454531&action=review >>>>>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:665 >>>>>> + data->endPoint->createSessionDescriptionFailed(WTFMove(error)); >>>>> >>>>> See? Nicely done here. >>>> >>>> We don't need the local variable + move, we can just pass promiseError.release() I think >>> >>> Carlos is right, yes. >> >> My compiler disagrees though: >> /app/webkit/Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:664:60: error: reference to type 'GUniquePtr<GError>' (aka 'unique_ptr<_GError, GPtrDeleter<_GError>>') could not bind to an rvalue of type '_GError *' >> data->endPoint->createSessionDescriptionFailed(promiseError.release()); >> ^~~~~~~~~~~~~~~~~~~~~~ >> /app/webkit/Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.h:96:61: note: passing argument to parameter here >> void createSessionDescriptionFailed(GUniquePtr<GError>&&); >> ^ >> 1 error generated. > > This is because promiseError.release() returns a GError* here. > > Why can't you call `data->endPoint->createSessionDescriptionFailed(WTFMove(promiseError))` and get rid of the error local variable? /app/webkit/Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:665:60: error: no viable conversion from 'typename remove_reference<GUniqueOutPtr<_GError> &>::type' (aka 'WTF::GUniqueOutPtr<_GError>') to 'GUniquePtr<GError>' (aka 'unique_ptr<_GError, GPtrDeleter<_GError>>') data->endPoint->createSessionDescriptionFailed(std::move<WTF::CheckMoveParameter>(promiseError)); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/lib/gcc/x86_64-unknown-linux-gnu/11.2.0/../../../../include/c++/11.2.0/bits/unique_ptr.h:320:12: note: candidate constructor template not viable: no known conversion from 'typename remove_reference<GUniqueOutPtr<_GError> &>::type' (aka 'WTF::GUniqueOutPtr<_GError>') to 'std::nullptr_t' (aka 'nullptr_t') for 1st argument constexpr unique_ptr(nullptr_t) noexcept ^ /usr/lib/gcc/x86_64-unknown-linux-gnu/11.2.0/../../../../include/c++/11.2.0/bits/unique_ptr.h:327:7: note: candidate constructor not viable: no known conversion from 'typename remove_reference<GUniqueOutPtr<_GError> &>::type' (aka 'WTF::GUniqueOutPtr<_GError>') to 'std::unique_ptr<_GError, WTF::GPtrDeleter<GError>> &&' for 1st argument unique_ptr(unique_ptr&&) = default; ^ /usr/lib/gcc/x86_64-unknown-linux-gnu/11.2.0/../../../../include/c++/11.2.0/bits/unique_ptr.h:468:7: note: candidate constructor not viable: no known conversion from 'typename remove_reference<GUniqueOutPtr<_GError> &>::type' (aka 'WTF::GUniqueOutPtr<_GError>') to 'const std::unique_ptr<_GError, WTF::GPtrDeleter<GError>> &' for 1st argument unique_ptr(const unique_ptr&) = delete; ^ /usr/lib/gcc/x86_64-unknown-linux-gnu/11.2.0/../../../../include/c++/11.2.0/bits/unique_ptr.h:340:2: note: candidate template ignored: could not match 'unique_ptr' against 'GUniqueOutPtr' unique_ptr(unique_ptr<_Up, _Ep>&& __u) noexcept ^ /usr/lib/gcc/x86_64-unknown-linux-gnu/11.2.0/../../../../include/c++/11.2.0/bits/unique_ptr.h:350:2: note: candidate template ignored: could not match 'auto_ptr' against 'GUniqueOutPtr' unique_ptr(auto_ptr<_Up>&& __u) noexcept; ^ /usr/lib/gcc/x86_64-unknown-linux-gnu/11.2.0/../../../../include/c++/11.2.0/bits/unique_ptr.h:281:2: note: explicit constructor is not a candidate unique_ptr(pointer __p) noexcept ^ /app/webkit/WebKitBuild/Release/WTF/Headers/wtf/glib/GUniquePtr.h:107:5: note: candidate function operator UnspecifiedBoolType() const { return m_ptr ? &GUniqueOutPtr::m_ptr : 0; } ^ /app/webkit/Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.h:96:61: note: passing argument to parameter here void createSessionDescriptionFailed(GUniquePtr<GError>&&); ^ 1 error generated.

Comment on attachment 454531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454531&action=review >>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:545 >>> + RELEASE_ASSERT(!gst_pad_is_linked(sinkPad.get())); >> >> Wouldn't it be better in this case to let it fail with just an ASSERT and leave the assertion for debug mode? > > I don't understand, why is a RELEASE_ASSERT a problem here? In cases like this, IMHO, it is ok to crash in debug mode but for release I'd prefer just a malfunction than a crash because of user experience.

Comment on attachment 454531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454531&action=review >> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:837 >> + g_value_unset(&value); > > It might be interesting to create the GValue in the heap and use gst_value_list_append_and_take_value instead. That way you don't need to unset later and you can save some copy. I'm not sure what you mean here. I used the usual pattern for creating and initializing GValues :)

Comment on attachment 454531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454531&action=review >>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:837 >>> + g_value_unset(&value); >> >> It might be interesting to create the GValue in the heap and use gst_value_list_append_and_take_value instead. That way you don't need to unset later and you can save some copy. > > I'm not sure what you mean here. I used the usual pattern for creating and initializing GValues :) I mean nothing, forget it. This would be tide to the comments below and it is clear that I misunderstood the GValue doc (or better said, GValue doc could be seriously improved).

Created attachment 454981 [details] Patch

(In reply to Chris Dumez from comment #43) > Comment on attachment 454531 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=454531&action=review > > >>>>> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:665 > >>>>> + data->endPoint->createSessionDescriptionFailed(WTFMove(error)); > >>>> > >>>> See? Nicely done here. > >>> > >>> We don't need the local variable + move, we can just pass promiseError.release() I think > >> > >> Carlos is right, yes. > > > > My compiler disagrees though: > > /app/webkit/Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:664:60: error: reference to type 'GUniquePtr<GError>' (aka 'unique_ptr<_GError, GPtrDeleter<_GError>>') could not bind to an rvalue of type '_GError *' > > data->endPoint->createSessionDescriptionFailed(promiseError.release()); > > ^~~~~~~~~~~~~~~~~~~~~~ > > /app/webkit/Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.h:96:61: note: passing argument to parameter here > > void createSessionDescriptionFailed(GUniquePtr<GError>&&); > > ^ > > 1 error generated. > > This is because promiseError.release() returns a GError* here. > > Why can't you call > `data->endPoint->createSessionDescriptionFailed(WTFMove(promiseError))` and > get rid of the error local variable? Because promiseError is a GUniqueOutPtr and createSessionDescriptionFailed expects a GUniquePtr

Comment on attachment 454981 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454981&action=review Other than the GDestroyNotify, it LGTM. I let Carlos do the final r+ > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:1153 > + g_signal_emit_by_name(m_webrtcBin.get(), "get-stats", nullptr, gst_promise_new_with_change_func([](GstPromise* rawPromise, gpointer userData) { > + auto promise = adoptGRef(rawPromise); > + auto result = gst_promise_wait(promise.get()); > + std::unique_ptr<GStreamerMediaEndpointHolder> data(static_cast<GStreamerMediaEndpointHolder*>(userData)); > + if (result != GST_PROMISE_RESULT_REPLIED) > + return; > + > + const auto* reply = gst_promise_get_reply(promise.get()); > + ASSERT(reply); > + if (gst_structure_has_field(reply, "error")) > + return; > + > + data->endPoint->onStatsDelivered(reply); > + }, data.release(), nullptr)); This still makes me worry. If this is called more than once, this is going to crash. You need to define the GDestroyNotify and not use the smartptr.

I already tried: struct GStreamerMediaEndpointHolder { Ref<GStreamerMediaEndpoint> endPoint; }; WEBKIT_DEFINE_ASYNC_DATA_STRUCT(GStreamerMediaEndpointHolder); /app/webkit/Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:648:33: error: call to implicitly-deleted default constructor of 'WebCore::GStreamerMediaEndpointHolder' WEBKIT_DEFINE_ASYNC_DATA_STRUCT(GStreamerMediaEndpointHolder); ^ /app/webkit/Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:646:33: note: default constructor of 'GStreamerMediaEndpointHolder' is implicitly deleted because field 'endPoint' has no default constructor Ref<GStreamerMediaEndpoint> endPoint; ^

(In reply to Philippe Normand from comment #51) > I already tried: > > struct GStreamerMediaEndpointHolder { > Ref<GStreamerMediaEndpoint> endPoint; > }; > WEBKIT_DEFINE_ASYNC_DATA_STRUCT(GStreamerMediaEndpointHolder); > > /app/webkit/Source/WebCore/Modules/mediastream/gstreamer/ > GStreamerMediaEndpoint.cpp:648:33: error: call to implicitly-deleted default > constructor of 'WebCore::GStreamerMediaEndpointHolder' > WEBKIT_DEFINE_ASYNC_DATA_STRUCT(GStreamerMediaEndpointHolder); > ^ > /app/webkit/Source/WebCore/Modules/mediastream/gstreamer/ > GStreamerMediaEndpoint.cpp:646:33: note: default constructor of > 'GStreamerMediaEndpointHolder' is implicitly deleted because field > 'endPoint' has no default constructor > Ref<GStreamerMediaEndpoint> endPoint; > ^ It should be RefPtr instead of Ref

Created attachment 455087 [details] Patch

Comment on attachment 455087 [details] Patch LGTM

Comment on attachment 455087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455087&action=review > Source/WebCore/Modules/mediastream/gstreamer/GStreamerIceTransportBackend.cpp:54 > + m_iceTransport.clear(); We don't need this if we land bug #238070 first. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:372 > +WEBKIT_DEFINE_ASYNC_DATA_STRUCT(SetDescriptionCallData); ; is not needed here. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:643 > +WEBKIT_DEFINE_ASYNC_DATA_STRUCT(GStreamerMediaEndpointHolder); Ditto. > Source/WebCore/Modules/mediastream/gstreamer/GStreamerStatsCollector.cpp:241 > +WEBKIT_DEFINE_ASYNC_DATA_STRUCT(CallbackHolder); Ditto. > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingMediaSourceGStreamer.cpp:148 > + m_sender.clear(); Ditto.

Created attachment 455092 [details] [fast-cq] Patch

Committed r291483 (248597@main): <https://commits.webkit.org/248597@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 455092 [details].

<rdar://problem/90484592>