GNOME Bugzilla – Bug 755357
timeline: Minor fixes for debuggability with ClutterMasterClockGdk
Last modified: 2015-09-22 18:08:32 UTC
Here are a few small fixes I made in the course of debugging a ClutterTimeline problem with MX (it turned out MX was causing one frame to take 400ms due to emitting ::style-changed on thousands of widgets). This wasn’t immediately obvious, and so I added some more debug output to ClutterTimeline. Since my understanding of ClutterMasterClock is somewhat limited, some of these fixes (such as using gdk_frame_clock_get_frame_time()) might not be correct, so close review is needed.
Created attachment 311770 [details] [review] timeline: Ensure waiting_first_tick is set before adding the timeline Just in case the timeline starts being prodded by the ClutterMasterClock before the add function returns. (This has not been verified.)
Created attachment 311771 [details] [review] timeline: Add more debug output
Created attachment 311772 [details] [review] gdk: Enable clock updates when timelines are added to the clock Enable animation updates from the GdkFrameClock whenever any timeline is added to the ClutterMasterClockGdk. This may improve animation smoothness (depending on the GDK backend in use) because it allows GDK to tweak its frame timing for animation purposes.
Created attachment 311773 [details] [review] gdk: Use frame time when calculating the tick time for clock updates This is how GdkFrameClock is meant to be used: the frame time is meant to be queried from the GdkFrameClock within its frame signals, rather from the system monotonic time source.
Created attachment 311774 [details] [review] gdk: Fix frame budget diagnostics Set the frame budget so that CLUTTER_ENABLE_DIAGNOSTIC correctly outputs timing diagnostics from the ClutterMasterClockGdk.
Review of attachment 311770 [details] [review]: Sure, this makes sense.
Review of attachment 311771 [details] [review]: Sure.
Review of attachment 311772 [details] [review]: This patch generates a warning: Gdk-CRITICAL **: gdk_frame_clock_idle_end_updating: assertion 'priv->updating_count > 0' failed Simply test with examples/actor-model. ::: clutter/gdk/clutter-master-clock-gdk.c @@ +303,3 @@ + /* Finish updating for each timeline in progress. */ + for (l = master_clock->timelines; l != NULL; l = l->next) + gdk_frame_clock_begin_updating (frame_clock); If this is 'Finish updating', shouldn't it be calling gdk_frame_clock_end_updating()?
Review of attachment 311773 [details] [review]: Yes, that's a good point.
Review of attachment 311774 [details] [review]: Sure.
Attachment 311770 [details] pushed as ae1a848 - timeline: Ensure waiting_first_tick is set before adding the timeline Attachment 311771 [details] pushed as 5545b69 - timeline: Add more debug output Attachment 311773 [details] pushed as 9e8da64 - gdk: Use frame time when calculating the tick time for clock updates Attachment 311774 [details] pushed as 3816d5a - gdk: Fix frame budget diagnostics
Created attachment 311782 [details] [review] gdk: Enable clock updates when timelines are added to the clock Enable animation updates from the GdkFrameClock whenever any timeline is added to the ClutterMasterClockGdk. This may improve animation smoothness (depending on the GDK backend in use) because it allows GDK to tweak its frame timing for animation purposes. --- > ::: clutter/gdk/clutter-master-clock-gdk.c > @@ +303,3 @@ > + /* Finish updating for each timeline in progress. */ > + for (l = master_clock->timelines; l != NULL; l = l->next) > + gdk_frame_clock_begin_updating (frame_clock); > > If this is 'Finish updating', shouldn't it be calling gdk_frame_clock_end_updating()? Indeed. :-( Fixed, and examples/actor-model seems to work now.
Review of attachment 311782 [details] [review]: Nope, still getting a warning. You need to press 'Enter' to enable the "activate" animation; at the end of the transition, you'll get a warning.
Created attachment 311803 [details] [review] gdk: Enable clock updates when timelines are added to the clock Enable animation updates from the GdkFrameClock whenever any timeline is added to the ClutterMasterClockGdk. This may improve animation smoothness (depending on the GDK backend in use) because it allows GDK to tweak its frame timing for animation purposes.
Hope you don't mind, trying to improve upon Philip's attempt. It's obviously a bit tricky. We need to keep track of the number of begin/end we submit to the GdkFrameClock.
Created attachment 311844 [details] [review] Here’s another version of my approach which fixes the warnings from examples/actor-model. It doesn’t use GObject data like Lionel’s approach. The problem was that clutter_master_clock_gdk_remove_timeline() was not checking whether the given timeline gdk: Enable clock updates when timelines are added to the clock Enable animation updates from the GdkFrameClock whenever any timeline is added to the ClutterMasterClockGdk. This may improve animation smoothness (depending on the GDK backend in use) because it allows GDK to tweak its frame timing for animation purposes.
(In reply to Lionel Landwerlin from comment #15) > Hope you don't mind, trying to improve upon Philip's attempt. > It's obviously a bit tricky. We need to keep track of the number of > begin/end we submit to the GdkFrameClock. I don’t mind at all, but I think it should be possible to track the number of begins/ends implicitly using the lists of timelines, which was what I was originally trying (and failing) to achieve. How does the new version look? :-)
Review of attachment 311844 [details] [review]: I don't think that will work. Let's say you have 2 stages relying on the same frame clock (basically 2 embed stages within the same GdkWindow). - You start with no timelines. - You add one stage -> no begin_updating() is called because no timeline - You add a second stage -> still no begin_updating() - You add one timeline -> begin_updating() is called once because the 2 stages share the same frame clock - You remove a stage -> end_updating() is called once because 1 timeline - You remove the other stage -> end_updating() is called once begin_updating count = 1 end_updating count = 2
(In reply to Lionel Landwerlin from comment #18) > Review of attachment 311844 [details] [review] [review]: > > I don't think that will work. > Let's say you have 2 stages relying on the same frame clock (basically 2 > embed stages within the same GdkWindow). > > - You start with no timelines. > - You add one stage -> no begin_updating() is called because no timeline > - You add a second stage -> still no begin_updating() > - You add one timeline -> begin_updating() is called once because the 2 > stages share the same frame clock > - You remove a stage -> end_updating() is called once because 1 timeline > - You remove the other stage -> end_updating() is called once > > begin_updating count = 1 > end_updating count = 2 Good point. Let’s go with your patch.
Review of attachment 311803 [details] [review]: Pushed to master.
Fixed then, I guess.