Closed Bug 1074056 Opened 5 years ago Closed 5 years ago

Add support for interpolation hints to CSS gradients

Categories

(Core :: Web Painting, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: cabanier, Assigned: cabanier)

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 9 obsolete files)

11.86 KB, patch
Details | Diff | Splinter Review
8.02 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → cabanier
OS: Mac OS X → All
Hardware: x86 → All
not for review yet. Needs tests and validation needs to be rewritten.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=41111be23be7

I'm unsure how I can check the rendering of gradient midpoints...
Attachment #8496688 - Attachment is obsolete: true
Attachment #8497292 - Flags: review?(roc)
Version: 30 Branch → Trunk
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.)
(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 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".
Attachment #8497292 - Flags: review?(roc) → review-
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
Attachment #8497292 - Attachment is obsolete: true
Attachment #8498628 - Flags: review?(dbaron)
Attachment #8498629 - Flags: review?(roc)
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
Attachment #8498629 - Flags: review?(roc) → review-
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.
(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.
(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.
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.
Attachment #8498629 - Attachment is obsolete: true
Attachment #8502274 - Flags: review?(roc)
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.
Attachment #8502274 - Flags: review?(roc) → review+
Was there a conclusion to the www-style discussion about the syntax?
(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.
How fast is painting of gradients with midpoints relative to gradients without them?
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.
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.)
(Also, how big is the error from using 9 intermediate stops?  And do blink and WebKit use the same approach?)
(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.
(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.
Attachment #8502274 - Attachment is obsolete: true
Do you have time to review this soon?
Flags: needinfo?(dbaron)
Yes.  Hopefully today or Monday.
Flags: needinfo?(dbaron)
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.
Attachment #8498628 - Flags: review?(dbaron) → review-
(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.
Attachment #8498628 - Attachment is obsolete: true
Attachment #8505887 - Attachment is obsolete: true
Attachment #8509210 - Attachment is obsolete: true
Attachment #8509212 - Flags: review?(dbaron)
(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 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
Attachment #8509212 - Flags: review?(dbaron) → review+
Attachment #8509211 - Attachment is obsolete: true
Summary: Add support for midpoints to CSS gradients → Add support for interpolation hints to CSS gradients
Attachment #8509809 - Attachment description: CSS implementation + updated test file for gradient midpoints → CSS implementation + updated test file for interpolation hints
Attachment #8509810 - Attachment description: Implementation of rendering of gradient mid points → Implementation of rendering of gradient interpolation hints
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/21282be9ad95
https://hg.mozilla.org/mozilla-central/rev/e430c434d9c7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.