Closed Bug 1356941 Opened 7 years ago Closed 7 years ago

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

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(3 files, 13 obsolete files)

Bug 1356941 - Use IntermediateColor for caret-color.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1356941 - Add box-shadow interpolation tests.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1356941 - Add text-shadow interpolation tests.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
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.
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?
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.
Blocks: 1353202
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 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 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 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 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+
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 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 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 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+
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.
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 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 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 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 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 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 on attachment 8860807 [details]
Bug 1356941 ComputedDistance for IntermediateRGBA.

https://reviewboard.mozilla.org/r/132780/#review135610
Attachment #8860807 - Flags: review?(boris.chiou) → 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+
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 on attachment 8860809 [details]
Bug 1356941 - ComputedDistance for IntermediateBoxShadow.

https://reviewboard.mozilla.org/r/132784/#review135620
Attachment #8860809 - Flags: review?(boris.chiou) → 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 on attachment 8860810 [details]
Bug 1356941 - ComputedDistance for IntermediateTextShadow.

https://reviewboard.mozilla.org/r/132786/#review135622
Attachment #8860810 - Flags: review?(boris.chiou) → review+
Attachment #8859101 - Attachment is obsolete: true
Attachment #8859102 - Attachment is obsolete: true
Attachment #8859103 - Attachment is obsolete: true
Attachment #8860806 - Attachment is obsolete: true
Attachment #8859104 - Attachment is obsolete: true
Attachment #8860807 - Attachment is obsolete: true
Attachment #8859105 - Attachment is obsolete: true
Attachment #8860808 - Attachment is obsolete: true
Attachment #8859109 - Attachment is obsolete: true
Attachment #8859110 - Attachment is obsolete: true
Attachment #8860809 - Attachment is obsolete: true
Attachment #8859111 - Attachment is obsolete: true
Attachment #8860810 - Attachment is obsolete: true
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: