User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.153 Safari/537.36 Steps to reproduce: In javascript: ctx = document.getElementById('canvas').getContext('2d'); ctx.save(); ctx.globalAlpha = 0.5; ctx.drawImage(..., 0, 0, 100, 100); ctx.restore(); Actual results: The image is drawn but the opacity is not applied. cf. http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-globalalpha Expected results: The image should be half transparent. It used to work in earlier firefox version.

Could you attach minimal test case?

Regression window(m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/04f5a7e8f14c Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140112194044 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/c305df97cbc6 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140112200445 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=04f5a7e8f14c&tochange=c305df97cbc6 Suspect : Bug 603488

Passes opacity down to gfxDrawable which uses fillWithOpacity instead of fill. I'll add reftests once I get a chance.

Comment on attachment 8455892 [details] [diff] [review] 1028288.patch Review of attachment 8455892 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/src/VectorImage.cpp @@ +880,4 @@ > aSVGContext, animTime, aFlags)); > } > > + gfxFloat opacity = 1.0; gfxFloat opacity = aSVGContext ? aSVGContext->GetGlobalOpacity() : 1.0;

This isn't optimal because we fall back to rasterization when opacity != 1.0, whereas in many situations we could be more clever, but it's a lot better than the status quo.

Yes, it definitely could be better. It still rasterizes it late enough to preserve the visual appearance fix from 603488 and fixes the regression 603488 caused. I'm crazy busy right now, so I don't have the time to write something more optimal.

Fixed if statement

Comment on attachment 8460740 [details] [diff] [review] Add reftests Review of attachment 8460740 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, just some minor comments... ::: layout/reftests/svg/as-image/canvas-drawImage-alpha-2.html @@ +11,5 @@ > + > + // Instantiate an image. Draw it at a larger size, & take a snapshot > + var image = new Image(); > + image.onload = function() { > + ctx.save(); Why this save() call? @@ +16,5 @@ > + ctx.globalAlpha = 0.75; > + ctx.drawImage(image, 0, 0, 200, 200); > + document.documentElement.removeAttribute("class"); > + } > + image.src ="squaredCircle-viewbox-100x100.svg"; Simpler to just declare an <img> next to the canvas and use that. Then we don't need to wait for its load event explicitly, since document load will be blocked by it. Same comments apply to other test.

Comment on attachment 8462289 [details] [diff] [review] Reftest patch (v2) Review of attachment 8462289 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!!!

Can somebody push this to the try-server?

This patch didn't apply. $ hg qimp -e 1028288.patch && hg qpush adding 1028288.patch to series file applying 1028288.patch patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh 1028288.patch $ patch -p1 -R < ../1028288.patch patching file content/canvas/src/CanvasRenderingContext2D.cpp Hunk #2 succeeded at 3432 (offset -4 lines). Hunk #4 succeeded at 3469 (offset -4 lines). patching file content/canvas/src/CanvasRenderingContext2D.h patching file gfx/thebes/gfxDrawable.cpp patching file gfx/thebes/gfxDrawable.h patching file gfx/thebes/gfxUtils.cpp Hunk #2 succeeded at 631 (offset -10 lines). patching file gfx/thebes/gfxUtils.h patching file image/src/VectorImage.cpp patch: **** malformed patch at line 233: @@ -912,7 +919,7 @@ $ hg import ../1028288.patch applying ../1028288.patch abort: bad hunk #1 @@ -880,18 +880,25 @@ (23 18 25 25) How did you generate this patch?

Did you manually edit the patch directly? Please do not! Please update the original source and take the diff again. And please read <https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F>.

Sorry, I submitted the wrong version. Also it's now fixed to use git formatting.

Switched to use git-style diff.

Comment on attachment 8463227 [details] [diff] [review] 1028288.patch (v3) This patch no longer applies because content/canvas/src has been moved to dom/canvas: applying 1028288.patch unable to find 'content/canvas/src/CanvasRenderingContext2D.cpp' for patching 3 out of 3 hunks FAILED -- saving rejects to file content/canvas/src/CanvasRende ringContext2D.cpp.rej unable to find 'content/canvas/src/CanvasRenderingContext2D.h' for patching 1 out of 1 hunks FAILED -- saving rejects to file content/canvas/src/CanvasRende ringContext2D.h.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh 1028288.patch

Rebased due to yesterday's code changes. (Reftest is still good)

Fixed reftest for case sensitivity.

Bitrotted again. Some non-obvious changes would be needed... $ hg qgoto 1 applying 1028288.patch patching file gfx/thebes/gfxDrawable.cpp Hunk #1 FAILED at 123 Hunk #2 FAILED at 157 2 out of 4 hunks FAILED -- saving rejects to file gfx/thebes/gfxDrawable.cpp.rej patching file gfx/thebes/gfxDrawable.h Hunk #1 FAILED at 33 Hunk #2 FAILED at 62 2 out of 4 hunks FAILED -- saving rejects to file gfx/thebes/gfxDrawable.h.rej patching file image/src/VectorImage.cpp Hunk #3 FAILED at 943 1 out of 3 hunks FAILED -- saving rejects to file image/src/VectorImage.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh 1028288.patch

Changed patch to handle new gfxSurfaceDrawable::Draw code. This is my last attempt for a while. I'm off to go get married.

Adding another entropy source for canvas fingerprinting :) https://tbpl.mozilla.org/?tree=Try&rev=3b16b819162e

!layersGPUAccelerated didn't exclude WinXP... https://tbpl.mozilla.org/?tree=Try&rev=8b8ea9be27cd

https://hg.mozilla.org/integration/mozilla-inbound/rev/950a3afc2b15 https://hg.mozilla.org/integration/mozilla-inbound/rev/dfb5303af6f9

https://hg.mozilla.org/mozilla-central/rev/950a3afc2b15 https://hg.mozilla.org/mozilla-central/rev/dfb5303af6f9

James, could you request uplift when you have time?

Comment on attachment 8463227 [details] [diff] [review] 1028288.patch (v3) The v3 patch would be appropriate for branches.

Please don't uplift this. I'd actually like this backed out. It's not safe to pass a default-constructed SVGPreserveAspectRatio value to imgIContainer::Draw. (I changed the API to make it possible to do something like what James wants to do in bug 1043560, but before that bug's changes you just couldn't use SVGImageContext this way.) Furthermore, I think the imagelib-related changes need another pass, so I'd like to review them before they land. Basically, I want to back this out and retroactively r- it. roc, as the reviewer, are you OK with that?

Sure.

OK, so this was backed out on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/d997e5accc3f

Comment on attachment 8465061 [details] [diff] [review] 1028288.patch (v5) Review of attachment 8465061 [details] [diff] [review]: ----------------------------------------------------------------- I'm sorry about backing out your patch, James, but it shouldn't be too hard to fix up these issues and get it landed again! ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +3471,5 @@ > > // FLAG_CLAMP is added for increased performance > uint32_t modifiedFlags = image.mDrawingFlags | imgIContainer::FLAG_CLAMP; > > + SVGImageContext svgContext(SVGPreserveAspectRatio(), opacity); This is the primary issue that motivated me to get this backed out. It's not safe to pass a default-constructed SVGPreserveAspectRatio value, as I mentioned in an earlier comment. Such a value has *_UNKNOWN values for both mAlign and mMeetOrSlice, and the code which handles preserveAspectRatio does not check for or handle those values. This is a known issue, and I thought there was already a bug open about it, but I couldn't find one, so I filed bug 1049949. To fix this, I'd recommend changing the type of the mPreserveAspectRatio member of SVGImageContext from SVGPreserveAspectRatio to Maybe<SVGPreserveAspectRatio>. I've actually already done this as part of bug 1043560, so by the time you return to this code that will probably already have been done for you. If for some reason that code hasn't landed yet when you come back, it should be easy enough to make the change in this patch instead. You can copy the code I wrote in part 3 of bug 1043560. ::: dom/canvas/CanvasRenderingContext2D.h @@ +667,5 @@ > void DrawDirectlyToCanvas(const nsLayoutUtils::DirectDrawInfo& image, > mozilla::gfx::Rect* bounds, double dx, double dy, > double dw, double dh, double sx, double sy, > + double sw, double sh, gfxIntSize imgSize, > + gfxFloat opacity); You probably don't need this extra argument, since you can call |CurrentState().globalAlpha| from inside DrawDirectlyToCanvas. ::: gfx/thebes/gfxDrawable.cpp @@ +50,5 @@ > DrawTarget* dt = aContext->GetDrawTarget(); > > if (aContext->CurrentOperator() == gfxContext::OPERATOR_CLEAR) { > dt->ClearRect(fillRect); > + } else if (aContext->CurrentOperator() == gfxContext::OPERATOR_SOURCE && aOpacity == 1.0) { Just a minor nit: we usually try to keep each line under 80 characters long. ::: image/src/VectorImage.cpp @@ +879,5 @@ > SurfaceKey(params.imageRect.Size(), params.scale, > aSVGContext, animTime, aFlags)); > } > > + gfxFloat opacity = aSVGContext ? aSVGContext->GetGlobalOpacity() : 1.0; |opacity| should become a field on the SVGDrawingParameters type, defined in this file, and you should set its value in the initializer list. (We're already passing aSVGContext to the SVGDrawingParameters constructor.) Once you make that change, you can get rid of many of the other changes in this file. @@ +892,5 @@ > return NS_OK; > } > > void > +VectorImage::CreateDrawableAndShow(const SVGDrawingParameters& aParams, gfxFloat aOpacity) Once |opacity| is a field on SVGDrawingParameters you won't need this change. @@ +955,5 @@ > } > > > void > +VectorImage::Show(gfxDrawable* aDrawable, const SVGDrawingParameters& aParams, gfxFloat aOpacity) And you won't need this one either. And that will mean you don't need to update any of the call sites. @@ +963,5 @@ > aParams.userSpaceToImageSpace, > aParams.subimage, aParams.sourceRect, > ThebesIntRect(aParams.imageRect), aParams.fill, > SurfaceFormat::B8G8R8A8, > + aParams.filter, aParams.flags, aOpacity); Here, instead of using |aOpacity|, you'll end up just using |aParams.opacity|. ::: layout/svg/SVGImageContext.h @@ +19,5 @@ > SVGImageContext() { } > > + SVGImageContext(SVGPreserveAspectRatio aPreserveAspectRatio, > + gfxFloat aOpacity = 1.0) > + : mPreserveAspectRatio(aPreserveAspectRatio), mGlobalOpacity(aOpacity) Another minor nit: Please initialize |mGlobalOpacity| on a separate line, lining up the "," with the ":" on the line above. @@ +34,2 @@ > bool operator==(const SVGImageContext& aOther) const { > return mPreserveAspectRatio == aOther.mPreserveAspectRatio; You need to update |operator==| to take mGlobalOpacity into account. @@ +39,5 @@ > return !(*this == aOther); > } > > uint32_t Hash() const { > return mPreserveAspectRatio.Hash(); You also need to update |Hash()| to take mGlobalOpacity into account. For an example, take a look at how SurfaceKey::Hash() is implemented in SurfaceCache.h.

That makes sense. Here's what I got working using the old maybe code. I'll add a real patch once the new maybe code is finalized.

Comment on attachment 8478081 [details] [diff] [review] 1028288.patch (v6) Review of attachment 8478081 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxDrawable.h @@ +38,5 @@ > const gfxRect& aFillRect, > bool aRepeat, > const GraphicsFilter& aFilter, > + const gfxMatrix& aTransform = gfxMatrix(), > + gfxFloat aOpacity = 1.0) = 0; I'd prefer the order of aTransform and aOpacity to be reversed, since I expect to need to pass aOpacity much more frequently than I need to pass aTransform. ::: layout/svg/SVGImageContext.h @@ +56,5 @@ > uint32_t Hash() const { > return HashGeneric(mViewportSize.width, > + mViewportSize.height, > + mPreserveAspectRatio.map(HashPAR).valueOr(0), > + HashBytes(&mGlobalOpacity, sizeof(gfxFloat))); Do you need HashBytes here? I'd think just passing mGlobalOpacity to HashGeneric directly would be enough. Also, is Splinter displaying this wrong, or did all of this get massively indented? If so, indent it back so that all of the arguments line up together.

Thanks for rebasing your patch and getting everything cleaned up, by the way! Again, I'm sorry for the backout. It looks like the results are worth it: the final version of the patch looks very good!

Fixed indentation and swapped the argument order. I'm using HashBytes instead of HashGeneric because HashGeneric casts to int and would give the same result for all mGlobalOpacity < 1.

Can somebody push this to try? Thanks!

Try looks good. https://hg.mozilla.org/integration/mozilla-inbound/rev/d02d3b4f448f https://hg.mozilla.org/integration/mozilla-inbound/rev/32ea974e1492

https://hg.mozilla.org/mozilla-central/rev/d02d3b4f448f https://hg.mozilla.org/mozilla-central/rev/32ea974e1492