[webkit-reviews] review granted: [Bug 178109] Support arbitrary video resolution in getUserMedia API : [Attachment 349669] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 178109: Support arbitrary video resolution in getUserMedia API
https://bugs.webkit.org/show_bug.cgi?id=178109

Attachment 349669: Patch

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




--- Comment #15 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 349669
  --> https://bugs.webkit.org/attachment.cgi?id=349669
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?