Closed Bug 1266390 Opened 4 years ago Closed 4 years ago
Image Bitmap(Image Bitmap) does not preserve alpha premultiplication data
811 bytes, text/html
MozReview Request: Bug 1266390: Preserver mIsPremultipliedAlpha when creating an ImageBitmap from an existing ImageBitmap. r=kaku
58 bytes, text/x-review-board-request
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.
Review commit: https://reviewboard.mozilla.org/r/48063/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/48063/
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 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 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!
You need to log in before you can comment on or make changes to this bug.