Closed Bug 1266390 Opened 4 years ago Closed 4 years ago

createImageBitmap(ImageBitmap) does not preserve alpha premultiplication data

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

Details

(Whiteboard: btpp-active)

Attachments

(2 files)

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.
Assignee: nobody → bas
Whiteboard: btpp-active
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.
Attachment #8743826 - Flags: review?(tkuo)
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/
Attachment #8743826 - Flags: review?(tkuo)
Attachment #8743826 - Flags: review?(tkuo) → review+
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!
https://hg.mozilla.org/mozilla-central/rev/572d7f93fbb1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1270207
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.