Bug 225584 – Use PixelBuffer rather than ImageData in platform/ code to fix layering violation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Use PixelBuffer rather than ImageData in platform/ code to fix layering violation

[Sam Weinig]

Created attachment 428149 [details]
Patch

[Sam Weinig]

Created attachment 428150 [details]
Patch

[Sam Weinig]

Created attachment 428151 [details]
Patch

[Sam Weinig]

Created attachment 428174 [details]
Patch

[Sam Weinig]

Created attachment 428180 [details]
Patch

[Said Abou-Hallawa]

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.

[Sam Weinig]

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

[Said Abou-Hallawa]

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.

[Sam Weinig]

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

[Sam Weinig]

Created attachment 428201 [details]
Patch

[Sam Weinig]

(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

[Darin Adler]

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

[Sam Weinig]

Created attachment 428221 [details]
Patch

[Sam Weinig]

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

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/77799571>