Closed
Bug 1239752
Opened 8 years ago
Closed 8 years ago
createImageBitmap(ImageData) does not preserve alpha
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: xidachen, Assigned: kaku, NeedInfo)
References
Details
(Keywords: testcase)
Attachments
(5 files, 2 obsolete files)
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.106 Safari/537.36 Steps to reproduce: 1. Open the attached .html in Firefox nightly build (version: 46.0a1 (2016-01-14)). 2. Press F12 and observe the output in console. Actual results: The output is now: 47, 135, 223, 64. I assume that the first three channels are computed by the input * 255/alpha. Expected results: The output should be: 12, 34, 56, 64
Comment 1•8 years ago
|
||
Thanks Xida for filing this! kaku, would you have time to look at this.
Flags: needinfo?(tkuo)
Comment 2•8 years ago
|
||
The problem is that ImageBitmap::CreateInternal (the overload that takes ImageData) is buggy. It's doing: 745 data = CreateImageFromRawData(imageSize, imageStride, FORMAT, 746 array.Data(), dataLength, 747 aCropRect); where CreateImageFromRawData ends up calling CreateSurfaceFromRawData which calls CreateWrappingDataSourceSurface. None of this stuff is documented in terms of what it expects the data to be, and neither is the SurfaceFormat enum, but I'm 99% sure surfaces are stored as premultiplied data. But ImageData stores non-premultiplied data. Note that putImageData in canvas code creates a gfxImageSurface and then converts to premultiplied data using gfxUtils::sPremultiplyTable. I expect you need to do something similar in the ImageBitmap code.
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #1) > Thanks Xida for filing this! > > kaku, would you have time to look at this. Yes, sure. So, the root cause is that CanvasRenderingContext::GetImageData() applies Un-PreMulitpyAlpha, however, just like bz mentioned, the ImageBitmap stores non-PreMultiplied data (at least in the create-from-ImageData case). Two possibilities here: 1) Apply pre-multiply alpha while creating ImageBitmap (from an ImageData). 2) Apply pre-multiply alpha while drawing to a Canvas and keep the data stored in ImageBitmap as what it presented in the source. I personally prefer the 2nd proposal since it keep the performance of creating an ImageBitmap. Thoughts?
Assignee: nobody → tkuo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(tkuo)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
Assignee | ||
Comment 4•8 years ago
|
||
BTW, the existing test cases use color with 255 alpha value which is not affected by an extra un-preMultiplyAlpha operation. So the existing test cases do not cover this issue.
Comment 5•8 years ago
|
||
> Thoughts?
I assume we can reliably detect (e.g. by storing it in a member) whether an ImageBitmap is using premultiplied alpha or not, right?
Do we have a feel for how often ImageBitmaps are created without drawing them? How often they're created and then drawn more than once? Would it make sense to create non-premultiplied and convert to premultiplied on first draw?
Flags: needinfo?(bzbarsky)
Comment 6•8 years ago
|
||
That sounds like a good and hopefully simple optimization.
Flags: needinfo?(bugs)
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #5) > I assume we can reliably detect (e.g. by storing it in a member) whether an > ImageBitmap is using premultiplied alpha or not, right? Yes. > Do we have a feel for how often ImageBitmaps are created without drawing them? For now, no. CanvasRenderingContext2D::drawImage() is the only WebAPI that accepts ImageBitmap. However, there are several proposals that are going to utilizes ImageBitmap for not only drawing but also a gate way to access image data. [1] Bug 1187812 - [Foxeye] Let WebGL::TexImageSource accept ImageBitmap as a source object (https://bugzilla.mozilla.org/show_bug.cgi?id=1187812) [2] Bug 1141979 - [FoxEye] Extend ImageBitmap with interfaces to access its underlying image data (https://bugzilla.mozilla.org/show_bug.cgi?id=1141979) [3] Bug 1235301 - [FoxEye] Introduce the non-realtime video source to the web platform. (https://bugzilla.mozilla.org/show_bug.cgi?id=1235301) > How often they're created and then drawn more than once? > Would it make sense to create non-premultiplied and convert to premultiplied on first draw? Assume those proposal were accepted, the ImageBitmap is then a handler to image data which is not only for drawing purpose, and I think it makes sense to delay the premultiply operation to the first draw.
Assignee | ||
Comment 8•8 years ago
|
||
The a color converted through gfxUtils::sPremultiplyTable/gfxUtils::sUnpremultiplyTable does not equals to its original value and the difference is quite large for colors with low alpha values.
Comment 9•8 years ago
|
||
Of course. Conversion to premultiplied alpha and back is lossy unless your color originally came from a premultiplied alpha color. You already get this with putImageData followed by getImageData on canvas.
Assignee | ||
Comment 10•8 years ago
|
||
So, the only way to verify the color value of an ImageBitmap is drawing it onto a canvas and then get an ImageData out (where a un-premultiply is applied), I can only test colors with higher alpha value.
Assignee | ||
Comment 11•8 years ago
|
||
Hi Seth, I would like to consult you about the premultiply-alpha mechanism in imgTools. Looks like that the imgTools decodes images file with premultiply-alpha applied, only for some conditions when the XXX_NO_PREMULTIPLY_ALPHA was applied, the decoding process does not premultiply-alpha, right? I am wondering if there is any way that I can invoke a decode task with premultiply/unpremultiply assigned? Or, is there any way that I can check whether the premultiply was applied or not, after the decode task is done?
Flags: needinfo?(seth)
Comment 12•8 years ago
|
||
> I can only test colors with higher alpha value
It depends on what you're trying to test. For low alpha values, some colors obviously round-trip with no change, and some do not....
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8711543 -
Flags: review?(bugs)
Assignee | ||
Comment 14•8 years ago
|
||
Remove spaces at the end of line.
Attachment #8711543 -
Attachment is obsolete: true
Attachment #8711543 -
Flags: review?(bugs)
Attachment #8711547 -
Flags: review?(bugs)
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8711547 [details] [diff] [review] Bug1239752 - create ImageBitmap from ImageData should preserve alpha; r?samug Review of attachment 8711547 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/ImageBitmap.h @@ +223,5 @@ > * to draw this ImageBitmap into a HTMLCanvasElement. > */ > gfx::IntRect mPictureRect; > + > + const bool mIsPreMyltiplyAlphaApplied; typo.
Assignee | ||
Comment 16•8 years ago
|
||
Fix some typos.
Attachment #8711547 -
Attachment is obsolete: true
Attachment #8711547 -
Flags: review?(bugs)
Attachment #8711549 -
Flags: review?(bugs)
Comment 17•8 years ago
|
||
Comment on attachment 8711549 [details] [diff] [review] Bug1239752 - create ImageBitmap from ImageData should preserve alpha; r?samug This is gfx-y change, so I'd prefer if roc reviewed this given that he (IIRC) reviewed the gfx-y part of ImageBitmap. + surface = nullptr; return surface.forget(); look odd code. Why not just return nullptr? r+ to structured cloning handling, if s/IsPreMultiplyAlphaApplied_/isPreMultiplyAlphaApplied_/ const uint32_t IsPreMultiplyAlphaApplied should be isPreMultiplyAlphaApplied Does the test make sure right alpha handling is preserved when transferring ImageBitmap to and from a worker?
Attachment #8711549 -
Flags: review?(bugs) → review?(roc)
Assignee | ||
Comment 18•8 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #17) > Does the test make sure right alpha handling is preserved when transferring > ImageBitmap to and from a worker? No, sorry, it doesn't. I will add one more patch for it.
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8711549 [details] [diff] [review] Bug1239752 - create ImageBitmap from ImageData should preserve alpha; r?samug (In reply to Olli Pettay [:smaug] from comment #17) > Comment on attachment 8711549 [details] [diff] [review] > Bug1239752 - create ImageBitmap from ImageData should preserve alpha; r?samug > > This is gfx-y change, so I'd prefer if roc reviewed this given that he > (IIRC) reviewed the gfx-y part of ImageBitmap. > > + surface = nullptr; > return surface.forget(); > look odd code. > Why not just return nullptr? > > r+ to structured cloning handling, if > s/IsPreMultiplyAlphaApplied_/isPreMultiplyAlphaApplied_/ > > const uint32_t IsPreMultiplyAlphaApplied > should be isPreMultiplyAlphaApplied Thanks for the review. I will modify this patch and resend the review via MozReview.
Attachment #8711549 -
Flags: review?(roc)
Assignee | ||
Comment 20•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/32409/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/32409/
Attachment #8712022 -
Flags: review?(roc)
Assignee | ||
Comment 21•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/32411/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/32411/
Attachment #8712023 -
Flags: review?(roc)
Assignee | ||
Comment 22•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/32413/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/32413/
Attachment #8712024 -
Flags: review?(roc)
Attachment #8712022 -
Flags: review?(roc)
Comment on attachment 8712022 [details] MozReview Request: Bug1239752 - create ImageBitmap from ImageData should preserve alpha; r=roc https://reviewboard.mozilla.org/r/32409/#review29223 ::: dom/canvas/ImageBitmap.h:60 (Diff revision 1) > + bool mIsPreMultiplyAlphaApplied; Call this mIsPremultipliedAlpha here and everywhere else. ::: dom/canvas/ImageBitmap.cpp (Diff revision 1) > - return surface.forget(); Why are you removing this? ::: dom/canvas/test/imagebitmap_bug1239752.js:7 (Diff revision 1) > + var a = Math.floor(Math.random() * (255 - 50) + 50); I don't think our tests should test random values. A test should always try to test the same values. We probably also don't need 100 runs. I suggest providing a fixed list of say 10 different rgba values.
Attachment #8712023 -
Flags: review?(roc) → review+
Comment on attachment 8712023 [details] MozReview Request: Bug1239752 - clean up test files; r=roc https://reviewboard.mozilla.org/r/32411/#review29225
Comment on attachment 8712024 [details] MozReview Request: Bug1239752 - add test cases to make sure that alpha preserving is handled during transferring; r=roc https://reviewboard.mozilla.org/r/32413/#review29227 ::: dom/canvas/test/imagebitmap_structuredclone_utils.js:118 (Diff revision 1) > + data.push(Math.floor(Math.random() * (255 - 50) + 50)); Again, don't use random values in tests.
Attachment #8712024 -
Flags: review?(roc)
Assignee | ||
Comment 26•8 years ago
|
||
https://reviewboard.mozilla.org/r/32409/#review29223 > Call this mIsPremultipliedAlpha here and everywhere else. Ok. > Why are you removing this? I thought that cropping is applied ecah time PrepareForDrawTarget() is called and premultiply-alpha is a one-time effort, but it is not, both operations are one-time effort. I will rewrite this part and use the mSurface directly. > I don't think our tests should test random values. A test should always try to test the same values. We probably also don't need 100 runs. I suggest providing a fixed list of say 10 different rgba values. Okay.
Assignee | ||
Comment 27•8 years ago
|
||
https://reviewboard.mozilla.org/r/32413/#review29227 > Again, don't use random values in tests. Okay.
Assignee | ||
Comment 28•8 years ago
|
||
Comment on attachment 8712022 [details] MozReview Request: Bug1239752 - create ImageBitmap from ImageData should preserve alpha; r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32409/diff/1-2/
Attachment #8712022 -
Flags: review?(roc)
Assignee | ||
Comment 29•8 years ago
|
||
Comment on attachment 8712023 [details] MozReview Request: Bug1239752 - clean up test files; r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32411/diff/1-2/
Assignee | ||
Comment 30•8 years ago
|
||
Comment on attachment 8712024 [details] MozReview Request: Bug1239752 - add test cases to make sure that alpha preserving is handled during transferring; r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32413/diff/1-2/
Attachment #8712024 -
Flags: review?(roc)
Comment on attachment 8712022 [details] MozReview Request: Bug1239752 - create ImageBitmap from ImageData should preserve alpha; r=roc https://reviewboard.mozilla.org/r/32409/#review29635 ::: dom/canvas/test/imagebitmap_bug1239752.js:41 (Diff revisions 1 - 2) > var duration = 5; 'duration' is not the right word here. 'tolerance' sounds better. ::: dom/canvas/test/imagebitmap_bug1239752.js:61 (Diff revisions 1 - 2) > a - duration <= newA && newA <= a + duration; Use Math.abs(r - newR) <= duration etc. Simpler code.
Attachment #8712022 -
Flags: review?(roc) → review+
Comment on attachment 8712024 [details] MozReview Request: Bug1239752 - add test cases to make sure that alpha preserving is handled during transferring; r=roc https://reviewboard.mozilla.org/r/32413/#review29637 ::: dom/canvas/test/imagebitmap_structuredclone_utils.js:78 (Diff revision 2) > + var randomY = Math.floor(Math.random() * imageBitmap.height); Again, don't use randomness like this.
Attachment #8712024 -
Flags: review?(roc)
Assignee | ||
Comment 33•8 years ago
|
||
Comment on attachment 8712022 [details] MozReview Request: Bug1239752 - create ImageBitmap from ImageData should preserve alpha; r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32409/diff/2-3/
Assignee | ||
Comment 34•8 years ago
|
||
Comment on attachment 8712023 [details] MozReview Request: Bug1239752 - clean up test files; r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32411/diff/2-3/
Assignee | ||
Comment 35•8 years ago
|
||
Comment on attachment 8712024 [details] MozReview Request: Bug1239752 - add test cases to make sure that alpha preserving is handled during transferring; r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32413/diff/2-3/
Attachment #8712024 -
Flags: review?(roc)
Comment on attachment 8712024 [details] MozReview Request: Bug1239752 - add test cases to make sure that alpha preserving is handled during transferring; r=roc https://reviewboard.mozilla.org/r/32413/#review32637
Attachment #8712024 -
Flags: review?(roc) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8712022 -
Attachment description: MozReview Request: Bug1239752 - create ImageBitmap from ImageData should preserve alpha; r?roc → MozReview Request: Bug1239752 - create ImageBitmap from ImageData should preserve alpha; r=roc
Assignee | ||
Comment 37•8 years ago
|
||
Comment on attachment 8712022 [details] MozReview Request: Bug1239752 - create ImageBitmap from ImageData should preserve alpha; r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32409/diff/3-4/
Assignee | ||
Comment 38•8 years ago
|
||
Comment on attachment 8712023 [details] MozReview Request: Bug1239752 - clean up test files; r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32411/diff/3-4/
Attachment #8712023 -
Attachment description: MozReview Request: Bug1239752 - clean up test files; r?roc → MozReview Request: Bug1239752 - clean up test files; r=roc
Assignee | ||
Comment 39•8 years ago
|
||
Comment on attachment 8712024 [details] MozReview Request: Bug1239752 - add test cases to make sure that alpha preserving is handled during transferring; r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32413/diff/3-4/
Attachment #8712024 -
Attachment description: MozReview Request: Bug1239752 - add test cases to make sure that alpha preserving is handled during transferring; r?roc → MozReview Request: Bug1239752 - add test cases to make sure that alpha preserving is handled during transferring; r=roc
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 40•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc9415599767 https://hg.mozilla.org/integration/mozilla-inbound/rev/e4a9a3c7181a https://hg.mozilla.org/integration/mozilla-inbound/rev/71c56930fb7f
Keywords: checkin-needed
Comment 41•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cc9415599767 https://hg.mozilla.org/mozilla-central/rev/e4a9a3c7181a https://hg.mozilla.org/mozilla-central/rev/71c56930fb7f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•