Summary: | Support arbitrary video resolution in getUserMedia API | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | michael.heuberger | ||||||||||||||
Component: | WebRTC | Assignee: | Eric Carlson <eric.carlson> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue, duanpei, eric.carlson, ews-watchlist, jianjun.zhu, lee, michael.heuberger, webkit-bug-importer, webkit, youennf | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | Safari 11 | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | iOS 11 | ||||||||||||||||
Attachments: |
|
Description
michael.heuberger
2017-10-09 19:12:26 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? 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! (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. 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. 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... Created attachment 349603 [details]
Patch
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.
Created attachment 349659 [details]
Patch
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.
Created attachment 349669 [details]
Patch
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 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 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 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
Created attachment 349772 [details]
Patch for landing
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 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 Created attachment 349783 [details]
Patch for landing
Comment on attachment 349783 [details] Patch for landing Clearing flags on attachment: 349783 Committed r236015: <https://trac.webkit.org/changeset/236015> All reviewed patches have been landed. Closing bug. *** Bug 181759 has been marked as a duplicate of this bug. *** |