Bug 168837 – [iOS] Throttle requestAnimationFrame to 30fps in low power mode

• Component: Animations
• Status: RESOLVED FIXED

[Chris Dumez]

Throttle requestAnimationFrame to 30fps in low power mode on iOS to save battery.

[Radar WebKit Bug Importer]

<rdar://problem/30700929>

[Chris Dumez]

Created attachment 302874 [details]
WIP patch

[Simon Fraser (smfr)]

Comment on attachment 302874 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302874&action=review

> Source/WebCore/dom/ScriptedAnimationController.cpp:52
> +// Interval to hit 30ps.
> +#define halfSpeedThrottlingAnimationInterval 0.030
> +#define aggressiveThrottlingAnimationInterval 10

"fps"

Did you test that .030ms gives us 30fps? We may not need to back off as much.

Would be nice if these were "Seconds".

> Source/WebCore/dom/ScriptedAnimationController.cpp:-95
> -    LOG(Animations, "%p - Setting RequestAnimationFrame throttling state to %d in frame %p (isMainFrame: %d)", this, isThrottled, m_document->frame(), m_document->frame() ? m_document->frame()->isMainFrame() : 0);

I would like to see a log when the throttling reasons changes (added or removed).

> Source/WebCore/dom/ScriptedAnimationController.h:83
> +    double animationIntervalForThrottlingState() const;

Return a Seconds

> Source/WebCore/page/Page.cpp:1119
> +    for (Frame* frame = &page.mainFrame(); frame; frame = frame->tree().traverseNext()) {
> +        auto* document = frame->document();
> +        if (!document)
> +            continue;
> +        auto* scriptedAnimationController = document->scriptedAnimationController();
> +        if (!scriptedAnimationController)
> +            continue;
> +
> +        if (operation == ThrottlingReasonOperation::Add)
> +            scriptedAnimationController->addThrottlingReason(reason);
> +        else
> +            scriptedAnimationController->removeThrottlingReason(reason);

What happens to unparented frames? Should we update a frame's throttling state when added to the DOM?

[Chris Dumez]

Comment on attachment 302874 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302874&action=review

>> Source/WebCore/dom/ScriptedAnimationController.cpp:52
>> +#define aggressiveThrottlingAnimationInterval 10
> 
> "fps"
> 
> Did you test that .030ms gives us 30fps? We may not need to back off as much.
> 
> Would be nice if these were "Seconds".

I am in the process of verifying the result on device.

What do you mean by "Seconds"? Do you think I should use static const variables of std::chrono type or Just append "Seconds" suffix to the defines.
I think we should kill those defines in favor of static const variables personally. I would also use std::chrono types for more clarity.

>> Source/WebCore/dom/ScriptedAnimationController.cpp:-95
>> -    LOG(Animations, "%p - Setting RequestAnimationFrame throttling state to %d in frame %p (isMainFrame: %d)", this, isThrottled, m_document->frame(), m_document->frame() ? m_document->frame()->isMainFrame() : 0);
> 
> I would like to see a log when the throttling reasons changes (added or removed).

Sure, I'll add back logging. RELEASE_LOG() or good old LOG()? I would go with RELEASE_LOG personally as long as it does not log too frequently.

>> Source/WebCore/page/Page.cpp:1119
>> +            scriptedAnimationController->removeThrottlingReason(reason);
> 
> What happens to unparented frames? Should we update a frame's throttling state when added to the DOM?

I haven't looked into this case yet. I'll make sure to look into it before uploading for review.

[Simon Fraser (smfr)]

(In reply to comment #4)
> Comment on attachment 302874 [details]
> WIP patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302874&action=review
> 
> >> Source/WebCore/dom/ScriptedAnimationController.cpp:52
> >> +#define aggressiveThrottlingAnimationInterval 10
> > 
> > "fps"
> > 
> > Did you test that .030ms gives us 30fps? We may not need to back off as much.
> > 
> > Would be nice if these were "Seconds".
> 
> I am in the process of verifying the result on device.
> 
> What do you mean by "Seconds"? Do you think I should use static const
> variables of std::chrono type or Just append "Seconds" suffix to the defines.
> I think we should kill those defines in favor of static const variables
> personally. I would also use std::chrono types for more clarity.

I meant the Seconds type. We're moving away from std::chrono in favor of wtf/Seconds etc.

> >> Source/WebCore/dom/ScriptedAnimationController.cpp:-95
> >> -    LOG(Animations, "%p - Setting RequestAnimationFrame throttling state to %d in frame %p (isMainFrame: %d)", this, isThrottled, m_document->frame(), m_document->frame() ? m_document->frame()->isMainFrame() : 0);
> > 
> > I would like to see a log when the throttling reasons changes (added or removed).
> 
> Sure, I'll add back logging. RELEASE_LOG() or good old LOG()? I would go
> with RELEASE_LOG personally as long as it does not log too frequently.

Sure

> >> Source/WebCore/page/Page.cpp:1119
> >> +            scriptedAnimationController->removeThrottlingReason(reason);
> > 
> > What happens to unparented frames? Should we update a frame's throttling state when added to the DOM?
> 
> I haven't looked into this case yet. I'll make sure to look into it before
> uploading for review.

Thanks. With a test! I could imagine a page unparenting an iframe while throttled, then reparenting it when unthrottled; it needs to get updated in that case.

[Chris Dumez]

Created attachment 302882 [details]
WIP Patch

[Chris Dumez]

Created attachment 302953 [details]
WIP patch

[Chris Dumez]

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 302874 [details]
> > WIP patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=302874&action=review
> > 
> > >> Source/WebCore/dom/ScriptedAnimationController.cpp:52
> > >> +#define aggressiveThrottlingAnimationInterval 10
> > > 
> > > "fps"
> > > 
> > > Did you test that .030ms gives us 30fps? We may not need to back off as much.
> > > 
> > > Would be nice if these were "Seconds".
> > 
> > I am in the process of verifying the result on device.
> > 
> > What do you mean by "Seconds"? Do you think I should use static const
> > variables of std::chrono type or Just append "Seconds" suffix to the defines.
> > I think we should kill those defines in favor of static const variables
> > personally. I would also use std::chrono types for more clarity.
> 
> I meant the Seconds type. We're moving away from std::chrono in favor of
> wtf/Seconds etc.
> 
> > >> Source/WebCore/dom/ScriptedAnimationController.cpp:-95
> > >> -    LOG(Animations, "%p - Setting RequestAnimationFrame throttling state to %d in frame %p (isMainFrame: %d)", this, isThrottled, m_document->frame(), m_document->frame() ? m_document->frame()->isMainFrame() : 0);
> > > 
> > > I would like to see a log when the throttling reasons changes (added or removed).
> > 
> > Sure, I'll add back logging. RELEASE_LOG() or good old LOG()? I would go
> > with RELEASE_LOG personally as long as it does not log too frequently.
> 
> Sure
> 
> > >> Source/WebCore/page/Page.cpp:1119
> > >> +            scriptedAnimationController->removeThrottlingReason(reason);
> > > 
> > > What happens to unparented frames? Should we update a frame's throttling state when added to the DOM?
> > 
> > I haven't looked into this case yet. I'll make sure to look into it before
> > uploading for review.
> 
> Thanks. With a test! I could imagine a page unparenting an iframe while
> throttled, then reparenting it when unthrottled; it needs to get updated in
> that case.

From the HTML spec:
"When an iframe element is removed from a document, the user agent must discard the element's nested browsing context, if it is not null, and then set the element's nested browsing context to null."

and

"When an iframe element is inserted into a document that has a browsing context, the user agent must create a new browsing context, set the element's nested browsing context to the newly-created browsing context, and then process the iframe attributes for the "first time"."

So when you remove an iframe from the document and then insert it back, it reloads, it does not keep its state. Upon reloading, the rAF would start again and get the right throttling (I'll add a test).

[Chris Dumez]

Created attachment 302961 [details]
Patch

[Chris Dumez]

Created attachment 302966 [details]
Patch

[Chris Dumez]

Comment on attachment 302966 [details]
Patch

Clearing flags on attachment: 302966

Committed r213169: <http://trac.webkit.org/changeset/213169>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.