Bug 225554 – Factor out pixel buffer from DOM specific ImageData class

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

[Sam Weinig]

Factor out pixel buffer from DOM specific ImageData class

[Sam Weinig]

Created attachment 428075 [details]
Patch

[Sam Weinig]

Created attachment 428086 [details]
Patch

[Sam Weinig]

Created attachment 428087 [details]
Patch

[Sam Weinig]

Created attachment 428090 [details]
Patch

[Darin Adler]

Comment on attachment 428090 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1085
> +        RefPtr<ArrayBuffer> arrayBuffer = imageData->data().possiblySharedBuffer();

We could either do auto/makeRefPtr, or just say RefPtr instead of RefPtr<ArrayBuffer>, adding deduction guides to RefPtr if needed. The future of RefPtr is now!

> Source/WebCore/html/ImageData.h:56
> +    ImageData(PixelBuffer&&);

explicit

> Source/WebCore/platform/graphics/PixelBuffer.h:31
> +#include <JavaScriptCore/Uint8ClampedArray.h>

Do we really need to include this header? Can we do a forward-declaration instead?

> Source/WebCore/platform/graphics/PixelBuffer.h:39
> +    ~PixelBuffer();

If the only thing this does is decrement the Uint8ClampedArray reference count, then maybe we can just use the default and not write this at all.

> Source/WebCore/platform/graphics/ShadowBlur.cpp:912
> +    auto& blurPixelArray = layerData->data();
> +    blurLayerImage(blurPixelArray.data(), blurRect.size(), blurRect.width() * 4);

Seems like we don’t need the local at all.

> Source/WebCore/platform/graphics/filters/FELighting.cpp:482
> +    auto& resutPixelArray = resultImage->data();

Existing typo, "resut", that you should fix.

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:250
> +    auto& dstPixelArray = resultImage->data();

Do we have to do the "dst" thing? Maybe write out the whole destination word.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:204
> +    Uint8ClampedArray& imageArray = m_premultipliedImageResult->data();

auto&

[Sam Weinig]

(In reply to Darin Adler from comment #5)
> Comment on attachment 428090 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428090&action=review
> 
> > Source/WebCore/bindings/js/SerializedScriptValue.cpp:1085
> > +        RefPtr<ArrayBuffer> arrayBuffer = imageData->data().possiblySharedBuffer();
> 
> We could either do auto/makeRefPtr, or just say RefPtr instead of
> RefPtr<ArrayBuffer>, adding deduction guides to RefPtr if needed. The future
> of RefPtr is now!

I went with auto/makeRefPtr, but I ashamed. `RefPtr arrayBuffer = foo();` looks just too weird for me :). I'll get used to it though.

> 
> > Source/WebCore/html/ImageData.h:56
> > +    ImageData(PixelBuffer&&);
> 
> explicit

Done.

> 
> > Source/WebCore/platform/graphics/PixelBuffer.h:31
> > +#include <JavaScriptCore/Uint8ClampedArray.h>
> 
> Do we really need to include this header? Can we do a forward-declaration
> instead?

The forward declaration for these types is non-trivial to say the least. Something like:

namespace JSC {

struct Uint8ClampedAdaptor;
template<typename Adaptor> class GenericTypedArrayView;
typedef GenericTypedArrayView<Uint8ClampedAdaptor> Uint8ClampedArray;

}

but getting that building everywhere is quite a challenge. I think PixelBuffer will evolve to use ArrayBuffer directly at some point, at which point it should be a bit easier and I will attempt it then.

> 
> > Source/WebCore/platform/graphics/PixelBuffer.h:39
> > +    ~PixelBuffer();
> 
> If the only thing this does is decrement the Uint8ClampedArray reference
> count, then maybe we can just use the default and not write this at all.

I wanted it out of line to avoid including all the inline header inclusion that would be required due to JSC header fun.

> 
> > Source/WebCore/platform/graphics/ShadowBlur.cpp:912
> > +    auto& blurPixelArray = layerData->data();
> > +    blurLayerImage(blurPixelArray.data(), blurRect.size(), blurRect.width() * 4);
> 
> Seems like we don’t need the local at all.

Agreed. Fixed.

> 
> > Source/WebCore/platform/graphics/filters/FELighting.cpp:482
> > +    auto& resutPixelArray = resultImage->data();
> 
> Existing typo, "resut", that you should fix.

Fixed.

> 
> > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:250
> > +    auto& dstPixelArray = resultImage->data();
> 
> Do we have to do the "dst" thing? Maybe write out the whole destination word.

Fixed. Used `destinationPixelArray`.

> 
> > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:204
> > +    Uint8ClampedArray& imageArray = m_premultipliedImageResult->data();
> 
> auto&

Fixed.

[Sam Weinig]

Created attachment 428098 [details]
Patch

[Sam Weinig]

Created attachment 428099 [details]
Patch

[Sam Weinig]

Created attachment 428100 [details]
Patch

[EWS]

Patch 428098 does not build

[EWS]

Committed r277237 (237506@main): <https://commits.webkit.org/237506@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/77702646>