Summary: | [iOS] Throttle requestAnimationFrame to 30fps in low power mode | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||
Component: | Animations | Assignee: | Chris Dumez <cdumez> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | barraclough, benjamin, cmarcelo, commit-queue, dbates, dino, esprehn+autocc, kangil.han, kling, koivisto, simon.fraser, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=165694 | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 168985 | ||||||||||||||
Attachments: |
|
Description
Chris Dumez
2017-02-24 11:58:15 PST
Created attachment 302874 [details]
WIP patch
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? 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. (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. Created attachment 302882 [details]
WIP Patch
Created attachment 302953 [details]
WIP patch
(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). Created attachment 302961 [details]
Patch
Created attachment 302966 [details]
Patch
Comment on attachment 302966 [details] Patch Clearing flags on attachment: 302966 Committed r213169: <http://trac.webkit.org/changeset/213169> All reviewed patches have been landed. Closing bug. |