Last Comment Bug 696569 - [webgl] Canvas problems with the Premultiplied Alpha context creation flag
: [webgl] Canvas problems with the Premultiplied Alpha context creation flag
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Jeff Gilbert [:jgilbert]
:
:
Mentors:
https://cvs.khronos.org/svn/repos/reg...
Depends on: 713143 730157 622184 650720 698169
Blocks: 728482 webgl-conformance
  Show dependency treegraph
 
Reported: 2011-10-22 04:51 PDT by Brian O'Keefe [:bokeefe]
Modified: 2012-03-24 13:42 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fixes WebGL context attrib for premultplied alpha, and unpack flag for premult-alpha (9.98 KB, patch)
2011-11-08 17:01 PST, Jeff Gilbert [:jgilbert]
jacob.benoit.1: feedback+
vladimir: feedback-
Details | Diff | Splinter Review
Prereq 1: Premultiply JPG data correctly (4.31 KB, patch)
2012-01-30 14:22 PST, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Treat data correctly based on its premultiplication status. (78.01 KB, patch)
2012-01-30 14:27 PST, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Allow for handling non-premult-alpha image/canvas data (38.41 KB, patch)
2012-03-19 13:59 PDT, Jeff Gilbert [:jgilbert]
jgilbert: review-
Details | Diff | Splinter Review
Change Cairo to clamp on premult values (1.47 KB, patch)
2012-03-19 14:03 PDT, Jeff Gilbert [:jgilbert]
jgilbert: review-
Details | Diff | Splinter Review
Add cairo patch (627 bytes, patch)
2012-03-19 14:04 PDT, Jeff Gilbert [:jgilbert]
jgilbert: review-
Details | Diff | Splinter Review
Allow for handling non-premult-alpha image/canvas data (35.96 KB, patch)
2012-03-19 17:27 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review-
Details | Diff | Splinter Review
Allow for handling non-premult-alpha image/canvas data (33.43 KB, patch)
2012-03-21 11:46 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
joe: review+
Details | Diff | Splinter Review
Allow for handling non-premult-alpha image/canvas data (33.43 KB, patch)
2012-03-23 14:19 PDT, Jeff Gilbert [:jgilbert]
jgilbert: review+
Details | Diff | Splinter Review

Description Brian O'Keefe [:bokeefe] 2011-10-22 04:51:36 PDT
The experimental-webgl context has some interesting behavior with regards to premultiplied alpha.

With premultipliedAlpha = true, the canvas stores premultiplied alpha.
With premultipliedAlpha = false, the canvas stores postmultiplied alpha.

Regardless of whether the canvas is storing pre- or post-multiplied alpha, Render always premultiplies the canvas. When using toDataUrl on WebGL canvases, this produces inconsistent results, causing the premultipliedAlpha test in the webgl conformance suite to fail.

(My laptop can't run WebGL, so I'll add the actual output when I can get to my desktop this afternoon).
Comment 1 Brian O'Keefe [:bokeefe] 2011-10-22 17:30:59 PDT
Results from the WebGL suite test (https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/context/premultiplyalpha-test.html)

Actual Results (r,g,b,a):
image/png,  premultiplied alpha:  34,66,128,128
no type,    premultiplied alpha:  33,65,129,128
image/png,  postmultiplied alpha: 1,1,1,1
no type,    postmultiplied alpha: 255,255,255,1
image/jpeg, postmultiplied alpha: 255,255,255,255*
image/jpeg, premultiplied alpha:  128,128,128,255*

Expected Results:
image/png,  premultiplied alpha:  64,128,255,128
no type,    premultiplied alpha:  64,128,255,128
image/png,  postmultiplied alpha: 255,192,128,1
no type,    postmultiplied alpha: 255,192,128,1
image/jpeg, postmultiplied alpha: 128,128,128,255
image/jpeg, premultiplied alpha:  128,128,128,255

* Bug 650720 changes these two results, so postmultiplied image/jpeg succeeds, and premultiplied image/jpeg fails
Comment 2 Jeff Gilbert [:jgilbert] 2011-11-08 17:01:57 PST
Created attachment 573047 [details] [diff] [review]
Fixes WebGL context attrib for premultplied alpha, and unpack flag for premult-alpha

This gets us most of the way there. In order to fully support this properly, we need to internally support surfaces with non-premultiplied-alpha RGBA formats, as well as fix JPEG handling of alpha as per bug 650720.
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2011-11-11 08:11:25 PST
Comment on attachment 573047 [details] [diff] [review]
Fixes WebGL context attrib for premultplied alpha, and unpack flag for premult-alpha

Review of attachment 573047 [details] [diff] [review]:
-----------------------------------------------------------------

Yup, I like all of that.
Comment 4 Vladimir Vukicevic [:vlad] [:vladv] 2011-11-14 16:34:50 PST
Comment on attachment 573047 [details] [diff] [review]
Fixes WebGL context attrib for premultplied alpha, and unpack flag for premult-alpha

>From: Jeff Gilbert <jgilbert@mozilla.com>
>Bug 696569 - Fix implementation of WebGL's unpack-premultiply-alpha setting and context attrib premultiplyAlpha
>
>diff --git a/content/canvas/src/WebGLContext.cpp b/content/canvas/src/WebGLContext.cpp
>--- a/content/canvas/src/WebGLContext.cpp
>+++ b/content/canvas/src/WebGLContext.cpp
>@@ -734,17 +734,20 @@ WebGLContext::Render(gfxContext *ctx, gf
>         return NS_OK;
> 
>     nsRefPtr<gfxImageSurface> surf = new gfxImageSurface(gfxIntSize(mWidth, mHeight),
>                                                          gfxASurface::ImageFormatARGB32);
>     if (surf->CairoStatus() != 0)
>         return NS_ERROR_FAILURE;
> 
>     gl->ReadPixelsIntoImageSurface(0, 0, mWidth, mHeight, surf);
>-    gfxUtils::PremultiplyImageSurface(surf);
>+
>+    if (!mOptions.premultipliedAlpha) {
>+        gfxUtils::PremultiplyImageSurface(surf);
>+    }

This looks good, and should fix the visible display of this canvas (unless a fast path for rendering is being used where we don't pull things out into a gfxImageSurface, note).


>diff --git a/content/canvas/src/WebGLContextGL.cpp b/content/canvas/src/WebGLContextGL.cpp
>--- a/content/canvas/src/WebGLContextGL.cpp
>+++ b/content/canvas/src/WebGLContextGL.cpp
>@@ -3960,18 +3960,16 @@ WebGLContext::DOMElementToImageSurface(n
>     gfxImageSurface *surf = nsnull;
> 
>     PRUint32 flags =
>         nsLayoutUtils::SFE_WANT_NEW_SURFACE |
>         nsLayoutUtils::SFE_WANT_IMAGE_SURFACE;
> 
>     if (mPixelStoreColorspaceConversion == LOCAL_GL_NONE)
>         flags |= nsLayoutUtils::SFE_NO_COLORSPACE_CONVERSION;
>-    if (!mPixelStorePremultiplyAlpha)
>-        flags |= nsLayoutUtils::SFE_NO_PREMULTIPLY_ALPHA;
> 
>     nsLayoutUtils::SurfaceFromElementResult res =
>         nsLayoutUtils::SurfaceFromElement(imageOrCanvas, flags);

This is wrong.  That flag has to be set to SurfaceFromElement, otherwise loading DOM images won't work correctly -- they'll always be premultiplied, regardless of whether the flag is set.

One additional bug might be that SurfaceFromElement needs to know about WebGL canvases specifically, and then needs to check the canvas' premultipliedAlpha flag and compare it to the NO_PREMULTIPLY_ALPHA flag that's passed in and do the right thing.

Note that if you have a premultiplied alpha webgl canvas, and you try using UNPACK_PREMULTIPLY_ALPHA_WEBGL = FALSE, I don't actually know what we can do.. we should un-premultiply certainly, but the data will already have been lost by that point.

> 
>+        bool dstPremultAlpha = mPixelStorePremultiplyAlpha;
>+

>         if (actualSrcFormat == dstFormat &&
>-            srcPremultiplied == mPixelStorePremultiplyAlpha &&
>+            srcPremultAlpha == dstPremultAlpha &&

This and similar is a no-op renaming change; ends up confusing this patch for no reason.


>@@ -4974,17 +4974,17 @@ WebGLContext::TexImage2D_dom(WebGLenum t
> 
>     PRUint32 byteLength = isurf->Stride() * isurf->Height();
> 
>     return TexImage2D_base(target, level, internalformat,
>                            isurf->Width(), isurf->Height(), isurf->Stride(), 0,
>                            format, type,
>                            isurf->Data(), byteLength,
>                            -1,
>-                           srcFormat, mPixelStorePremultiplyAlpha);
>+                           srcFormat, true);    // gfxImageSurface's RGBA is by definition alpha-premultiplied

This isn't correct; it will be either premultiplied or not, depending on the flag passed to SurfaceFromElement.
Comment 5 Jeff Gilbert [:jgilbert] 2011-11-14 17:27:53 PST
As for 'bool dstPremultAlpha = mPixelStorePremultiplyAlpha;', I think that this much better self-describes what the code is doing, and makes the use of 'srcPremultAlpha == dstPremultAlpha' really obvious. In this case, clarity doesn't cost anything (no perf cost), and is maybe more verbose, but I think it's really worth it, especially for eventual maintenance.
Comment 6 Vladimir Vukicevic [:vlad] [:vladv] 2011-11-21 20:09:56 PST
Sure, I wasn't necessarily saying that it was a bad change; just that it ends up confusing this patch when trying to read it -- should be done as a separate patch/checkin.
Comment 7 Jeff Gilbert [:jgilbert] 2011-12-22 16:59:45 PST
If IMG elements store their data as alpha-premultiplied, it will not be possible to pass test 3 on the test page. Tests 1, 3, 5, and 6 all depend on bug 713163, in order to guarantee that they work properly.

Tests 2 and 4 should be trivial, and we should pass those yesterday.
Comment 8 Jeff Gilbert [:jgilbert] 2012-01-04 02:36:02 PST
My current build is passing all but 1 and 3. For 1, (which should definitely be passable) we seem to be decoding the data properly in the PNG decoder (a printf there shows the correct RGBA values), but the data we get back is totally wrong, seemingly having been premult'd and unpremul'd an extra time. (This kills the data)

I'll see if I can get what I need out of joe.

It looks like test 3 should be passable as well, since there appears to be support for re-decoding image data, if our current data is the wrong premult value.
Comment 9 Jeff Gilbert [:jgilbert] 2012-01-30 14:22:39 PST
Created attachment 592860 [details] [diff] [review]
Prereq 1: Premultiply JPG data correctly
Comment 10 Jeff Gilbert [:jgilbert] 2012-01-30 14:27:08 PST
Created attachment 592861 [details] [diff] [review]
Treat data correctly based on its premultiplication status.

This is the current WIP patch. It works properly while the affected canvases are on-screen, but doesn't work 100% correctly for offscreen/background-tab canvases. Also, it leaks gfxASurfaces from somewhere.
Also also, it is a mess, because I don't know exactly what I need to do where in RasterImage and imgFrame.

Jeff, could you give me any pointers, or redirect me to someone better suited to going through this?
Comment 11 Jeff Gilbert [:jgilbert] 2012-02-22 15:13:01 PST
Comment on attachment 592861 [details] [diff] [review]
Treat data correctly based on its premultiplication status.

jlebar, joe said you might be able to give me some feedback on this RasterImage and imgFrame stuff. I don't really understand how the different parts are used, but I've gotten it to the point where as long as we render the image to the screen, it doesn't extraneously premultiply and unpremultiply itself. However, if the image is off-screen/in a background tab, it /does/ do an unnecessary pre+unpremult, losing data.

There is also a leak of gfxASurfaces, but I'm going to throw it through valgrind and see if I can't fix that.
Comment 12 Justin Lebar (not reading bugmail) 2012-02-23 02:52:44 PST
I'm pretty clueless about this stuff, but I'll try to help...

> as long as we render the image to the screen, it doesn't extraneously premultiply and 
> unpremultiply itself. However, if the image is off-screen/in a background tab, it /does/ 
> do an unnecessary pre+unpremult, losing data.

This, plus the sequences of 

 imgFrame->Lock()
 imgFrame->Unlock()

I see in the patch, suggest that you're seeing something to do with discarding.  Presumably you should force a complete sync decode of the image before doing anything with its data.  If you do this and the data isn't discarded in the meantime, I'm not sure why you'd see different behavior with or without on-screen data.

Those Lock(); Unlock(); pairs are pretty suspicious, by the way...
Comment 13 Jeff Gilbert [:jgilbert] 2012-02-23 16:27:47 PST
I've been having major issues with sync-decode, where it seems to sync-decode, but the data I get back is premult+unpremult'd, thus garbage. This is largely because of how the optimization for single-color surfaces works.

This stuff is really tricky because cairo will often mangle non-premultiplied data.

I think I'm just going to file a bug about this and have someone more knowledgeable take over.
Comment 14 Jeff Gilbert [:jgilbert] 2012-02-23 16:35:15 PST
Filed bug 730157 about this non-premultiplied data mangling.
Comment 15 Jeff Gilbert [:jgilbert] 2012-03-18 22:49:32 PDT
The end is in sight.
Most recent try push running at:
https://tbpl.mozilla.org/?tree=Try&rev=dd31601605d0
Comment 16 Jeff Gilbert [:jgilbert] 2012-03-19 05:44:22 PDT
Clean try run with passing premult-alpha tests.
I'll clean up the patch ASAP and post for review.
Comment 17 Jeff Gilbert [:jgilbert] 2012-03-19 13:59:26 PDT
Created attachment 607305 [details] [diff] [review]
Allow for handling non-premult-alpha image/canvas data

Minimize and cleaned up the patch.
I did remove a few things I think were unneeded, so it's back up to try.
Try running at:
https://tbpl.mozilla.org/?tree=Try&rev=6919c8d25756
Comment 18 Jeff Gilbert [:jgilbert] 2012-03-19 14:03:46 PDT
Created attachment 607307 [details] [diff] [review]
Change Cairo to clamp on premult values

This is somewhat terrifying, but seems to work fine.
Comment 19 Jeff Gilbert [:jgilbert] 2012-03-19 14:04:34 PDT
Created attachment 607308 [details] [diff] [review]
Add cairo patch

And here's adding the patch to Cairo's patch list.
Comment 20 Jeff Gilbert [:jgilbert] 2012-03-19 15:39:29 PDT
Comment on attachment 607307 [details] [diff] [review]
Change Cairo to clamp on premult values

Since we're already working on removing single-color optimizations for non-trivial surfaces, we're just going to disallow single-color opts for non-premult surfaces. This means we don't need non-premult support for single-color inputs to Cairo.
Comment 21 Jeff Gilbert [:jgilbert] 2012-03-19 15:40:19 PDT
Comment on attachment 607305 [details] [diff] [review]
Allow for handling non-premult-alpha image/canvas data

Removing support for non-premult single-color optimizations.
Comment 22 Jeff Muizelaar [:jrmuizel] 2012-03-19 15:52:00 PDT
Comment on attachment 607307 [details] [diff] [review]
Change Cairo to clamp on premult values

Review of attachment 607307 [details] [diff] [review]:
-----------------------------------------------------------------

We can avoid the need for this by not optimizing to solid colors when necessary.
Comment 23 Jeff Gilbert [:jgilbert] 2012-03-19 17:27:57 PDT
Created attachment 607388 [details] [diff] [review]
Allow for handling non-premult-alpha image/canvas data

Running on try at:
https://tbpl.mozilla.org/?tree=Try&rev=dd792ffb86b5
Comment 24 Benoit Jacob [:bjacob] (mostly away) 2012-03-20 04:13:57 PDT
Comment on attachment 607388 [details] [diff] [review]
Allow for handling non-premult-alpha image/canvas data

Review of attachment 607388 [details] [diff] [review]:
-----------------------------------------------------------------

I reviewed all except the image/ files. You need another reviewer for them. Joe maybe?

r- for nits, especially the boolean argument.

::: content/canvas/public/nsICanvasElementExternal.h
@@ +77,5 @@
>     * to the given gfxContext at the origin of its coordinate space.
>     */
>    NS_IMETHOD RenderContextsExternal(gfxContext *ctx,
> +                                    gfxPattern::GraphicsFilter aFilter,
> +                                    bool premultAlpha = true) = 0;

I hate bool arguments, as it makes user code very hard to read (what does "true" mean in RenderContextsExternal(ctx, filter, true)? ). Both MFBT style and Google style proscribe them. The simplest fix is to replace that by a enum argument (NoPremult, Premult). But if there is the slightest chance that there might be more such bool args to pass later, then a bit-field might be a good idea.

::: content/canvas/public/nsICanvasRenderingContextInternal.h
@@ +84,5 @@
>  
>    NS_IMETHOD InitializeWithSurface(nsIDocShell *docShell, gfxASurface *surface, PRInt32 width, PRInt32 height) = 0;
>  
>    // Render the canvas at the origin of the given gfxContext
> +  NS_IMETHOD Render(gfxContext *ctx, gfxPattern::GraphicsFilter aFilter, bool premultAlpha = true) = 0;

Same.

::: content/canvas/src/WebGLContext.cpp
@@ +560,5 @@
>      return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +WebGLContext::Render(gfxContext *ctx, gfxPattern::GraphicsFilter f, bool premultAlpha)

Same.

@@ +572,5 @@
>          return NS_ERROR_FAILURE;
>  
>      gl->ReadPixelsIntoImageSurface(0, 0, mWidth, mHeight, surf);
> +    unsigned char* data = surf->Data();
> +    printf_stderr("[WebGLContext::Render] readPixels (BGRA): %d, %d, %d, %d\n", data[0], data[1], data[2], data[3]);

remove debug printf.

@@ +578,5 @@
> +    bool srcPremultAlpha = mOptions.premultipliedAlpha;
> +    bool dstPremultAlpha = premultAlpha;
> +
> +    printf_stderr("[WebGLContext::Render] srcPremultAlpha: %s\n", srcPremultAlpha ? "true" : "false");
> +    printf_stderr("[WebGLContext::Render] dstPremultAlpha: %s\n", dstPremultAlpha ? "true" : "false");

remove debug printf. More occurrences below, please remove all.

::: content/canvas/src/WebGLContext.h
@@ +541,5 @@
>      NS_IMETHOD InitializeWithSurface(nsIDocShell *docShell, gfxASurface *surface, PRInt32 width, PRInt32 height)
>          { return NS_ERROR_NOT_IMPLEMENTED; }
>      NS_IMETHOD Reset()
>          { /* (InitializeWithSurface) */ return NS_ERROR_NOT_IMPLEMENTED; }
> +    NS_IMETHOD Render(gfxContext *ctx, gfxPattern::GraphicsFilter f, bool premultAlpha = true);

Same.

::: content/canvas/src/WebGLContextGL.cpp
@@ +5058,5 @@
>          size_t dstPlainRowSize = texelSize * width;
>          size_t unpackAlignment = mPixelStoreUnpackAlignment;
>          size_t dstStride = ((dstPlainRowSize + unpackAlignment-1) / unpackAlignment) * unpackAlignment;
>  
> +        PRUint8* pixels = (PRUint8*)data;

Isn't this cast useless?   PRUint8* pixels(data);

::: content/canvas/src/nsCanvasRenderingContext2D.cpp
@@ +349,5 @@
>      NS_IMETHOD SetDimensions(PRInt32 width, PRInt32 height);
>      void Initialize(nsIDocShell *shell, PRInt32 width, PRInt32 height);
>      NS_IMETHOD InitializeWithSurface(nsIDocShell *shell, gfxASurface *surface, PRInt32 width, PRInt32 height);
>      bool EnsureSurface();
> +    NS_IMETHOD Render(gfxContext *ctx, gfxPattern::GraphicsFilter aFilter, bool premultAlpha = true);

Same, etc. Not mentioning other occurrences of this.

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +1416,5 @@
>  
> +  if (!premultAlpha) {
> +      nsRefPtr<gfxASurface> curSurface = ctx->CurrentSurface();
> +      nsRefPtr<gfxImageSurface> gis = curSurface->GetAsImageSurface();
> +      NS_ABORT_IF_FALSE(!!gis, "If non-premult alpha, must be able to get image surface!");

The !! is useless here, as just gis will be casted to bool, which will use the cast to plain pointer and check for null.

::: content/canvas/test/webgl/failing_tests_linux.txt
@@ -1,1 @@
> -conformance/context/premultiplyalpha-test.html

\o/

::: gfx/thebes/gfxUtils.cpp
@@ +245,5 @@
> +            src[1] = buffer[1];
> +            src[2] = buffer[2];
> +            //src[3] = buffer[3];
> +
> +            src += 4;

Redundant index/pointer increments. Use the pointer as the loop index, like:
T *start, stop;
for (T *ptr = start; ptr < stop; ptr++)
  do_something(ptr);

@@ +248,5 @@
> +
> +            src += 4;
> +        }
> +    } else {
> +        for (PRUint32 i = 0; i < dim; ++i) {

Same.
Comment 25 Jeff Gilbert [:jgilbert] 2012-03-21 02:10:31 PDT
Try run is clean.

Thought I remove the printfs, but oh well.
Bitfield is probably best for the premult argument. Hopefully nothing will need to be added like this, but it'll make it 'easy' to do so.

I'll clean up the index/pointer stuff.

I'll post the fix later today and attach Joe or someone as an additional reviewer.
Comment 26 Jeff Gilbert [:jgilbert] 2012-03-21 11:46:37 PDT
Created attachment 608043 [details] [diff] [review]
Allow for handling non-premult-alpha image/canvas data

Try at:
https://tbpl.mozilla.org/?tree=Try&rev=00d1404a4151
Comment 27 Jeff Gilbert [:jgilbert] 2012-03-21 11:47:29 PDT
Comment on attachment 608043 [details] [diff] [review]
Allow for handling non-premult-alpha image/canvas data

Adding Joe for image/* review.
Comment 28 Benoit Jacob [:bjacob] (mostly away) 2012-03-23 08:30:19 PDT
Comment on attachment 608043 [details] [diff] [review]
Allow for handling non-premult-alpha image/canvas data

Review of attachment 608043 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but please address the nits below:

::: content/canvas/public/nsICanvasElementExternal.h
@@ +67,5 @@
>  public:
>    NS_DECLARE_STATIC_IID_ACCESSOR(NS_ICANVASELEMENTEXTERNAL_IID)
>  
> +  enum {
> +    FLAG_RENDER_PREMULT_ALPHA = 0x1

We don't use THAT_STYLE for enum constants, rather we tend to use CamelCaseLikeThis.

::: gfx/thebes/gfxUtils.cpp
@@ +215,5 @@
> +                            gfxImageSurface *aDestSurface) {
> +    if (!aDestSurface)
> +        aDestSurface = aSourceSurface;
> +
> +    NS_ASSERTION(aSourceSurface->Format() == aDestSurface->Format() &&

We're risk a crash if this assertion is false, right? So it should at least be NS_ABORT_IF_FALSE. Won't make a difference in release buids, but will help catching/debugging.

@@ +222,5 @@
> +                 aSourceSurface->Stride() == aDestSurface->Stride(),
> +                 "Source and destination surfaces don't have identical characteristics");
> +
> +    NS_ASSERTION(aSourceSurface->Stride() == aSourceSurface->Width() * 4,
> +                 "Source surface stride isn't tightly packed");

Do you know a case where this assertion might fail? I am tempted to make it NS_ABORT_IF_FALSE so we'll know if that ever happens (in debug builds).

@@ +225,5 @@
> +    NS_ASSERTION(aSourceSurface->Stride() == aSourceSurface->Width() * 4,
> +                 "Source surface stride isn't tightly packed");
> +
> +    NS_ASSERTION(aSourceSurface->Format() == gfxASurface::ImageFormatARGB32,
> +                 "Surfaces must be ARGB32");

Since this seems to be guarding against a plain programming mistake on the part of the caller of this function, NS_ABORT_IF_FALSE seems appropriate.
Comment 29 Jeff Gilbert [:jgilbert] 2012-03-23 14:19:13 PDT
Created attachment 608861 [details] [diff] [review]
Allow for handling non-premult-alpha image/canvas data

Carrying forward r+ with nit fixes.
Comment 31 Ed Morley [:emorley] 2012-03-24 13:42:39 PDT
https://hg.mozilla.org/mozilla-central/rev/1e64ae9b4009

Note You need to log in before you can comment on or make changes to this bug.