When calculating the start time for a pending compositor animation, the playback rate should be included

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Basically, we should be doing the same calculation as Animation::StartTimeFromReadyTime.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8863623 [details]
Bug 1361260 - Incorporate playbackRate when calculating the start time of a pending compositor animation;

https://reviewboard.mozilla.org/r/135412/#review138378

::: dom/animation/test/mozilla/file_deferred_start.html:132
(Diff revision 2)
> +// NOTE: As with the previous test, it is important that we DON'T use
> +// SpecialPowers.DOMWindowUtils.advanceTimeAndRefresh here since that takes
> +// us through a different code path.
> +promise_test(function(t) {

It would be nice to have an assertion to check DOMWindowUtils.isTestControllingRefreshes?

::: dom/animation/test/mozilla/file_deferred_start.html:141
(Diff revision 2)
> +  const div = addDiv(t);
> +  div.classList.add('target');

Nit: addDiv(t, { class: 'target' });

::: dom/animation/test/mozilla/file_deferred_start.html:155
(Diff revision 2)
> +    var transformStr =
> +      SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'transform');
> +
> +    var matrixComponents =
> +      transformStr.startsWith('matrix(')
> +      ? transformStr.substring('matrix('.length, transformStr.length-1)
> +                    .split(',')
> +                    .map(component => Number(component))
> +      : [];
> +    assert_equals(matrixComponents.length, 6,
> +                  'Got a valid transform matrix on the compositor'
> +                  + ' (got: "' + transformStr + '")');

This should be factored out as a function and reuse it in above test as well?

::: gfx/layers/Layers.cpp:295
(Diff revision 2)
> +          // This mirrors the calculation in Animation::StartTimeFromReadyTime.
>            if (anim.startTime().IsNull() && !anim.isNotPlaying()) {
> -            anim.startTime() = aReadyTime - anim.holdTime();
> +            anim.startTime() =
> +              anim.playbackRate() == 0
> +              ? aReadyTime
> +              : aReadyTime - anim.holdTime().MultDouble(1.0 /
> +                                                        anim.playbackRate());

This change is really great!
Attachment #8863623 - Flags: review?(hikezoe) → review+
Comment hidden (mozreview-request)

Comment 5

2 years ago
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc0edea49a70
Incorporate playbackRate when calculating the start time of a pending compositor animation; r=hiro

Comment 6

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