1266390 - createImageBitmap(ImageBitmap) does not preserve alpha premultiplication data

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bas Schouten (:bas.schouten)]

Attachment: canvas-createImageBitmap-data-preserves-alpha.html

When calling CreateImageBimap using an ImageBitmap as a source the premultiplication of the data inside the original ImageBitmap is not propagated. See the attached testcase.

Patch upcoming.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: MozReview Request: Bug 1266390: Preserver mIsPremultipliedAlpha when creating an ImageBitmap from an existing ImageBitmap. r=kaku

Review commit: https://reviewboard.mozilla.org/r/48063/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48063/

Comment 2

[Tzuhao Kuo [:kaku]]

Comment on attachment 8743826 [details]
MozReview Request: Bug 1266390: Preserver mIsPremultipliedAlpha when creating an ImageBitmap from an existing ImageBitmap. r=kaku

https://reviewboard.mozilla.org/r/48063/#review45083

Can you also come out a mochitest fot this specific case?

::: dom/canvas/ImageBitmap.h:227
(Diff revision 1)
>     * The mPictureRect will be used at PrepareForDrawTarget() while user is going
>     * to draw this ImageBitmap into a HTMLCanvasElement.
>     */
>    gfx::IntRect mPictureRect;
>  
> -  const bool mIsPremultipliedAlpha;
> +  bool mIsPremultipliedAlpha;

I think we should not make *mIsPremultipliedAlpha* non-const and modify it.

ImageBitmap itself keeps two buffers, *mData* and *mSurface*. *mData* is its original source image and *mSurface* is a cache for drawing the ImageBitmap onto a HTMLCanvasElement.

*mIsPremultipliedAlpha* is used to indicate whether or not the mData is alpha-premultiplied and, after a ImageBitmap was created, we never modify the *mData*, so we should never modify the *mIsPremultipliedAlpha* too.

Moreover, considering structured-clone and transferring, we only clone/transfer the *mData* but not the *mSurface*. So, modifying *mIsPremultipliedAlpha* leads to another bug in structured-clone and transferring.

::: dom/canvas/ImageBitmap.cpp:556
(Diff revision 1)
>          bufferPtr += 4;
>        }
>      }
>  
>      mSurface = dataSourceSurface;
> +    mIsPremultipliedAlpha = true;

I guess what you're doing here is to prevent applying premultiply-alpha multiple times, right?

It's indeed a bug, however, according to my commnet in the header file, I think we should come up other solution for this bug. Maybe open another bug for it? Also, following the logic of *mIsPremultipliedAlpha*, I think the *mPictureRect* shold not be modified too. A brief idea, we can just move the code of coprring and premultiply-alpha in ImageBitmap::PrepareForDrawTarget() into this if-block(https://dxr.mozilla.org/mozilla-central/source/dom/canvas/ImageBitmap.cpp#436). WDYT?

::: dom/canvas/ImageBitmap.cpp:887
(Diff revision 1)
>      aRv.Throw(NS_ERROR_NOT_AVAILABLE);
>      return nullptr;
>    }
>  
>    RefPtr<layers::Image> data = aImageBitmap.mData;
> -  RefPtr<ImageBitmap> ret = new ImageBitmap(aGlobal, data);
> +  RefPtr<ImageBitmap> ret = new ImageBitmap(aGlobal, data, aImageBitmap.mIsPremultipliedAlpha);

For the scope of this bug, I think this line itself is enough.

Comment 3

[Bas Schouten (:bas.schouten)]

Comment on attachment 8743826 [details]
MozReview Request: Bug 1266390: Preserver mIsPremultipliedAlpha when creating an ImageBitmap from an existing ImageBitmap. r=kaku

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48063/diff/1-2/

Comment 4

[Tzuhao Kuo [:kaku]]

Comment on attachment 8743826 [details]
MozReview Request: Bug 1266390: Preserver mIsPremultipliedAlpha when creating an ImageBitmap from an existing ImageBitmap. r=kaku

https://reviewboard.mozilla.org/r/48063/#review45421

Thanks for the update!

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/572d7f93fbb1

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/572d7f93fbb1