1028288 - globalAlpha doesn't work for drawImage

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Smeeckaert Martin]

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.

Comment 1

[Alice0775 White]

Could you attach minimal test case?

Comment 2

[Smeeckaert Martin]

Attachment: Svg image i will draw

Comment 3

[Smeeckaert Martin]

Attachment: Html file with javascript bug

Comment 4

[Smeeckaert Martin]

Attachment: Result in chrome

Comment 5

[Smeeckaert Martin]

Attachment: Result in firefox

Comment 6

[Alice0775 White]

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

Comment 7

[James Kolb]

Attachment: 1028288.patch

Passes opacity down to gfxDrawable which uses fillWithOpacity instead of fill.

I'll add reftests once I get a chance.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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;

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 10

[James Kolb]

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.

Comment 11

[James Kolb]

Attachment: Add reftests

Comment 12

[James Kolb]

Attachment: 1028288.patch (v2)

Fixed if statement

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 14

[James Kolb]

Attachment: Reftest patch (v2)

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8462289 [details] [diff] [review]
Reftest patch (v2)

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

Thanks!!!

Comment 16

[James Kolb]

Can somebody push this to the try-server?

Comment 17

[Masatoshi Kimura [:emk]]

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?

Comment 18

[Masatoshi Kimura [:emk]]

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

Comment 19

[James Kolb]

Attachment: 1028288.patch (v3)

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

Comment 20

[James Kolb]

Attachment: reftest.patch (v3)

Switched to use git-style diff.

Comment 21

[Masatoshi Kimura [:emk]]

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

Comment 22

[James Kolb]

Attachment: 1028288.patch (v4)

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

Comment 23

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=c956eaa2c3e0

Comment 24

[James Kolb]

Attachment: reftest.patch (v4)

Fixed reftest for case sensitivity.

Comment 25

[Masatoshi Kimura [:emk]]

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

Comment 26

[James Kolb]

Attachment: 1028288.patch (v5)

Changed patch to handle new gfxSurfaceDrawable::Draw code.

This is my last attempt for a while. I'm off to go get married.

Comment 27

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=a2b3b9920452

Comment 28

[Masatoshi Kimura [:emk]]

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

Comment 29

[Masatoshi Kimura [:emk]]

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

Comment 30

[Masatoshi Kimura [:emk]]

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

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Masatoshi Kimura [:emk]]

James, could you request uplift when you have time?

Comment 33

[Masatoshi Kimura [:emk]]

Comment on attachment 8463227 [details] [diff] [review]
1028288.patch (v3)

The v3 patch would be appropriate for branches.

Comment 34

[Seth Fowler [:seth] [:s2h]]

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?

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sure.

Comment 36

[Seth Fowler [:seth] [:s2h]]

OK, so this was backed out on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d997e5accc3f

Comment 37

[Seth Fowler [:seth] [:s2h]]

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.

Comment 38

[James Kolb]

Attachment: 1028288 with old maybe code

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 39

[James Kolb]

Attachment: 1028288.patch (v6)

Comment 40

[Seth Fowler [:seth] [:s2h]]

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.

Comment 41

[Seth Fowler [:seth] [:s2h]]

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!

Comment 42

[James Kolb]

Attachment: 1028288.patch (v7)

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.

Comment 43

[James Kolb]

Can somebody push this to try? Thanks!

Comment 44

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=3e0947f215b3

Comment 45

[Masatoshi Kimura [:emk]]

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

Comment 46

[Ryan VanderMeulen [:RyanVM]]

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