Re: [g-a-devel][Fwd: GNOME magnifier changes]

[Brian Cameron <Brian Cameron Sun COM>]

Bill:

> Thanks a lot for doing this work and finding areas for improvement. 
> Generally the patch looks good, but there are a couple of things I am
> concerned about.
> ,
> Firstly, I'd like to see this as two patches, one for the timing-test
> data only, and a subsequent patch that changes behavior and priorities
> of idle handlers, etc.  That is so that we can actually benefit
> immediately from the benchmark information and do before/after
> comparisons on the algorithm changes you propose.

No problem.


> > In zoom_region_queue_update, I now add the zoom_region_process_updates
> > idle handler with a priority of "GDK_PRIORITY_REDRAW - 1".  This
> > ensures that the scaling is done before GTK updates the screen, which
> > (if my understanding is correct) will ensure that the update refreshes
> > the screen with the most current data, rather than from the data from
> > the last iteration of the loop.  This seems, to my eyeball, to make the
> > magnifier look 
>
> My initial reaction is that this is not correct.  Process_updates should
> never block a gtk update; the objective is to make panning response
> maximally responsive, and to update the screen later.  

Okay, I'll change that.

> The other performance-related changes look good.
>
> I would prefer to see some summary output in the --timing-output
> results; could you give min/max/average values for things like scrolling
> time, frames-per-second in scrolling mode, and pixels-moved-per-second ?

I can easily add scrolling time summary results.  However, I am not sure
exactly what you mean by certain terms.  Perhaps you can help me relate
them to the code.

It seems that for every cycle it generally does a single Scroll Idle
and two Zoom Regions.  The Scroll Idle, I believe corresponds to the
updating of the screen.  And the two Zoom Region corresponds to the
two calls to zoom_region_queue_update that get called from
zoom_region_scroll.  I assume one is to pull the new region on the
x-axis and the other for the y-axis.

So, when you ask for "scroll time", I believe that you mean the
Scroll Idle time, or perhaps you mean Scroll Idle + First Zoom
Region + Second Zoom Region?

I am not sure what you mean by "frames per second" since I believe the
updates are controlled by the -r command-line argument, which is a
constant - unless you just want "1/Scroll Idle" or
"1/(Scroll Idle + First Zoom Region + Second Zoom Region)".

I also am not exactly sure what you want for "pixels-moved-per-second".
zoom_region_scroll is called with dx and dy.  So, is the number of
pixels-moved-per-frame (dx * expose_rect_v) + (dy * expose_rect_h)
and pixels-moved-per-second would just be the average
pixels-moved-per-frame / scroll time

Am I on the right track here, or do you want something else?

> That said, this work is enormously helpful and I can hardly wait to get
> the timing test information into cvs.  I also am very much looking
> forward to benchmarking the individual performance changes; however I
> would prefer that the initial patches do not change the process_updates
> versus gdk_expose processing order.

Okay.  I'll provide the patch once I better understand exactly what
timing statistics you want.

--

Brian