GNOME Bugzilla – Bug 331470
abuse of progress update in plugins
Last modified: 2006-02-20 07:36:52 UTC
I modified a plugin to mesure that time required to update the progress bar. Here are the results normalized for 1000 updates with different GTK themes. My system is an Athlon 64 3200+ (in 32bit mode) with a Radeon 9200. The window was maximized (1200x1000). TIME FOR 1000 PROGRESS UPDATES: ----------------------- LighthouseBlue 0.6s Clearlooks 0.7s Wasp 1.3s Geramik 3.2s SphereCrystal 4.5s ----------------------- So the question is how many progress updates are performed. I took the case of someone processing a typical digital camera image of size 3000x2000. Here are the numbers for most plugins. The method I used was only reporting each sequence of updates. color enhance 400 normalize 400 strech contrast 400 strech hsv 400 value invert 200 retinex 0 max rgb 200 blur 300 gaussian blur 400 motion blur 1700 pixelize 1700 sel gaussian blur 300 deinterlace 100 unsharp mask 900 curve bend 1600 iwarp 94200 we have a winner mosaic 16200 newsprint 1600 page curl 1600 ripple 1600 value propagate 1600 video 1600 whirl and pinch 1100 wind 2000 gradient flare 1600 lens flare 400 ligthing effect 2000 sparkle 1200 / 6600 / 50000+ (reapplied on its own result) supernova 1600 apply lens 700 glass tile 400 HSV noise 200 HUrl 300 Pick 200 RBG Noise 200 Slur 200 Spread 1600 Diff of Gaussian 1600 Edge 1600 Laplace 200 Neon 300 Sobel 100 Convolution Matrix 200 Dilate 500 Erode 500 Depth Merge 2000 Apply Canvas 500 Cartoon 2600 Clothify 3500 Cubism 12200 Oilify 400 Photocopy 4100 Predator 3400 Soft Glow 3900 Van Gogh (LIC) 2000 (very slow) Weave 23400 bump map 2000 displace 1600 fractal trace 2000 illusion 1600 make seamless 3100 paper tile 200 (default settings. small number of large tiles ) paper tile 18600 (changed settings. large number of small tiles ) small tiles 1600 warp 8800 (5 iterations) plasma 2800 solid noise 1600 Flame ???? (not done! eat all memory) IFS Fractal 82800 (65 iterations) checkboarder 1600 diffract patterns 100 divisions 100 grid 200 jigsaw 100 maze 1600 qbist 100 sinus 1600 circuit 3500 lava 16400 line nova 0 - starts fast and slows down ... marching ant problem? Just to give you an idea of what that mean, consider the plugin 'Solid Noise' which is typically used to create fog effects. With the theme Clearlooks, it take about 5s at 3000x2000 which means that the time spent in the progress updates is approximatively (1600/1000)*0.7 = 1.12s. So that is 4s for the plugin itself and 1s for the update (20%). Now, since SphereCrystal is 7.5 times slower, I can predict that the plugin should take approximatively: 4+1*.1*7.5 = 12.2s I just tried and the real time was 14s. 70% of the time was lost to update the progress bar. In practice, There are no reasons to do the update more that 100 or 200 times. All plugins could be modified but a more efficient approach is to limit the number of updates inside gimp_progress_update().
Created attachment 59538 [details] [review] round the progress values to avoid redraw A very simple patch to round the progress value. the progress bar is not refreshed when the value is identical. A few performance numbers on a 3000x2000 image using SphereCrystal theme BEFORE AFTER SPEEDUP Unsharp Mask 11s 6s 1.8 Motion Blur 15s 6s 2.5 Clouds 14s 4s 3.5 iWarp 630s 7s 90.0 :-)
The patch above is very efficient but it would be even better not to call gimp at all when the rounded value did not change.
Created attachment 59539 [details] [review] fix the huge number of progress updates in iwarp The patch moves 2 gimp_progress_updates out of some critical loops in iwarp. Compared to the previous plugin, the execution times drops to 3s (was 630s to 7s after previous patch).
Created attachment 59543 [details] [review] patch for ifscompose That patch removes one of the 2 gimp_progress_update used in the IFS Fractal plugin. That update was in a fast loop and was not really usefull. The patch also reduces the number of times the second gimp_progress_update. The initial method using modulo 5000 was not enough for large image. The new method should reduce the number of update to less than 32 in each of the 65 iterations. The execution time for a 3200x2000 image using the SphereCrystal theme: 209s initial time 50s with patch #59538 31s with patch #59538 + this one
Comment on attachment 59538 [details] [review] round the progress values to avoid redraw This is generated code. While your change is indeed reasonable, you cannot do the rounding here. Also, next time, please attach patches in unified diff format.
2006-02-17 Sven Neumann <sven@gimp.org> * plug-ins/common/iwarp.c * plug-ins/ifscompose/ifscompose.c * plug-ins/ifscompose/ifscompose_utils.c: applied patches from Stephane Chauveau. Reduces number of progress updates (bug #331470).
Since it isn't trivial to figure out how to properly implement the change suggested in comment #1, I am attaching a patch that does it. I am however not certain if this is the right approach. IMO we should either fix this in the progressbar widgets or try to completely avoid calling into the core. The suggested approach of rounding the value to percentages is somewhat arbitrary. For a wide statusbar this is going to give a visually unpleasant progressbar update. We should perhaps take the size of the progressbar and the time since the last update into account when deciding whether to suppress an update or not.
Created attachment 59560 [details] [review] reimplementation of proposed changes to libgimp
Why not just give a fix size to the progress bar? I do not see why it has to fill the complete width of the screen. We already have a nice Cancel button that could be transformed into a progress bar.
Created attachment 59591 [details] mockup with the cancel button transformed into a small progress bar
The image window isn't the only place we are displaying a progress bar. I also think that your proposed setup of using the Cancel button as progressbar introduces a possible misunderstanding. It isn't obvious any longer how to cancel the operation and it is also not obvious that the progressbar belongs to the text displayed in the statusbar while itself is labelled "Cancel". Perhaps we should focus on the implementation here and leave a possible redesign of the progressbar in the image window as a separate problem that deserves a separate bug-report if you feel it should be discussed further.
Ok! The unpleasant progressbar update is indeed a problem. The current limit of 100 is probably a bit too low. Allowing 200, 300 or even 500 is still acceptable for most themes (peoples should not use a slow theme like SphereCrystal if their PC cannot handle it). Another approach would be to have a 2 step filtering of the number of calls. First in the plugin itself (up to 1000 should be acceptable) and then in Gimp according to the width of the progress bar. It could even make sense to add a user preference for controling the 'smoothness versus speed' of the progress bar and other animations. I also realized that the current rounding method transforms a regular progress sequence into an irregular one. For example, consider a plug-in that would call the progress bar with a step of 8/1000 : 8 16 24 32 40 48 56 64 72 80 88 96 104 ... The rounding every to multiple of 10/1000 would produces the following sequence: 0 1 2 3 4 4 5 6 7 8 8 9 10 ... This irregularity is increase the 'un-smoothness' of the progress bar. What do So I came with that method instead: static gdouble progress_current = 0.0 ; static gdouble progress_step = 1.0 / 300.0 ; gboolean gimp_progress_update (gdouble percentage) { if ( fabs(progress_current-percentage) > step ) { progress_current = percentage ; return _gimp_progress_update (progress_current) ; } return TRUE ; } That implementation always produces a regular sequence of updates if the input sequence is regular . Also, unnecessary calls to Gimp are avoided. Is that ok to use global variables? I was also wondering if the cases 0.0 and 1.0 should be handled seperately to insure that the progress bar is always totally cleared and filled. Something like that maybe: gbool doit; if ( percentage <= 0.0 ) { doit = (progress_current != 0.0) ; percentage = 0.0 ; } else if ( percentage >= 1.0 ) { doit = (progress_current != 1.0) ; percentage = 1.0 ; } else { doit = ( fabs(progress_current-percentage) > step ) ; } if ( doit ) { progress_current = percentage ; return _gimp_progress_update (progress_current) ; }
That is basically the approach that I also came up with after thinking about your patch for a while. I have a few comments about your changes. It would be nice if you could incorporate them and attach a unified diff to this bug report. - Can you please try to adhere to the GIMP coding style as described in the file HACKING. Otherwise I will have to edit all your changes and we can't allow you to commit your changes to CVS yourself. - Using global variables should be OK because a plug-in always only has a single progress. However we probably need to make sure that gimp_progress_init() resets the global progress value.
Created attachment 59726 [details] [review] implementation of clever progress update The patch replaces patch 59560. The call to gimp is only executed when the update changes. The method used to limit the number of calls is more clever and should produce more regular updates.
I have committed a slightly modified version of your patch. Closing this report as FIXED. 2006-02-20 Sven Neumann <sven@gimp.org> * tools/pdbgen/pdb/progress.pdb * libgimp/gimpprogress.[ch]: applied slightly modified patch from Stephane Chauveau. Wraps the gimp_progress_update() PDB call so that redundant progress updates are suppressed in libgimp. This gives a noticeable speedup for all plug-ins that update the progress too often (bug #331470). * libgimp/gimpprogress_pdb.[ch]: regenerated.