globalAlpha doesn't work for drawImage

RESOLVED FIXED in mozilla34

Status

()

Core
Canvas: 2D
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Smeeckaert Martin, Assigned: James Kolb)

Tracking

({regression, testcase})

29 Branch
mozilla34
x86_64
Windows 7
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox30 affected, firefox31 affected, firefox32 affected, firefox33 affected, firefox-esr24 unaffected)

Details

Attachments

(6 attachments, 10 obsolete attachments)

1.94 KB, image/svg+xml
Details
400 bytes, text/html
Details
4.87 KB, image/png
Details
18.17 KB, image/png
Details
4.60 KB, patch
James Kolb
: review+
Details | Diff | Splinter Review
17.37 KB, patch
James Kolb
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Component: Untriaged → Canvas: 2D
Product: Firefox → Core
Keywords: regressionwindow-wanted

Comment 1

4 years ago
Could you attach minimal test case?
Flags: needinfo?(martin.smeeckaert)
Keywords: testcase-wanted
(Reporter)

Comment 2

4 years ago
Created attachment 8445413 [details]
Svg image i will draw
(Reporter)

Comment 3

4 years ago
Created attachment 8445414 [details]
Html file with javascript bug
Flags: needinfo?(martin.smeeckaert)
(Reporter)

Comment 4

4 years ago
Created attachment 8445417 [details]
Result in chrome
(Reporter)

Comment 5

4 years ago
Created attachment 8445418 [details]
Result in firefox

Comment 6

4 years ago
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
Blocks: 603488
Status: UNCONFIRMED → NEW
status-firefox30: --- → affected
status-firefox31: --- → affected
status-firefox32: --- → affected
status-firefox33: --- → affected
status-firefox-esr24: --- → unaffected
Ever confirmed: true
Keywords: regressionwindow-wanted, testcase-wanted → regression, testcase
Version: 30 Branch → 29 Branch
(Assignee)

Updated

4 years ago
Assignee: nobody → jck1089
(Assignee)

Comment 7

4 years ago
Created attachment 8455892 [details] [diff] [review]
1028288.patch

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

I'll add reftests once I get a chance.
Attachment #8455892 - Flags: review?(roc)
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;
Attachment #8455892 - Flags: review?(roc) → review+
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.
(Assignee)

Comment 10

4 years ago
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.
(Assignee)

Comment 11

4 years ago
Created attachment 8460740 [details] [diff] [review]
Add reftests
Attachment #8460740 - Flags: review?(roc)
(Assignee)

Comment 12

4 years ago
Created attachment 8460741 [details] [diff] [review]
1028288.patch (v2)

Fixed if statement
Attachment #8455892 - Attachment is obsolete: true
Attachment #8460741 - Flags: review+
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.
Attachment #8460740 - Flags: review?(roc) → review-
(Assignee)

Comment 14

4 years ago
Created attachment 8462289 [details] [diff] [review]
Reftest patch (v2)
Attachment #8460740 - Attachment is obsolete: true
Attachment #8462289 - Flags: review?(roc)
Comment on attachment 8462289 [details] [diff] [review]
Reftest patch (v2)

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

Thanks!!!
Attachment #8462289 - Flags: review?(roc) → review+
(Assignee)

Comment 16

4 years ago
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>.
Flags: needinfo?(jck1089)
(Assignee)

Comment 19

4 years ago
Created attachment 8463227 [details] [diff] [review]
1028288.patch (v3)

Sorry, I submitted the wrong version. Also it's now fixed to use git formatting.
Attachment #8460741 - Attachment is obsolete: true
Attachment #8463227 - Flags: review+
Flags: needinfo?(jck1089)
(Assignee)

Comment 20

4 years ago
Created attachment 8463228 [details] [diff] [review]
reftest.patch (v3)

Switched to use git-style diff.
Attachment #8462289 - Attachment is obsolete: true
Attachment #8463228 - Flags: review+
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
(Assignee)

Comment 22

4 years ago
Created attachment 8463763 [details] [diff] [review]
1028288.patch (v4)

Rebased due to yesterday's code changes. (Reftest is still good)
Attachment #8463227 - Attachment is obsolete: true
Attachment #8463763 - Flags: review+
(Assignee)

Comment 24

4 years ago
Created attachment 8464378 [details] [diff] [review]
reftest.patch (v4)

Fixed reftest for case sensitivity.
Attachment #8463228 - Attachment is obsolete: true
Attachment #8464378 - Flags: review+
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
(Assignee)

Comment 26

4 years ago
Created attachment 8465061 [details] [diff] [review]
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.
Attachment #8463763 - Attachment is obsolete: true
Attachment #8465061 - Flags: review?(roc)
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/mozilla-central/rev/950a3afc2b15
https://hg.mozilla.org/mozilla-central/rev/dfb5303af6f9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
James, could you request uplift when you have time?
Flags: needinfo?(jck1089)
Comment on attachment 8463227 [details] [diff] [review]
1028288.patch (v3)

The v3 patch would be appropriate for branches.
Attachment #8463227 - Attachment is obsolete: false
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?
Flags: needinfo?(roc)
OK, so this was backed out on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d997e5accc3f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8463227 - Attachment is obsolete: true
Attachment #8465061 - Flags: review?(seth)
Flags: in-testsuite+ → in-testsuite?
Target Milestone: mozilla34 → ---
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.
Attachment #8465061 - Flags: review?(seth) → review-
(Assignee)

Updated

4 years ago
Depends on: 913586
Flags: needinfo?(jck1089)
(Assignee)

Comment 38

4 years ago
Created attachment 8474311 [details] [diff] [review]
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.
Attachment #8465061 - Attachment is obsolete: true
(Assignee)

Comment 39

4 years ago
Created attachment 8478081 [details] [diff] [review]
1028288.patch (v6)
Attachment #8474311 - Attachment is obsolete: true
Attachment #8478081 - Flags: review?(seth)
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.
Attachment #8478081 - Flags: review?(seth) → review+
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!
(Assignee)

Comment 42

4 years ago
Created attachment 8479596 [details] [diff] [review]
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.
Attachment #8478081 - Attachment is obsolete: true
Attachment #8479596 - Flags: review+
(Assignee)

Comment 43

4 years ago
Can somebody push this to try? Thanks!
https://hg.mozilla.org/mozilla-central/rev/d02d3b4f448f
https://hg.mozilla.org/mozilla-central/rev/32ea974e1492
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34

Updated

4 years ago
Duplicate of this bug: 1074948
You need to log in before you can comment on or make changes to this bug.