Bug 189734 - [GStreamer] Support arbitrary video resolution in getUserMedia API
Summary: [GStreamer] Support arbitrary video resolution in getUserMedia API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thibault Saunier
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-09-19 00:32 PDT by Claudio Saavedra
Modified: 2018-10-11 14:30 PDT (History)
5 users (show)

See Also:


Attachments
Patch (15.74 KB, patch)
2018-09-27 08:43 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch (18.38 KB, patch)
2018-09-27 14:11 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch for landing (20.93 KB, patch)
2018-10-11 13:46 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch for landing (18.95 KB, patch)
2018-10-11 13:50 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.