Bug 189734

Summary: [GStreamer] Support arbitrary video resolution in getUserMedia API
Product: WebKit Reporter: Claudio Saavedra <csaavedra>
Component: WebKitGTKAssignee: Thibault Saunier <tsaunier>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, bugs-noreply, calvaris, commit-queue, tsaunier
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Patch for landing none

Description Claudio Saavedra 2018-09-19 00:32:01 PDT
fast/mediastream/MediaStreamTrack-getCapabilities.html is failing since r236015 landed in bug 178109. That added support for arbitrary video resolutions in the getUserMedia API. Not sure if something is missing in the gstreamer side or actually the testing infrastructure, but filing this for now.

The failure:

--- /home/buildbot/wpe/wpe-linux-64-release-tests/build/layout-test-results/fast/mediastream/MediaStreamTrack-getCapabilities-expected.txt
+++ /home/buildbot/wpe/wpe-linux-64-release-tests/build/layout-test-results/fast/mediastream/MediaStreamTrack-getCapabilities-actual.txt
@@ -4,12 +4,12 @@
 
 
 video track capabilities:
-  capabilities.aspectRatio = { max: 1.778, min: 1 }
+  capabilities.aspectRatio = { max: 1.778, min: 1.333 }
   capabilities.deviceId = <UUID>
   capabilities.facingMode = [ user ]
   capabilities.frameRate = { max: 30, min: 5 }
-  capabilities.height = { max: 720, min: 112 }
-  capabilities.width = { max: 1280, min: 112 }
+  capabilities.height = { max: 720, min: 480 }
+  capabilities.width = { max: 1280, min: 640 }
 
 audio track capabilities:
   capabilities.deviceId = <UUID>
Comment 1 Claudio Saavedra 2018-09-19 00:48:06 PDT
Other tests failing, same revision:

fast/mediastream/apply-constraints-advanced.html
fast/mediastream/apply-constraints-video.html
fast/mediastream/getUserMedia-default.html

From what I see, the problem might be that in the gstreamer backend the minimum size of video is 640x480, whereas this expect it to be 112x112. Similarly, the default should be 320x240. This from looking at the test results, not that I really have any idea about this.
Comment 2 Thibault Saunier 2018-09-27 08:43:35 PDT
Created attachment 350963 [details]
Patch
Comment 3 Thibault Saunier 2018-09-27 14:11:15 PDT
Created attachment 350999 [details]
Patch
Comment 4 Xabier Rodríguez Calvar 2018-10-01 10:23:37 PDT
Comment on attachment 350999 [details]
Patch

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

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:206
> +    GRefPtr<GstCaps> caps = adoptGRef(m_capturer->caps());

Suggestion for the future:

I think GStreamerCapturer::caps() should return GRefPtr<GstCaps>, it would make the lifecycle of this line easier to understand.

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:207
> +    for (guint i = 0; i < gst_caps_get_size(caps.get()); i++) {

guint -> unsigned

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:236
> +            guint frameRatesLength = static_cast<guint>(gst_value_list_get_size(frameRateValues));

unsigned and cast to the same

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:238
> +            for (guint j = 0; j < frameRatesLength; j++) {

ditto

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:254
> +        GST_INFO("Could not find any presets for caps: %" GST_PTR_FORMAT " just let anything go out.",
> +            caps.get());

You can put this in the same line

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:264
> +        setSupportedPresets(WTFMove(presets));
> +    } else
> +        setSupportedPresets(WTFMove(presets));

It looks like you can set the supported presets out of the if and the else, right?
Comment 5 Thibault Saunier 2018-10-11 13:46:43 PDT
Created attachment 352073 [details]
Patch for landing
Comment 6 Thibault Saunier 2018-10-11 13:50:41 PDT
Created attachment 352074 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2018-10-11 14:30:41 PDT
Comment on attachment 352074 [details]
Patch for landing

Clearing flags on attachment: 352074

Committed r237048: <https://trac.webkit.org/changeset/237048>
Comment 8 WebKit Commit Bot 2018-10-11 14:30:42 PDT
All reviewed patches have been landed.  Closing bug.