Summary: | Remove ImageBuffer::toBGRA() and replace its uses with the more general ImageBuffer::getPixelBuffer() | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||
Component: | New Bugs | Assignee: | Sam Weinig <sam> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | calvaris, cdumez, cgarcia, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gustavo, gyuyoung.kim, hta, jer.noble, menard, philipj, pnormand, sabouhallawa, sergio, tommyw, vjaquez, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=229665 | ||||||||||||
Attachments: |
|
Description
Sam Weinig
2021-05-19 11:40:46 PDT
Created attachment 429080 [details]
Patch
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. (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. Created attachment 429092 [details]
Patch
Created attachment 429095 [details]
Patch
Created attachment 429106 [details]
Patch
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]. |