GNOME Bugzilla – Bug 760310
Add a saturation filter
Last modified: 2016-01-09 19:04:34 UTC
Image editing applications usually have a filter to tweak the saturation. We have a desaturation filter in the form of gegl:gray which removes all saturation from an image, which is a special case of this. Here is a saturation filter that works in the CIE LCH colour space that is built into gnome-photos at the moment.
Created attachment 318467 [details] [review] Add a saturation filter
Created attachment 318468 [details] [review] perf: Test gegl:saturation performance
(In reply to Debarshi Ray from comment #0) > Image editing applications usually have a filter to tweak the saturation. We > have a desaturation filter in the form of gegl:gray which removes all > saturation from an image, which is a special case of this. > > Here is a saturation filter that works in the CIE LCH colour space that is > built into gnome-photos at the moment. I have the suspect that this kind of operations should operate in Lab color space: scaling the C component of LCH (ab) should be equivalent to scale a and b of Lab by the same amount. if your sources are not already in LCH you double the number of multiplication per pixel, but avoid a sin, a cos, a sqrt and a atan2 call per pixel.
Created attachment 318478 [details] [review] Add a saturation filter If the input buffer is in anything other than CIE LCH(ab), then use CIE Lab, as Massimo suggested. This does improve performance: @ saturation (RGB): 64.74 megabytes/second @ saturation (RGB) (OpenCL): 64.06 megabytes/second @ saturation (RGBA): 168.27 megabytes/second @ saturation (RGBA) (OpenCL): 168.17 megabytes/second @ saturation (CIE LCH(ab)): 1183.32 megabytes/second @ saturation (CIE LCH(ab)) (OpenCL): 1329.39 megabytes/second @ saturation (CIE LCH(ab) alpha): 948.99 megabytes/second @ saturation (CIE LCH(ab) alpha) (OpenCL): 955.59 megabytes/second Earlier they would be between 50 to 55 megabytes/second for RGB and RGBA buffers. The conversion to CIE LCH(ab) was overwhelming the gains from avoiding the alpha conversions. The alpha optimization is now visible for CIE LCH(ab) buffers. We are missing some Babl fast paths for "RGB float" to "CIE Lab float". That is why RGBA buffers are 10 times faster than RGB ones. If anything, RGB ought to be faster as in the CIE LCH(ab) case.
Created attachment 318479 [details] [review] perf: Test gegl:saturation performance
Another comment: I don't think that setting the 'process' member of the point_filter_class object in 'prepare' is a good idea: it would make rendering separate graphs with different in/output concurrently in different threads unstable as one thread prepare could change the process of the other and similarly those operations that during their process render a graph (cartoon, photocopy, soft-glow) could possibly change the output of successive operations during the prepare of the inner graph. Given that these operations are often used as templates I think that if operating in more than one babl_format is deemed worth, it is better to do something like operations/common/opacity.c, but I would always operate in "CIE Lab alpha float".
(In reply to Massimo from comment #6) > Another comment: > > I don't think that setting the 'process' member of the > point_filter_class object in 'prepare' is a good idea: > > it would make rendering separate graphs with different > in/output concurrently in different threads unstable > as one thread prepare could change the process of the other > > and similarly those operations that during their process > render a graph (cartoon, photocopy, soft-glow) could possibly > change the output of successive operations during the prepare > of the inner graph. > > Well, I think it's enough a graph with 2 instances of gegl:saturation with different input (with/without alpha) and one of them would use the wrong process function because IIRC the whole graph is firstly prepared and then processed
(In reply to Massimo from comment #6) > Another comment: > > I don't think that setting the 'process' member of the > point_filter_class object in 'prepare' is a good idea: Of course, you are right. Setting a class-wide variable based on an instance-level parameter is obviously wrong. Absolutely stupid mistake on my part. My objective was to: (i) Avoid needless pixel conversions when possible. (ii) Avoid needless branches in 'process' when possible. This is probably a micro-optimization, but if it is possible, then why not. > Given that these operations are often used as templates > I think that if operating in more than one babl_format > is deemed worth, it is better to do something like > operations/common/opacity.c, but I would always operate > in "CIE Lab alpha float". Yes, o->user_data is what we should be using here, but I think the pattern in operations/common/opacity.c can be marginally improved. :)
Created attachment 318591 [details] [review] Add a saturation filter
(In reply to Massimo from comment #6) > but I would always operate > in "CIE Lab alpha float". If we always operate in "CIE Lab" or "CIE Lab alpha" depending on the presence of alpha, then the performance figures look like this: @ saturation (RGB): 65.70 megabytes/second @ saturation (RGB) (OpenCL): 65.55 megabytes/second @ saturation (RGBA): 169.07 megabytes/second @ saturation (RGBA) (OpenCL): 169.33 megabytes/second @ saturation (CIE LCH(ab)): 50.51 megabytes/second @ saturation (CIE LCH(ab)) (OpenCL): 50.47 megabytes/second @ saturation (CIE LCH(ab) alpha): 49.03 megabytes/second @ saturation (CIE LCH(ab) alpha) (OpenCL): 48.99 megabytes/second The performance of "CIE LCH(ab) alpha" buffers drop almost 20 times, and even more when alpha is absent. If someone ever creates a graph that might have such buffers flowing through it, then it would be nice to avoid such losses, no?
Apart from Lab versus LCH(ab) optimization, I also tried to verify the usefulness of the alpha optimization: @ saturation (RGB): 65.74 megabytes/second @ saturation (RGB) (OpenCL): 65.74 megabytes/second @ saturation (RGBA): 169.04 megabytes/second @ saturation (RGBA) (OpenCL): 169.11 megabytes/second @ saturation (CIE Lab): 1331.86 megabytes/second @ saturation (CIE Lab) (OpenCL): 1301.16 megabytes/second @ saturation (CIE Lab alpha): 945.83 megabytes/second @ saturation (CIE Lab alpha) (OpenCL): 950.44 megabytes/second @ saturation (CIE LCH(ab)): 1367.77 megabytes/second @ saturation (CIE LCH(ab)) (OpenCL): 1359.03 megabytes/second @ saturation (CIE LCH(ab) alpha): 981.46 megabytes/second @ saturation (CIE LCH(ab) alpha) (OpenCL): 973.23 megabytes/second It does get drowned when RGB to CIE conversions are involved, but if you pass a buffer that doesn't need to be converted, then there is some visible difference.
Created attachment 318593 [details] [review] perf: Test gegl:saturation performance
From #gegl on GIMPNet: 18:12 <pippin> rishi: with the race condition issues removed; I'd say feel free to push the saturation op
Created attachment 318596 [details] [review] CIE: Add conversion from "RGB float" to "CIE Lab float" and vice versa This makes "RGB float" buffers as fast as "RGBA float" ones. ie. 10 times faster than they are at the moment.
From #gegl on GIMPNet: 18:55 <rishi> Attached a new babl conversion. 18:56 <pippin> attached - to what, the bug report? 18:56 <rishi> pippin: Yes. 18:58 <pippin> push that as well then :)
commit 80dc657d6c04a8f15c7dd84450a7f87fe108e444 Author: Debarshi Ray <debarshir@gnome.org> Date: Fri Jan 8 10:43:03 2016 +0100 perf: Test gegl:saturation performance https://bugzilla.gnome.org/show_bug.cgi?id=760310 commit f885ca324f1b4c7fa20470d5b68b480953367301 Author: Debarshi Ray <debarshir@gnome.org> Date: Fri Jan 8 10:36:18 2016 +0100 Add a saturation filter https://bugzilla.gnome.org/show_bug.cgi?id=760310
commit b4535d6efc205c790c8263ba3f9568f9b442027d Author: Debarshi Ray <debarshir@gnome.org> Date: Sat Jan 9 19:41:29 2016 +0100 CIE: Add conversion from "RGB float" to "CIE Lab float" and vice versa Conversions from "RGB float" to "CIE Lab float" are 10 times slower without this. One use case is passing a "RGB float" buffer to a saturation operation that works in the "CIE Lab" colour space. https://bugzilla.gnome.org/show_bug.cgi?id=760310