Bug 159679 – Move the pixel data of ImageFrame to a separate class named ImageBackingStore

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

The goal is to have one class for the ImageFrame. Currently we have two very similar implementations: one is defined for non CG ports "ImageFrame", which is defined in /platform/image-decoders/ImageDecoder.h, and the second is "FrameData", which is defined in platform/graphics/BitmapImage.h.

We begin by moving the pixel data manipulation part in ImageFrame into a separate class and then make it optional (allocated on demand). So for the CG case, BitmapPixels object will not be allocated because all the decoding is done by CG.

[Said Abou-Hallawa]

Created attachment 283425 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283427 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283429 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283431 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283433 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283437 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283439 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283440 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283443 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283449 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283450 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283461 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283466 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283467 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283469 [details]
Patch for review

[Said Abou-Hallawa]

Created attachment 288426 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288428 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288429 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288447 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288451 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288459 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288461 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288463 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288466 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288468 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288472 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288475 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 288475 [details]
Patch

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

I ran the tests for GTK+ and got mostly the same results, so I don't think this is introducing any regression.

> Source/WebCore/platform/graphics/ImageBackingStore.h:38
> +class ImageBackingStore {
> +public:

This could probably be fast allocated.

> Source/WebCore/platform/graphics/ImageBackingStore.h:47
> +    static std::unique_ptr<ImageBackingStore> create(const IntSize& size, bool premultiplyAlpha)
> +    {
> +        return std::make_unique<ImageBackingStore>(size, premultiplyAlpha);
> +    }
> +
> +    static std::unique_ptr<ImageBackingStore> create(const ImageBackingStore& other)
> +    {
> +        return std::make_unique<ImageBackingStore>(other);
> +    }

Do we need create methods if they just call macke_unique and constructors are public?

> Source/WebCore/platform/graphics/ImageBackingStore.h:49
> +    ImageBackingStore(const IntSize& size, bool premultiplyAlpha = true)

So, premultiplyAlpha is optional here but not in the create method.

> Source/WebCore/platform/graphics/ImageBackingStore.h:59
> +        if (this == &other)
> +            return;

I don't understand this.

> Source/WebCore/platform/graphics/ImageBackingStore.h:71
> +    {
> +        if (!m_pixels.tryReserveCapacity(size.area()))

Is it ok to set an empty size? otherwise I would add an assert here.

> Source/WebCore/platform/graphics/ImageBackingStore.h:89
> +    IntSize size() const { return m_size; }

Could we return const IntSize& here?

> Source/WebCore/platform/graphics/ImageBackingStore.h:90
> +    IntRect frameRect() const { return m_frameRect; }

And const IntRect& here?

> Source/WebCore/platform/image-decoders/ImageDecoder.h:93
> +        IntSize size() const { return m_backingStore->size(); }

const IntSize& ?

> Source/WebCore/platform/image-decoders/ImageDecoder.h:110
> +        void setBackingStore(const ImageBackingStore*);

Where is this implemented?

> Source/WebCore/platform/image-decoders/cairo/ImageBackingStoreCairo.cpp:38
> +NativeImagePtr ImageBackingStore::image() const
> +{
> +    return adoptRef(cairo_image_surface_create_for_data(
> +        reinterpret_cast<unsigned char*>(const_cast<RGBA32*>(m_pixelsPtr)),
> +        CAIRO_FORMAT_ARGB32, size().width(), size().height(), size().width() * sizeof(RGBA32)));
> +}

So, I guess we can remove ImageDecoderCairo.cpp now.

[Said Abou-Hallawa]

Created attachment 288598 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 288475 [details]
Patch

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

Thanks for the code review and for verifying the layout tests.

>> Source/WebCore/platform/graphics/ImageBackingStore.h:38
>> +public:
> 
> This could probably be fast allocated.

Done.

>> Source/WebCore/platform/graphics/ImageBackingStore.h:47
>> +    }
> 
> Do we need create methods if they just call macke_unique and constructors are public?

Done. Constructors became private and I have to abandon calling std::make_unique() since it requires the constructor to be public.

>> Source/WebCore/platform/graphics/ImageBackingStore.h:49
>> +    ImageBackingStore(const IntSize& size, bool premultiplyAlpha = true)
> 
> So, premultiplyAlpha is optional here but not in the create method.

Done. The premultiplyAlpha is now optional in both the constructor and the create method.

>> Source/WebCore/platform/graphics/ImageBackingStore.h:59
>> +            return;
> 
> I don't understand this.

You are right. This constructor was originally implemented as an assignment operator. I forgot to change its body to be suitable for a copy constructor.

>> Source/WebCore/platform/graphics/ImageBackingStore.h:71
>> +        if (!m_pixels.tryReserveCapacity(size.area()))
> 
> Is it ok to set an empty size? otherwise I would add an assert here.

A check for !size.isEmpty() its added to this if-statment.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:93
>> +        IntSize size() const { return m_backingStore->size(); }
> 
> const IntSize& ?

Done.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:110
>> +        void setBackingStore(const ImageBackingStore*);
> 
> Where is this implemented?

setBackingStore() is not implemented and this declaration is not needed.

>> Source/WebCore/platform/image-decoders/cairo/ImageBackingStoreCairo.cpp:38
>> +}
> 
> So, I guess we can remove ImageDecoderCairo.cpp now.

You are right. ImageDecoderCairo.cpp is now deleted.

[Said Abou-Hallawa]

Created attachment 288604 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 288604 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageBackingStore.h:37
> +class ImageBackingStore {

I'm surprised that this class doesn't pad rowBytes to some multiple of 4 or 8 bytes.

> Source/WebCore/platform/graphics/ImageBackingStore.h:85
> +        size_t rectWidthInBytes = rect.width() * sizeof(RGBA32);

This is normally called rowBytes

> Source/WebCore/platform/graphics/ImageBackingStore.h:98
> +        size_t rectWidthInBytes = rect.width() * sizeof(RGBA32);

Ditto.

> Source/WebCore/platform/graphics/ImageBackingStore.h:107
> +    RGBA32* at(int x, int y) const

I don't like this name.

> Source/WebCore/platform/graphics/ImageBackingStore.h:112
> +    void setRGBA(int x, int y, unsigned r, unsigned g, unsigned b, unsigned a)

I would call this setPixel()

> Source/WebCore/platform/graphics/ImageBackingStore.h:127
> +    void overRGBA(RGBA32* dest, unsigned r, unsigned g, unsigned b, unsigned a)

I don't like this name.

[Said Abou-Hallawa]

Created attachment 288626 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 288626 [details]
Patch

Clearing flags on attachment: 288626

Committed r205841: <http://trac.webkit.org/changeset/205841>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.