Bug 210926

This crash currently happens because the mock realtime media source center creates a video mock source instead of a display capture mock source.

I'm working on a patch touching mock sources and now those tests timeout.

I will skip them, we know getDisplayMedia doesn't work yet and adding 3 or 4 timeouts to the already very slow bots isn't helping.

FYI, no longer crashing but only timing out.

fast/mediastream/getDisplayMedia-max-constraints1.html is a flaky Crash:

https://results.webkit.org/?platform=WPE&platform=GTK&suite=layout-tests&test=fast%2Fmediastream%2FgetDisplayMedia-max-constraints1.html

fast/mediastream/getDisplayMedia-max-constraints3.html had only one Crash since r261853.

Latest crash log for getDisplayMedia-max-constraints1.html:

Crash-log: https://build.webkit.org/results/WPE%20Linux%2064-bit%20Release%20(Tests)/r263069%20(18619)/fast/mediastream/getDisplayMedia-max-constraints1-crash-log.txt

STDERR:
STDERR: (WPEWebProcess:13235): GLib-GObject-CRITICAL **: 18:15:49.770: g_object_ref: assertion 'old_val > 0' failed

Thread 1 (Thread 0x7fb084cfe700 (LWP 14447)):
#0  0x00007fb16b67eee5 in _g_log_abort (breakpoint=1) at ../glib/gmessages.c:554
#1  0x00007fb16b6801c9 in g_logv (log_domain=0x7fb16be6b2f7 "GLib-GObject", log_level=G_LOG_LEVEL_CRITICAL, format=<optimized out>, args=args@entry=0x7fb084cfa470) at ../glib/gmessages.c:1373
#2  0x00007fb16b680393 in g_log (log_domain=log_domain@entry=0x7fb16be6b2f7 "GLib-GObject", log_level=log_level@entry=G_LOG_LEVEL_CRITICAL, format=format@entry=0x7fb16b6d177f "%s: assertion '%s' failed") at ../glib/gmessages.c:1415
#3  0x00007fb16b680b8d in g_return_if_fail_warning (log_domain=log_domain@entry=0x7fb16be6b2f7 "GLib-GObject", pretty_function=pretty_function@entry=0x7fb16be6e978 <__func__.15604> "g_object_ref", expression=expression@entry=0x7fb16be6d715 "old_val > 0") at ../glib/gmessages.c:2771
#4  0x00007fb16be3e9fa in g_object_ref (_object=0x7fb098057310) at ../gobject/gobject.c:3368
#5  0x00007fb16be3e9fa in g_object_ref (_object=0x7fb098057310) at ../gobject/gobject.c:3360
#6  0x00007fb16be3eb08 in g_value_object_collect_value (value=0x7fb084cfa588, n_collect_values=<optimized out>, collect_values=<optimized out>, collect_flags=<optimized out>) at ../gobject/gobject.c:4007
#7  0x00007fb16b585a46 in gst_structure_set_valist_internal (structure=0x7fb06c045a60, fieldname=<optimized out>, varargs=varargs@entry=0x7fb084cfa630) at ../gst/gststructure.c:631
#8  0x00007fb16b586dc6 in gst_structure_set (structure=structure@entry=0x7fb06c045a60, field=field@entry=0x7fb16c2f1a19 "context") at ../gst/gststructure.c:663
#9  0x00007fb16c2dc8e5 in gst_gl_handle_context_query (element=element@entry=0x55a06c4547c0 [GstGLUploadElement], query=query@entry=0x55a06c265590 [GstQuery], display=<optimized out>, gl_context=0x7fb098057310 [GstGLContextEGL], other_context=<optimized out>) at ../gst-libs/gst/gl/gstglutils.c:550
#10 0x00007fb16c2b9b1b in gst_gl_base_filter_query (trans=0x55a06c4547c0 [GstGLUploadElement], direction=GST_PAD_SRC, query=0x55a06c265590 [GstQuery]) at ../gst-libs/gst/gl/gstglbasefilter.c:224
#11 0x00007fb16b563238 in gst_pad_query (pad=pad@entry=0x7fb098047b20 [GstPad], query=query@entry=0x55a06c265590 [GstQuery]) at ../gst/gstpad.c:4072
#12 0x00007fb16b56399b in gst_pad_peer_query (pad=pad@entry=0x7fb0980478d0 [GstPad], query=query@entry=0x55a06c265590 [GstQuery]) at ../gst/gstpad.c:4204
#13 0x00007fb16c2db9b6 in pad_query (item=<optimized out>, value=0x7fb084cfa970, user_data=0x55a06c265590) at ../gst-libs/gst/gl/gstglutils.c:108
#14 0x00007fb16b55113c in gst_iterator_fold (it=it@entry=0x7fb0980048a0, func=func@entry=0x7fb16c2db990 <pad_query>, ret=ret@entry=0x7fb084cfa970, user_data=user_data@entry=0x55a06c265590) at ../gst/gstiterator.c:617
#15 0x00007fb16c2dbb26 in gst_gl_run_query (element=<optimized out>, query=query@entry=0x55a06c265590 [GstQuery], direction=direction@entry=GST_PAD_SINK) at ../gst-libs/gst/gl/gstglutils.c:136
#16 0x00007fb16c2dcabd in gst_gl_query_local_gl_context (element=0x55a06c454b30 [GstGLColorConvertElement], direction=direction@entry=GST_PAD_SINK, context_ptr=context_ptr@entry=0x55a06c454d78) at ../gst-libs/gst/gl/gstglutils.c:591
#17 0x00007fb16c2b964e in _find_local_gl_context (filter=0x55a06c454b30 [GstGLColorConvertElement]) at ../gst-libs/gst/gl/gstglbasefilter.c:197
#18 0x00007fb16c2b9df5 in gst_gl_base_filter_find_gl_context (filter=0x55a06c454b30 [GstGLColorConvertElement]) at ../gst-libs/gst/gl/gstglbasefilter.c:432
#19 0x00007fb109f434b1 in gst_gl_color_convert_element_transform_caps (bt=0x55a06c454b30 [GstGLColorConvertElement], direction=GST_PAD_SRC, caps=0x55a06c498f70 [GstCaps], filter=0x0) at ../ext/gl/gstglcolorconvertelement.c:145
#20 0x00007fb16c50ce71 in gst_base_transform_transform_caps (trans=trans@entry=0x55a06c454b30 [GstGLColorConvertElement], direction=GST_PAD_SRC, caps=caps@entry=0x55a06c498f70 [GstCaps], filter=filter@entry=0x0) at ../libs/gst/base/gstbasetransform.c:474

Update on these tests

fast/mediastream/getDisplayMedia-max-constraints.html was removed in r260638.

r272778 "Make RemoteRealtimeVideoSource a RealtimeVideoCaptureSource" made the timeouts vanish for constraints1 and constraints2, but still flaky timeout in constraints3.

Will update the expectations with Failures for 1&2 and skip 3.

Failures still present:

--- /home/buildbot/worker/wpe-linux-64-release-tests/build/layout-test-results/fast/mediastream/getDisplayMedia-max-constraints1-expected.txt
+++ /home/buildbot/worker/wpe-linux-64-release-tests/build/layout-test-results/fast/mediastream/getDisplayMedia-max-constraints1-actual.txt
@@ -1,4 +1,4 @@
 
 PASS setup
-PASS Maximize the width if max height is too big
+FAIL Maximize the width if max height is too big assert_greater_than: waitForHeight expected a number greater than 0 but got 0

--- /home/buildbot/worker/wpe-linux-64-release-tests/build/layout-test-results/fast/mediastream/getDisplayMedia-max-constraints2-expected.txt
+++ /home/buildbot/worker/wpe-linux-64-release-tests/build/layout-test-results/fast/mediastream/getDisplayMedia-max-constraints2-actual.txt
@@ -1,4 +1,4 @@
 
 PASS setup
-PASS Maximize the height if max width is too big
+FAIL Maximize the height if max width is too big assert_greater_than: waitForWidth expected a number greater than 0 but got 0


--- /home/buildbot/worker/wpe-linux-64-release-tests/build/layout-test-results/fast/mediastream/getDisplayMedia-max-constraints3-expected.txt
+++ /home/buildbot/worker/wpe-linux-64-release-tests/build/layout-test-results/fast/mediastream/getDisplayMedia-max-constraints3-actual.txt
@@ -1,4 +1,4 @@
 
 PASS setup
-PASS No effect of the max values if they are too big
+FAIL No effect of the max values if they are too big assert_greater_than: waitForHeight expected a number greater than 0 but got 0

*** Bug 191007 has been marked as a duplicate of this bug. ***

Created attachment 433289 [details]
Patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Comment on attachment 433289 [details]
Patch

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

> Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.cpp:275
> +    CaptureDevice captureDevice(WTF::numberToStringUnsigned<String>(fd), CaptureDevice::DeviceType::Screen, makeString("Capture Display"));

I see there's a 

XdpSessionType xdp_session_get_session_type (XdpSession *session);

I don't know if that's what you need.

Comment on attachment 433289 [details]
Patch

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

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.cpp:275
>> +    CaptureDevice captureDevice(WTF::numberToStringUnsigned<String>(fd), CaptureDevice::DeviceType::Screen, makeString("Capture Display"));
> 
> I see there's a 
> 
> XdpSessionType xdp_session_get_session_type (XdpSession *session);
> 
> I don't know if that's what you need.

Sadly, unrelated. I went this path already and found:

/**
 * XdpSessionType:
 * @XDP_SESSION_SCREENCAST: a screencast session.
 * @XDP_SESSION_REMOTE_DESKTOP: a remote desktop session.
 *
 * The type of a session.
 */
typedef enum {
  XDP_SESSION_SCREENCAST,
  XDP_SESSION_REMOTE_DESKTOP
} XdpSessionType;

I'll move the API in a separate patch...

Created attachment 433400 [details]
Patch

Comment on attachment 433400 [details]
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:96
> +    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.

Comment on attachment 433400 [details]
Patch

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

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.cpp:234
>> +        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.

AFAICS it's a singleton, NeverDestroyed<GStreamerDisplayCaptureDeviceManager>.

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.cpp:274
>> +    // user, so hardcode Screen here. ð¤·
> 
> Weird characters at the end?

Shrug emoji.

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.h:96
>> +    GRefPtr<XdpSession> m_session;
> 
> I guess these are GObjects and we don't need to specify refGPtr and derefGPtr, right?

Right!

Committed r279940 (239684@main): <https://commits.webkit.org/239684@main>

<rdar://problem/80625215>

Comment on attachment 433400 [details]
Patch

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

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:115
> +    auto size = m_source->size();

This broke an API test, see https://bugs.webkit.org/show_bug.cgi?id=228759.

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:114
> +    return CaptureSourceOrError(WTFMove(source));

To enable resizing and so on, we usually wrap the realtime video capture source with a realtime video source.
Something like: "return CaptureSourceOrError(RealtimeVideoSource::create(WTFMove(source)));"

I wonder whether doing so would allow removing some of the changes to the RealtimeXXSource classes.

(In reply to youenn fablet from comment #17)
> Comment on attachment 433400 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433400&action=review
> 
> > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:115
> > +    auto size = m_source->size();
> 
> This broke an API test, see https://bugs.webkit.org/show_bug.cgi?id=228759.
> 
> > Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:114
> > +    return CaptureSourceOrError(WTFMove(source));
> 
> To enable resizing and so on, we usually wrap the realtime video capture
> source with a realtime video source.
> Something like: "return
> CaptureSourceOrError(RealtimeVideoSource::create(WTFMove(source)));"
> 
> I wonder whether doing so would allow removing some of the changes to the
> RealtimeXXSource classes.

Does not help.