Closed Bug 591600 Opened 14 years ago Closed 10 years ago

CSS gradients should work on premultiplied colors

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36
Tracking Flags:
Tracking Status
relnote-firefox --- 36+

People

(Reporter: dbaron, Assigned: cabanier)

References

(Blocks 2 open bugs,
URL
)

Details

(Keywords: css3, dev-doc-complete, Whiteboard: [parity-chrome][parity-opera][parity-safari][parity-ie])

Attachments

(3 files, 9 obsolete files)

Tests
4.18 KB, patch
Details | Diff | Splinter Review
HTML Test Case
1.48 KB, text/html
Details
Implementation for premultiplied gradients
8.92 KB, patch
Details | Diff | Splinter Review
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?
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?
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").
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."
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.
> 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).
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.)
This affects: * InterpolateColor in nsCSSRendering (for endpoints) * something inside cairo (we call cairo_pattern_add_color_stop_rgba)
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.
Attached patch Tests — — Splinter Review
Attachment #641711 - Flags: review?(dbaron)
This seems to only fix it when the alpha component is 0, but it really matters for all cases where the alpha components differ.
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?
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.
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 on attachment 641710 [details] [diff] [review] Add extra color-stop to simulate interpolation in premultiplied color space I got it. Thank you.
Attachment #641710 - Flags: review?(dbaron)
Attachment #641711 - Flags: review?(dbaron)
Attached file HTML Test Case —
Added a clean HTML page demonstrating the problem, for quick eye checking
(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.
Anyone still working on this? 'transparent' is useless in this context and have to revert to workarounds using rgba().
Whiteboard: [parity-chrome][parity-opera][parity-safari][parity-ie]
Assignee: nobody → cabanier
Attachment #641710 - Attachment is obsolete: true
How does this look? The rendering is identical (to the naked eye) to Chrome and IE
Attachment #8505936 - Attachment is obsolete: true
Flags: needinfo?(dbaron)
Attachment #8505937 - Attachment is obsolete: true
Flags: needinfo?(dbaron)
Attachment #8506482 - Attachment is obsolete: true
Comment on attachment 8506483 [details] [diff] [review] Implementation for premultiplied gradients try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0ba919361a0a
Attachment #8506483 - Flags: review?(dbaron)
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.
Attachment #8506483 - Flags: review?(dbaron) → review-
(Also, is this on top of the patch in bug 1074056?)
(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.
(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?
Flags: needinfo?(dbaron)
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.
Flags: needinfo?(dbaron)
(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.)
(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?
(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
Attachment #8506483 - Attachment is obsolete: true
Attachment #8506643 - Flags: review?(matt.woodrow)
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]
Keywords: dev-doc-needed
Attachment #8506643 - Flags: review?(matt.woodrow) → review?(mstange)
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.
Attachment #8506643 - Flags: review?(mstange)
(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.
Attachment #8506643 - Attachment is obsolete: true
Attachment #8508451 - Flags: review?(mstange)
Attachment #8508451 - Attachment is obsolete: true
Attachment #8508451 - Flags: review?(mstange)
Attachment #8508453 - Flags: review?(mstange)
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
Attachment #8508453 - Flags: review?(mstange)
Attachment #8508453 - Attachment is obsolete: true
Attachment #8508814 - Flags: review?(mstange)
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) {
Attachment #8508814 - Flags: review?(mstange) → review+
(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.
There was an unrelated bad patch in that push queue. Better try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e28ff5e50f07
Keywords: checkin-needed
Blocks: 1087119
https://hg.mozilla.org/mozilla-central/rev/32dac7dce97a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Attachment #736196 - Attachment mime type: text/plain → text/html
I can confirm that it's working. Great job! Sebastian
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
relnote-firefox: --- → ?
Blocks: 1088578
>(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
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.
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.
relnote-firefox: ? → 36+
(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
Blocks: 1241717
Depends on: 1242145
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: