[webkit-reviews] review granted: [Bug 210926] [GStreamer][Pipewire] Implement getDisplayMedia() backend : [Attachment 433400] 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 210926: [GStreamer][Pipewire] Implement getDisplayMedia() backend
https://bugs.webkit.org/show_bug.cgi?id=210926

Attachment 433400: Patch

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




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

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

> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:1104
> +    auto deviceType = this->deviceType();
> +    if (deviceType != CaptureDevice::DeviceType::Screen && deviceType !=
CaptureDevice::DeviceType::Window)
> +	   ASSERT(!m_hashedID.isEmpty());

I think we should flag all this if #ifndef NDEBUG .

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.cpp
:234
> +    xdp_portal_create_screencast_session(m_portal.get(),
static_cast<XdpOutputType>(XDP_OUTPUT_MONITOR | XDP_OUTPUT_WINDOW),
XDP_SCREENCAST_FLAG_NONE, nullptr, [](GObject* source, GAsyncResult* result,
gpointer userData) {
> +	   GUniqueOutPtr<GError> error;
> +	   auto* session =
xdp_portal_create_screencast_session_finish(XDP_PORTAL(source), result,
&error.outPtr());
> +	   if (!session) {
> +	       WTFLogAlways("Failed to create screencast session: %s",
error->message);
> +	       return;
> +	   }
> +	   auto& manager =
*reinterpret_cast<GStreamerDisplayCaptureDeviceManager*>(userData);

Are we sure this is going to outlive this callback? Otherwise we might even
want to turn GStreamerDisplayCaptureDeviceManager into RefCounted or WeakPtr
and use it here.

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.cpp
:251
> +    g_signal_connect_swapped(m_session.get(), "closed",
G_CALLBACK(+[](GStreamerDisplayCaptureDeviceManager* manager, XdpSession*) {
> +	   manager->sessionWasClosed();
> +    }), this);

Ditto.

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.cpp
:255
> +    xdp_session_start(m_session.get(), nullptr, nullptr, [](GObject* source,
GAsyncResult* result, gpointer userData) {
> +	   auto* session = XDP_SESSION(source);
> +	   GUniqueOutPtr<GError> error;
> +	   auto& manager =
*reinterpret_cast<GStreamerDisplayCaptureDeviceManager*>(userData);

Ditto

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.cpp
:274
> +    // user, so hardcode Screen here. ð¤·

Weird characters at the end?

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.h:9
6
> +    GRefPtr<XdpPortal> m_portal;
> +    GRefPtr<XdpSession> m_session;

I guess these are GObjects and we don't need to specify refGPtr and derefGPtr,
right?

> Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:109
> +	       gst_pad_add_probe(pad.get(),
GST_PAD_PROBE_TYPE_EVENT_DOWNSTREAM, [](GstPad*, GstPadProbeInfo* info, void*
userData) -> GstPadProbeReturn {
> +		   auto* event = gst_pad_probe_info_get_event(info);
> +		   if (GST_EVENT_TYPE(event) != GST_EVENT_CAPS)
> +		       return GST_PAD_PROBE_OK;
> +
> +		   callOnMainThread([event, capturer =
reinterpret_cast<GStreamerCapturer*>(userData)] {

Ditto.