[webkit-reviews] review granted: [Bug 189734] [GStreamer] Support arbitrary video resolution in getUserMedia API : [Attachment 350999] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 1 10:23:37 PDT 2018


Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Thibault Saunier
<tsaunier at gnome.org>'s request for review:
Bug 189734: [GStreamer] Support arbitrary video resolution in getUserMedia API
https://bugs.webkit.org/show_bug.cgi?id=189734

Attachment 350999: Patch

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




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

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

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:2
06
> +    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:2
07
> +    for (guint i = 0; i < gst_caps_get_size(caps.get()); i++) {

guint -> unsigned

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

unsigned and cast to the same

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

ditto

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:2
54
> +	   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:2
64
> +	   setSupportedPresets(WTFMove(presets));
> +    } else
> +	   setSupportedPresets(WTFMove(presets));

It looks like you can set the supported presets out of the if and the else,
right?


More information about the webkit-reviews mailing list