Bug 178109 - Support arbitrary video resolution in getUserMedia API
Summary: Support arbitrary video resolution in getUserMedia API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: Safari 11
Hardware: PC iOS 11
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
: 181759 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-10-09 19:12 PDT by michael.heuberger
Modified: 2018-09-19 11:11 PDT (History)
10 users (show)

See Also:


Attachments
Patch (114.25 KB, patch)
2018-09-12 17:15 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (122.46 KB, patch)
2018-09-13 05:44 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (122.53 KB, patch)
2018-09-13 08:07 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.43 MB, application/zip)
2018-09-13 10:10 PDT, Build Bot
no flags Details
Patch for landing (131.52 KB, patch)
2018-09-14 10:28 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing (131.52 KB, patch)
2018-09-14 11:23 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description michael.heuberger 2017-10-09 19:12:26 PDT
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!
Comment 1 youenn fablet 2017-10-09 19:58:49 PDT
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?
Comment 2 youenn fablet 2017-10-09 20:00:57 PDT
Looking at your example, it might be 'ideal' constraints that are not working as expected.
Comment 3 michael.heuberger 2017-10-09 20:09:22 PDT
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!
Comment 4 youenn fablet 2017-10-09 20:19:13 PDT
(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.
Comment 5 michael.heuberger 2017-10-09 20:24:42 PDT
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.
Comment 6 youenn fablet 2017-10-09 20:34:42 PDT
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.
Comment 7 Radar WebKit Bug Importer 2017-10-19 14:53:04 PDT
<rdar://problem/35083128>
Comment 8 Chad Phillips 2018-04-23 20:58:01 PDT
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...
Comment 9 Eric Carlson 2018-09-12 17:15:23 PDT
Created attachment 349603 [details]
Patch
Comment 10 Build Bot 2018-09-12 17:17:20 PDT
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.
Comment 11 Eric Carlson 2018-09-13 05:44:24 PDT
Created attachment 349659 [details]
Patch
Comment 12 Build Bot 2018-09-13 05:45:55 PDT
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.
Comment 13 Eric Carlson 2018-09-13 08:07:51 PDT
Created attachment 349669 [details]
Patch
Comment 14 Build Bot 2018-09-13 08:10:24 PDT
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.
Comment 15 youenn fablet 2018-09-13 09:44:49 PDT
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?
Comment 16 Build Bot 2018-09-13 10:10:00 PDT
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
Comment 17 Build Bot 2018-09-13 10:10:02 PDT
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
Comment 18 Eric Carlson 2018-09-14 10:28:09 PDT
Created attachment 349772 [details]
Patch for landing
Comment 19 Eric Carlson 2018-09-14 10:35:05 PDT
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.
Comment 20 WebKit Commit Bot 2018-09-14 10:57:21 PDT
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
Comment 21 Eric Carlson 2018-09-14 11:23:24 PDT
Created attachment 349783 [details]
Patch for landing
Comment 22 WebKit Commit Bot 2018-09-14 12:01:48 PDT
Comment on attachment 349783 [details]
Patch for landing

Clearing flags on attachment: 349783

Committed r236015: <https://trac.webkit.org/changeset/236015>
Comment 23 WebKit Commit Bot 2018-09-14 12:01:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Eric Carlson 2018-09-19 11:11:23 PDT
*** Bug 181759 has been marked as a duplicate of this bug. ***