stylo: Need a way to store/interpolate colors that RGBA component values exceed 255

RESOLVED FIXED in Firefox 55

Status

()

P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 13 obsolete attachments)

59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
(Assignee)

Description

a year ago
While calculating interpolation value of colors, we shouldn't clamp each components, we should clamp the component values right before we insert animated values into cascade.
(Assignee)

Comment 1

a year ago
The basic idea what I am thinking is that 

* Introduce IntermediateRGBA struct to store out of range component values
* Add animation_value_type option to Longhand something like this: animation_value_type="IntermediateRGBA"
* Specify the animation_value_type for properties that need color animation
* Use specified animation_value_type in AnimationValue instead of computed_value::T
* Implement From traits to convert (clamp) IntermediateRGBA to RGBA
* Use the From traits when we need to uncompute or some other functions
* Introduce some new structs which have IntermediateRGBA for some properties (e.g. box-shadow)

The reason why I did not re-use animation_type as the 'animation_value_type' is that if we re-use it, expected animation_type values are something like this:

 ["none", "normal", "discrete", "IntermediateRGBA"]

This is something weird. If we decided to use CamelCase for the animation_type, 'none' becomes 'None', it misleads to 'None' value of Option.  Anyway, this way might conflict what Brian is going to do for SMIL, any feedback would be appreciated

A try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d88c6670fb61977ce1fd44ca747010f6ad59dd58
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> The reason why I did not re-use animation_type as the 'animation_value_type'
> is that if we re-use it, expected animation_type values are something like
> this:
> 
>  ["none", "normal", "discrete", "IntermediateRGBA"]

Why not just rename "normal" to "ComputedValue"? (And possibly rename animation_type to animation_value_type)

That seems better than having two partially overlapping properties?
(Assignee)

Comment 3

a year ago
You mean ["none", "discrete", "ComputedValue", "IntermediateRGBA"]?

Actually in the patch set in the try, I added:

"IntermediateColor", "Either<IntermediateColor, Auto>", "IntermediateBoxShadowList", "IntermediateTextShadowList"

We are now checking specifed animation_types in data.py [1], we will check only 'none' and 'discrete' there?
Currently I did add a check that animation_value_type is specified only if animation_type is 'normal'.
Anyway I don't have any strong preference for this, so I am OK with the change.

[1] https://hg.mozilla.org/mozilla-central/file/a374c3546993/servo/components/style/properties/data.py#l172
[2] https://hg.mozilla.org/try/rev/5970471c32860c1d2df7bf4295e82871fc4f0990#l1.32
Yes, one property seems preferable to two. Adding a separate a animation_value_type which only works with one type of animation_type seems to introduce another possibility for error.
(Assignee)

Updated

a year ago
Blocks: 1353202
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

a year ago
mozreview-review
Comment on attachment 8859101 [details]
Bug 1356941 - Introduce IntermediateRGBA type.

https://reviewboard.mozilla.org/r/131130/#review135534

::: commit-message-48c02:5
(Diff revision 1)
> +Bug 1356941 - Introduce IntermediateRGBA type. r?birtles
> +
> +We need to store color values that each components exceeds its normal range
> +(i.e. [0, 1]) during animation operations (e.g. interpolation). We clamp it
> +right before we insert animated values into cascade.

(Nit: into the cascade)

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1979
(Diff revision 1)
>      }
>  }
> +
> +#[derive(Copy, Clone, Debug, PartialEq)]
> +#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
> +/// Unlike RGBA, each component value may exceeed its range [0.0, 1.0].

s/exceeed/exceed/

(Nit: s/exceeed its range/exceed the range/ might be more clear.)

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2009
(Diff revision 1)
> +        let alpha = try!(self.alpha.interpolate(&other.alpha, progress));
> +        if alpha == 0. {
> +            Ok(IntermediateRGBA::transparent())

I'm curious about whether or not this is right. It seems to roughly match the behavior of AddWeightedColors in StyleAnimationValue and it makes sense because we can't divide by zero later on, but if you interpolate between transparent red and transparent red, i.e. { 1., 0., 0., 0. } and { 1., 0., 0., 0. }, then it seems like the result should be transparent red? But I guess that so long as we use premulitiplied alpha that won't happen?

If that's the case, then I guess we just need a comment, but I'm not sure exactly what it is.

Comment 17

a year ago
mozreview-review
Comment on attachment 8859102 [details]
Bug 1356941 - Rename animation_type to animation_value_type.

https://reviewboard.mozilla.org/r/131132/#review135538
Attachment #8859102 - Flags: review?(bbirtles) → review+

Comment 18

a year ago
mozreview-review
Comment on attachment 8859103 [details]
Bug 1356941 - Check animation_value_type is not 'discrete' instead of 'normal' for ComputeDistance.

https://reviewboard.mozilla.org/r/131134/#review135540
Attachment #8859103 - Flags: review?(bbirtles) → review+

Comment 19

a year ago
mozreview-review
Comment on attachment 8859105 [details]
Bug 1356941 - Introduce IntermediateColor to store currentcolor or IntermeditateRGBA.

https://reviewboard.mozilla.org/r/131138/#review135546

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2077
(Diff revision 1)
> +impl Interpolate for IntermediateColor {
> +    #[inline]
> +    fn interpolate(&self, other: &Self, progress: f64) -> Result<Self, ()> {
> +        match (*self, *other) {
> +            (IntermediateColor::IntermediateRGBA(ref this), IntermediateColor::IntermediateRGBA(ref other)) => {
> +                this.interpolate(other, progress).map(IntermediateColor::IntermediateRGBA)
> +            }
> +            _ => Err(()),
> +        }

Are we going to implement interpolation of currentColor in a different bug? Do we need to put a comment here?
Attachment #8859105 - Flags: review?(bbirtles) → review+

Comment 20

a year ago
mozreview-review
Comment on attachment 8859104 [details]
Bug 1356941 - Use IntermediateRGBA to store overflowed RGBA components during interpolation.

https://reviewboard.mozilla.org/r/131136/#review135544

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2001
(Diff revision 1)
> +impl <'a> From<<&'a IntermediateRGBA> for RGBA {
> +    fn from(extended_rgba: &IntermediateRGBA) -> RGBA {
> +        RGBA::from_floats(extended_rgba.red,
> +                          extended_rgba.green,
> +                          extended_rgba.blue,
> +                          extended_rgba.alpha)
> +    }
> +}

Should we add a comment here saying that we will clamp these values later and explaining why we don't do that here? (I haven't got to that part of the patch series yet so I'm not sure what the reason is yet.)
Attachment #8859104 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 21

a year ago
mozreview-review-reply
Comment on attachment 8859101 [details]
Bug 1356941 - Introduce IntermediateRGBA type.

https://reviewboard.mozilla.org/r/131130/#review135534

> I'm curious about whether or not this is right. It seems to roughly match the behavior of AddWeightedColors in StyleAnimationValue and it makes sense because we can't divide by zero later on, but if you interpolate between transparent red and transparent red, i.e. { 1., 0., 0., 0. } and { 1., 0., 0., 0. }, then it seems like the result should be transparent red? But I guess that so long as we use premulitiplied alpha that won't happen?
> 
> If that's the case, then I guess we just need a comment, but I'm not sure exactly what it is.

This is somewhat related a bug I filed before (I couldn't find the bug though).  I prefer to use red transparent { 1., 0., 0., 0. } for this case but, yes this is what gecko does. I will add a comment there.

Comment 22

a year ago
mozreview-review
Comment on attachment 8859106 [details]
Bug 1356941 - Use IntermediateColor for caret-color.

https://reviewboard.mozilla.org/r/131140/#review135566
Attachment #8859106 - Flags: review?(bbirtles) → review+

Comment 23

a year ago
mozreview-review
Comment on attachment 8859107 [details]
Bug 1356941 - Add box-shadow interpolation tests.

https://reviewboard.mozilla.org/r/131142/#review135568

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:943
(Diff revision 1)
> +    test(function(t) {
> +      var idlName = propertyToIDL(property);
> +      var target = createTestElement(t, setup);
> +      var animation =
> +        target.animate({ [idlName]: [ 'none',
> +                                      'rgb(100, 100, 100) 10px 10px 10px 0px'] },
> +                       { duration: 1000, fill: 'both' });
> +      testAnimationSamples(animation, idlName,
> +                                  // Premultiplied
> +        [{ time: 500,  expected: 'rgba(100, 100, 100, 0.5) 5px 5px 5px 0px' }]);
> +    }, property + ': none');

Is it worth testing the opposite combination too? i.e. something -> none ?

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:980
(Diff revision 1)
> +    test(function(t) {
> +      var idlName = propertyToIDL(property);
> +      var target = createTestElement(t, setup);
> +      var animation =
> +        target.animate({ [idlName]: [ 'rgb(200, 200, 200) 20px 20px 20px 20px',
> +                                      'rgb(100, 100, 100) 10px 10px 10px 0px, '
> +                                    + 'rgb(100, 100, 100) 10px 10px 10px 0px'] },
> +                       { duration: 1000, fill: 'both' });
> +      testAnimationSamples(animation, idlName,
> +        [{ time: 500,  expected: 'rgb(150, 150, 150) 15px 15px 15px 10px, '
> +                               + 'rgba(100, 100, 100, 0.5) 5px 5px 5px 0px' }]);
> +    }, property + ': shadow list (mismatched list length)');

Likewise, is it worth testing the opposite combinatio here too?
i.e. shorter list -> longer list?
Attachment #8859107 - Flags: review?(bbirtles) → review+

Comment 24

a year ago
mozreview-review
Comment on attachment 8859108 [details]
Bug 1356941 - Add text-shadow interpolation tests.

https://reviewboard.mozilla.org/r/131144/#review135570

Looks good but if you decide to add the extra tests I mentioned for the previous patch them we should add them here as well.
Attachment #8859108 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 25

a year ago
mozreview-review-reply
Comment on attachment 8859104 [details]
Bug 1356941 - Use IntermediateRGBA to store overflowed RGBA components during interpolation.

https://reviewboard.mozilla.org/r/131136/#review135544

> Should we add a comment here saying that we will clamp these values later and explaining why we don't do that here? (I haven't got to that part of the patch series yet so I'm not sure what the reason is yet.)

Actually this is the place that we clamp the component values.  This From trait is used when we uncompute AnimationValue (from_floats clamps each components values). I will add a comment here.
(Assignee)

Comment 26

a year ago
mozreview-review-reply
Comment on attachment 8859105 [details]
Bug 1356941 - Introduce IntermediateColor to store currentcolor or IntermeditateRGBA.

https://reviewboard.mozilla.org/r/131138/#review135546

> Are we going to implement interpolation of currentColor in a different bug? Do we need to put a comment here?

Yeah, I think it will be handled in bug 1345709 (or followup?).

Comment 27

a year ago
mozreview-review
Comment on attachment 8859109 [details]
Bug 1356941 - Introduce a macro to implement Interpolate trait for box-shadow and text-shadow.

https://reviewboard.mozilla.org/r/131146/#review135576

::: servo/components/style/properties/helpers/animated_properties.mako.rs:949
(Diff revision 1)
> -        Ok(TextShadow {
> -            offset_x: try!(self.offset_x.interpolate(&other.offset_x, progress)),
> -            offset_y: try!(self.offset_y.interpolate(&other.offset_y, progress)),
> +            % if "Box" in item:
> +            if self.inset != other.inset {
> +                return Err(());
> -            blur_radius: try!(self.blur_radius.interpolate(&other.blur_radius, progress)),
> -            color: try!(self.color.interpolate(&other.color, progress)),
> -        })
> -    }
> -}
> +            }
> +            % endif

Can we add a comment explaining this part?
Attachment #8859109 - Flags: review?(bbirtles) → review+

Comment 28

a year ago
mozreview-review
Comment on attachment 8859110 [details]
Bug 1356941 - Use IntermediateColor for box-shadow.

https://reviewboard.mozilla.org/r/131148/#review135578

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2131
(Diff revision 1)
> +    #[allow(missing_docs)]
> +    pub struct Intermediate${type}Shadow {

Should we add docs for this? A one sentence description would be fine.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2145
(Diff revision 1)
> +    #[allow(missing_docs)]
> +    pub struct Intermediate${type}ShadowList(pub Vec<Intermediate${type}Shadow>);

Likewise, should we add docs for this?
Attachment #8859110 - Flags: review?(bbirtles) → review+

Comment 29

a year ago
mozreview-review
Comment on attachment 8859111 [details]
Bug 1356941 - Use IntermediateColor for text-shadow.

https://reviewboard.mozilla.org/r/131150/#review135580
Attachment #8859111 - Flags: review?(bbirtles) → review+

Comment 30

a year ago
mozreview-review
Comment on attachment 8859101 [details]
Bug 1356941 - Introduce IntermediateRGBA type.

https://reviewboard.mozilla.org/r/131130/#review135582

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2009
(Diff revision 1)
> +        let alpha = try!(self.alpha.interpolate(&other.alpha, progress));
> +        if alpha == 0. {
> +            Ok(IntermediateRGBA::transparent())

Ok, r=me once we have a suitable comment explaining this behavior.
Attachment #8859101 - Flags: review?(bbirtles) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 47

a year ago
mozreview-review
Comment on attachment 8859103 [details]
Bug 1356941 - Check animation_value_type is not 'discrete' instead of 'normal' for ComputeDistance.

https://reviewboard.mozilla.org/r/131134/#review135604
Attachment #8859103 - Flags: review?(boris.chiou) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 64

a year ago
mozreview-review
Comment on attachment 8860807 [details]
Bug 1356941 ComputedDistance for IntermediateRGBA.

https://reviewboard.mozilla.org/r/132780/#review135610
Attachment #8860807 - Flags: review?(boris.chiou) → review+

Comment 65

a year ago
mozreview-review
Comment on attachment 8860808 [details]
Bug 1356941 - ComputedDistance for IntermediateColor.

https://reviewboard.mozilla.org/r/132782/#review135614
Attachment #8860808 - Flags: review?(boris.chiou) → review+
(Assignee)

Comment 66

a year ago
mozreview-review
Comment on attachment 8860806 [details]
Bug 1356941 - Rename 'normal' of animation_value_type to 'ComputedValue'.

https://reviewboard.mozilla.org/r/132778/#review135612

Hey MozReview, why do you think this patch is needed to be reviwed again?

Comment 67

a year ago
mozreview-review
Comment on attachment 8860809 [details]
Bug 1356941 - ComputedDistance for IntermediateBoxShadow.

https://reviewboard.mozilla.org/r/132784/#review135620
Attachment #8860809 - Flags: review?(boris.chiou) → review+

Comment 68

a year ago
mozreview-review
Comment on attachment 8860806 [details]
Bug 1356941 - Rename 'normal' of animation_value_type to 'ComputedValue'.

https://reviewboard.mozilla.org/r/132778/#review135618

I'm pretty sure I already reviewed this. I guess MozReview got confused again.
Attachment #8860806 - Flags: review?(bbirtles) → review+

Comment 69

a year ago
mozreview-review
Comment on attachment 8860810 [details]
Bug 1356941 - ComputedDistance for IntermediateTextShadow.

https://reviewboard.mozilla.org/r/132786/#review135622
Attachment #8860810 - Flags: review?(boris.chiou) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8859101 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8859102 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8859103 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8860806 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8859104 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8860807 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8859105 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8860808 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8859109 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8859110 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8860809 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8859111 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8860810 - Attachment is obsolete: true

Comment 75

a year ago
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e3b8389a53ca
Use IntermediateColor for caret-color. r=birtles
https://hg.mozilla.org/integration/autoland/rev/5714c00effca
Add box-shadow interpolation tests. r=birtles
https://hg.mozilla.org/integration/autoland/rev/d1b02c96cec5
Add text-shadow interpolation tests. r=birtles

Comment 76

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e3b8389a53ca
https://hg.mozilla.org/mozilla-central/rev/5714c00effca
https://hg.mozilla.org/mozilla-central/rev/d1b02c96cec5
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.