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).

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

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 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 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.

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.

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.

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.

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.

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 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.

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...

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.

Filed bug 730157 about this non-premultiplied data mangling.

The end is in sight. Most recent try push running at: https://tbpl.mozilla.org/?tree=Try&rev=dd31601605d0

Clean try run with passing premult-alpha tests. I'll clean up the patch ASAP and post for review.

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

This is somewhat terrifying, but seems to work fine.

And here's adding the patch to Cairo's patch list.

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 on attachment 607305 [details] [diff] [review] Allow for handling non-premult-alpha image/canvas data Removing support for non-premult single-color optimizations.

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.

Running on try at: https://tbpl.mozilla.org/?tree=Try&rev=dd792ffb86b5

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.

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.

Try at: https://tbpl.mozilla.org/?tree=Try&rev=00d1404a4151

Comment on attachment 608043 [details] [diff] [review] Allow for handling non-premult-alpha image/canvas data Adding Joe for image/* review.

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.

Carrying forward r+ with nit fixes.