Bug 159721 – Get rid of the m_premultiplyAlpha flag of the ImageFrame class

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

This flag was only needed when calling BitmapPixels::setSize(). Instead we can pass the ImageDecoder::m_premultiplyAlpha to ImageFrame::setSize() which will pass it to BitmapPixels::setSize().

[Said Abou-Hallawa]

Created attachment 283537 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283557 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283565 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283569 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283571 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283577 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283578 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283580 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283589 [details]
Patch for review

[Said Abou-Hallawa]

Created attachment 288644 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 288644 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        This flag was only needed when calling ImageBackingStore::setSize(). Instead
> +        we can pass the ImageDecoder::m_premultiplyAlpha to ImageFrame::setSize()
> +        which will pass it to ImageBackingStore::setSize().

Why does setting the size care about premultiplied alpha?

[Carlos Garcia Campos]

(In reply to comment #11)
> Comment on attachment 288644 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288644&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        This flag was only needed when calling ImageBackingStore::setSize(). Instead
> > +        we can pass the ImageDecoder::m_premultiplyAlpha to ImageFrame::setSize()
> > +        which will pass it to ImageBackingStore::setSize().
> 
> Why does setting the size care about premultiplied alpha?

It doesn't, I think it's because we only want the m_premultiplyAlpha to be passed to the ImageBackingStore constructor and ImageBackingStore is created in setSize().

[Carlos Garcia Campos]

I agree that passing premultiplyAlpha to setSize() is weird from the API point of view. But also because setSize() is weird as it is implemented. It can't be called if the ImageFrame already has a backing store, and it always creates a new  ImageBackingStore. So, maybe if we rename the method as initializeBackingStore or simply initialize() it makes more sense to pass a size and premultiplyalpha as initialization parameters. We could probably rename copyBitmapData() also as initialize(). Another possibility (in addition to the rename that I think we should do anyway, because setSize and copyBitmapData are confusing), would be to let the decoder pass premultiplyAlpha to setPixel and blendPixel methods, that would just pass it to the backing store, since those are the only methods needing it.

[Said Abou-Hallawa]

Created attachment 288695 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288696 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288697 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288698 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #13)
> I agree that passing premultiplyAlpha to setSize() is weird from the API
> point of view. But also because setSize() is weird as it is implemented. It
> can't be called if the ImageFrame already has a backing store, and it always
> creates a new  ImageBackingStore. So, maybe if we rename the method as
> initializeBackingStore or simply initialize() it makes more sense to pass a
> size and premultiplyalpha as initialization parameters. We could probably
> rename copyBitmapData() also as initialize().

I like this suggestion. I renamed setSize() and copyBitmapData() to initializeBackingStore(). I preferred it over initialize() because ImageFrame will be shared between CG and non-CG code and initialize() is very generic name. Calling initializeBackingStore() makes it clearer that we want to initialize the backing store only which is only needed by non CG ports.

> Another possibility (in
> addition to the rename that I think we should do anyway, because setSize and
> copyBitmapData are confusing), would be to let the decoder pass
> premultiplyAlpha to setPixel and blendPixel methods, that would just pass it
> to the backing store, since those are the only methods needing it.

I am not sure about this suggestion. It can actually make creating the backing store simpler and can make setPixel() and blendPixel() static or even global functions. But having the backing store not aware whether its pixels are premultiplied or not, seems weird to me. Anyway this seems a big change and I can do it in a separate patch if we need it.

[Simon Fraser (smfr)]

Comment on attachment 288698 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        to ImageFrame::setSize(), which should be renamed ImageFrame::initializeBackingStore().

s/should be/is/

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:179
> +    if (!backingStore)
> +        return false;

Should backingStore just be a reference instead?

> Source/WebCore/platform/image-decoders/ImageDecoder.h:174
> +        bool premultiplyAlpha() const { return m_premultiplyAlpha; }

premultipiesAlpha()?

[Said Abou-Hallawa]

Created attachment 288717 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288719 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 288719 [details]
Patch

Clearing flags on attachment: 288719

Committed r205877: <http://trac.webkit.org/changeset/205877>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.