1103207 - OMTA animations with lower FPS (steps) should not run the compositor at 60 FPS

Status: NEW

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

Something like this:
http://jsfiddle.net/simurai/CGmCe/light/

should run the compositor at the frame rate of the animation and not at 60 FPS like we currently do.

This is important because on B2G it lets us use OMTA for animations while reducing the frame rate.

Comment 1

[Markus Stange [:mstange]]

Attachment: testcase with steps(5)

Comment 2

[Markus Stange [:mstange]]

Attachment: testcase with steps(1) and keyframes

Comment 3

[Sotaro Ikeda [:sotaro]]

[Blocking Requested - why for this release]: This problem increase compositor thread's cpu usage. It could cause the performance problem when high cpu usage application like WebRTC is running.

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> [Blocking Requested - why for this release]: This problem increase
> compositor thread's cpu usage. It could cause the performance problem when
> high cpu usage application like WebRTC is running.

And this bug blocks Bug 1097235.

Comment 5

[No-Jun Park [:njpark]]

Any update on this bug? If we can't resolve this for 2.1, should it be moved to 2.2 instead?

Comment 6

[Benoit Girard (:BenWa)]

I was hoping to get to this before I left but unfortunately we hit too many graphics glitches. I'll put this on the list of possible hand off. Otherwise this will be postponed to Q3.

Comment 7

[No-Jun Park [:njpark]]

Milan, what do you think about noming this bug for 2.2?  If it is not an urgent issue, (that is, its only major fallback is the slight increase in power consumption) should we put it for 3.0?

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

This blocks bug 1097235, but that isn't tagged as a B2G blocker as well.  Let's move this to 3.0

Comment 9

[ldumetz20]

stay off my fucken internet before a sue each and eveyone of u.

Comment 10

[Benoit Girard (:BenWa)]

I'll try and get to this after my scrollbar patch.

Note that we have a good work around for this: Use a GIF at the wanted frame rate, so it's not really critical that we use this. But it's certainly something that we want.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

[Blocking Requested - why for this release]:
Not sure this is a 2.5 blocker as such, as it requires some Gaia work in order to get a payoff on B2G.

Comment 12

[Mahendra Potharaju [:mahe]]

[Tracking Requested - why for this release]:Moving to backlog from 2.5 list

Comment 13

[Benoit Girard (:BenWa)]

Fairly important platform power bug. I have some time to look at this now.

Comment 14

[Benoit Girard (:BenWa)]

My patch computes the next composite times and schedule that.

However we can take another direction and keep ticking the refresh driver but instead just leave it up to the invalidation code to decide that the animation/frame is identical and skip the recomposite. This is simpler but causes more wake-ups (which the refresh driver will do some anyways).

Comment 15

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1103207 - OMTAnimation should recomposite at animation rate, not refresh rate. r=mchang

Bug 1103207 - OMTAnimation should recomposite at animation rate, not refresh rate. r=mchang

Comment 16

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8676901 [details]
MozReview Request: Bug 1103207 - OMTAnimation should recomposite at animation rate, not refresh rate. r=mchang

https://reviewboard.mozilla.org/r/22689/#review20353

Mostly looks good, thanks for doing this! My only concern is keeping a reference to the future schedule composite task as we could leak if the compositor gets destroyed before it runs. In that case, just cancel the task. Thanks!

::: gfx/layers/composite/AsyncCompositionManager.cpp:613
(Diff revision 1)
> +    int steps = animData.mFunctions[segmentIndex]->GetSteps();

nit: extract this out to a function.

::: gfx/layers/composite/AsyncCompositionManager.cpp:664
(Diff revision 1)
> -  return activeAnimations;
> +  if (earliestNextSample.IsNull()) {

nit: delete debug printfs.

::: gfx/layers/composite/AsyncCompositionManager.cpp:686
(Diff revision 1)
> +      nextSampleTime = TimeStamp::Now();

Is this just to make sure we composite again next time? Why is it not just the MinimumNextSample from above again?

::: gfx/layers/ipc/CompositorParent.h:280
(Diff revision 1)
> +  void ScheduleFutureComposition(TimeStamp nextFrame);

nit: I never followed up on that dev-platform list, but do args still have to be in aNextFrame format?

::: gfx/layers/ipc/CompositorParent.cpp:1005
(Diff revision 1)
> +  CancelableTask *renderTask = NewRunnableMethod(this, &CompositorParent::ScheduleComposition);

I'm not sure this is safe. What happens if we have to destroy the compositor before the task runs? We probably have to hold a ref onto the task and cancel if during CompositorParent::Destroy().

::: gfx/layers/ipc/CompositorParent.cpp:1142
(Diff revision 1)
> +      ScheduleFutureComposition(nextFrameTime);

nit: add a comment explaining what this is doing please.

Comment 17

[Benoit Girard (:BenWa)]

(In reply to Mason Chang [:mchang] from comment #16)
> ::: gfx/layers/composite/AsyncCompositionManager.cpp:613
> (Diff revision 1)
> > +    int steps = animData.mFunctions[segmentIndex]->GetSteps();
> 
> nit: extract this out to a function.

I tried but the function would need at least 6 arguments or more which I think is a sign that it's better left inline. Agreed?
(aPoint, animation, segment, animationFunction, positionInSegment, computedTiming)

Comment 18

[Benoit Girard (:BenWa)]

(In reply to Mason Chang [:mchang] from comment #16)
> ::: gfx/layers/ipc/CompositorParent.cpp:1005
> (Diff revision 1)
> > +  CancelableTask *renderTask = NewRunnableMethod(this, &CompositorParent::ScheduleComposition);
> 
> I'm not sure this is safe. What happens if we have to destroy the compositor
> before the task runs? We probably have to hold a ref onto the task and
> cancel if during CompositorParent::Destroy().
> 

I checked, CompositorParent is reference counted. NewRunnableMethod will grab a strong reference. If we're destroyed when the task fires we should just do nothing.

Comment 19

[Benoit Girard (:BenWa)]

Comment on attachment 8676901 [details]
MozReview Request: Bug 1103207 - OMTAnimation should recomposite at animation rate, not refresh rate. r=mchang

Bug 1103207 - OMTAnimation should recomposite at animation rate, not refresh rate. r=mchang

Comment 20

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8676901 [details]
MozReview Request: Bug 1103207 - OMTAnimation should recomposite at animation rate, not refresh rate. r=mchang

https://reviewboard.mozilla.org/r/22689/#review20395

Thanks!

Comment 21

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3247d8b4f70

Comment 22

[Mahendra Potharaju [:mahe]]

Setting 2.5+ flag to land it on 2.5 branch.

Comment 23

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=05044d6002b0

Comment 24

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4a50e769880

Comment 25

[Mahendra Potharaju [:mahe]]

Benoit, 

Can you please update this bug with any progress? This is a 2.5 blocker and needs to land. 

THanks

Comment 26

[Benoit Girard (:BenWa)]

This was only a blocker because bug 1097235 was deemed a blocker/important at the time. It is not anymore. I'm not sure that this bug would fit the bill for a blocker bug if we were to re-triage that.

Regardless right now I'm still dealing with correctness issue. This patch is regression prone so I'd rather not uplift/rush it unless we have a direct usage.

B2G/Gaia wont benefit from this unless it was to make CSS changes to include step() to reduce the animation FPS.

Comment 27

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #26)
> Regardless right now I'm still dealing with correctness issue. This patch is
> regression prone so I'd rather not uplift/rush it unless we have a direct
> usage.
> 
> B2G/Gaia wont benefit from this unless it was to make CSS changes to include
> step() to reduce the animation FPS.

When this is fixed, we can use an async animation for the loading indicator, which would be much nicer (but it definitely isn't a 2.5 blocker)

Comment 28

[Benoit Girard (:BenWa)]

Do you know if there's a lot of places where we would start using this right away? If we're eagerly waiting for this to make several optimizations I'll bump up the priority.

Comment 29

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #28)
> Do you know if there's a lot of places where we would start using this right
> away? If we're eagerly waiting for this to make several optimizations I'll
> bump up the priority.

The loading bar is the only one that immediately comes to mind - we could probably use it for the utility tray icons for data transmission and network-scanning (in place of animated gifs that I think are used at the moment) too, but not sure if that'd actually be considerably more efficient?

As far as I know, we don't have a great many frame-based animations in Gaia system.

Comment 30

[Mahendra Potharaju [:mahe]]

Thanks for the info Chris and Benoit.

Removing the 2.5+ flag and moving it to backlog.

Comment 31

[Josh Cheng [:josh]]

Hi ShianYow,
Bug 1105509 comment 29 mention this bug should be TV P2 blocker. Do we still block on this?
Thanks!

Comment 32

[Shian-Yow Wu [:swu]]

(In reply to Josh Cheng [:josh] from comment #31)
> Hi ShianYow,
> Bug 1105509 comment 29 mention this bug should be TV P2 blocker. Do we still
> block on this?
> Thanks!

Yes.  The 2D gaming/animation apps might be affected by this bug.  As gaming is a priority item for TV, we need to block on this.

Comment 33

[Benoit Girard (:BenWa)]

It's not clear to me how this bug is related to bug 1105509 and/or gaming. Bug 1105509 is about offscreen elements which is orthogonal to this issue. This bug would only help gaming in the case where they would specify step() in their CSS animation for performance reason which AFAIK currently is no games. As well for games we wouldn't be aligning the animation tick so we would like be ticking at 60 FPS still anyways as soon as we have more than one animated element.

Comment 34

[Benoit Girard (:BenWa)]

Comment on attachment 8676901 [details]
MozReview Request: Bug 1103207 - OMTAnimation should recomposite at animation rate, not refresh rate. r=mchang

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

Comment 35

[Benoit Girard (:BenWa)]

Comment on attachment 8676901 [details]
MozReview Request: Bug 1103207 - OMTAnimation should recomposite at animation rate, not refresh rate. r=mchang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22689/diff/3-4/

Comment 36

[Benoit Girard (:BenWa)]

Comment on attachment 8676901 [details]
MozReview Request: Bug 1103207 - OMTAnimation should recomposite at animation rate, not refresh rate. r=mchang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22689/diff/4-5/

Comment 37

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

Comment on attachment 8676901 [details]
MozReview Request: Bug 1103207 - OMTAnimation should recomposite at animation rate, not refresh rate. r=mchang

https://reviewboard.mozilla.org/r/22689/#review24699

::: dom/animation/ComputedTimingFunction.cpp:77
(Diff revision 4)
> +    double v = 1.0 - StepPrevDiscontinuity(mSteps, 1.0 - aPortion);

I don't really understand how this works, or why StepPrevDiscontinuity is needed in the first place. With the exception of when portion == 0 or portion == 1, the points of discontinuity should be the same for step-start and step-end.

Desk-checking a simple example with step-start and steps = 2 we should get discontinuities at 0.5 only.

If I try with aPortion = 0.25 I get:

double v = 1.0 - StepPrevDiscontinuity(2, 1.0 - 0.25)

StepPrevDiscontinuity(2, 0.75):
  uint32_t step = uint32_t(0.75 * 2)
                = uint32_t(1.5);
                = 1;
  return std::max(0.0, (1 - 1.0) / double(2))
       = std::max(0.0, 0.0 / 2.0)
       = std::max(0.0, 0.0)
       = 0.0

i.e. double v = 1.0 - 0.0 = 1.0

But I think I should get 0.5 here?

Am I missing something?

In any case, it seems like GetNextDiscontinuity should be sufficient for both step-start and step-end.

Discontinuities are as follows:
steps(1, start) = [ ] ([ 0 ] if you allow aPortion < 0.0)
steps(1, end) = [ 1 ]
steps(2, start) = [ 0.5 ] ( [ 0, 0.5 ] if you allow aPortion < 0.0)
steps(2, end) = [ 0.5, 1 ]

But for the purpose of this optimization I think it would be find if we ignored the type of step.

::: dom/animation/ComputedTimingFunction.cpp:81
(Diff revision 4)
> +  double v = StepNextDiscontinuity(mSteps, aPortion);

Please add an assertion before this that mType == nsTimingFunction::Type::StepEnd since we'll likely extend the set of timing function types in future (including adding StepMiddle).

::: dom/animation/KeyframeEffect.cpp:9
(Diff revision 4)
> +#include <algorithm>

Just curious why this is needed?

::: gfx/layers/composite/AsyncCompositionManager.h:95
(Diff revision 4)
> +  // Returns now to recomposite on the next frame according to vsync.

What does 'now' refer to here? TimeStamp::Now()? Something else?

::: gfx/layers/composite/AsyncCompositionManager.cpp:560
(Diff revision 4)
> -static bool
> +static mozilla::TimeStamp

No need for mozilla:: here I think.

::: gfx/layers/composite/AsyncCompositionManager.cpp:560
(Diff revision 4)
> -static bool
> +static mozilla::TimeStamp

A brief comment explaining what this function returns would be good?

::: gfx/layers/ipc/CompositorParent.cpp:1161
(Diff revision 4)
> +      // used by CSS OMTA animation using step().

"This is typically used by animations using step() timing functions."

(i.e. don't mention CSS OMTA animation since this applies to transitions too and, in the very near future, script-generated animations.)

Looks great, but I'd like to understand how StepPrevDiscontinuity is supposed to work. Also, we'll have to be careful to disable this optimization when implementing bug 1216842.

Comment 38

[Benoit Girard (:BenWa)]

I was under the impression that StepPrevDiscontinuity was needed but I'll have another look with your comments in mind. With the all-hands I'll get back to this on the 14th. Thanks for the review!

Comment 39

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

(In reply to Benoit Girard (:BenWa) from comment #38)
> I was under the impression that StepPrevDiscontinuity was needed but I'll
> have another look with your comments in mind. With the all-hands I'll get
> back to this on the 14th. Thanks for the review!

Sure, that's fine. Just thinking about why StepPrevDiscontinuity might be needed, I realized I'm not sure we're correctly handling the case of a negative playback rate or a reverse (or alternate-reverse) playback direction. In fact, I'm not sure we're handling playback rate != 1.0 correctly either.

Comment 40

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

I had a bit more a think about how to structure this to make it more robust and I think we need to re-arrange it a little bit as follows:

* Add some method somewhere that tells us of the mProgress is currently going up or down based on the combination of the "direction" property of the effect and the sign of its Animation's playback rate. Then we will know if we should look for the previous or next discontinuity.

* Add a static KeyframeEffectReadOnly::UncomputeTiming -- ideally this would just take a ComputedTiming object as its object and give you back an animation time but our ComputedTiming objects don't currently contain all the information they should so I think we'll end up passing the AnimationTiming object as well.

The function should basically do the reverse of KeyframeEffectReadOnly::GetComputedTimingAt. It should return a Maybe<> or Nullable<> object. In cases where we can't invert the timing calculation (due to presence of an effect-level timing function (introduced by bug 1216842) we should just give up and return null/empty.

This will take care of the animation direction for us.

* Then we should take the result of UncomputeTiming and pass it to Animation::AnimationTimeToTimeStamp (possibly reworking it to return a TimeDuration instead if that's what we need). That will take care of the playback rate for us.