Closed Bug 1294614 Opened 8 years ago Closed 8 years ago

StyleAnimationValue::AddWeighted() for colors returns unexpected values in case when aCoeff1 + aCoeff2 != 1.0

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(3 files)

Currently StyleAnimationValue::AddWeighted() premultiplies before calculation and unpremultiplies after the calculation.  This works fine if aCoeff1 + aCoeff2 == 1.0.  But it does not work if aCoeff1 + aCoeff2 != 1.0. For example;

 color1: rgb(100,0,0)
 color2: rgb(200,0,0)
 aCoeff1: 0.5
 aCoeff2: 0

 premultiplied color 1 red: 100 * 1 = 100
 premultiplied color 2 red: 200 * 1 = 100

 calculated alpha: 1 * 0.5 + 1 * 0 = 0.5
 calculated red: 100 * 0.5 + 200 * 0 = 50;

 unpremultiplied red: 50 * (1 / 0.5) = 100

It seems to me that the result should be 50 in this case.
The caller of StyleAnimationValue::AddWeighted() with aCoeff1 + aCoeff2 != 1.0 value is
http://hg.mozilla.org/mozilla-central/file/233ab21b64b5/layout/style/StyleAnimationValue.cpp#l2593 .
> The caller of StyleAnimationValue::AddWeighted() with aCoeff1 + aCoeff2 !=
> 1.0 value is
> http://hg.mozilla.org/mozilla-central/file/233ab21b64b5/layout/style/
> StyleAnimationValue.cpp#l2593 .

Actually the return value for shadow list at here is correct, but AddWeighted() is somewhat misleading. 
We should provide a new function to calculate the values what we need for mismatched shadow list.
So, the caller code in question here (linked in comment 1) is:

>        while (longShadow) {
>          // Passing coefficients that add to less than 1 produces the
>          // desired result of interpolating "0 0 0 transparent" with
>          // the current shadow.
>          if (!AddShadowItems(longCoeff, longShadow->mValue,
>                              0.0, longShadow->mValue,
>                              resultTail)) {

It gets triggered when we're interpolating between two shadow-lists, and one list is longer than the other. So we're effectively pretending that the shorter list was padded with "0 0 0 transparent".  We could get this effect by interpolating between an actual "0 0 0 transparent" shadow-value and longShadow->mValue (using our actual aCoeff1 and aCoeff2) -- but the code currently just uses a zero coefficient and a bogus shadow value (longShadow) since it happens to know it'll produce the same result.

But if that's problematic, let's just use an actual "0 0 0 transparent" shadow value here, with the correct coefficient (not 0).  Let's just create a helper-function, "const nsCSSValue& GetTransparentShadow() {...}", which will lazily initialize a static local nsCSSValue variable (give it a shadow value that represents "0 0 0 transparent" the first time the function is called).  This function would always return that same nsCSSValue whenever it's called.

Would that solve this problem?
Flags: needinfo?(hiikezoe)
One other case to consider -- AddWeighted does also get called with not-necessarily-summing-to-1 coefficients via the StyleAnimationValue::Add() codepath, too... (which is for handling accumulation from repeats in SMIL animation of SVG's CSS properties like "stroke" and "fill", for example)

If we coefficients not summing to 1 cause problems for us, then I expect they'll cause problems in that case too -- not just from this shadowitems callsite.
Hmm, looks like we also have non-summing-to-1 coefficients in AddFilterFunction() (via its first two calls to AddFilterFunctionImpl):
https://dxr.mozilla.org/mozilla-central/rev/233ab21b64b5d5e9f2f16ea2d4cfb4c8b293c5c4/layout/style/StyleAnimationValue.cpp#1779

Would we need to fix those as well, to sufficiently work around this?

Maybe it is worth having a "2nd coefficient is 0" special-case in AddWeightedColor code (or a custom function after all... (rather than making dummy values as I suggested in comment 3 here)
Thank you, Daniel.  I had never noticed the StyleAnimationValue::Add() in the SMIL code. 

(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #5)
> Hmm, looks like we also have non-summing-to-1 coefficients in
> AddFilterFunction() (via its first two calls to AddFilterFunctionImpl):
> https://dxr.mozilla.org/mozilla-central/rev/
> 233ab21b64b5d5e9f2f16ea2d4cfb4c8b293c5c4/layout/style/StyleAnimationValue.
> cpp#1779
> 
> Would we need to fix those as well, to sufficiently work around this?
> 
> Maybe it is worth having a "2nd coefficient is 0" special-case in
> AddWeightedColor code (or a custom function after all... )

Yeah, it sounds reasonable to me.  I am going to fix this bug first.

1) write test cases to check CSS filter list transition which has different lengths list (It seems we have no such test cases, we have shadow cases though)
2) factor out AddWeighedColors which will be similar to part 4 patch in bug 1216843 but its arguments are nscolor.  In bug 1216843 the arguments will be changed nsCSSValue.
3) create a custom function (will be named diluteColor?) for the special case and call it from AddWeighted().
 
I would like to handle SMIL case here as well, but it's not yet clear to me.  I presume I need to write test cases for it and AddWeighedColors can be used for it.
Assignee: nobody → hiikezoe
Blocks: 1216843
Status: NEW → ASSIGNED
Flags: needinfo?(hiikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> Thank you, Daniel.  I had never noticed the StyleAnimationValue::Add() in
> the SMIL code. 
> 
> (In reply to Daniel Holbert [:dholbert] (recovering from vacation
> reviews/bugmail) from comment #5)
> > Hmm, looks like we also have non-summing-to-1 coefficients in
> > AddFilterFunction() (via its first two calls to AddFilterFunctionImpl):
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 233ab21b64b5d5e9f2f16ea2d4cfb4c8b293c5c4/layout/style/StyleAnimationValue.
> > cpp#1779
> > 
> > Would we need to fix those as well, to sufficiently work around this?
> > 
> > Maybe it is worth having a "2nd coefficient is 0" special-case in
> > AddWeightedColor code (or a custom function after all... )
> 
> Yeah, it sounds reasonable to me.  I am going to fix this bug first.
> 
> 1) write test cases to check CSS filter list transition which has different
> lengths list (It seems we have no such test cases, we have shadow cases
> though)

We have already it.
http://hg.mozilla.org/mozilla-central/file/6e191a55c3d2/layout/style/test/test_transitions_per_property.html#l898
Comment on attachment 8782763 [details]
Bug 1294614 - Part 1: Test cases for SMIL animation for RGBA colors.

https://reviewboard.mozilla.org/r/72814/#review70722

Thanks for adding these testcases!  r=me with the following comment-nits addressed:

::: dom/smil/test/db_smilCSSFromBy.js:29
(Diff revision 1)
> +                           { midComp: "rgba(45, 48, 52, 0.6)",
> +                             // (rgb(10, 20, 30) * 0.2 + rgb(50, 50, 50) * 1) / 1.0
> +                             toComp:  "rgb(52, 54, 56)"}),
> +    // SMIL spec does not say how interpolate colors in the case when
> +    // an end value becomes out of range of RGB color ideally.
> +    // Now our implematation just interpolates between the upper boundary.

Two nits here:

(1) I don't think this comment is correct -- the SVG spec on animation *does* have some text on how to do this:
"User agents should clamp color values to allow color range values as late as possible"
https://www.w3.org/TR/SVG11/animate.html#AnimateColorElement

That "as late as possible" spec-text implies that the spec wants us to clamp just before painting, or as close as possible. (We don't really do that right now.)

(2) it was not initially clear to me that there were any out-of-bounds values in play here. (It's only the "By" part that makes the addition go out-of-bounds.)  Please clarify that in the comment here, too.

SO: this comment should be reworded to clarify what the spec actually says & to explain how overflowed-channels actually come into play in this testcase.  Perhaps something like the following (feel free to copypaste and/or adjust as you see fit):
    // Note: technically, the "from" and "by" values in the testcase below
    // would overflow the maxium color-channel values when added together.
    // (e.g. for red [ignoring alpha for now], 100 + 240 = 340 which is > 255)
    // The SVG Animation spec says we should clamp color values "as late as
    // possible," i.e. allow the channel overflow and clamp at paint-time.
    // But for now, we instead clamp the implicit "to" value for the animation
    // and interpolate up to that clamped result.

::: dom/smil/test/db_smilCSSPaced.js:41
(Diff revision 1)
>                              comp1_3: "rgb(50, 50, 50)",
>                              comp2_3: "rgb(128, 100, 128)",
>                              comp1:   "rgb(206, 150, 206)"
>                            }),
> +                          // Use the same RGB component values to make
> +                          // premultication effect more easier

s/more easier/easier to compute/

(both to clarify what you mean, and to clean up redundancy -- "more easier" is redundant.  "easier" already means "more easy")
Attachment #8782763 - Flags: review?(dholbert) → review+
Comment on attachment 8782764 [details]
Bug 1294614 - Part 2: Factor out AddWeightedColors.

https://reviewboard.mozilla.org/r/72816/#review70752

r=me
Attachment #8782764 - Flags: review?(dholbert) → review+
Comment on attachment 8782764 [details]
Bug 1294614 - Part 2: Factor out AddWeightedColors.

https://reviewboard.mozilla.org/r/72816/#review70758

::: layout/style/StyleAnimationValue.cpp:1165
(Diff revision 1)
>                          eCSSUnit_Number);
>  }
>  
> +static nscolor
> +AddWeightedColors(double aCoeff1, const nscolor& aColor1,
> +                  double aCoeff2, const nscolor& aColor2)

Sorry, one late nit on part 2 -- please adjust the type here:
s/const nscolor&/nscolor/

i.e. just pass |aColor1| and |aColor2| by value here -- they're just 32-bit values, so there's no need for them to be const references (no need to try to share memory with the copy in the caller).
Comment on attachment 8782765 [details]
Bug 1294614 - Part 3: Add DiluteColor(). .

https://reviewboard.mozilla.org/r/72818/#review70756

r=me wit the following:

::: layout/style/StyleAnimationValue.cpp:1199
(Diff revision 1)
>    uint8_t Gres = ClampColor((G1 * aCoeff1 + G2 * aCoeff2) * factor);
>    uint8_t Bres = ClampColor((B1 * aCoeff1 + B2 * aCoeff2) * factor);
>    return NS_RGBA(Rres, Gres, Bres, Ares);
>  }
>  
> +// Multiplies |aColor| by |aDilutionRatio| with premultiplication.

Please add a comment here saying something like:
// (The logic here should pretty closely match AddWeightedColors()' logic.)

...since ideally we don't want these functions to diverge very much.

::: layout/style/StyleAnimationValue.cpp:1201
(Diff revision 1)
>    return NS_RGBA(Rres, Gres, Bres, Ares);
>  }
>  
> +// Multiplies |aColor| by |aDilutionRatio| with premultiplication.
> +static nscolor
> +DiluteColor(const nscolor& aColor, double aDilutionRatio)

As noted above for part 2, please just use "nscolor" here (not "const nscolor&")

::: layout/style/StyleAnimationValue.cpp:2413
(Diff revision 1)
>      }
>      case eUnit_Color: {
>        nscolor color1 = aValue1.GetColorValue();
>        nscolor color2 = aValue2.GetColorValue();
> -      nscolor resultColor =
> -        AddWeightedColors(aCoeff1, color1, aCoeff2, color2);
> +      nscolor resultColor;
> +      if (aCoeff2 == 0.0 && aCoeff1 != 1.0) {

Two things:

 (1) I think you can get rid of the second check, and simplify this to just "if (aCoeff2 == 0)".  I don't think we need to care whether aCoeff1 happens to be 1.0 here -- in that case, I suspect it doesn't matter whether we use DiluteColor vs AddWeightedColors -- so we might as well use DiluteColor since it's simpler & cheaper.  (Correct me if I'm missing something, though.)

 (2) Please add a comment here explaining why we need this special case at all (and why we need it specifically for colors but not other types).
Attachment #8782765 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #14)
> > +      if (aCoeff2 == 0.0 && aCoeff1 != 1.0) {
> [...]
>  (1) I think you can get rid of the second check, and simplify this to just
> "if (aCoeff2 == 0)".

Sorry - I meant "0.0" there, of course. (as you've got it)
Comment on attachment 8782763 [details]
Bug 1294614 - Part 1: Test cases for SMIL animation for RGBA colors.

https://reviewboard.mozilla.org/r/72814/#review70722

> Two nits here:
> 
> (1) I don't think this comment is correct -- the SVG spec on animation *does* have some text on how to do this:
> "User agents should clamp color values to allow color range values as late as possible"
> https://www.w3.org/TR/SVG11/animate.html#AnimateColorElement
> 
> That "as late as possible" spec-text implies that the spec wants us to clamp just before painting, or as close as possible. (We don't really do that right now.)
> 
> (2) it was not initially clear to me that there were any out-of-bounds values in play here. (It's only the "By" part that makes the addition go out-of-bounds.)  Please clarify that in the comment here, too.
> 
> SO: this comment should be reworded to clarify what the spec actually says & to explain how overflowed-channels actually come into play in this testcase.  Perhaps something like the following (feel free to copypaste and/or adjust as you see fit):
>     // Note: technically, the "from" and "by" values in the testcase below
>     // would overflow the maxium color-channel values when added together.
>     // (e.g. for red [ignoring alpha for now], 100 + 240 = 340 which is > 255)
>     // The SVG Animation spec says we should clamp color values "as late as
>     // possible," i.e. allow the channel overflow and clamp at paint-time.
>     // But for now, we instead clamp the implicit "to" value for the animation
>     // and interpolate up to that clamped result.

Thanks for the proofreading!

I did not notice the part of the spec.  I think we can clamp color values just before painting after fixing bug 1295107.  Hopefully I can take time to do it later.
Comment on attachment 8782764 [details]
Bug 1294614 - Part 2: Factor out AddWeightedColors.

https://reviewboard.mozilla.org/r/72816/#review70758

> Sorry, one late nit on part 2 -- please adjust the type here:
> s/const nscolor&/nscolor/
> 
> i.e. just pass |aColor1| and |aColor2| by value here -- they're just 32-bit values, so there's no need for them to be const references (no need to try to share memory with the copy in the caller).

Thanks.  My muscle memory has been repeatedly mistaking that values other than standard data types, e.g. int, double, etc., are classes.
Pushed by hiikezoe@mozilla-japan.org:
https://hg.mozilla.org/integration/autoland/rev/e6adad24f5ec
Part 1: Test cases for SMIL animation for RGBA colors. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/e262963372ae
Part 2: Factor out AddWeightedColors. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/ac729cd10150
Part 3: Add DiluteColor(). r=dholbert.
Backed out for failing own test test_smilCSSFromBy.xhtml on Linux pgo:

https://hg.mozilla.org/integration/autoland/rev/fffba8176d3be2533a29712e07439866f166073f
https://hg.mozilla.org/integration/autoland/rev/3364778da09494f86d35d404b09bbc3726f7b9b1
https://hg.mozilla.org/integration/autoland/rev/1eb486173bd7cb8045d259e1130fa133f9f46fed

Failure log:
non-e10s: https://treeherder.mozilla.org/logviewer.html#?job_id=2528858&repo=autoland
e10s: https://treeherder.mozilla.org/logviewer.html#?job_id=2528886&repo=autoland

03:50:07     INFO -  455 INFO TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | fill: checking value halfway through animation - got "rgba(186, 186, 186, 0.898)", expected "rgba(186, 186, 186, 0.9)"
03:50:07     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:268:5
03:50:07     INFO -      AnimTestcase.prototype.seekAndTest@dom/smil/test/smilTestUtils.js:407:7
03:50:07     INFO -      AnimTestcase.prototype.runTest@dom/smil/test/smilTestUtils.js:363:5
03:50:07     INFO -      TestcaseBundle.prototype.go@dom/smil/test/smilTestUtils.js:307:11
03:50:07     INFO -      testBundleList@dom/smil/test/smilTestUtils.js:856:5
03:50:07     INFO -      main@dom/smil/test/test_smilCSSFromBy.xhtml:39:3
03:50:07     INFO -      EventListener.handleEvent*@dom/smil/test/test_smilCSSFromBy.xhtml:44:1
Flags: needinfo?(hiikezoe)
I am going to check from now on, but if the failure happens only on PGO build, it seems like a GCC bug.  I will figure out how to avoid the error.
I can't reproduce the failure on linux64 PGO locally.  It seems an issue specific for linux32 PGO build.

I did a try with the modification that the opacity value in the middle of the animation becomes 0.8.  

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34e4faee822e&selectedJob=26376273
Flags: needinfo?(hiikezoe)
Pushed by hiikezoe@mozilla-japan.org:
https://hg.mozilla.org/integration/autoland/rev/3785d2edc6e9
Part 1: Test cases for SMIL animation for RGBA colors. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/ec23440f9483
Part 2: Factor out AddWeightedColors. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/bbb2c999fa87
Part 3: Add DiluteColor(). r=dholbert.
Depends on: 1328535
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: