[webkit-reviews] review granted: [Bug 190519] [MediaStream] Consolidate all image conversion and resizing into one class : [Attachment 352570] 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 190519: [MediaStream] Consolidate all image conversion and resizing into
one class
https://bugs.webkit.org/show_bug.cgi?id=190519

Attachment 352570: Patch

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




--- Comment #14 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 352570
  --> https://bugs.webkit.org/attachment.cgi?id=352570
Patch

It seems some of the minor changes could impact tests, for instance deviceID.

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

> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:31
> +#include "MediaSampleAVFObjc.h"

Is this one needed here? It is already included in the .mm

> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:55
> +    RefPtr<MediaSample> createMediaSample(IOSurfaceRef, const MediaTime&,
const IntSize&, MediaSample::VideoRotation rotation =
MediaSample::VideoRotation::None, bool mirrored = false);

s/rotation =/=/ here and above.

> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.h:59
> +    WEBCORE_EXPORT ImageTransferSessionVT(uint32_t);

explicit. Maybe add pixelFormat since this is not clear without.
I wonder whether we should consider typing the pixelFormat like we do for
ObjectIdentifier.

> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:62
> +    VTSessionSetProperty(transferSession,
kVTPixelTransferPropertyKey_ScalingMode, kVTScalingMode_Trim);

Should we check status for all of these and return nullptr in create in case
this fails?
Or output some debug/release logging if failing?

> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:131
> +	   RELEASE_LOG(Media, "ImageTransferSessionVT::createPixelBuffer:
CMSampleBufferGetImageBuffer returned nullptr");

No need for this release logging, error case will be logged in
convertPixelBuffer.

> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:258
> +    return createCMSampleBuffer(pixelBuffer.get(), sampleTime, size);

There will be some redundant checks related to size.
Not a big deal but maybe a helper function could be used to share the same code
between the two createCMSampleBuffer variants?

> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:284
> +    int32_t extendedBottom = roundUpToMacroblockMultiple(height) - height;

auto as well.
Maybe add a comment to explain this?

> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:295
> +    }

I do not understand well this code, in particular why we have
m_ioSurfaceBufferAttributes sometimes not having any
kCVPixelBufferExtendedPixelsRightKey/kCVPixelBufferExtendedPixelsBottomKey

> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:325
> +	   return nullptr;

These checks are done within below createPixelBuffer call.

> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:343
> +	   return nullptr;

Isn't setSize called within convertCMSampleBuffer?

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.h:106
> +    void processNewFrame(RefPtr<MediaSample>);

RefPtr<MediaSample>&& probably.
Could we make it a Ref<MediaSample>&&?

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:564
> +    scheduleDeferredTask([this, sample = WTFMove(sample)] {

sample = sample.releaseNonNull() should do the trick to make it a Ref

> Source/WebCore/platform/mediastream/mac/DisplayCaptureSourceCocoa.cpp:174
> +    auto frame = generateFrame();

Can frame be null?
If not, we can probably remove the if checks within the switchOn lambdas.

> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:85
> +	   return;

Put this if inside the previous if.
I wonder whether we need release logging here, maybe
ImageTransferSessionVT::create is already doing that for us.

> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:90
> +	   return;

Ditto here for logging.

> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:93
> +    dispatchMediaSampleToObservers(*sampleBuffer.get());

s/.get()// ?