Last Comment Bug 666097 - [Azure] Radial gradients in canvas don't always work as expected
: [Azure] Radial gradients in canvas don't always work as expected
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: mozilla8
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
Depends on: 1164912 672646
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-21 19:14 PDT by Bas Schouten (:bas.schouten)
Modified: 2015-05-14 12:36 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Fix Azure radial gradient behavior. (31.54 KB, patch)
2011-07-07 12:03 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Review
Fix Azure radial gradient behavior v2 (31.72 KB, patch)
2011-07-07 12:15 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Review
Fix Azure radial gradient behavior v3 (12.90 KB, patch)
2011-07-07 12:23 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Review
Properly report passing (6.96 KB, patch)
2011-07-07 12:33 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
asa: approval‑mozilla‑aurora+
Details | Diff | Review
Fix Azure radial gradient behavior v3 (31.72 KB, patch)
2011-07-07 12:35 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Review
Part 1: Fix azure radial gradient behavior v4 (290.82 KB, patch)
2011-07-08 11:57 PDT, Bas Schouten (:bas.schouten)
bas: review+
asa: approval‑mozilla‑aurora+
Details | Diff | Review

Description Bas Schouten (:bas.schouten) 2011-06-21 19:14:09 PDT
In Azure canvas radial gradients will behave incorrectly if the inner circle intersects with, or lies outside of the outer circle.
Comment 1 Bas Schouten (:bas.schouten) 2011-07-07 12:03:06 PDT
Created attachment 544575 [details] [diff] [review]
Fix Azure radial gradient behavior.

This patch makes Azure use special shaders to create radial gradients.
Comment 2 Bas Schouten (:bas.schouten) 2011-07-07 12:15:15 PDT
Created attachment 544579 [details] [diff] [review]
Fix Azure radial gradient behavior v2

This fixes some minor bugs.
Comment 3 Bas Schouten (:bas.schouten) 2011-07-07 12:23:08 PDT
Created attachment 544582 [details] [diff] [review]
Fix Azure radial gradient behavior v3

Fix another small bug.
Comment 4 Bas Schouten (:bas.schouten) 2011-07-07 12:33:57 PDT
Created attachment 544588 [details] [diff] [review]
Properly report passing
Comment 5 Bas Schouten (:bas.schouten) 2011-07-07 12:35:15 PDT
Created attachment 544589 [details] [diff] [review]
Fix Azure radial gradient behavior v3

Upload the right patch
Comment 6 Jeff Muizelaar [:jrmuizel] 2011-07-08 07:10:59 PDT
Comment on attachment 544588 [details] [diff] [review]
Properly report passing

Doesn't this need changes for the new tests that pass?
Comment 7 Jeff Muizelaar [:jrmuizel] 2011-07-08 07:48:52 PDT
Comment on attachment 544589 [details] [diff] [review]
Fix Azure radial gradient behavior v3

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

::: gfx/2d/DrawTargetD2D.cpp
@@ +1408,5 @@
> +    rawStops.resize(stops->mStopCollection->GetGradientStopCount());
> +    stops->mStopCollection->GetGradientStops(&rawStops.front(), rawStops.size());
> +
> +    RefPtr<ID3D10Texture1D> tex = CreateGradientTexture(rawStops);
> +

I'd move the stops getting into the CreateGradientTexture fucntion.

@@ +1445,5 @@
> +
> +    mPrivateData->mEffect->GetVariableByName("DeviceSpaceToUserSpace")->
> +      AsMatrix()->SetMatrix(matrix);
> +
> +    float A = pow(dc.x, 2) + pow(dc.y, 2) - pow(dr, 2);

I don't know if you care but VS2008 (didn't check 2010) doesn't turn pow(x, 2) into x*x

@@ +1454,5 @@
> +      mPrivateData->mEffect->GetVariableByName("A")->AsScalar()->SetFloat(A);
> +      mPrivateData->mEffect->GetTechniqueByName("SampleRadialGradient")->
> +        GetPassByIndex(0)->Apply(0);
> +    }
> +  }

Can this hunk be a separate function?

::: gfx/2d/HelpersD2D.h
@@ +172,5 @@
> +  }
> +
> +  return false;
> +}
> +

Can we come up with a better name for IsComplexPattern()?

::: gfx/2d/ShadersD2D.fx
@@ +126,5 @@
> +    Output.PixelCoord.y = ((1.0f - Output.Position.y) / 2.0f) * dimensions.y;
> +    Output.PixelCoord.xy = mul(float3(Output.PixelCoord.x, Output.PixelCoord.y, 1.0f), DeviceSpaceToUserSpace).xy;
> +    return Output;
> +}
> +

Can you add some comments about what this shader is doing.

@@ +186,5 @@
> +    }
> +
> +    return tex.Sample(sSampler, float2(t, 0.5)) * mask.Sample(sMaskSampler, In.MaskTexCoord).a;
> +};
> +

Some more documentation here wouldn't hurt. It's also worth saying something like: for a more detailed derivation see pixman.
Comment 8 Bas Schouten (:bas.schouten) 2011-07-08 11:41:50 PDT
Comment on attachment 544589 [details] [diff] [review]
Fix Azure radial gradient behavior v3

This patch is one of the fixes for Azure we need to take in the early mozilla-aurora space.
Comment 9 Bas Schouten (:bas.schouten) 2011-07-08 11:57:01 PDT
Created attachment 544876 [details] [diff] [review]
Part 1: Fix azure radial gradient behavior v4

Updated with review comments. Carrying r+.
Comment 10 Joe Drew (not getting mail) 2011-07-08 15:16:19 PDT
I folded these two patches together and pushed them to inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/c0eaec585ea7
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-07-09 20:23:41 PDT
http://hg.mozilla.org/mozilla-central/rev/c0eaec585ea7
Comment 12 Bas Schouten (:bas.schouten) 2011-07-09 22:44:28 PDT
Keeping this open for aurora approval!
Comment 13 Bas Schouten (:bas.schouten) 2011-07-12 09:18:44 PDT
Pushed:

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

Sadly with the wrong bug number in the comment :(.
Comment 14 Jeff Muizelaar [:jrmuizel] 2011-07-19 15:12:18 PDT
The condition returned from IsPatternSupportedByD2D is reversed.
Comment 15 AndreiD[QA] 2011-08-24 06:46:59 PDT
Build:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0) Gecko/20100101 Firefox/7.0

Setting this bug as verified fixed for firefox 7. The fix that landed on Aurora is visible in the Beta repository (i.e. http://hg.mozilla.org/releases/mozilla-beta/file/51f4341b6871/content/canvas/src/nsCanvasRenderingContext2DAzure.cpp). Also there are no failures with the attached tests to check the radial gradient (i.e. test_2d.gradient.radial.cone.top.html)

Note You need to log in before you can comment on or make changes to this bug.