1074056 - Add support for interpolation hints to CSS gradients

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Rik Cabanier]

CSS images level 4 defines support for gradient midpoints: http://dev.w3.org/csswg/css-images-4/#color-interpolation-hint
This patch will implement this.

Comment 1

[Rik Cabanier]

Attachment: implementation for gradient midpoints

not for review yet. Needs tests and validation needs to be rewritten.

Comment 2

[Rik Cabanier]

Attachment: implementation + test cases for gradient midpoints

Try run: https://tbpl.mozilla.org/?tree=Try&rev=41111be23be7

I'm unsure how I can check the rendering of gradient midpoints...

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

How does this relate to https://wiki.mozilla.org/WebAPI/ExposureGuidelines ?  (Is this spec at all stable?  I don't recall much discussion of it.)

Comment 4

[Rik Cabanier]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #3)
> How does this relate to https://wiki.mozilla.org/WebAPI/ExposureGuidelines ?
WebKit is going to take this feature (https://bugs.webkit.org/show_bug.cgi?id=137171). 
I didn't take it up with Blink yet because I would like someone else to implement it there.

Should I send an email to dev-platform?

> (Is this spec at all stable?  I don't recall much discussion of it.)

I think so. Tab is going to clean it up a bit because it had some old information.
It's not controversial and pretty minor so it didn't cause big email threads.

Comment 5

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

Comment on attachment 8497292 [details] [diff] [review]
implementation + test cases for gradient midpoints

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

Please split this patch into style-system and rendering parts. I shouldn't review the style-system changes.

::: layout/base/nsCSSRendering.cpp
@@ +356,2 @@
>    double mPosition; // along the gradient line; 0=start, 1=end
> +  bool mIsMidpoint;

It seems to me mIsMidpoint should not be part of a "resolved color stop".

Comment 6

[Rik Cabanier]

Comment on attachment 8497292 [details] [diff] [review]
implementation + test cases for gradient midpoints

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

I'll break the patch up and assign the CSS part to dbaron.

::: layout/base/nsCSSRendering.cpp
@@ +356,2 @@
>    double mPosition; // along the gradient line; 0=start, 1=end
> +  bool mIsMidpoint;

The ColorStop structure is used by the function that resolves the gradient stops.
mIsMidpoint is used in the first part of the function but is then ignored.

Do you think we need an extra class to save the memory? This would mean extra copying for the majority of gradients that don't have midpoints

Comment 7

[Rik Cabanier]

Attachment: CSS implementation + updated test file for gradient midpoints

Comment 8

[Rik Cabanier]

Attachment: Implementation of rendering of gradient mid points

Comment 9

[Rik Cabanier]

Try build: https://tbpl.mozilla.org/?tree=Try&rev=3d7bad2d7872

Comment 10

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

Comment on attachment 8498629 [details] [diff] [review]
Implementation of rendering of gradient mid points

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

Personally I'm not sure using a magical color stop with no color is the best syntax for this feature. I think it might make more sense to have a dedicated function to make it clear that this is not a real color stop, just interpolation control, e.g. interpolate(), and make it part of the previous or next color stop instead of making it syntactically its own color stop.

::: layout/base/nsCSSRendering.cpp
@@ +2511,4 @@
>    }
>  
> +  // resolve midpoints
> +  for(size_t x = 1; x < stops.Length() - 1;) {

space before (

@@ +2517,5 @@
> +      continue;
> +    }
> +
> +    gfxRGBA color1 = stops[x-1].mColor;
> +    gfxRGBA color2 = stops[x+1].mColor;

What if stop x+1 also has mIsMidpoint true?

@@ +2524,5 @@
> +    float offset = stops[x].mPosition;
> +    // check if everything coincides. If so, ignore the midpoint.
> +    if (offset1 == offset && offset2 == offset) {
> +      stops.RemoveElementAt(x);
> +      continue;

Is this case really necessary? I suspect not.

@@ +2549,5 @@
> +    float midpoint = (offset - offset1) / (offset2 - offset1);
> +    ColorStop newStops[9];
> +    if (midpoint > .5f) {
> +      for (size_t y = 0; y < 7; y++)
> +        newStops[y].mPosition = offset1 + (offset - offset1) * (7 + y) / 13;

{}

@@ +2572,5 @@
> +      gfxFloat blue = color1.b + multiplier * (color2.b - color1.b);
> +      gfxFloat alpha = color1.a + multiplier * (color2.a - color1.a);
> +
> +      newStops[y].mColor = gfxRGBA(red, green, blue, alpha);
> +    }

This code for calculating interpolated stops is rather obscure. It should be moved to its own function and given some comments. For example, the choice of 9 stops should be justified.

@@ +2576,5 @@
> +    }
> +
> +    stops.ReplaceElementsAt(x, 1, newStops, 9);
> +    x += 9;
> +    }

fix indent

Comment 11

[Rik Cabanier]

Comment on attachment 8498629 [details] [diff] [review]
Implementation of rendering of gradient mid points

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

A dedicated interpolate() function seems like overkill and also harder to use.

::: layout/base/nsCSSRendering.cpp
@@ +2517,5 @@
> +      continue;
> +    }
> +
> +    gfxRGBA color1 = stops[x-1].mColor;
> +    gfxRGBA color2 = stops[x+1].mColor;

That is not possible because we test for that during CSS parsing.

@@ +2524,5 @@
> +    float offset = stops[x].mPosition;
> +    // check if everything coincides. If so, ignore the midpoint.
> +    if (offset1 == offset && offset2 == offset) {
> +      stops.RemoveElementAt(x);
> +      continue;

It's just an optimisation so we don't generate color stop that aren't useful. 
For the WK patch I changed it to:
  if(offset - offset1 == offset2 - offset)
which catches more cases

@@ +2572,5 @@
> +      gfxFloat blue = color1.b + multiplier * (color2.b - color1.b);
> +      gfxFloat alpha = color1.a + multiplier * (color2.a - color1.a);
> +
> +      newStops[y].mColor = gfxRGBA(red, green, blue, alpha);
> +    }

Sure. I'll move it to a separate function and put in a link to the spec.

Comment 12

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

(In reply to Rik Cabanier from comment #11)
> A dedicated interpolate() function seems like overkill and also harder to
> use.

I'll raise it on www-style.

Comment 13

[Rik Cabanier]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (on vacation Sep 27 to Oct 12) from comment #12)
> (In reply to Rik Cabanier from comment #11)
> > A dedicated interpolate() function seems like overkill and also harder to
> > use.
> 
> I'll raise it on www-style.

Sounds good! FWIW it was discussed at a couple of F2F meetings, weekly CSS calls and a bit on www-style about 6 months ago.

Comment 14

[Rik Cabanier]

Attachment: Implementation of rendering of gradient mid points

Patch that addressed your comments. Still needs tests.
Are you still unconvinced of the midpoint syntax?

FYI blink is about to land it as well.

Comment 15

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

Comment on attachment 8502274 [details] [diff] [review]
Implementation of rendering of gradient mid points

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

OK I guess with that.

::: layout/base/nsCSSRendering.cpp
@@ +2514,5 @@
> +  for (size_t x = 1; x < stops.Length() - 1;) {
> +    if (!stops[x].mIsMidpoint) {
> +      x++;
> +      continue;
> +    }

Please move this entire loop to its own helper function.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Was there a conclusion to the www-style discussion about the syntax?

Comment 17

[Rik Cabanier]

(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #16)
> Was there a conclusion to the www-style discussion about the syntax?

People didn't seem to mind the existing syntax.
Webkit and Blink both implemented what is in the current spec.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

How fast is painting of gradients with midpoints relative to gradients without them?

Comment 19

[Rik Cabanier]

I haven't done performance tests, but I doubt that they will be much slower.
We created a page to test them out: http://codepen.io/adobe/full/fhnBJ/
In WK and Blink, there is no observable lag when you add midpoints.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Ah, you're just representing them as 9 intermediate stops?

If that's sufficient, could I talk you into fixing bug 591600 the same way?  (Something like the patch in the bug would be sufficient where one of the stops is transparent; you'd just need this approach when two adjacent stops have different alpha values and neither are 0.)

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Also, how big is the error from using 9 intermediate stops?  And do blink and WebKit use the same approach?)

Comment 22

[Rik Cabanier]

(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #20)
> Ah, you're just representing them as 9 intermediate stops?

Yes.

> If that's sufficient, could I talk you into fixing bug 591600 the same way? 
> (Something like the patch in the bug would be sufficient where one of the
> stops is transparent; you'd just need this approach when two adjacent stops
> have different alpha values and neither are 0.)

OK, I will take a look.

Comment 23

[Rik Cabanier]

(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #21)
> (Also, how big is the error from using 9 intermediate stops?  

Illustrator uses an algorithm to calculate the number of color stops to approximate the midpoint. I tried various midpoint settings and I was not able to have it generate any more than these.

> And do blink and WebKit use the same approach?)
Yes. I'm hoping that blink adds true support for midpoints to skia, but for now they use a similar algorithm.
Apple could implement it correctly but that API is very slow to draw a gradient so I couldn't use it.

Comment 24

[Rik Cabanier]

Attachment: Implementation of rendering of gradient mid points

Comment 25

[Rik Cabanier]

Do you have time to review this soon?

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yes.  Hopefully today or Monday.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8498628 [details] [diff] [review]
CSS implementation + updated test file for gradient midpoints

So one problem I have here is that the terminology in the code doesn't
match the terminology in the spec.  Please either:
 (1) change all the references to midpoint in the code to be hint or
     interpolation hint, including function/member/variable names, or
 (2) convince the editors to change the terminology in the spec to
     midpoint, if you think that's correct.



nsCSSValue.h:

>+            (mIsMidpoint == true || mColor == aOther.mColor));

Prefer "mIsMidpoint" over "mIsMidpoint == true".


Please document in code comments that when mIsMidpoint (or mIsHint) is
true, there is no color, but there is a location.

nsCSSParser.cpp:

>+  bool PreviousPointWasMidpoint = true;

start the name with a lowercase p.

Also perhaps put aGradient->mStops[x].mIsMidpoint in a local variable
thisPointIsMidpoint (or Hint).


> size_t
> nsCSSValueGradientStop::SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
> {
>   size_t n = 0;
>   n += mLocation.SizeOfExcludingThis(aMallocSizeOf);
>   n += mColor   .SizeOfExcludingThis(aMallocSizeOf);
>+  n += sizeof(mIsMidpoint);
>+
>   return n;
> }

Undo this change.  You don't need any change to this method.  mIsMidpoint
is part of the size of |this|.

nsComputedDOMStyle.cpp:

Please make and use a local variable:
  const auto& stop = aGradient->mStops[i];
so that aGradient->mStops[i] isn't repeated so many times inside
the loop.


nsRuleNode.cpp and nsStyleStruct.h:

You either (a) need to always initialize mColor to the same value
for midpoints, or (b) fix nsStyleGradient::operator== to handle
it appropriately.

You should probably also declare an operator== and operator!= on
nsStyleGradientStop as MOZ_DELETE so that people don't use it.

Otherwise you'll trigger extra repaints due to the comparison of
uninitialized memory making us decide that the gradient has changed.

review- because I want to look at how you address this issue, though I
should be able to turn around a re-review quickly.

Otherwise this looks good.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) from comment #27)
> You should probably also declare an operator== and operator!= on
> nsStyleGradientStop as MOZ_DELETE so that people don't use it.

Or, alternatively, and perhaps better, move the stop comparison from nsStyleGradient::operator== to a new operator== and operator!= on nsStyleGradientStop.

Comment 29

[Rik Cabanier]

Attachment: CSS implementation + updated test file for gradient midpoints

Comment 30

[Rik Cabanier]

Attachment: Implementation of rendering of gradient mid points

Comment 31

[Rik Cabanier]

Attachment: CSS implementation + updated test file for gradient midpoints

Comment 32

[Rik Cabanier]

(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) from comment #27)
> Comment on attachment 8498628 [details] [diff] [review]
...
> 
> You should probably also declare an operator== and operator!= on
> nsStyleGradientStop as MOZ_DELETE so that people don't use it.
> 
> Otherwise you'll trigger extra repaints due to the comparison of
> uninitialized memory making us decide that the gradient has changed.
> 

That was indeed a problem. I made sure that the color is initialized and also updated the operator== so it takes the interpolation hint into account.

try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a076dd332236
I will need to rebase because of the premultiplied gradient patch.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8509212 [details] [diff] [review]
CSS implementation + updated test file for gradient midpoints

Fix the commit message to call them interpolation hints rather than midpoints.

(And, actually, this bug's summary, too.)


>+    } else {
>+      stop.mColor = NS_RGB(0, 0, 0);

Maybe add a comment like:

  // always initialize to the same color so we don't need to worry
  // about comparisons


You didn't need to fix the comparison problem both ways, but doing
that is fine, though.

nsStyleStruct.h:

above the deleted operator==/!=, add a comment saying that callers
should use ==/!= on nsStyleGradient.


r=dbaron with that

Comment 34

[Rik Cabanier]

Attachment: CSS implementation + updated test file for interpolation hints

Comment 35

[Rik Cabanier]

Attachment: Implementation of rendering of gradient interpolation hints

Comment 36

[Rik Cabanier]

try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=27e23254b3a3

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21282be9ad95
https://hg.mozilla.org/integration/mozilla-inbound/rev/e430c434d9c7

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21282be9ad95
https://hg.mozilla.org/mozilla-central/rev/e430c434d9c7