[webkit-reviews] review granted: [Bug 238071] VideoFrame does not need to inherit from MediaSample : [Attachment 455384] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted  review:
Bug 238071: VideoFrame does not need to inherit from MediaSample
https://bugs.webkit.org/show_bug.cgi?id=238071

Attachment 455384: Patch

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




--- Comment #3 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 455384
  --> https://bugs.webkit.org/attachment.cgi?id=455384
Patch

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

r=me once the bots are happy

> Source/WebCore/platform/VideoFrame.cpp:69
> +#if PLATFORM(COCOA) && USE(AVFOUNDATION)
>  RefPtr<VideoFrameCV> VideoFrame::asVideoFrameCV()
>  {
> +    if (isCV())
> +	   return downcast<VideoFrameCV>(this);
> +
>      auto buffer = pixelBuffer();
>      if (!buffer)
>	   return nullptr;
> -
>      return VideoFrameCV::create(presentationTime(), isMirrored(),
rotation(), buffer);
>  }
> +#endif // PLATFORM(COCOA) && USE(AVFOUNDATION)
> +
> +#if !PLATFORM(COCOA)
> +RefPtr<JSC::Uint8ClampedArray> VideoFrame::getRGBAImageData() const
> +{
> +    return nullptr;
> +}
>  #endif

Can these move to VideoFrame.mm?

> Source/WebCore/platform/VideoFrame.mm:46
> +    const OSType imageFormat = kCVPixelFormatType_32RGBA;
> +    RetainPtr<CFNumberRef> imageFormatNumber =
adoptCF(CFNumberCreate(nullptr,  kCFNumberIntType,  &imageFormat));
> +
> +    RetainPtr<CFMutableDictionaryRef> conformerOptions =
adoptCF(CFDictionaryCreateMutable(0, 1, &kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks));
> +    CFDictionarySetValue(conformerOptions.get(),
kCVPixelBufferPixelFormatTypeKey, imageFormatNumber.get());
> +    PixelBufferConformerCV pixelBufferConformer(conformerOptions.get());

I think this be simplified to something like:

    auto pixelBufferConformer(@{ (__bridge NSString
*)kCVPixelBufferPixelFormatTypeKey: @(kCVPixelFormatType_32RGBA) });