[webkit-reviews] review granted: [Bug 225976] Remove ImageBuffer::toBGRA() and replace its uses with the more general ImageBuffer::getPixelBuffer() : [Attachment 429080] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Sam Weinig
<sam at webkit.org>'s request for review:
Bug 225976: Remove ImageBuffer::toBGRA() and replace its uses with the more
general ImageBuffer::getPixelBuffer()
https://bugs.webkit.org/show_bug.cgi?id=225976

Attachment 429080: Patch

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




--- Comment #2 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 429080
  --> https://bugs.webkit.org/attachment.cgi?id=429080
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:828
> +    return MediaSampleAVFObjC::createImageSample(*imageBuffer);
>  #elif USE(GSTREAMER)
> -    makeRenderingResultsAvailable();
> -    return
MediaSampleGStreamer::createImageSample(imageBuffer->toBGRAData(), size());
> +    return MediaSampleGStreamer::createImageSample(*imageBuffer);

Why did you choose to pass ImageBuffer& instead of PixelBuffer&& to both
MediaSampleAVFObjC::createImageSample() and
MediaSampleGStreamer::createImageSample()? Down in the code, I see the passed
ImageBuffer is only used to call "getPixelBuffer()".

> Source/WebCore/page/PageColorSampler.cpp:112
> +    if (pixelBuffer->data().length() < 4)
> +	   return WTF::nullopt;

I think getPixelBuffer() will return nullopt if the size is less than 1 pixel.
Maybe this should be replaced by an ASSERT(pixelBuffer->data().length() >= 4).

> Source/WebCore/platform/graphics/avfoundation/objc/MediaSampleAVFObjC.mm:53
> +    auto pixelBuffer = imageBuffer.getPixelBuffer({
AlphaPremultiplication::Unpremultiplied, PixelFormat::BGRA8,
DestinationColorSpace::SRGB }, { { }, imageBuffer.logicalSize() });
> +    if (!pixelBuffer)
> +	   return nullptr;

I think this code can be moved to the caller and having it pass a PixelBuffer&&
instead.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaSampleAVFObjC.mm:56
> +    auto height = pixelBuffer->size().width();

typo: width() => height().

> Source/WebCore/platform/graphics/gstreamer/MediaSampleGStreamer.cpp:110
> +	   return nullptr;

This can't return a nullptr for Ref<>. But if it is passed a PixelBuffer&& then
there is no need for this early return.