672646 - Radial gradients in <canvas> are opaque (regression from Azure)

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Benjamin Smedberg]

Attachment: Screenshot

In the Aero Window Title extension, I use a canvas to draw the shadow around the window title. The radial gradients I draw, which are a gradient from rgba(100%, 100%, 100%, 0.3)  to rgba(100%, 100%, 100%, 0) are now drawing completely opaque. Screenshot attached. The extension in question is https://addons.mozilla.org/en-US/firefox/addon/aero-window-title/ and has a screenshot of how it should look.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110719 Firefox/8.0a1

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Can you reproduce something similar in a canvas in an html document?

Comment 2

[Benjamin Smedberg]

And I have verified that disabling azure via pref fixes the problem.

The code which draws these gradients can be seen at http://hg.mozilla.org/users/bsmedberg_mozilla.com/aero-window-title/file/5ef85c7389dc/aero-overlay.xul#l50

Comment 3

[Benjamin Smedberg]

Attachment: Web-content testcase

Yes, I can reproduce this with plain web content as well, HTML testcase attached.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

It looks like the cause of this is us assuming premultiplied data when the data is not actually premultiplied. The fix should be easy.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Attachment: Premultiply before blending

I don't know who would be a good reviewer for this. It's all pretty straightforward so I'll take a review from anyone who's willing to give it.

Comment 6

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

Wouldn't it make more sense to have DrawTargetD2D::CreateGradientTexture create the texture already premultiplied? Less work per pixel.

Besides, we probably should be interpolating in premultiplied space.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #6)
> Wouldn't it make more sense to have DrawTargetD2D::CreateGradientTexture
> create the texture already premultiplied? Less work per pixel.

Yeah, but more non parallelized work on the CPU. No-one will notice an extra multiply per pixel on the GPU. If we really cared we could even hide the multiply in the output merger and no-one would ever notice it there.

> Besides, we probably should be interpolating in premultiplied space.

Afaict, gradients aren't interpolated in premultiplied space.

Comment 8

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

Comment on attachment 546944 [details] [diff] [review]
Premultiply before blending

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

Yeah, OK.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Attachment: reftest

Comment 10

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

Comment on attachment 547160 [details] [diff] [review]
reftest

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

Comment 11

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/6d355f495e8d

Comment 12

[(no longer active)]

Also: http://hg.mozilla.org/mozilla-central/rev/a9e310db3ac0

Comment 13

[Asa Dotzler [:asa]]

We discussed this in triage and we need risk/reward on this bug and we need an honest assessment of whether Azure is complete enough to ship in 7.  Is this a blocker to shipping Azure canvas (are semi-transparent radial gradients in canvas used in the wild enough that we wouldn't want to ship this bug?) How confident are we about this fix and are there any additional outstanding Azure canvas bugs that will be requesting approval in the coming days or weeks?

Comment 14

[Jeff Muizelaar [:jrmuizel]]

The risk of this patch is quite low, it only impacts an area that is not as commonly used and the fix can only have an impact in that area.

There was a small bug in it (bug 674003) but the problem there is even more rare and the resulting rendering less bad.

These are the only two issues with Azure that I've seen so far.

I would say that this issue would probably block shipping Azure in 7 and given how tiny the fix is it would be a mistake to disable Azure because we didn't take this.

Comment 15

[LegNeato]

Comment on attachment 546944 [details] [diff] [review]
Premultiply before blending

Approved for releases/mozilla-aurora

Comment 16

[Jeff Muizelaar [:jrmuizel]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/ea86edb8c31c

Comment 17

[AndreiD[QA]]

Build ID: Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Setting this bug as Verified. The issue reported is fixed on Fx7 Beta. Tested following the steps in the description.

However, it's unclear what changes should be visible if loading the "Web-content testcase" html? Thanks