app/internal/window: [wasm] fix animation framerate #7
Conversation
Before this change, Gio could spawn multiple `w.requestAnimationFrame.Invoke`, which generates multiples `w.draw`, faster than the monitor frame-rate. This patch aims to call `w.requestAnimationFrame.Invoke` everyframe, but it will only draw (`w.draw`) when animating. Signed-off-by: Inkeliz <inkeliz@inkeliz.com>
| w.animating = anim | ||
| w.mu.Unlock() |
| } | ||
|
|
||
| func (w *window) SetAnimating(anim bool) { | ||
| w.mu.Lock() | ||
| defer w.mu.Unlock() | ||
| if anim && !w.animating { | ||
| w.requestAnimationFrame.Invoke(w.redraw) |
There was a problem hiding this comment.
Without requestAnimationFrame here, how will animation start? With your change, SetAnimating only sets w.animating, and that's it.
There was a problem hiding this comment.
It starts inside NewWindow, after the w.draw(true).
| w.mu.Unlock() | ||
| if anim { | ||
| w.draw(false) | ||
| } | ||
| w.requestAnimationFrame.Invoke(w.redraw) |
There was a problem hiding this comment.
This is called unconditionally now. When will animation stop?
There was a problem hiding this comment.
The if anim {} is responsable to prevent the redraw. The "animation callback" will never stop. The callback will be triggred each ~16ms (the refresh-rate of the monitor). However, it will not draw a new frame if it's not animating. If you are not animating it's ignored. :)
The old way of calling requestAnimationFrame: triggers the callback faster than the refresh-rate. I think it happens because SetAnimation can be true and false in one frame. Or, it can change one frame to another, which might call requestAnimationFrame inside the SetAnimation AND calls requestAnimationFrame in the current animCallback, so there's two calls in the same frame (or more).
There was a problem hiding this comment.
I don't think it's a good idea to always have requestAnimationFrame running, even if it is nearly a no-op. There must be some other way of fixing the piled up requests. How about tracking whether there is a pending requestAnimationFrame, and not submit another if so?
There was a problem hiding this comment.
First, I did some:
t := time.Now()
for e := range w.Events() {
switch e := e.(type) {
case system.FrameEvent:
gtx := layout.NewContext(&ops, e)
e.Frame(gtx.Ops)
fmt.Println("FRAME", t.Sub(time.Now()))
t = time.Now()
w.Invalidate()
The fmt.Println prints the time betwwen each frame. On Windows/Android it's 16ms. On WASM it's 6ms (or 2ms with #4 applied). Anyway, the WASM is creating frame faster than intended!
I found that the SetAnimating calls the w.requestAnimationFrame and also it's called inside the callback, so I guess... It's calling twice. I insert some fmt.Println, the w.SetAnimating sometimes is false and them true (I don't know if it's one the same frame, or not), but... Since it's false > true, it could call w.requestAnimationFrame again, and again.
If you test the same code (above) with that PR, the timing will be ~16ms.
There was a problem hiding this comment.
I'm sure the PR fixes what it sets out to do (16 ms) frame, but I'm saying it's not worth the cost: always having requestAnimationFrame callbacks, even if the program is otherwise idle.
I suggested tracking whether a requestAnimationFrame has been called, and only submitting a new one if not. I believe that will achieve the same effect: reducing the updates to once every ~16ms, but without callbacks while idle (not animating).
There was a problem hiding this comment.
I don't know how to better fix that. In theory the old code was suppose to prevent multiples w.requestAnimationFrame.Invoke:
if anim && !w.animating {
w.requestAnimationFrame.Invoke(w.redraw)
| w.mu.Unlock() | ||
| if anim { | ||
| w.draw(false) | ||
| } | ||
| w.requestAnimationFrame.Invoke(w.redraw) |
There was a problem hiding this comment.
I'm sure the PR fixes what it sets out to do (16 ms) frame, but I'm saying it's not worth the cost: always having requestAnimationFrame callbacks, even if the program is otherwise idle.
I suggested tracking whether a requestAnimationFrame has been called, and only submitting a new one if not. I believe that will achieve the same effect: reducing the updates to once every ~16ms, but without callbacks while idle (not animating).
|
I can't reproduce the < 16 ms frame times (on macOS Firefox), but I suspect what's fixing your case is the move of requestAnimationFrame after draw. Does fix your < 16 ms frame times? |
|
The suggestion of moving I insert one I think it's switching between |
Indeed, SetAnimation may be called multiple times per frame with some efficiency cost. However, the important problem is whether drawing happens more than once per monitor refresh. Even on my somewhat fast Linux machine running either Firefox or Chromium I get frame times ~16ms. I'm using the hello example to minimize drawing overhead, patched with Output: Writing this, I note that you're using Window.Invalidate to trigger a re-draw. Invalidate is only meant for externally triggered events, such as receiving a response to a network request, and not for animation. Use InvalidateOp for animation. If you'd like to make Window.Invalidate more robust against misuse, I think one way is to cancel any outstanding animation frame callback with cancelAnimationFrame. Or better yet, understand why Window.Invalidate can avoid calling SetAnimating multiple times. |
|
Using bandicam.2021-01-07.14-23-00-192.mp4 |
|
I still can't reproduce the issue, but it sounds like multiple requestAnimationCallbacks are triggered for each frame. Does https://gioui.org/commit/5f6fa25 help? |
…ight Track whether requestAnimationCallback has been called when SetAnimating changes the animated state of the window. Multiple callbacks result in wasteful redraws. Without this change, my browser becomes unresponsive when Window.Invalidate is called every frame. Calling Invalidate every frame is a misuse (InvalidateOp should be used for animation), but it's nice to have reasonable behaviour. This change might also fix the issues described in #7. Signed-off-by: Elias Naur <mail@eliasnaur.com>
|
Yes, it fixes the issue. |
|
Elias:
Can you elaborate on why this is? I might be using it wrong. |
|
@theclapp see https://gioui.org/commit/759b796 where I clarify the difference between Invalidate/InvalidateOp, and add an opportunistic check for Invalidate to make it almost as good as an InvalidateOp. |
|
👍 Thanks, that helps. |
- This is the 1st commit message: ScrollTo, PagePrev/Next, etc This is a combination of 5 commits. - layout: add List.ScrollTo, ScrollPages, and 2 more Also add PagePrev & PageNext. Include tests for all. - app,app/internal/window,io/system: macOS menus A strawman interface to the macOS menuing system. - Make menus compile on other architectures - app/internal/window: Add modifiers to macOS menu event - Update to latest Gio main - This is the commit message gioui#2: Delete app/internal/wm/window.go No longer used in main branch. - This is the commit message gioui#3: Move menu changes into their own files. - This is the commit message gioui#4: io/key,widget: Make editor hotkey filter more specific Only look for up/down/pageup/pagedown if the editor is multiline. Only look for enter/return if the editor is Submit == true. Fix modifier for delete-backward & forward. Implement some key.Set builder functions and use them in the above code. Fixes: https://todo.sr.ht/~eliasnaur/gio/399 - This is the commit message gioui#5: widget: Tweak editor hotkey event listener For delete-forward and backward, only listen for ShortAlt. Only listen for Short-[C,X] (copy selection and cut selection) when there's a selection. - This is the commit message gioui#6: Add system.MenuEvent to Window.processEvent - This is the commit message gioui#7: key: rename BuildKeySet and BuildKeyGroup to BuildKeyset and BuildKeygroup. - This is the commit message gioui#8: Sleep 2ms after onClose. Signed-off-by: Larry Clapp <larry@theclapp.org>
- This is the 1st commit message: ScrollTo, PagePrev/Next, etc This is a combination of 5 commits. - layout: add List.ScrollTo, ScrollPages, and 2 more Also add PagePrev & PageNext. Include tests for all. - app,app/internal/window,io/system: macOS menus A strawman interface to the macOS menuing system. - Make menus compile on other architectures - app/internal/window: Add modifiers to macOS menu event - Update to latest Gio main - This is the commit message gioui#2: Delete app/internal/wm/window.go No longer used in main branch. - This is the commit message gioui#3: Move menu changes into their own files. - This is the commit message gioui#4: io/key,widget: Make editor hotkey filter more specific Only look for up/down/pageup/pagedown if the editor is multiline. Only look for enter/return if the editor is Submit == true. Fix modifier for delete-backward & forward. Implement some key.Set builder functions and use them in the above code. Fixes: https://todo.sr.ht/~eliasnaur/gio/399 - This is the commit message gioui#5: widget: Tweak editor hotkey event listener For delete-forward and backward, only listen for ShortAlt. Only listen for Short-[C,X] (copy selection and cut selection) when there's a selection. - This is the commit message gioui#6: Add system.MenuEvent to Window.processEvent - This is the commit message gioui#7: key: rename BuildKeySet and BuildKeyGroup to BuildKeyset and BuildKeygroup. - This is the commit message gioui#8: Sleep 2ms after onClose. Signed-off-by: Larry Clapp <larry@theclapp.org>
Before this change, Gio could spawn multiple
w.requestAnimationFrame.Invoke,which generates multiples
w.draw, faster than the monitor refresh-rate.This patch aims to call
w.requestAnimationFrame.Invokeeveryframe, but itwill only draw (
w.draw) when animating.