Bug 178109 – Support arbitrary video resolution in getUserMedia API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

It seems the getUserMedia API you recently have implemented does not support the width nor height constraints yet:

navigator.mediaDevices.getUserMedia(constraints)

throws an error when you have for example this constraint in the JSON:

constraints.video.width = 360

This forces me to code a special routine for Safari only, see this if-else block:
https://github.com/binarykitchen/videomail-client/blob/develop/src/wrappers/visuals/recorder.js#L521

Very easy to reproduce. Clone this repo, comment out that if-else block and load the page on Safari. An "Invalid Constraints" error will come up.

Thanks!

[youenn fablet]

width/height constraints should be supported.
For instance, https://webrtc.github.io/samples/src/content/getusermedia/resolution/ is expected to work.
Only specific resolutions are supported, like VGA and HD on both Mac and iOS.

Can you confirm that you observe the same thing?
Can you reuse a similar strategy as the webrtc.github.io example?

[youenn fablet]

Looking at your example, it might be 'ideal' constraints that are not working as expected.

yeah, my videomail app doesn not want to stick to those specific resolutions only. 

that said, yeah, the `ideal` constraints are not working as expected!

[youenn fablet]

(In reply to michael.heuberger from comment #3)
> yeah, my videomail app doesn not want to stick to those specific resolutions
> only. 

Oh, so you would like arbitrary resolutions or just some additional specific resolutions?
Currently, what is exposed is what the camera can produce natively.
This allows a simple and very efficient implementation.

arbitrary resolutions - they work great on chrome, firefox and edge. 

or these browsers have some robust, internal fallbacks instead of throwing an error. dunno know.

specs at
https://w3c.github.io/mediacapture-main/getusermedia.html#def-constraint-width

say following:

"The width or width range, in pixels. As a capability, the range should span the video source's pre-set width values with min being the smallest width and max being the largest width."

i will never be able to know the pre-set width values of every webcam. that said, an ideal constraint would be a great help. it tells the browser to figure out the most close pre-set width instead of throwing an error i think.

[youenn fablet]

Let's keep this bug for arbitrary resolutions.
I filed https://bugs.webkit.org/show_bug.cgi?id=178115 for the case where 'ideal' constraints are not working.

[Radar WebKit Bug Importer]

<rdar://problem/35083128>

[Chad Phillips]

Any update on this?

Looks like as of r230941 this is still an existing limitation, and a real killer for multiparty scenarios, as it's crucial to be able to send thumbnail resolutions in this case.

Curious if it's blocked on something in particular, or just not getting attention...

[Eric Carlson]

Created attachment 349603 [details]
Patch

[EWS Watchlist]

Attachment 349603 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/cv/PixelBufferResizer.mm:56:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 1 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 349659 [details]
Patch

[EWS Watchlist]

Attachment 349659 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/cv/PixelBufferResizer.mm:56:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 1 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 349669 [details]
Patch

[EWS Watchlist]

Attachment 349669 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/cv/PixelBufferResizer.mm:56:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 1 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Comment on attachment 349669 [details]
Patch

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

> Source/WebCore/platform/graphics/cv/PixelBufferResizer.h:44
> +    RetainPtr<CMSampleBufferRef> resize(RetainPtr<CMSampleBufferRef>);

Should they take CMSampleBufferRef/CVPixelBufferRef as input instead of RetainPtr?

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:61
> +Vector<Ref<VideoPreset>>& RealtimeVideoSource::presets()

Can we constify this method since we have a setter?

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:196
> +bool RealtimeVideoSource::supportsCaptureSize(std::optional<int> width, std::optional<int> height, const WTF::Function<bool(const IntSize&)>&& function)

Can we have a more descriptive name for function?
Something like matchWidthAndHeight?
Could also be a const Function& probably.

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:264
> +            continue;

Could be moved above supportsCaptureSize call?

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:267
> +        auto lookForAspectRatioMatch = [&] (const IntSize& size) -> bool {

Could the lambda capture this only?

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:285
> +            auto& minStandardSize = standardVideoSizes()[0];

cont auto&

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:293
> +            for (auto& standardSize : standardVideoSizes()) {

const auto&

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:312
> +        result.requestedSize = exactSizePreset->size;

Could return early here and below.
Would need to move result.requestedFrameRate = requestedFrameRate.value(); above.

> Source/WebCore/platform/mediastream/RealtimeVideoSource.h:102
> +struct VideoPreset : public RefCounted<VideoPreset> {

We usually do not have struct that are RefCounted.
I would go with a class there and a private constructor.

Maybe we should move VideoPreset/VideoPresetData to its own header file as well.

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:317
> +    AVVideoPreset* avPreset = preset ? static_cast<AVVideoPreset*>(preset.get()) : nullptr;

Could use downcast mechanism to ensure this is correct.
Can it be rewritten to:
auto* avPreset = static_cast<AVVideoPreset*>(preset.get())

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:340
> +        LockHolder lock(m_presetMutex);

Maybe we can remove the lock if we create m_pixelBufferResizer in the capture thread whenever our requested size is different from the frame size?

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:435
> +    double epsilon = 0.001;

Add a helper routine in RealtimeMediaSource to match frameRate with 0.001 accuracy?

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:539
> +

Not needed

> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:54
> +static const OSType videoCaptureFormat = kCVPixelFormatType_420YpCbCr8Planar;

Should we try to match exactly what we are doing with real devices, with different format for Mac and iOS?

[EWS Watchlist]

Comment on attachment 349669 [details]
Patch

Attachment 349669 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9203632

New failing tests:
fast/animation/css-animation-resuming-when-visible-with-style-change2.html

[EWS Watchlist]

Created attachment 349675 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Eric Carlson]

Created attachment 349772 [details]
Patch for landing

[Eric Carlson]

Comment on attachment 349669 [details]
Patch

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

>> Source/WebCore/platform/graphics/cv/PixelBufferResizer.h:44
>> +    RetainPtr<CMSampleBufferRef> resize(RetainPtr<CMSampleBufferRef>);
> 
> Should they take CMSampleBufferRef/CVPixelBufferRef as input instead of RetainPtr?

Sure, fixed.

>> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:61
>> +Vector<Ref<VideoPreset>>& RealtimeVideoSource::presets()
> 
> Can we constify this method since we have a setter?

Yes, fixed.

>> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:196
>> +bool RealtimeVideoSource::supportsCaptureSize(std::optional<int> width, std::optional<int> height, const WTF::Function<bool(const IntSize&)>&& function)
> 
> Can we have a more descriptive name for function?
> Something like matchWidthAndHeight?
> Could also be a const Function& probably.

I think the name makes sense, it is checking to see if the size is supported or not.

>> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:264
>> +            continue;
> 
> Could be moved above supportsCaptureSize call?

Good point, fixed.

>> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:267
>> +        auto lookForAspectRatioMatch = [&] (const IntSize& size) -> bool {
> 
> Could the lambda capture this only?

It also needs preset and encodingSize, but made explicit.

>> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:285
>> +            auto& minStandardSize = standardVideoSizes()[0];
> 
> cont auto&

Fixed.

>> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:293
>> +            for (auto& standardSize : standardVideoSizes()) {
> 
> const auto&

Fixed.

>> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:312
>> +        result.requestedSize = exactSizePreset->size;
> 
> Could return early here and below.
> Would need to move result.requestedFrameRate = requestedFrameRate.value(); above.

Fixed.

>> Source/WebCore/platform/mediastream/RealtimeVideoSource.h:102
>> +struct VideoPreset : public RefCounted<VideoPreset> {
> 
> We usually do not have struct that are RefCounted.
> I would go with a class there and a private constructor.
> 
> Maybe we should move VideoPreset/VideoPresetData to its own header file as well.

Sure, made a class and moved to a new header file.

>> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:317
>> +    AVVideoPreset* avPreset = preset ? static_cast<AVVideoPreset*>(preset.get()) : nullptr;
> 
> Could use downcast mechanism to ensure this is correct.
> Can it be rewritten to:
> auto* avPreset = static_cast<AVVideoPreset*>(preset.get())

Changed to <downcast>.

>> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:340
>> +        LockHolder lock(m_presetMutex);
> 
> Maybe we can remove the lock if we create m_pixelBufferResizer in the capture thread whenever our requested size is different from the frame size?

Good idea, moved all access to the resize to the capture thread and got rid of the lock.

>> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:435
>> +    double epsilon = 0.001;
> 
> Add a helper routine in RealtimeMediaSource to match frameRate with 0.001 accuracy?

Fixed.

>> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:539
>> +
> 
> Not needed

Removed.

>> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:54
>> +static const OSType videoCaptureFormat = kCVPixelFormatType_420YpCbCr8Planar;
> 
> Should we try to match exactly what we are doing with real devices, with different format for Mac and iOS?

Good point, fixed.

[WebKit Commit Bot]

Comment on attachment 349772 [details]
Patch for landing

Rejecting attachment 349772 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 349772, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/PAL/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/9217867

[Eric Carlson]

Created attachment 349783 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 349783 [details]
Patch for landing

Clearing flags on attachment: 349783

Committed r236015: <https://trac.webkit.org/changeset/236015>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

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