Bug 240229 - [GStreamer][VideoCapture] Add support for capturing encoded video streams from a webcam
Summary: [GStreamer][VideoCapture] Add support for capturing encoded video streams fro...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-05-09 05:03 PDT by Loïc Le Page
Modified: 2022-05-14 09:02 PDT (History)
12 users (show)

See Also:


Attachments
Capture video from a webcam and show effective stream configuration (858 bytes, text/html)
2022-05-09 05:03 PDT, Loïc Le Page
no flags Details
Patch (8.04 KB, patch)
2022-05-09 07:49 PDT, Loïc Le Page
no flags Details | Formatted Diff | Diff
Patch (7.87 KB, patch)
2022-05-10 08:49 PDT, Loïc Le Page
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (7.87 KB, patch)
2022-05-10 08:58 PDT, Loïc Le Page
no flags Details | Formatted Diff | Diff
Patch (11.44 KB, patch)
2022-05-12 02:47 PDT, Loïc Le Page
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Loïc Le Page 2022-05-09 05:03:05 PDT
Created attachment 459040 [details]
Capture video from a webcam and show effective stream configuration

Video streams captured from a webcam using the GStreamer API are limited to the raw video type only.
Some webcams may offer higher resolutions or frame rates with encoded streams (like image/jpeg or video/x-h264 for example).
Those resolutions and frame rates should be taken into account while choosing the webcam configuration.

Let's take for example a specific webcam with those characteristics:
video/x-raw, width=1280, height=720, framerate=10/1
image/jpeg, width=1280, height=720, framerate=30/1

When calling `navigator.mediaDevices.getUserMedia(constraints)` with those constraints:
```
video: {
    width: 1280,
    height: 720,
    frameRate: 30
}
```

The captured video stream configuration is 1280x720@10fps whereas the expected result should be 1280x720@30fps.

The attached index.html file try to capture a video stream from a webcam at 1280x720@30fps and shows the characteristics of the effective video stream configuration.
Comment 1 Loïc Le Page 2022-05-09 07:49:44 PDT
Created attachment 459042 [details]
Patch
Comment 2 Philippe Normand 2022-05-10 03:56:22 PDT
Comment on attachment 459042 [details]
Patch

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

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.cpp:158
> +        std::string mimeType;

We don't use std::string much here, we use String, AtomString, CString from WTF::. I wonder though could this one be const char*?

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.cpp:182
> +        selector.stopCondition.frameRate = static_cast<double>(numerator) / static_cast<double>(denominator);

gst_util_fraction_to_double()

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.cpp:198
> +            if (!gst_structure_get_int(structure, "width", &width)
> +                || !gst_structure_get_int(structure, "height", &height)
> +                || !gst_structure_get_fraction(structure, "framerate", &numerator, &denominator))

One line, you can go up to 120 chars per line.

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.cpp:201
> +            double frameRate = static_cast<double>(numerator) / static_cast<double>(denominator);

Ditto

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.cpp:204
> +            if ((width >= selector->stopCondition.width)
> +                && (height >= selector->stopCondition.height)
> +                && (frameRate >= selector->stopCondition.frameRate)) {

One line and no need for so many () I think?

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.cpp:211
> +            if ((width >= selector->maxWidth)
> +                && (height >= selector->maxHeight)
> +                && (frameRate >= selector->maxFrameRate)) {

Ditto
Comment 3 Loïc Le Page 2022-05-10 08:30:40 PDT
Comment on attachment 459042 [details]
Patch

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

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.cpp:158
>> +        std::string mimeType;
> 
> We don't use std::string much here, we use String, AtomString, CString from WTF::. I wonder though could this one be const char*?

Effectively, the string memory is hold by deviceCaps which is only destroyed when the function returns, so it can perfectly be a simple const char*.
Thanks for seeing it ;)

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.cpp:182
>> +        selector.stopCondition.frameRate = static_cast<double>(numerator) / static_cast<double>(denominator);
> 
> gst_util_fraction_to_double()

done

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.cpp:198
>> +                || !gst_structure_get_fraction(structure, "framerate", &numerator, &denominator))
> 
> One line, you can go up to 120 chars per line.

Here it takes more than 120 chars if you try to group the conditions together on the same line.

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.cpp:201
>> +            double frameRate = static_cast<double>(numerator) / static_cast<double>(denominator);
> 
> Ditto

done

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.cpp:204
>> +                && (frameRate >= selector->stopCondition.frameRate)) {
> 
> One line and no need for so many () I think?

done

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.cpp:211
>> +                && (frameRate >= selector->maxFrameRate)) {
> 
> Ditto

done
Comment 4 Loïc Le Page 2022-05-10 08:49:47 PDT
Created attachment 459121 [details]
Patch
Comment 5 Loïc Le Page 2022-05-10 08:58:49 PDT
Created attachment 459122 [details]
Patch
Comment 6 Philippe Normand 2022-05-10 09:19:26 PDT
Comment on attachment 459122 [details]
Patch

LGTM, thanks!
Comment 7 Loïc Le Page 2022-05-10 10:18:43 PDT
I forgot to take into account the caps ranges in the algo.
I need to correct it and I will file a new patch.
Comment 8 Philippe Normand 2022-05-10 10:21:13 PDT
OK, good thing I haven't set commit-queue+ on this then :)
Comment 9 Philippe Normand 2022-05-10 10:21:52 PDT
(In reply to Philippe Normand from comment #8)
> OK, good thing I haven't set commit-queue+ on this then :)

You can update this patch if you prefer, since it hasn't landed yet.
Comment 10 Loïc Le Page 2022-05-12 02:47:54 PDT
Created attachment 459216 [details]
Patch
Comment 11 Loïc Le Page 2022-05-12 02:51:35 PDT
The latest patch corrects what was lacking previously. It has been tested on two different machines, one of it having ranges and arrays for device caps to be sure it now takes correctly into account those caps.

It also corrects another issue with ghost pads that were not correctly connected inside the converter bin.
Comment 12 Philippe Normand 2022-05-12 04:37:27 PDT
Comment on attachment 459216 [details]
Patch

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

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.cpp:71
> +    auto bin = makeGStreamerBin("capsfilter caps=\"video/x-raw\" name=mimetype-filter ! decodebin3 ! videoscale ! videoconvert ! videorate drop-only=1 average-period=1 name=videorate", false);

So passing true as second argument here wasn't working? Was it due to decodebin3?
Comment 13 Loïc Le Page 2022-05-12 08:37:41 PDT
Comment on attachment 459216 [details]
Patch

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

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.cpp:71
>> +    auto bin = makeGStreamerBin("capsfilter caps=\"video/x-raw\" name=mimetype-filter ! decodebin3 ! videoscale ! videoconvert ! videorate drop-only=1 average-period=1 name=videorate", false);
> 
> So passing true as second argument here wasn't working? Was it due to decodebin3?

Yes, this is because of decobin3.

As src pad is a "sometimes" pad the automatic ghost pad was connected to the videoscale right after and so the mimetype was correctly forced on the first capsfilter but it was useless as the bin src pad was bypassing it.
Comment 14 Philippe Normand 2022-05-12 09:16:47 PDT
Comment on attachment 459216 [details]
Patch

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

>>> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.cpp:71
>>> +    auto bin = makeGStreamerBin("capsfilter caps=\"video/x-raw\" name=mimetype-filter ! decodebin3 ! videoscale ! videoconvert ! videorate drop-only=1 average-period=1 name=videorate", false);
>> 
>> So passing true as second argument here wasn't working? Was it due to decodebin3?
> 
> Yes, this is because of decobin3.
> 
> As src pad is a "sometimes" pad the automatic ghost pad was connected to the videoscale right after and so the mimetype was correctly forced on the first capsfilter but it was useless as the bin src pad was bypassing it.

Ok then. Maybe later we could rework this part a bit, constructing the bin manually and handling db3 pads with the pad-added signal. But if this approach works fine let's land it :)
Comment 15 EWS 2022-05-12 09:25:09 PDT
Committed r294104 (250489@main): <https://commits.webkit.org/250489@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 459216 [details].
Comment 16 Radar WebKit Bug Importer 2022-05-12 09:26:15 PDT
<rdar://problem/93179669>
Comment 17 Philippe Normand 2022-05-14 09:02:59 PDT
Introduced regression: https://bugs.webkit.org/show_bug.cgi?id=240420