Last Comment Bug 591600 - CSS gradients should work on premultiplied colors
: CSS gradients should work on premultiplied colors
Status: RESOLVED FIXED
[parity-chrome][parity-opera][parity-...
: css3, dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal with 8 votes (vote)
: mozilla36
Assigned To: Rik Cabanier
:
Mentors:
data:text/html;charset=utf-8,<!DOCTYP...
: 607859 858988 1068012 1078344 (view as bug list)
Depends on: 1242145
Blocks: 619834 1087119 1241717 752187 1088578
  Show dependency treegraph
 
Reported: 2010-08-28 04:15 PDT by David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
Modified: 2016-01-23 11:20 PST (History)
32 users (show)
dao+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
36+


Attachments
Add extra color-stop to simulate interpolation in premultiplied color space (1.93 KB, patch)
2012-07-12 20:24 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Tests (4.18 KB, patch)
2012-07-12 20:24 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
HTML Test Case (1.48 KB, text/html)
2013-04-11 02:23 PDT, David Marland
no flags Details
First implementation for premultiplied gradients (4.24 KB, patch)
2014-10-15 21:59 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
First implementation for premultiplied gradients (4.29 KB, patch)
2014-10-15 22:03 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Implementation for premultiplied gradients (4.50 KB, patch)
2014-10-16 15:53 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Implementation for premultiplied gradients (4.56 KB, patch)
2014-10-16 15:54 PDT, Rik Cabanier
dbaron: review-
Details | Diff | Splinter Review
Implementation for premultiplied gradients (4.78 KB, patch)
2014-10-16 22:02 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Implementation for premultiplied gradients (4.90 KB, patch)
2014-10-20 21:37 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Implementation for premultiplied gradients (4.91 KB, patch)
2014-10-20 21:40 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Implementation for premultiplied gradients (4.89 KB, patch)
2014-10-21 10:12 PDT, Rik Cabanier
mstange: review+
Details | Diff | Splinter Review
Implementation for premultiplied gradients (8.92 KB, patch)
2014-10-21 10:39 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-08-28 04:15:06 PDT
Gradients to/from transparent look bad because they go through gray in the middle.  This is because we're not doing gradients on premultiplied colors, but I think we should be.

For example, see:
data:text/html;charset=utf-8,<!DOCTYPE html><body style%3D"background%3A-moz-linear-gradient(left%2C yellow%2C transparent)">

(This isn't an issue in the SVG spec because SVG doesn't have rgba colors, and it thus has separate stop-color and stop-opacity.)

Does this seem reasonable?
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-08-28 04:33:14 PDT
Yes.
Comment 2 Tab Atkins Jr. 2010-08-28 13:04:06 PDT
Agreed; you should have the ability to go from #ffff00ff to #ffff0000 and get an attractive transition.

Does this mean I'll need to revise the spec and specify how to determine the color between stops myself?
Comment 3 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-08-28 13:13:33 PDT
I think the spec should specify something; right now it looks like it doesn't say anything.  I think specifying an appropriately weighted average, in premultiplied RGBA space, of the colors at the adjacent stops should be sufficient, and not require a lot of text (although you might want to be a little more specific than "appropriately weighted").
Comment 4 Tab Atkins Jr. 2010-08-28 13:25:34 PDT
At the moment it explicitly defers to SVG: "Between two color-stops, the colors are interpolated as SVG gradients."

I've just committed a change so that it says "Between two color-stops, the line's color is linearly interpolated between the colors of the two color-stops, with the interpolation taking place in premultiplied RGBA space."
Comment 5 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-08-28 14:27:22 PDT
Though, actually, SVG also defines the 'color-interpolation' property, which should probably apply.  Except I'm really not sure if linearRGB makes sense along with premultiplication.

But I guess the spec probably should at least say that interpolation takes place in sRGB.
Comment 6 Boris Zbarsky [:bz] (TPAC) 2010-08-29 08:41:49 PDT
> At the moment it explicitly defers to SVG: "Between two color-stops, the colors
> are interpolated as SVG gradients."

Thats underdefined, since SVG doesn't allow alpha != 1 in colors and hence only defines gradient behavior for colors with alpha == 1, as mentioned in comment 0.  So in effect the CSS spec doesn't define the behavior here at all at the moment (modulo whatever changes happened in comment 5).
Comment 7 Tab Atkins Jr. 2010-08-29 10:05:47 PDT
Ah, gotcha bz.  I didn't catch the implication of that statement in comment 0.  Well, it should be well-defined now, as long as "linearly interpolated in premultiplied sRGBA space" is sufficient.

(Let me know if the term "sRGBA space" is nonsensical, and I'll split out the color and opacity language explicitly.)
Comment 8 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-08-30 09:02:18 PDT
This affects:
 * InterpolateColor in nsCSSRendering (for endpoints)
 * something inside cairo (we call cairo_pattern_add_color_stop_rgba)
Comment 9 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-10-27 20:18:13 PDT
*** Bug 607859 has been marked as a duplicate of this bug. ***
Comment 10 aja+bugzilla 2012-07-10 16:01:46 PDT
Example 23 in Image Values spec Gradient Color Stops section http://www.w3.org/TR/css3-images/#color-stop-syntax
is another example...might be a good one to use for a test case.
Comment 11 Masatoshi Kimura [:emk] 2012-07-12 20:24:32 PDT
Created attachment 641710 [details] [diff] [review]
Add extra color-stop to simulate interpolation in premultiplied color space
Comment 12 Masatoshi Kimura [:emk] 2012-07-12 20:24:57 PDT
Created attachment 641711 [details] [diff] [review]
Tests
Comment 13 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-07-13 15:29:30 PDT
This seems to only fix it when the alpha component is 0, but it really matters for all cases where the alpha components differ.
Comment 14 Masatoshi Kimura [:emk] 2012-07-14 14:32:30 PDT
Is there a difference when the alpha component is not 0?
I thought a rgba() value itself is an unpremultiplied color even if interpolation is calculated in premultiplied color space. Is it incorrect?
Comment 15 Tab Atkins Jr. 2012-07-14 15:29:28 PDT
Yes, the transition is different between pre and unpre any time both the hue and the alpha change.

You are correct that the rgba() function specifies an unpremultiplied color.  That doesn't affect what I said above.
Comment 16 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-07-14 21:53:35 PDT
For example, the gradient from rgb(0, 255, 0) to rgba(255, 0, 0, 0.01) should be basically indistinguishable from the gradient from the same start color to transparent, or to rgba(*, *, *, 0.01), since as premultiplied colors, all values of rgba(*, *, *, 0.01) are nearly the same for any values of *.
Comment 17 Masatoshi Kimura [:emk] 2012-07-14 21:57:30 PDT
Comment on attachment 641710 [details] [diff] [review]
Add extra color-stop to simulate interpolation in premultiplied color space

I got it. Thank you.
Comment 18 Boris Zbarsky [:bz] (TPAC) 2013-04-08 05:53:55 PDT
*** Bug 858988 has been marked as a duplicate of this bug. ***
Comment 19 David Marland 2013-04-11 02:23:22 PDT
Created attachment 736196 [details]
HTML Test Case

Added a clean HTML page demonstrating the problem, for quick eye checking
Comment 20 Tab Atkins Jr. 2013-04-11 09:50:32 PDT
(In reply to David Marland from comment #19)
> Created attachment 736196 [details]
> HTML Test Case
> 
> Added a clean HTML page demonstrating the problem, for quick eye checking

Just as a note, the easiest possible no-way-to-misinterpret-or-mistake-it way to test this is to just put a gradient from transparent->white over a white background.  If you're premultiplied, you'll see nothing; if you're not, you'll see a gradient from white->light gray->white.
Comment 21 Erwin Dokter 2014-03-01 07:57:44 PST
Anyone still working on this? 'transparent' is useless in this context and have to revert to workarounds using rgba().
Comment 22 Boris Zbarsky [:bz] (TPAC) 2014-09-16 11:51:07 PDT
*** Bug 1068012 has been marked as a duplicate of this bug. ***
Comment 23 Matthias Versen [:Matti] 2014-10-06 14:10:35 PDT
*** Bug 1078344 has been marked as a duplicate of this bug. ***
Comment 24 Rik Cabanier 2014-10-15 21:59:37 PDT
Created attachment 8505936 [details] [diff] [review]
First implementation for premultiplied gradients
Comment 25 Rik Cabanier 2014-10-15 22:03:21 PDT
Created attachment 8505937 [details] [diff] [review]
First implementation for premultiplied gradients

How does this look?
The rendering is identical (to the naked eye) to Chrome and IE
Comment 26 Rik Cabanier 2014-10-16 15:53:13 PDT
Created attachment 8506482 [details] [diff] [review]
Implementation for premultiplied gradients
Comment 27 Rik Cabanier 2014-10-16 15:54:29 PDT
Created attachment 8506483 [details] [diff] [review]
Implementation for premultiplied gradients
Comment 28 Rik Cabanier 2014-10-16 15:55:18 PDT
Comment on attachment 8506483 [details] [diff] [review]
Implementation for premultiplied gradients

try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0ba919361a0a
Comment 29 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2014-10-16 16:31:22 PDT
Comment on attachment 8506483 [details] [diff] [review]
Implementation for premultiplied gradients

I think this is mostly good (but I didn't look very closely), except I think you don't need to do anything interesting if two adjacent stops have the same alpha.  (And then I don't think you need any condition testing alpha != 1.0.)


Also, I think once you fix that, :mstange could probably review this.
Comment 30 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2014-10-16 16:31:49 PDT
(Also, is this on top of the patch in bug 1074056?)
Comment 31 Rik Cabanier 2014-10-16 16:48:46 PDT
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #30)
> (Also, is this on top of the patch in bug 1074056?)

No. It just happened that those were in my queue. I will do another try before landing.
Comment 32 Rik Cabanier 2014-10-16 16:52:20 PDT
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #29)
> Comment on attachment 8506483 [details] [diff] [review]
> Implementation for premultiplied gradients
> 
> I think this is mostly good (but I didn't look very closely), except I think
> you don't need to do anything interesting if two adjacent stops have the
> same alpha.  (And then I don't think you need any condition testing alpha !=
> 1.0.)

This line already accomplishes that:
   float steps = fabs(leftStop.mColor.a - rightStop.mColor.a) * 10;

If the alpha is the same (or close) there are no additional stops.

> Also, I think once you fix that, :mstange could probably review this.

Do you still want me to update the patch?
Comment 33 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2014-10-16 17:13:56 PDT
Yes -- it would be nice to do that check much earlier, and definitely before you've done any floating point math.

Also, steps should almost certainly be an integral type, both for faster comparison and so you get evenly distributed steps.
Comment 34 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2014-10-16 17:14:57 PDT
(Can you do the equal-alpha check before the 0-alpha checks?  If so, that would probably be good, since it's the common case.)
Comment 35 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2014-10-16 17:24:03 PDT
(In reply to Rik Cabanier from comment #31)
> (In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from
> comment #30)
> > (Also, is this on top of the patch in bug 1074056?)
> 
> No. It just happened that those were in my queue. I will do another try
> before landing.

Will you need to merge the two?  Or should they be sharing code?
Comment 36 Rik Cabanier 2014-10-16 21:56:53 PDT
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #35)
> (In reply to Rik Cabanier from comment #31)
> > (In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from
> > comment #30)
> > > (Also, is this on top of the patch in bug 1074056?)
> > 
> > No. It just happened that those were in my queue. I will do another try
> > before landing.
> 
> Will you need to merge the two?  Or should they be sharing code?

I don't need to merge them as they don't share code. I need to make sure that midpoint calculation happens before the premultiply logic
Comment 37 Rik Cabanier 2014-10-16 22:02:46 PDT
Created attachment 8506643 [details] [diff] [review]
Implementation for premultiplied gradients
Comment 38 Jean-Yves Perrier [:teoli] 2014-10-17 00:12:33 PDT
I've added dev-doc-needed because I want to check, once this lands, that the terminology is coherent in the MDN and that we speak about the premultiplied space (also there likely will lead to a new comment in the browser compat section of gradient-related articles) [And a release note]
Comment 39 Markus Stange [:mstange] 2014-10-20 06:02:54 PDT
Comment on attachment 8506643 [details] [diff] [review]
Implementation for premultiplied gradients

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

The approach looks good to me, but I'd like to see the revised version of this patch before I can r+ it.

It looks like Skia and Direct2D natively support premultiplied gradient interpolation. Do you have any plans for using that support in the future?

::: layout/base/nsCSSRendering.cpp
@@ +2219,5 @@
>  
>    return false;
>  }
>  
> +static void ResolvePremultipliedAlpha(nsTArray<ColorStop>& stops)

add a line break between void and ResolvePremultipliedAlpha, and call the argument aStops

Also, please add a comment above the function, something like "Adjusts and adds color stops in such a way that drawing the gradient with unpremultiplied interpolation looks nearly the same as if it were drawn with premultiplied interpolation."

@@ +2221,5 @@
>  }
>  
> +static void ResolvePremultipliedAlpha(nsTArray<ColorStop>& stops)
> +{
> +  ColorStop leftStop = stops[0];

Doing leftStop = stops[x - 1] at the start of every loop iteration would make the invariant clearer and not be noticeably slower.

@@ +2223,5 @@
> +static void ResolvePremultipliedAlpha(nsTArray<ColorStop>& stops)
> +{
> +  ColorStop leftStop = stops[0];
> +
> +  for (size_t x = 1; x < stops.Length();) {

I think putting the x++ in here, and additionally calling x++ whenever you do stops.Insert*, would make it easier to understand what's happening.

@@ +2224,5 @@
> +{
> +  ColorStop leftStop = stops[0];
> +
> +  for (size_t x = 1; x < stops.Length();) {
> +    ColorStop rightStop = stops[x];

Can you make leftStop and rightStop const? I get confused when these variables change their values halfway through.

@@ +2225,5 @@
> +  ColorStop leftStop = stops[0];
> +
> +  for (size_t x = 1; x < stops.Length();) {
> +    ColorStop rightStop = stops[x];
> +    

end-of-line whitespace

@@ +2227,5 @@
> +  for (size_t x = 1; x < stops.Length();) {
> +    ColorStop rightStop = stops[x];
> +    
> +    // if the left and right stop have the same alpha value, we don't need
> +    // to do anything

Comments that are whole sentences should start with a capital letter and end with a full stop.

@@ +2239,5 @@
> +    // of the right stop
> +    if (leftStop.mColor.a == 0) {
> +      leftStop.mColor = rightStop.mColor;
> +      leftStop.mColor.a = 0;
> +      stops[x - 1] = leftStop;

You could add a helper function TransparentColor and make this

stops[x - 1].mColor = TransparentColor(rightStop.mColor);

@@ +2245,5 @@
> +
> +    // Is the stop on the right completely transparent?
> +    // If so, duplicate it and assign it the color on the left.
> +    if (rightStop.mColor.a == 0) {
> +      ColorStop NewStop = rightStop;

newStop with lowercase n, please

@@ +2248,5 @@
> +    if (rightStop.mColor.a == 0) {
> +      ColorStop NewStop = rightStop;
> +      rightStop.mColor = leftStop.mColor;
> +      rightStop.mColor.a = 0;
> +      stops[x] = rightStop;

stops[x].mColor = TransparentColor(leftStop.mColor);

@@ +2253,5 @@
> +      x++;
> +
> +      // If the right stop is the last colorstop, don't add a new
> +      // one; we're done.
> +      if (x == stops.Length() + 1) {

Invert this if and put the stuff up to (and including) stops.InsertElementAt(x, newStop) inside it.

@@ +2258,5 @@
> +        continue;
> +      }
> +      leftStop = NewStop;
> +      // if the next stop is in the same position,
> +      // don't add a new stop

Why not?

@@ +2269,5 @@
> +      x++;
> +      continue;
> +    }
> +
> +    // Now handle cases where one of the stops in transparent

in -> is, and add a full stop at the end

@@ +2271,5 @@
> +    }
> +
> +    // Now handle cases where one of the stops in transparent
> +    if (leftStop.mColor.a != 1.0f || rightStop.mColor.a != 1.0f) {
> +      gfxRGBA premulLeftColor(leftStop.mColor.Packed(gfxRGBA::PACKED_ABGR_PREMULTIPLIED));

Understanding this requires the reader to check that the gfxRGBA constructor uses PACKED_ABGR as the default value for its colorType argument (and not PACKED_ARGB_PREMULTIPLIED). Please add a dedicated helper function for this, e.g. static gfxRGBA Premultiply(const gfxRGBA& aColor);.

@@ +2274,5 @@
> +    if (leftStop.mColor.a != 1.0f || rightStop.mColor.a != 1.0f) {
> +      gfxRGBA premulLeftColor(leftStop.mColor.Packed(gfxRGBA::PACKED_ABGR_PREMULTIPLIED));
> +      gfxRGBA premulRightColor(rightStop.mColor.Packed(gfxRGBA::PACKED_ABGR_PREMULTIPLIED));
> +      // calculate how many extra steps. We do a step per 10% transparency
> +      float steps = fabs(leftStop.mColor.a - rightStop.mColor.a) * 10;

Call this stepCount, make it a size_t and use NSToIntFloor. And instead of * 10, make it / 0.1, and put the 0.1 in a static const kAlphaIncrementPerGradientStep above this function.

@@ +2277,5 @@
> +      // calculate how many extra steps. We do a step per 10% transparency
> +      float steps = fabs(leftStop.mColor.a - rightStop.mColor.a) * 10;
> +      for (int y = 1; y < steps; y++) {
> +        ColorStop NewStop(Interpolate(leftStop.mPosition, rightStop.mPosition, y / steps),
> +                          gfxRGBA(InterpolateColor(premulLeftColor, premulRightColor, y / steps).Packed(), gfxRGBA::PACKED_ABGR_PREMULTIPLIED));

Use a dedicated Unpremultiply helper function.

@@ +2582,5 @@
>      }
>      stops.AppendElement(ColorStop(firstStop, lastColor));
>    }
>  
> +  // resolve premultiplied colors

This comment can go, once you add a more descriptive comment above the function definition.
Comment 40 Rik Cabanier 2014-10-20 21:36:38 PDT
(In reply to Markus Stange [:mstange] from comment #39)
> Comment on attachment 8506643 [details] [diff] [review]
> Implementation for premultiplied gradients
> 
> Review of attachment 8506643 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The approach looks good to me, but I'd like to see the revised version of
> this patch before I can r+ it.
> 
> It looks like Skia and Direct2D natively support premultiplied gradient
> interpolation. Do you have any plans for using that support in the future?

I didn't know that D2D support it. I will add follow up patches to enable it.
I believe I addressed your comments; the code ended up much simpler.
Comment 41 Rik Cabanier 2014-10-20 21:37:10 PDT
Created attachment 8508451 [details] [diff] [review]
Implementation for premultiplied gradients
Comment 42 Rik Cabanier 2014-10-20 21:40:22 PDT
Created attachment 8508453 [details] [diff] [review]
Implementation for premultiplied gradients
Comment 43 Markus Stange [:mstange] 2014-10-21 00:52:13 PDT
Comment on attachment 8508453 [details] [diff] [review]
Implementation for premultiplied gradients

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

::: layout/base/nsCSSRendering.cpp
@@ +2229,5 @@
> +
> +// Adjusts and adds color stops in such a way that drawing the gradient with
> +// unpremultiplied interpolation looks nearly the same as if it were drawn with
> +// premultiplied interpolation.
> +static const float kAlphaIncrementPerGradientStep = 0.1f;

In this location the comment looks like it describes the Premultiply function and not the ResolvePremultipliedAlpha function. Please move the comment + the constant down directly above ResolvePremultipliedAlpha.

@@ +2232,5 @@
> +// premultiplied interpolation.
> +static const float kAlphaIncrementPerGradientStep = 0.1f;
> +static gfxRGBA
> +Premultiply(const gfxRGBA& aColor) {
> +  return aColor.Packed(gfxRGBA::PACKED_ABGR_PREMULTIPLIED);

You don't need to pack+unpack the color just to premultiply it. Let's reimplement Premultiply and Unpremultiply in place.

static gfxRGBA
Premultiply(const gfxRGBA& aColor)
{
  gfxFloat a = aColor.a;
  return gfxRGBA(aColor.r * a, aColor.g * a, aColor.b * a, a);
}

static gfxRGBA
Unpremultiply(const gfxRGBA& aColor)
{
  gfxFloat a = aColor.a;
  return (a > 0.0) ? gfxRGBA(aColor.r / a, aColor.g / a, aColor.b / a, a)
                   : aColor;
}

@@ +2239,5 @@
> +static gfxRGBA Unpremultiply(uint32_t aColor) {
> +  return gfxRGBA(gfxRGBA(aColor).Packed(), gfxRGBA::PACKED_ABGR_PREMULTIPLIED);
> +}
> +
> +static gfxRGBA TransparentColor(gfxRGBA aColor) {

Mozilla coding style requires two more line breaks in this line.

@@ +2262,5 @@
> +    // of the right stop
> +    if (leftStop.mColor.a == 0) {
> +      ColorStop newStop = leftStop;
> +      newStop.mColor = TransparentColor(rightStop.mColor);
> +      aStops[x - 1] = newStop;

Do you need newStop? Can't you just change aStops[x - 1].mColor?

@@ +2283,5 @@
> +      gfxRGBA premulRightColor = Premultiply(rightStop.mColor);
> +      // Calculate how many extra steps. We do a step per 10% transparency.
> +      size_t stepCount = NSToIntFloor(fabs(leftStop.mColor.a - rightStop.mColor.a) / kAlphaIncrementPerGradientStep);
> +      for (size_t y = 1; y < stepCount; y++) {
> +        ColorStop newStop(Interpolate(leftStop.mPosition, rightStop.mPosition, static_cast<float>(y) / stepCount),

You could factor out "static_cast<float>(y) / stepCount" into a new variable, e.g. "frac", since you're using it in three places.

@@ +2586,5 @@
>      }
>      stops.AppendElement(ColorStop(firstStop, lastColor));
>    }
>  
> +  // resolve premultiplied colors

remove comment
Comment 44 Rik Cabanier 2014-10-21 10:12:23 PDT
Created attachment 8508814 [details] [diff] [review]
Implementation for premultiplied gradients
Comment 45 Markus Stange [:mstange] 2014-10-21 10:16:46 PDT
Comment on attachment 8508814 [details] [diff] [review]
Implementation for premultiplied gradients

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

Do you have some tests you can add?

::: layout/base/nsCSSRendering.cpp
@@ +2239,5 @@
> +  gfxFloat a = aColor.a;
> +  return (a > 0.0) ? gfxRGBA(aColor.r / a, aColor.g / a, aColor.b / a, a) : aColor;
> +}
> +
> +static gfxRGBA TransparentColor(gfxRGBA aColor) {

Three lines:

static gfxRGBA
TransparentColor(gfxRGBA aColor)
{
Comment 46 Rik Cabanier 2014-10-21 10:24:38 PDT
(In reply to Markus Stange [:mstange] from comment #45)
> Comment on attachment 8508814 [details] [diff] [review]
> Implementation for premultiplied gradients
> 
> Review of attachment 8508814 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do you have some tests you can add?

yes. I'll add a couple simple gradient test that have a stop at 0% opacity.
Comment 47 Rik Cabanier 2014-10-21 10:39:12 PDT
Created attachment 8508862 [details] [diff] [review]
Implementation for premultiplied gradients

try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=688f2ac57044
Comment 48 Rik Cabanier 2014-10-21 15:31:02 PDT
There was an unrelated bad patch in that push queue.
Better try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e28ff5e50f07
Comment 50 Ryan VanderMeulen [:RyanVM] 2014-10-22 13:13:51 PDT
https://hg.mozilla.org/mozilla-central/rev/32dac7dce97a
Comment 51 Sebastian Zartner [:sebo] 2014-10-23 09:33:39 PDT
I can confirm that it's working. Great job!

Sebastian
Comment 52 Sebastian Zartner [:sebo] 2014-10-23 22:50:38 PDT
Release Note Request (optional, but appreciated)
[Why is this notable]: Fixed an issue with CSS gradients web devs often run into
[Suggested wording]: CSS gradients work on premultiplied colors
[Links (documentation, blog post, etc)]: (MDN dev-doc-needed)

May also be worth noting in b2g, though as I don't use a Firefox OS device to try that out, I didn't request a release note for it.

Sebastian
Comment 53 The 8472 2014-11-30 11:07:08 PST
>(This isn't an issue in the SVG spec because SVG doesn't have rgba colors, and it thus has separate stop-color and stop-opacity.)

It seems to be an issue with canvas gradients, since they do not support stop opacities, unlike SVG

change "white" to "transparent" in this demo:
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D.createLinearGradient
Comment 54 Tab Atkins Jr. 2014-12-01 11:42:41 PST
And it *is* a problem in SVG gradients anyway; while SVG was originally written assuming opaque colors, everyone just implements the full CSS color spec for it, so you can get partially-transparent colors in the stop-color attribute as well.

You can *avoid* the issue by using opaque colors and stop-opacity, but that doesn't mean the issue isn't still there.
Comment 55 Sylvestre Ledru [:sylvestre] 2014-12-02 16:35:57 PST
Added to the 36 release notes as a known issue being fixed in 36 (and not as a new feature). Let me know if you disagree.
Comment 56 Sebastian Zartner [:sebo] 2014-12-02 22:17:27 PST
(In reply to Sylvestre Ledru [:sylvestre] from comment #55)
> Added to the 36 release notes as a known issue being fixed in 36 (and not as
> a new feature).

That was my intention. :-) So that's totally fine.

Sebastian

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