Bug 225976 – Remove ImageBuffer::toBGRA() and replace its uses with the more general ImageBuffer::getPixelBuffer()

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Remove ImageBuffer::toBGRA() and replace its uses with the more general ImageBuffer::getPixelBuffer()

[Sam Weinig]

Created attachment 429080 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 429080 [details]
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.

[Sam Weinig]

(In reply to Said Abou-Hallawa from comment #2)
> Comment on attachment 429080 [details]
> 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()".

I did it that way because then the MediaSample create function could request the specific pixel format it wanted honestly I don't feel very strongly one way or the other. 

> 
> > 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).

It also fails if the buffer is huge and fails to allocate or overflows, but your point stands. Will fix.

> 
> > 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.

Ok, will move the PixelBuffer stuff to the caller.

Thanks for the review.

[Sam Weinig]

Created attachment 429092 [details]
Patch

[Sam Weinig]

Created attachment 429095 [details]
Patch

[Sam Weinig]

Created attachment 429106 [details]
Patch

[EWS]

Committed r277763 (237927@main): <https://commits.webkit.org/237927@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429106 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78232460>