Closed Bug 1239752 Opened 8 years ago Closed 8 years ago

createImageBitmap(ImageData) does not preserve alpha

Categories

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

46 Branch
defect
Not set
normal

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
Component: Untriaged → DOM
Keywords: testcase
Product: Firefox → Core
Thanks Xida for filing this!

kaku, would you have time to look at this.
Flags: needinfo?(tkuo)
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.
(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)
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.
> 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)
That sounds like a good and hopefully simple optimization.
Flags: needinfo?(bugs)
(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.
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.
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.
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.
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)
> 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....
Remove spaces at the end of line.
Attachment #8711543 - Attachment is obsolete: true
Attachment #8711543 - Flags: review?(bugs)
Attachment #8711547 - Flags: review?(bugs)
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.
Fix some typos.
Attachment #8711547 - Attachment is obsolete: true
Attachment #8711547 - Flags: review?(bugs)
Attachment #8711549 - Flags: review?(bugs)
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)
(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.
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)
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.
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)
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.
https://reviewboard.mozilla.org/r/32413/#review29227

> Again, don't use random values in tests.

Okay.
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)
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/
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)
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/
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/
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+
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
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/
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
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
Keywords: checkin-needed
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
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Component: DOM → DOM: Core & HTML
Regressions: 1739454
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: