1371480 - stylo: Paced animation of paint servers does not work

Status: RESOLVED FIXED

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

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Debugging layout/reftests/svg/smil/style/anim-css-fill-2-paced-rgb.svg (which is failing on stylo) it seems that we always get back zero from Servo_AnimationValues_ComputeDistance.

The reason is that compute_distance for IntermediateSVGPaint does not work correctly.

IntermediateSVGPaint is an enum containing two members: a paint and a fallback color (for context-fill/stroke). We successfully calculate the distance of the paint member but the fallback color is an Option and the distance calculation for Option is as follows:

    fn compute_squared_distance(&self, other: &Self) -> Result<f64, ()> {
        match (self, other) {
            (&Some(ref this), &Some(ref other)) => {
                this.compute_squared_distance(other)
            },
            _ => Err(()),
        }
    }

In the case that the two paints have an empty fallback color this will return Err(()).

Since the compute_distance calculation for IntermediateSVGPaint is as follows:

    fn compute_squared_distance(&self, other: &Self) -> Result<f64, ()> {
        Ok(self.kind.compute_squared_distance(&other.kind)? +
            self.fallback.compute_squared_distance(&other.fallback)?)
    }

We will end up returning Err from there as well.

Then back in Servo_AnimationValues_ComputeDistance, we will convert that to zero:

  from_value.compute_distance(to_value).unwrap_or(0.0)

It seems like we just need to add a branch to Option's compute_distance calculation to handle the "both none" case (and return zero).

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Reduced test case

Reduced test case. This will animate in discrete steps in stylo but it should animate smoothly.

Comment 2

[Robert Longson [:longsonr]]

stylo doesn't support fallback being none, it only supports fallback being a colour or not set.

Not set means use the default colour, for context-fill that's black, for everything else that's transparent. See bug 1360935, bug 1352258 and bug 1347409 comment 7 for more details.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Robert Longson from comment #2)
> stylo doesn't support fallback being none, it only supports fallback being a
> colour or not set.

Sorry, by none I mean not set (since rust's Option type is either Some or None).

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Brian Birtles (:birtles) from comment #0)
> It seems like we just need to add a branch to Option's compute_distance
> calculation to handle the "both none" case (and return zero).

I tried this fix:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7418b906bfdecbca64e153fa8097f458a0bba93c

Comment 5

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1371480 - Return zero when computing distance of Option with both value is none.

Review commit: https://reviewboard.mozilla.org/r/148364/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148364/

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1371480 - Remove test fail annotation of paced animation of paint servers on stylo.

Review commit: https://reviewboard.mozilla.org/r/148370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148370/

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8877030 [details]
Bug 1371480 - Return zero when computing distance of Option with both value is none.

https://reviewboard.mozilla.org/r/148364/#review152798

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8877031 [details]
Bug 1371480 - Remove test fail annotation of paced animation of paint servers on stylo.

https://reviewboard.mozilla.org/r/148370/#review152800

Comment 9

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 2 changesets with 2 changes to 2 files
remote: (84157bfa7121 modifies servo/components/style/properties/helpers/animated_properties.mako.rs from Servo; not enforcing peer review)
remote: (1 changesets contain changes to protected servo/ directory: 84157bfa7121)
remote: ************************************************************************
remote: you do not have permissions to modify files under servo/
remote: 
remote: the servo/ directory is kept in sync with the canonical upstream
remote: repository at https://github.com/servo/servo
remote: 
remote: changes to servo/ are only allowed by the syncing tool and by sheriffs
remote: performing cross-repository "merges"
remote: 
remote: to make changes to servo/, submit a Pull Request against the servo/servo
remote: GitHub project
remote: ************************************************************************
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.e_prevent_vendored hook failed
abort: push failed on remote

Comment 10

[Mantaroh Yoshinaga[:mantaroh]]

Oops, I missed landing.
I forgot this patch included the servo part..

Comment 11

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Servo PR, #17306

Comment 12

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8877031 [details]
Bug 1371480 - Remove test fail annotation of paced animation of paint servers on stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148370/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/012342c75d4b
Remove test fail annotation of paced animation of paint servers on stylo. r=birtles

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/012342c75d4b