Use PixelBuffer rather than ImageData in platform/ code to fix layering violation
Created attachment 428149 [details] Patch
Created attachment 428150 [details] Patch
Created attachment 428151 [details] Patch
Created attachment 428174 [details] Patch
Created attachment 428180 [details] Patch
Can't we keep ImageData in platform code and use something like DOMImageData in the binding code? I see many idl files have names like DOM<interface>.idl: DOMWindow.idl, DOMCache.idl, DOMFileSystem.idl, etc.
(In reply to Said Abou-Hallawa from comment #6) > Can't we keep ImageData in platform code and use something like DOMImageData > in the binding code? I see many idl files have names like > DOM<interface>.idl: DOMWindow.idl, DOMCache.idl, DOMFileSystem.idl, etc. We could, but what benefit would there be? There doesn't seem to be a need for RefCounted object here.
The benefit is we do not have to invent a new name 'PixelBuffer'. Relating ImageData and DOMImageData is trivial and this name convention has precedent in the code.
(In reply to Said Abou-Hallawa from comment #8) > The benefit is we do not have to invent a new name 'PixelBuffer'. Relating > ImageData and DOMImageData is trivial and this name convention has precedent > in the code. I think the new name is better and more descriptive. I'd rather not conflate DOM names and platform names if we can avoid it and it seems easy to do so here.
Created attachment 428201 [details] Patch
(In reply to Sam Weinig from comment #9) > (In reply to Said Abou-Hallawa from comment #8) > > The benefit is we do not have to invent a new name 'PixelBuffer'. Relating > > ImageData and DOMImageData is trivial and this name convention has precedent > > in the code. > > I think the new name is better and more descriptive. I'd rather not conflate > DOM names and platform names if we can avoid it and it seems easy to do so > here. I'm also up for other names if you have any that you prefer, but I think tying to DOM concepts is the opposite of what we want to do in the platform layer. Other names I considered were: - PixelData - ImageBufferDirect
Comment on attachment 428201 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428201&action=review > Source/WebCore/platform/graphics/PixelBuffer.cpp:39 > + static constexpr unsigned bytesPerPixel = 4; No need for "static" here. > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:575 > + ASSERT(m_unmultipliedImageResult == WTF::nullopt); I might use "!" or "!x.hasValue()" instead. > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:593 > + ASSERT(m_premultipliedImageResult == WTF::nullopt); Ditto. > Source/WebCore/platform/graphics/filters/FilterEffect.h:30 > +#include "PixelBuffer.h" Don’t need to both include and forward-declare PixelBuffer. > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:587 > + case DisplayList::ItemType::GetPixelBuffer: { > ASSERT_NOT_REACHED(); > break; > } No need for these braces. Also seems like we want "return WTF::nullopt" or maybe not even bother with "break". > Source/WebKit/Shared/RemoteLayerTree/CGDisplayListImageBufferBackend.h:54 > + void putPixelBuffer(WebCore::AlphaPremultiplication inputFormat, const WebCore::WebCore::PixelBuffer&, const WebCore::IntRect& srcRect, const WebCore::IntPoint& destPoint, WebCore::AlphaPremultiplication destFormat) override; Does this compile? WebCore::WebCore::PixelBuffer
Created attachment 428221 [details] Patch
(In reply to Darin Adler from comment #12) > Comment on attachment 428201 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428201&action=review > > > Source/WebCore/platform/graphics/PixelBuffer.cpp:39 > > + static constexpr unsigned bytesPerPixel = 4; > > No need for "static" here. Fixed. > > > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:575 > > + ASSERT(m_unmultipliedImageResult == WTF::nullopt); > > I might use "!" or "!x.hasValue()" instead. Fixed. > > > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:593 > > + ASSERT(m_premultipliedImageResult == WTF::nullopt); > > Ditto. Fixed. > > > Source/WebCore/platform/graphics/filters/FilterEffect.h:30 > > +#include "PixelBuffer.h" > > Don’t need to both include and forward-declare PixelBuffer. Fixed. > > > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:587 > > + case DisplayList::ItemType::GetPixelBuffer: { > > ASSERT_NOT_REACHED(); > > break; > > } > > No need for these braces. Also seems like we want "return WTF::nullopt" or > maybe not even bother with "break". Fixed. > > > Source/WebKit/Shared/RemoteLayerTree/CGDisplayListImageBufferBackend.h:54 > > + void putPixelBuffer(WebCore::AlphaPremultiplication inputFormat, const WebCore::WebCore::PixelBuffer&, const WebCore::IntRect& srcRect, const WebCore::IntPoint& destPoint, WebCore::AlphaPremultiplication destFormat) override; > > Does this compile? WebCore::WebCore::PixelBuffer It did not. Fixed.
Committed r277313 (237573@main): <https://commits.webkit.org/237573@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428221 [details].
<rdar://problem/77799571>