Closed Bug 1467622 Opened 6 years ago Closed 6 years ago

nsStyleSVGPaint: Replace nsColor with StyleComplexColor

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: u480271, Assigned: u480271)

References

Details

Attachments

(6 files)

Bug 1467622 - P1: nsStyleSVGPaint - Change nscolor to StyleComplexColor.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1467622 - P2: Change SMIL paced tests to represent new distance calc.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1467622 - P3: Cleanup transition tests with currentcolor.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1467622 - P4: Correct background/foreground ratio mixing.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1467622 - P5: Reftests for SVG pserver currentcolor override.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1467622 - P6: Mark anim-css-fill reftest fuzzy on Windows.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
      No description provided.
Assignee: nobody → dglastonbury
Blocks: 760345
Priority: -- → P3
Comment on attachment 8989657 [details]
Bug 1467622 - P2: Change SMIL paced tests to represent new distance calc.

https://reviewboard.mozilla.org/r/254664/#review261518
Attachment #8989657 - Flags: review?(bbirtles) → review+
Comment on attachment 8989656 [details]
Bug 1467622 - P1: nsStyleSVGPaint - Change nscolor to StyleComplexColor.

https://reviewboard.mozilla.org/r/254662/#review261522

::: layout/style/nsStyleStruct.h:2984
(Diff revision 1)
> -  void SetPaintServer(mozilla::css::URLValue* aPaintServer) {
> +  void SetPaintServer(mozilla::css::URLValue* aPaintServer);
> -    SetPaintServer(aPaintServer, eStyleSVGFallbackType_NotSet,
> -                   NS_RGB(0, 0, 0));
> -  }

Is there any reason you need to de-inlinify this method and the second `SetContextValue` method below? It's not clear to me any.

If there is nothing specific, I'd probably prefer keeping it inline here.
Attachment #8989656 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8989658 [details]
Bug 1467622 - P3: Cleanup transition tests with currentcolor.

https://reviewboard.mozilla.org/r/254666/#review261530
Attachment #8989658 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8989659 [details]
Bug 1467622 - P4: Correct background/foreground ratio mixing.

https://reviewboard.mozilla.org/r/254668/#review261542

I don't really like this "optimization". It feels error-prone. Could you elaborate on how that case was buggy and why this fixes that issue? I see no transparent in that testcase.
Attachment #8989659 - Flags: review?(xidorn+moz)
Comment on attachment 8989660 [details]
Bug 1467622 - P5: Reftests for SVG pserver currentcolor override.

https://reviewboard.mozilla.org/r/254670/#review261588

Looks good.  (These probably would be even better as WPT tests, so other browsers can be tested against them.)

::: layout/reftests/svg/currentColor-override-pserver-stroke.svg:4
(Diff revision 1)
> +<svg xmlns="http://www.w3.org/2000/svg">
> +  <!-- pattern inherits fill color via currentcolor -->
> +  <defs>
> +    <pattern id ="MyPattern" patternUnits="userSpaceOnUse"

Nit: no space after the `id` (and in the other files).
Attachment #8989660 - Flags: review?(cam) → review+
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive until July 7) from comment #14)
> Comment on attachment 8989659 [details]
> Bug 1467622 - P4: Optimise animated::Color for addition of currentcolor and
> transparent.
> 
> https://reviewboard.mozilla.org/r/254668/#review261542
> 
> I don't really like this "optimization". It feels error-prone. Could you
> elaborate on how that case was buggy and why this fixes that issue? I see no
> transparent in that testcase.

This helped to fix failing test case from layout/reftests/svg/smil/style/anim-css-fill-1-by-ident-curcol.svg. 
The testcase is animating from indigo `by` currentcolor, ie. to indigo + currentcolor, by different amounts. In a majority of the cases the blue component was +1 what was expected.

I'll choose just one failing example to illustrate; <animate> that results in interpolating by 0.375.

Compose animation 0
t1  := RGBA { transparent } + Foreground => Complex(RGBA { transparent }, bg: 1.0, fg: 1.0)
t2  := RGBA { transparent } lerp(0.375) t1 => Complex(RGBA { transparent }, bg: 1.0, fg: 0.375)
col := RGBA { indigo }) + t2 => Complex(RGBA { indigo, alpha: 0.5 }, bg: 2.0, fg: 0.375)

When CalcColor is called with currentcolor, #AAF573, the result is #8B5CAE instead of expected. #8B5CAD

temporary color, t1, results from the <animate by="currentcolor"> property. Because there's no explicit `from` attr, SMIL uses transparent. t2, is calculating the animated lerp of 37.5% from `indigo` to `indigo + currentcolor`.

The "optimisation" was to recognize that adding transparent to a color should result in the same color, so to just skip the creation of a complex color in that situation.

Compose animation 3
t1  := RGBA { transparent } + Foreground => Foreground
t2  := RGBA { transparent }) lerp(0.375) t1 => Complex(RGBA { transparent }, bg: 0.625, fg: 0.375)
col := RGBA { indigo } +  t2 => Complex(RGBA { indigo, alpha: 0.61538464 }, bg: 1.625, fg: 0.375)

When CalcColor is called this result in #8B5CAD, the expected value. Infact all the failing tests pass.

The change fixes the issue by simplifying the calculations and we get "lucky" enough that all the failure cases work again.  I chose this approach for lack of a good specification for the handling of alpha when adding colors.

Relevant spec appear to be [1] and [2], "The values in the from/to/by and values attributes may specify negative and out of gamut values for colors. [...] However, as described in The animation sandwich model, the implementation should only correct the final combined result of all animations for a given attribute, and should not correct the effect of individual animations.

Values are corrected by "clamping" the values to the correct range."

To correctly, each step needs to be checked to determine why the desired result of:

col := Complex(RGBA { indigo }, bg: 1, fg: 0.375) 

doesn't result. (Note: 2 * { indigo, a: 0.5 } = indigo and 1.625 * { indigo, a: .61538464 } ~= indigo) 

[1] https://drafts.csswg.org/css-transitions/#animtype-color
[2] https://www.w3.org/TR/SMIL3/smil-animation.html#q46
(In reply to Cameron McCormack (:heycam) (busy until July 12) from comment #15)
> Comment on attachment 8989660 [details]
> Bug 1467622 - P5: Reftests for SVG pserver currentcolor override.
> 
> https://reviewboard.mozilla.org/r/254670/#review261588
> 
> Looks good.  (These probably would be even better as WPT tests, so other
> browsers can be tested against them.)

What's the best folder for these to go into for WPT? layout/reftests/w3c-css/submitted/...?
testing/web-platform/tests/svg/...
And probably specifically testing/web-platform/tests/svg/painting/ -- I guess that's the best matching chapter from SVG.
On inspections of the failing calculation, this is what is happening:

The complex color is mColor=0x8082004B, mBgRatio=2, mFgRatio=0.375.

p1 = 2,
a1 = 128/255 = 0.5019607...
b1 = a1 * b1 = 128/255 * 130 = 64.7539411...

p2 = 0.375
a2 = 255/255 = 1.
b1 = 115

a = clamp(p1 * a1 + p2 * a2) = clamp(2*128/255 + 0.375*255/255) = 1
b = clamp((p1 * b1 + p2 * b2) / a) = (2 * 130 * 128/255 + 0.375 * 115) / 1 = 173.635

Oh no, that rounds up to 174! (0xAE)

Correct answer is:
p1 = 1
a1 = 255/255 = 1
b1 = a1 * b1 = 255/255 * 129 = 129

p2 = 0.375
a2 = 255/255 = 1
b2 = a2 * b2 = 255/255 * 115 = 115

a = clamp(p1 * a1 + p2 * a2) = clamp(1 + 0.375) = 1
b = clamp((p1 * b1 + p2 * b2) / a) = (130 + 0.375 * 115) / 1 = 173.125

Yes, that rounds to 173 (0xAD)

The issue is the alpha 0x80 is just a smidge too big when scaled back.  It should be 0x7F. (Or aBgRatio should 1 not 2)
The round error happens because of the alpha losing its precision during the conversion to integer.

So I think we should not do the optimization because the correctness needs further reasoning and it can be error-prone, and the underlying issue isn't really fixed here. (It cannot be fixed until we add more bits to alpha.)

I would prefer we change the test to make it work properly in this case (or just change the reference with a comment saying the exact correct result).
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive until July 7) from comment #21)
> The round error happens because of the alpha losing its precision during the
> conversion to integer.
> 
> So I think we should not do the optimization because the correctness needs
> further reasoning and it can be error-prone, and the underlying issue isn't
> really fixed here. (It cannot be fixed until we add more bits to alpha.)
> 
> I would prefer we change the test to make it work properly in this case (or
> just change the reference with a comment saying the exact correct result).

I've worked through the equations for addition and lerping in this case and can see a better strategy, that I'm going to implement.  On paper, these changes would result in aBgRatio not becoming 2 and alpha not becoming 0x80.

I can walk through the details in Sydney, if you wish?
I can imagine how such strategy may work given this, and that should be better than the current special-casing I think. I'm fine if you do that I guess.
Attachment #8989660 - Flags: review+ → review?(cam)
Attachment #8989660 - Flags: review?(cam) → review+
Comment on attachment 8989659 [details]
Bug 1467622 - P4: Correct background/foreground ratio mixing.

https://reviewboard.mozilla.org/r/254668/#review262750

::: servo/components/style/values/animated/color.rs:104
(Diff revision 3)
> +                red: color.red * ratios.bg,
> +                green: color.green * ratios.bg,
> +                blue: color.blue * ratios.bg,

This function is also used in compute distance, and I suspect that one probably shouldn't be changed this way...

::: servo/components/style/values/animated/color.rs:173
(Diff revision 3)
> +                //    c1 op c2
> +                //    = (bg1 * mColor1 + fg1 * foreground) op (bg2 * mColor2 + fg2 * foreground)
> +                //    = (bg1 * mColor1 op bg2 * mColor2) + (fg1 op fg2) * foreground

Maybe worth commenting that it works because `c1 op c2` means `c1 * p + c2 * q`, and thus this equation.

::: servo/components/style/values/animated/color.rs:193
(Diff revision 3)
> -                // Then we compute the final background ratio, and derive
> -                // the final alpha value from the effective alpha value.
> +
> +                // To calculate the final foreground color rations, perform
> +                // animation on effective ratios.
>                  let self_ratios = self.effective_ratios();
>                  let other_ratios = other.effective_ratios();
>                  let ratios = self_ratios.animate(&other_ratios, procedure)?;

If you are only animating `fg` maybe you can `self_ratios.fg.animate(&other_ratios.fg, procedure)?` instead, and that way `ComplexColorRatios` probably don't need `Animate` anymore.
Attachment #8989659 - Flags: review?(xidorn+moz)
Comment on attachment 8989659 [details]
Bug 1467622 - P4: Correct background/foreground ratio mixing.

https://reviewboard.mozilla.org/r/254668/#review262750

> This function is also used in compute distance, and I suspect that one probably shouldn't be changed this way...

Good call. I've exacted the scaling of background color into a helper function that is local to animate().

> Maybe worth commenting that it works because `c1 op c2` means `c1 * p + c2 * q`, and thus this equation.

I expanded the comment explaining the refactoring, including changing variable names to match the explanation. HTH.
Comment on attachment 8989659 [details]
Bug 1467622 - P4: Correct background/foreground ratio mixing.

https://reviewboard.mozilla.org/r/254668/#review263010
Attachment #8989659 - Flags: review?(xidorn+moz) → review+
Pushed by dglastonbury@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/90cede21bad1
P1: nsStyleSVGPaint - Change nscolor to StyleComplexColor. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/56142038cc7f
P2: Change SMIL paced tests to represent new distance calc. r=birtles
https://hg.mozilla.org/integration/autoland/rev/e1dbee410e98
P3: Cleanup transition tests with currentcolor. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/74533cad9479
P4: Correct background/foreground ratio mixing. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/8694fe928b04
P5: Reftests for SVG pserver currentcolor override. r=heycam
Backed out 5 changesets (bug 1467622) or reftest failures on layout/reftests/svg/smil/style/anim-css-fill-1-from-by-curcol-hex.svg.

Backout: https://hg.mozilla.org/integration/autoland/rev/5a5c74e9ccc025350edeb6009e9071ee1f3e2891

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8694fe928b04200f132a4c45c77b9d3e6353bf93&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&selectedJob=187539823

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=187539823&repo=autoland&lineNumber=7211

06:21:10     INFO -  REFTEST TEST-START | file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-from-by-curcol-hex.svg == file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-ref.svg
06:21:10     INFO -  REFTEST TEST-LOAD | file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-from-by-curcol-hex.svg | 19 / 102 (18%)
06:21:10     INFO -  ++DOMWINDOW == 18 (000001BAA767C400) [pid = 7140] [serial = 48] [outer = 000001BAB6ACE200]
06:21:10     INFO -  --DOMWINDOW == 17 (000001BAB6FDE800) [pid = 7140] [serial = 35] [outer = 0000000000000000] [url = data:text/html;charset=UTF-8,%3C%21%2D%2DCLEAR%2D%2D%3E]
06:21:10     INFO -  --DOMWINDOW == 16 (000001BAB6AB8400) [pid = 7140] [serial = 33] [outer = 0000000000000000] [url = data:text/html;charset=UTF-8,%3C%21%2D%2DCLEAR%2D%2D%3E]
06:21:10     INFO -  --DOMWINDOW == 15 (000001BAB6AB7400) [pid = 7140] [serial = 34] [outer = 0000000000000000] [url = file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-from-by-hex-hex.svg]
06:21:10     INFO -  --DOMWINDOW == 14 (000001BAACE2F400) [pid = 7140] [serial = 32] [outer = 0000000000000000] [url = file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-by-ident-hex.svg]
06:21:10     INFO -  --DOMWINDOW == 13 (000001BAACE2F800) [pid = 7140] [serial = 36] [outer = 0000000000000000] [url = file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-from-by-ident-hex.svg]
06:21:10     INFO -  --DOMWINDOW == 12 (000001BAA7696800) [pid = 7140] [serial = 38] [outer = 0000000000000000] [url = file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-from-to-hex-hex.svg]
06:21:10     INFO -  --DOMWINDOW == 11 (000001BAB6FE4000) [pid = 7140] [serial = 37] [outer = 0000000000000000] [url = data:text/html;charset=UTF-8,%3C%21%2D%2DCLEAR%2D%2D%3E]
06:21:10     INFO -  --DOMWINDOW == 10 (000001BAB6FDD800) [pid = 7140] [serial = 39] [outer = 0000000000000000] [url = data:text/html;charset=UTF-8,%3C%21%2D%2DCLEAR%2D%2D%3E]
06:21:10    ERROR -  REFTEST TEST-UNEXPECTED-FAIL | file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-from-by-curcol-hex.svg == file:///C:/Users/task_1531289684/build/tests/reftest/tests/layout/reftests/svg/smil/style/anim-css-fill-1-ref.svg | image comparison, max difference: 1, number of differing pixels: 442
Flags: needinfo?(dglastonbury)
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11917 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR was closed without merging
Flags: needinfo?(dglastonbury)
Spoke to :xidorn and :heycam and the agreement was to mark the test as fuzzy. (The test is already marked fuzzy for skiaContent)
Comment on attachment 8991518 [details]
Bug 1467622 - P6: Mark anim-css-fill reftest fuzzy on Windows.

https://reviewboard.mozilla.org/r/256424/#review263322

::: layout/reftests/svg/smil/style/reftest.list:34
(Diff revision 1)
>  fuzzy-if(skiaContent,1,550) == anim-css-fill-1-to-ident-hex.svg         anim-css-fill-1-ref.svg
>  fuzzy-if(skiaContent,1,550) == anim-css-fill-1-to-ident-ident.svg       anim-css-fill-1-ref.svg
>  
>  # 'fill' property, from/to/by, with 'currentColor' keyword
>  fuzzy-if(skiaContent,1,550) == anim-css-fill-1-by-ident-curcol.svg      anim-css-fill-1-ref.svg
> -fuzzy-if(skiaContent,1,550) == anim-css-fill-1-from-by-curcol-hex.svg   anim-css-fill-1-ref.svg
> +fuzzy-if(/^Windows\x20NT/.test(http.oscpu),1,442) fuzzy-if(skiaContent,1,550) == anim-css-fill-1-from-by-curcol-hex.svg anim-css-fill-1-ref.svg # Bug 1467622

Just use `winWidget` should be enough for this case.

Adding comment pointing to this bug doesn't make much sense. Probably just remove it. I consider bug number in comment to mean "the failure type can be removed with bug XXX fixed", which it's not the case here.

It is usually easy to find bug which adds the fuzzy via blame, so adding this comment doesn't add much value, and the bug adding it is also not very likely to be a useful information for fixing it in general.
Attachment #8991518 - Flags: review?(xidorn+moz) → review+
Pushed by dglastonbury@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a85379bad09f
P1: nsStyleSVGPaint - Change nscolor to StyleComplexColor. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/04de06202c82
P2: Change SMIL paced tests to represent new distance calc. r=birtles
https://hg.mozilla.org/integration/autoland/rev/9ce0bf688f66
P3: Cleanup transition tests with currentcolor. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/0b95ed91dcea
P4: Correct background/foreground ratio mixing. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/787756851d80
P5: Reftests for SVG pserver currentcolor override. r=heycam
https://hg.mozilla.org/integration/autoland/rev/7d7fe1b01693
P6: Mark anim-css-fill reftest fuzzy on Windows. r=xidorn
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Depends on: 1512597
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: