Created attachment 92448 [details] [review]
Patch

FWIW, applying essentially the same fix to GtkScrolledWindow indeed fixes the problem. The think I dislike about this approach is that you need to be very careful about the expose frequency. If you do too many the UI becomes too slow, if you do too few very fast scrolling looks very bad. Around 15/second seemed to work ok on the N800 for most cases, but this magic-number approach is essentially crappy IMHO. That being said, this is way better than the current scrolling artifacts we have.

Why does scrolling cause "data functions which access other objects" to run;
scrolling itself should basically just update a few double values, blit the
surface contents around and queue a redraw on the newly exposed area.
Even on a N800 there shouldn't be much, if any, problem keeping up with
the incoming event stream for that.

Can you get a profile of what is going on when rendering is blocked?

(I'm wondering if the problem here might be related to not clipping invalid
regions when they are scrolled out of the invisible region, so we might
spend lots of time rendering stuff that is not visible. The signature of
this would be that the profile would show most time going to expose events.)

Created attachment 92465 [details]
OProfile log

This is an oprofile log from Contacts running on a Sharp Zaurus PDA, where I was continually dragging scrolling a treeview.  The treeview contains a single string renderer, which is mapped directly to the first column in the model.

If I scroll this at the right speed, I can cause the lack of complete redraws to cause the same string to be repeated over the entire area of the treeview, or just garbage, or fragments of the area repeated.  I'm guessing these artifacts are caused by the guffer scrolling done by X.

Sadly the profile is not that useful...

I spent some time with Owen on IRC, and discovered that the main bottleneck here is rendering the scrollbar.  By removing the call to gdk_window_process_updates() in gtk_range_adjustment_value_changed not only is scrolling a lot smoother, but my test application starts faster because it can concentrate on loading the data and not redrawing the scrollbar.

Another experiment is to do the work in gtk_range_motion_notify into an idle handler.

Good catch guys, I owe you one for next GUADEC. First thing tomorrow I will play with this and see the effect on all the scenarios.

Created attachment 92545 [details] [review]
Another attack

This patch defers updating the scrollbar position into an idle handler.  It seems to perform well for me, the scrollbar lags but not too much, and the content is redrawn far more smoothly.

Looks very good, but I get some huge jumps from time to time. Maybe you should try to give it a higher priority?

I tried with G_PRIORITY_HIGH+30, so that it is called before the expose events, and then we're back to the original behaviour.

Just to confirm I also tried with G_PRIORITY_HIGH, and it is jumpy.

make sure you remove the idle in destroy() or unrealize()

Created attachment 92556 [details] [review]
Revised patch

If a GtkRange is destroyed will unrealize always be called?  I'd have thought so but maybe not.  Anyway, attached patch removes the idle if it has been set, and even puts GDK_THREAD_ENTER/LEAVE around the idle too.

Created attachment 92557 [details] [review]
Real patch

Well, that was the wrong gtk+ tree...

Created attachment 92558 [details] [review]
Cleaner patch

This patch moves the idle management to separate functions, in line with the timeouts.

(In reply to comment #2)
> FWIW, applying essentially the same fix to GtkScrolledWindow indeed fixes the
> problem. The think I dislike about this approach is that you need to be very
> careful about the expose frequency. If you do too many the UI becomes too slow,
> if you do too few very fast scrolling looks very bad. Around 15/second seemed
> to work ok on the N800 for most cases, but this magic-number approach is
> essentially crappy IMHO. That being said, this is way better than the current
> scrolling artifacts we have.

the 15Hz you approximated is not so magic, but actually based on the single-image perception rate of the human eye/brain combination, see: http://beast.gtk.org/wiki:UsabilityGuidelines#gui-event-optimization

(In reply to comment #14)
> Created an attachment (id=92558) [edit]
> Cleaner patch
> 
> This patch moves the idle management to separate functions, in line with the
> timeouts.

please create patches with diff -up to spare every single diff reader from the need to rediff against the right tree on his own.

Created attachment 94039 [details] [review]
[PATCH] Defer GtkRange updates to an idle handler


This patch defers updating the scrollbar position into an idle handler.  It
seems to perform well for me, the scrollbar lags but not too much, and the
content is redrawn far more smoothly.

Original patch by: Ross Burton <ross@openedhand.com>
---
 gtk/gtkrange.c |   38 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 37 insertions(+), 1 deletions(-)

Can you use the same priority that is used for redraws?

If there is another window consuming lots of CPU drawing (think doing animations  and Swfdec) the scrollbar won't update at all.

Created attachment 94244 [details] [review]
[PATCH] Defer GtkRange updates to an idle handler


This patch defers updating the scrollbar position into an idle handler.  It
seems to perform well for me, the scrollbar lags but not too much, and the
content is redrawn far more smoothly.

Original patch by: Ross Burton <ross@openedhand.com>
---
 gtk/gtkrange.c |   60 ++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 50 insertions(+), 10 deletions(-)

the patch above uses GDK_PRIORITY_REDRAW when scheduling the idle source.

i see one basic quirk with this approach. as mentioned in comment #15, for visible GUI updates, frequencies slightly above 16Hz are generally good. however if the slider value changes are hooked to some other feedback mechanism like oscillator frequencies, an artifical lag and also collapsing multiple value changes will make the input/feedback-process noticably more coarse. so i'm not yet persuaded that using an idler to collapse multiple motions is the right approach.
please also note the existance of: http://developer.gnome.org/doc/API/2.0/gtk/GtkRange.html#gtk-range-set-update-policy
which can be used if user perceivable update coalescing is actually desired.

from the discussion, it seems what needs to be solved is the excessive redraw rate upon motion events that is caused by the call to gdk_window_process_updates() from gtk_range_adjustment_value_changed(). simply removing that call already has the effect of throttling range redraws to an acceptable threshold and get handled at PRIORITY_REDRAW.
also, i don't see a compelling reason to keep the current code:
>      /* This is so we don't lag the widget being scrolled. */
>      if (GTK_WIDGET_REALIZED (range))
>        gdk_window_process_updates (GTK_WIDGET (range)->window, FALSE);
the comment is clearly bogus (at least since GtkRange became a NO_WINDOW widget) because the process_updates() call is actually causing aditional lag.

so unless someone points out a significant problem with removing those three lines (note: significantly bigger than the lag caused by forefull redraws of GtkRange->parent->window), i'd rather go with removing those as a solution.

(In reply to comment #21)

> so unless someone points out a significant problem with removing those three
> lines (note: significantly bigger than the lag caused by forefull redraws of
> GtkRange->parent->window), i'd rather go with removing those as a solution.
> 

FWIW, I've been using GTK+ for some time without those lines with pretty good results. The idle hack introduced some strange jumps and slight lag the last time I tried it.
Still, I'd like to know why this started to be a big deal recently, something must have changed elsewhere?

Nothing changed, this redraw lag has always been a problem on slower devices.  Try Poky 3 (using stock GTK+ 2.6) on a slow device and you'll see exactly the same behaviour.

(In reply to comment #23)
> Nothing changed, this redraw lag has always been a problem on slower devices. 
> Try Poky 3 (using stock GTK+ 2.6) on a slow device and you'll see exactly the
> same behaviour.
> 

True, same happens in with maemo and 770/N800 with GTK+ 2.6. Still I could swear now it's even worse, and I've started seeing the same thing on desktop (see bug 469048). It may or may not be related though.

Removing those three lines on my Zaurus running GTK+ 2.11 causes even worse redraw behaviour than without any patches.

(In reply to comment #25)
> Removing those three lines on my Zaurus running GTK+ 2.11 causes even worse
> redraw behaviour than without any patches.

Ross, can you please be more specific? what is "worse redraw behaviour"? note that removing these lines (and using an otherwise unpatched gtk+) will result in *less* redraws. unless you have an already overloaded CPU anyway, it'll also be snappier as a consequence, because less load is generated. pretty much the only thing that could become "worse" is the delay between scrolled-widget (treeview) updates and slider updates. i.e. the slider lagging behind the scrolled contents, that's the only thing those three lines try to "fix".
so is slider-lags the "worse behaviour" you're experiencing, or is it anything else (in which case i had a hard time connecting that to the three line removal).

When I drag the scroll bar it doesn't actually redraw the scroll bar at all, only moving to the correct position when it finished scrolling.  Once I've fixed a crasher in my test application I'll see if I can record a video of it or something.

Finally managed to get a video off this online.

http://burtonini.com/temp/bug-460534.mp4

Watch carefully and you'll see the slider not actually repainting until I've stopped scrolling.  You can't see it very well, but the text in the tree view isn't repainting properly either, it's mostly repeated fragments.

This is with fairly stock 2.11.6 with the three lines in comment #21 removed.

thanks Ross et all.

i've spent some time analyzing the situation, and basically figured that there's no perfect and obvious solution here. if the CPU is idle enough, slider and scroll-area repaints will appear to be allmost instantly and no problematic scrolling behavior can be observed. if we're scrolling with a fully loaded CPU however, it's hard to make the best tradeoffs:

a) GtkAdjustment::changed emissions should still be emitted instantly to allow normal program logic to process user input in realtime (important for audio).
b) slider updates shouldn't be lagging behind the real scroll position too much, so the user still gets feedback for his slider adjustments eventhough the scroll area content updates might be very slow.
c) scroll area update performance shouldn't be thwarted due to excessive amounts of slider updates (e.g. upon every motion event, of which there can be many per second).

on busy CPUs, the current code in Gtk+ trigegrs (c). removing all forefull updates from gtkrange.c doesn't comply with (b) and applying the patch from Comment #19 violates (a).

so i've settled with a different solution now. that is, on busy CPUs slider updates will be throttled to a "lowest bearable minimum" of ~5Hz now, leaving more CPU to scroll area updates. idle CPUs can still render instant updates though and also (a) is preserved by only delaying slider exposes but not adjustment updates.

2007-09-06 13:37:28  Tim Janik  <timj@imendio.com>

        * gtk/gtkrange.c (gtk_range_adjustment_value_changed): removed code
        that forced range repaints upon every motion event, because these tend  
        to stall other repaints on busy CPUs.                                                                    
        added a timer to still force repaints every once in a while (roughly                                                                                 
        5Hz atm) to avoid leaving the user without feedback on the range.          
        fixes bug #460534.

Since the idle approach seems to work better for some people, what about the following: I think, while a) holds true for things like audio, it does not hold true for cases like scrollbars, where the user does not care a lot whether the slider follows the pointer precisely, but visual smoothness is much more immediate issue. So what about adding a GtkUpdateType GTK_UPDATE_IDLE, that will delay updating the adjustment to an idle-handler, similar to Ross/Emmanuele's patch? I can submit a patch for that, if the idea is liked.

On a side note for people that want to workaround this issue right now: apps can catch the "change-value"-signal of their scrollbars by returning TRUE and then install an idle-handler that will update the underlying adjustment.

(In reply to comment #30)
> Since the idle approach seems to work better for some people, what about the

"better"? that is not clear to me (assuming you mean ebassi's patch).
especially not for all cases.
note that we're discussing semi-pathological scenarios here anyway, i.e. the CPU load has to be >=1 to actually exhibit "bad" scrolling behavior. for that, a mixture of bearable slider lag and reduced exposes (to also reduce CPU load) is probably the better choice, since it attacks the problem scenario from two sides.

> following: I think, while a) holds true for things like audio, it does not hold
> true for cases like scrollbars, where the user does not care a lot whether the
> slider follows the pointer precisely, but visual smoothness is much more
> immediate issue.

YMMV, GtkRange has to work well in a wide range of usage scenarios.
i don't agree that one of (a),(b),(c) is so much more important than any of the respective other two to ignore it. (e.g. forget (a) for the sake of "visual smoothness", or force fastest (b) updates and forego (c), etc.)

> So what about adding a GtkUpdateType GTK_UPDATE_IDLE, that
> will delay updating the adjustment to an idle-handler, similar to
> Ross/Emmanuele's patch?

that is what the current code and my patch do already. if the CPU usage is <100%, we _already_ have perfect idle handler processing of the needed updates through Gtk+'s normal redraw queue. the timer logic from my patch only kicks in for >100% loads and then ensures we still get 5.5Hz updates. using a high prio idler with 0 timeout to force more exposes here would not have a positive effect on the cpu load and tend to force exposes at uselessly high frame rates anyway (anything much beyond 16Hz).

> I can submit a patch for that, if the idea is liked.

the timer i added also acts like an idler, just with a configurable instead of 0 delay. you can experiment with both by slowly decreasing the 181ms in the following line:
  range->layout->repaint_id = gdk_threads_add_timeout_full (GDK_PRIORITY_EVENTS, 
                                                            181, force_repaint,
                                                            range, NULL);
if you s/181/0/, you're effectively using an idler, so there's not much to patch here.

> On a side note for people that want to workaround this issue right now: apps
> can catch the "change-value"-signal of their scrollbars by returning TRUE and
> then install an idle-handler that will update the underlying adjustment.

applications that can live well with delayed updates and tend to hit 100% CPU usage scenrios at all can already adjust the update behavior with the existing API:
http://library.gnome.org/devel/gtk/unstable/GtkRange.html#gtk-range-set-update-policy