GNOME Bugzilla – Bug 762642
Stack first frame can be jerky
Last modified: 2016-06-04 17:01:53 UTC
Created attachment 322277 [details] [review] add simple transition to widget factory We are running a lot of gtk apps on low powered hardware, and have been battling to get stack transitions smooth. One big problem we've run into is the first frame in the stack animation is often much longer than latter frames, resulting in a jerky beginning to the animation. There's a lot of initial setup that only needs to happen on the first frame. For homogeneous stacks, its the only frame that needs an allocate, and its the only frame where you will need to render the old stack contents into a cairo surface to animate offscreen. Outside of gtk, there will often be application work (such as widget creation) that come along with a stack page switch. Here's some GDK_DEBUG=frames output for gtk3-widget-factory after adding a simple page transition (patch attached). 35: interval=162.2 layout_start=0.5 paint_start=194.6 frame_end=377.2 predicted=16.7 36: interval=388.7 layout_start=0.6 paint_start=24.5 frame_end=124.4 predicted=16.7 37: interval=134.7 layout_start=0.5 paint_start=4.4 frame_end=186.1 predicted=16.7 38: interval=244.0 layout_start=0.5 paint_start=3.9 frame_end=186.9 predicted=16.7 39: interval=198.0 layout_start=0.4 paint_start=3.6 frame_end=212.9 predicted=16.7 41: interval=219.3 layout_start=0.1 frame_end=3.1 predicted=16.7 40: interval=223.3 layout_start=5.7 paint_start=8.6 frame_end=207.7 predicted=16.7 Performance is pretty bad in general, but you can see interval between the first and second frame is double the rest.
Talked to a few people on irc about this, one possibility would be to only add the tick callback after initial frame setup is done. In the case where the first allocate and draw is blazing fast (<16ms) you've wasted a small amount of time you could have been transitioning. But in a case like above, the interval between your first two animation frames will be in line with the rest. Attaching a patch
Created attachment 322278 [details] [review] add tick callback after initial frame
Created attachment 322321 [details] [review] add tick callback after initial frame
Created attachment 322370 [details] [review] stack: Start animation at first frame callback Causes animations to not have a too fast first frame when there was a long setup time during animations.
Here's a simpler patch. Would that be good enough?
Not for us no. That was the first thing I tried, modeled on this https://git.gnome.org/browse/gnome-shell/tree/js/ui/tweener.js#n196 But with it you'll still get your tick callback waking up before the allocate and first draw (with the extra rendering into cairo surface). So in the frame output above, you are still calculating all your timings including that monster interval between the first and second frames. One thing I'd also looked at doing was going off gdk_frame_timings_get_presentation_time instead of the beginning of the frame. But that seemed too against the way everything other widget operates with tick callbacks and didn't play out very simply. Owen suggested this approach and its the simplest I found so far that actually fixes the problem for us.
Review of attachment 322321 [details] [review]: ::: gtk/gtkstack.c @@ +425,3 @@ + if (priv->transition_pos < 1.0) + gtk_stack_set_transition_position (stack, 1.0); + This looks like a behavior change thats unrelated to avoiding first-frame jitter. @@ +911,2 @@ priv->transition_pos = pos; + if (was_running != gtk_stack_get_transition_running (stack)) Can gtk_stack_get_transition_running return different results between lines 910 and 912 ? I don't see how
Review of attachment 322321 [details] [review]: ::: gtk/gtkstack.c @@ +425,3 @@ + if (priv->transition_pos < 1.0) + gtk_stack_set_transition_position (stack, 1.0); + Hopefully not a behavior change? Since we no longer schedule the tick callback immediately, wanted to handle the case where the stack was unmapped before the tick callback got scheduled. So removed the mapped check from the tick callback and just handled everything in the vfunc here. Make sure we clear the callback and complete the animation. @@ +911,2 @@ priv->transition_pos = pos; + if (was_running != gtk_stack_get_transition_running (stack)) Eh, maybe I'm being too tricksy here. We can no longer use the tick_id to decide whether the transition is running. So I use transition_pos instead, which seemed pretty intuitive. See change on 1684 So to figure out whether I should notify here, call gtk_stack_get_transition_running before and after setting transition_pos and see if it changed. Better way to write this?
(In reply to Matt Watson from comment #6) > But with it you'll still get your tick callback waking up before the > allocate and first draw (with the extra rendering into cairo surface). So in > the frame output above, you are still calculating all your timings including > that monster interval between the first and second frames. > Is this an issue only in this particular case or a general problem, like for CSS animations or other in other widgets? Because if it is, it might make sense to write a little helper struct that does animations for us, kinda like: little_helper_start_animation (helper, duration); little_helper_stop (helper); little_helper_next_frame (helper, timestamp); little_helper_is_running (helper); little_helper_get_progress (helper); Obviously with a better name. But then the helper object could do things like ignore the first frame, start counting on the 2nd frame and all that and it would be split into its own file with documentation of why this stuff happens. And we could port all the other animations to it, too! Because I'm worried that if you do this change, it will stop working the next time somebody touches the code because nobody would think it's supposed to work like this. I still don't understand what's happening exactly, I consider myself fairly knowledgable wrt animations...
Created attachment 322427 [details] [review] add tick callback after initial frame
(In reply to Matt Watson from comment #8) > Review of attachment 322321 [details] [review] [review]: > > ::: gtk/gtkstack.c > @@ +425,3 @@ > + if (priv->transition_pos < 1.0) > + gtk_stack_set_transition_position (stack, 1.0); > + > > Hopefully not a behavior change? Since we no longer schedule the tick > callback immediately, wanted to handle the case where the stack was unmapped > before the tick callback got scheduled. > > So removed the mapped check from the tick callback and just handled > everything in the vfunc here. Make sure we clear the callback and complete > the animation. Oh, I hadn't picked up on these changes. Can I suggest to do these things in a preparatory patch ? "Decouple running state of animation from tick cb" or something like that. That might make it more obvious to slow readers like me...
Comment on attachment 322427 [details] [review] add tick callback after initial frame > @@ -2173,6 +2182,10 @@ gtk_stack_render (GtkCssGadget *gadget, > */ > gtk_widget_draw (priv->last_visible_child->widget, pattern_cr); > cairo_destroy (pattern_cr); > + /* Setup for animation should be done now, we can schedule the tick > + * callback. We don't do this earlier to avoid a jerky first frame > + */ > + gtk_stack_schedule_ticks (stack); > } > > cairo_rectangle (cr, > Very bad idea. Render functions must not do anything but render. No side effects allowed. (Also for obscured stacks you wouldn't start the transition until it was unobscured, but that's way less of a problem here.) The code should handle this in the tick function, not by some magic elsewhere.
Yeah was wondering if scheduling tick in draw was safe; good to know. Thanks for review! Back on this. I'll give a shot a the little helper as I think I have a clear enough picture of it. My one concern with moving all this to a general helper is I'm not sure skipping the first frame would be wise for all animations in gtk. Its always good to wait to start the animation until the first frame, that way you are robust to an app triggering an animation and then hogging the main loop for a while before the first frame can start. Skipping the first frame feels more stack specific, it works in the example above because the stack does a lot of extra work inside the allocate and draw of the first frame. But that's not true across the board for gtk animations. So that could be a configurable option of the animation helper, stack could avoid calling little_helper_next_frame the first time, or if skipping feels too hacky we could drop that entirely from the changes...
I had two more general comments on the 'setup cost' issue here: - Why is this so expensive anyway ? We're basically rendering page 1 into an offscreen surface so that we can use it in the animation - but if rendering a single page is already consuming much more than a single frame, why are we even thinking about animating between two of them ? - It feels a bit redundant that we have to explictly render the page into the offscreen surface, since we've already rendered it: it is there on the screen. Maybe there is a way to capture and preserve the last frame we've rendered before the animation, without doing it all over again ?
(In reply to Matthias Clasen from comment #14) > - It feels a bit redundant that we have to explictly render the page into > the offscreen surface, since we've already rendered it: it is there on the > screen. Maybe there is a way to capture and preserve the last frame we've > rendered before the animation, without doing it all over again ? > - "capturing" something that has been drawn is something graphics engines suck at. What we could do is use a pixelcache in anticipation of somebody doing a transition, but that would mean keeping a double-buffer surface around all the time. - The stack may have resized if it isn't homogeneous. Afaik we try to reallocate the other child in that situation (no idea if that's actually a good idea or not). - I think the old widget has a different state while transitioning. All :hover and :focus effects have been cleared when rendering to the surface.
I think this was addressed in 62b224a8dff39703bec39e13ea954b22029ba3df