GNOME Bugzilla – Bug 789554
shadows-highlights: Passthrough when properties evaluate to NOP
Last modified: 2017-11-09 17:52:07 UTC
There is no need to compute the gaussian blur, when the properties evaluate to NOP. Short-circuiting the operation in those cases can be nice for non-destructive editors like gnome-photos where the entire stack of operations is re-evaluated multiple times.
Created attachment 362400 [details] [review] shadows-highlights: Passthrough when properties evaluate to NOP This isn't quite ready, yet. It crashes if I wiggle the slider in gnome-photos too quickly:
+ Trace 238121
(In reply to Debarshi Ray from comment #1) > Created attachment 362400 [details] [review] [review] > shadows-highlights: Passthrough when properties evaluate to NOP > > This isn't quite ready, yet. It crashes if I wiggle the slider in > gnome-photos too quickly: That was because the property redirections were not being broken when the child GeglNode was destroyed.
Created attachment 362423 [details] [review] shadows-highlights: Passthrough when properties evaluate to NOP
Created attachment 362424 [details] [review] operation-meta: Break property redirections when the child is gone
I am not sure that we can have a GeglOperation::is_nop virtual method that can cover meta-operations without making the graph processing more complicated. To short-circuit the processing, the base class will have to rewire the sub-graph behind the meta-operation's back. Instead, maybe we can settle for GeglOperationFilter/Composer::is_nop? That's much easier and we have a lot more of those than meta-operations (my count says 13).
The patches are also available in the gegl:wip/rishi/binding-shadhi branch.
I don't know if it is worth it to complicate so much the wrapper meta operation, when the real operation itself is still in workshop and so perhaps in development. I mean I see optimization opportunities in the operation if it is final. For example it requests an aux input as "CIE Lab float", but only uses aux[0] and so it seems to me that it could require only the L channel of the blurred input and the wrapper could input to gaussian-blur the output of a convert format to "Y float", so only one channel would be blurred and and converted to "CIE L float". > https://git.gnome.org/browse/gegl/tree/operations/workshop/shadows-highlights-correction.c#n122 In the two while (shadows/highlightsd2 > 0) it is possible to simplify the computation of ta[1] and ta[2] observing that tb[1] and tb[2] are set to 0 and never modified. > https://git.gnome.org/browse/gegl/tree/operations/workshop/shadows-highlights-correction.c#n123 Supposing this code is as is because something is still missing I wonder if the conditions that today reduce the op to a nop will always be the same
(In reply to Massimo from comment #7) > I don't know if it is worth it to complicate so much > the wrapper meta operation, when the real operation itself > is still in workshop and so perhaps in development. gegl:shadows-highlights is more under QA than development. :) It's a straight port of Darktable's operation, so there isn't that much R&D to do. The blocking bug at the moment is that the OpenCL path produces strange "bands" when adjusting shadows but not highlights (or was it the other way around?). > I mean I see optimization opportunities in the operation > if it is final. For example it requests an aux input as > "CIE Lab float", but only uses aux[0] and so it seems to me > that it could require only the L channel of the blurred input > and the wrapper could input to gaussian-blur the output of > a convert format to "Y float", so only one channel would be > blurred and and converted to "CIE L float". > > > https://git.gnome.org/browse/gegl/tree/operations/workshop/shadows-highlights-correction.c#n122 > > In the two while (shadows/highlightsd2 > 0) it is possible > to simplify the computation of ta[1] and ta[2] observing that > tb[1] and tb[2] are set to 0 and never modified. > > > https://git.gnome.org/browse/gegl/tree/operations/workshop/shadows-highlights-correction.c#n123 Yes, you are right. I left those that way so as not to diverge too much from DT's code until we have nailed down the OpenCL issue. I also saw that the xform calculation in OpenCL has an extra multiplication by a fraction. Ideally, we'd have SIMD optimized CPU paths for all the operations (because OpenCL isn't as useful in practice), but yeah simple optimizations first, I guess. Regardless, since gnome-photos is a non-destructive editor, it is important that the NOP states don't add needless overhead because the entire graph is repeatedly processed while the image is edited. > Supposing this code is as is because something is still missing > I wonder if the conditions that today reduce the op to a nop > will always be the same Yes, they will be the same.
(In reply to Debarshi Ray from comment #8) > (In reply to Massimo from comment #7) > > I don't know if it is worth it to complicate so much > > the wrapper meta operation, when the real operation itself > > is still in workshop and so perhaps in development. > > gegl:shadows-highlights is more under QA than development. :) It's a > straight port of Darktable's operation, so there isn't that much R&D to do. > The blocking bug at the moment is that the OpenCL path produces strange > "bands" when adjusting shadows but not highlights (or was it the other way > around?). > That bug is here: > https://git.gnome.org/browse/gegl/tree/opencl/shadows-highlights-correction.cl#n43 > https://git.gnome.org/browse/gegl/tree/opencl/shadows-highlights-correction.cl#n66 float3 cl types have a different alignment than C types so to unpack them from a packed array of float you need to declare the passed ptr as float * and use vload3 () or [3 * gid], like it is done here for a similar case: > https://git.gnome.org/browse/gegl/tree/opencl/colors.cl#n75
(In reply to Massimo from comment #7) > I mean I see optimization opportunities in the operation > if it is final. For example it requests an aux input as > "CIE Lab float", but only uses aux[0] and so it seems to me > that it could require only the L channel of the blurred input > and the wrapper could input to gaussian-blur the output of > a convert format to "Y float", so only one channel would be > blurred and and converted to "CIE L float". I filed some patches in bug 790111 to make gegl:gblur-1d work natively with CIE Lab and then leverage that in gegl:shadows-highlights.
(In reply to Massimo from comment #8) > Regardless, since gnome-photos is a non-destructive editor, it is important > that the NOP states don't add needless overhead because the entire graph is > repeatedly processed while the image is edited. gcut doesn't have a chain of inactive ops, nor does the mrg ui for GEGL and I do not think GIMP will when it finally becomes non-destructive. This need for nop states and a fixed pipeline is not really an attribute of being non-destructive but a result of gnome-photos architecture choices of not giving op's a priority but hard-coding an ordered list of ops to always be present.
commit abbbd25f13f0f7e21333d78179fa655ec15eaaf4 Author: Øyvind Kolås <pippin@gimp.org> Date: Thu Nov 9 12:28:07 2017 +0100 opencl: fix alignment issue for aux of shadows-highlights Fix following observations by Massimo in bug #789554. commit e82b6019136890fc0a30f35768111c3317b5133b Author: Debarshi Ray <debarshir@gnome.org> Date: Fri Oct 27 12:42:26 2017 +0200 shadows-highlights: Passthrough when properties evaluate to NOP https://bugzilla.gnome.org/show_bug.cgi?id=789554 commit f6d506194c6350a74aec12b0d451c514fe24b8ef Author: Debarshi Ray <debarshir@gnome.org> Date: Fri Oct 27 18:26:59 2017 +0200 operation-meta: Break property redirections when the child is gone ... by porting the code to use GBinding. https://bugzilla.gnome.org/show_bug.cgi?id=789554
(In reply to Massimo from comment #9) > That bug is here: > > > https://git.gnome.org/browse/gegl/tree/opencl/shadows-highlights-correction.cl#n43 > > > https://git.gnome.org/browse/gegl/tree/opencl/shadows-highlights-correction.cl#n66 > > > float3 cl types have a different alignment than C types so to unpack them > from a packed array of float you need to declare the passed ptr as float * > and use vload3 () or [3 * gid], like it is done here for a similar case: > > > https://git.gnome.org/browse/gegl/tree/opencl/colors.cl#n75 With beignet opencl I see no difference before/after this commit; thus I hope for other opencl implementations that do have issues it fixes the glitch.
(In reply to Øyvind Kolås (pippin) from comment #11) > (In reply to Massimo from comment #8) > > Regardless, since gnome-photos is a non-destructive editor, it is important > > that the NOP states don't add needless overhead because the entire graph is > > repeatedly processed while the image is edited. > > gcut doesn't have a chain of inactive ops, nor does the mrg ui for GEGL and > I do not think GIMP will when it finally becomes non-destructive. This need > for nop states and a fixed pipeline is not really an attribute of being > non-destructive but a result of gnome-photos architecture choices of not > giving op's a priority but hard-coding an ordered list of ops to always be > present. Removing an inactive operation is a bit inconvenient because there is no way for the GeglOperation user/app to know if the op is in its NOP state. Not impossible, but a bit inconvenient. Would be nice to have it though. The "removal" itself can be either actually removing the GeglOperation from the graph, or marking the GeglNode as passthrough=TRUE. I just find it more convenient to construct a chain of passthrough GeglNodes and have a GHashTable mapping each operation's name to the object. Actually removing the operation would require iterating over the chain. The O(1) vs O(n) is probably irrelevant, but not having to fiddle with graph's connections seems a tad reassuring to me. But I can imagine changing this in future if it creates problems elsewhere.
(In reply to Øyvind Kolås (pippin) from comment #13) > (In reply to Massimo from comment #9) > > That bug is here: > > > > > https://git.gnome.org/browse/gegl/tree/opencl/shadows-highlights-correction.cl#n43 > > > > > https://git.gnome.org/browse/gegl/tree/opencl/shadows-highlights-correction.cl#n66 > > > > > > float3 cl types have a different alignment than C types so to unpack them > > from a packed array of float you need to declare the passed ptr as float * > > and use vload3 () or [3 * gid], like it is done here for a similar case: > > > > > https://git.gnome.org/browse/gegl/tree/opencl/colors.cl#n75 > > With beignet opencl I see no difference before/after this commit; thus I > hope for other opencl implementations that do have issues it fixes the > glitch. https://git.gnome.org/browse/gegl/tree/opencl/shadows-highlights-correction.cl#n66 if you use 'aux[3 * gid]' you have to remove the '.x', at this point it is a float array
commit 9df4d1d580a991ae813180e98308589565df668d Author: Massimo <sixtysix@inwind.it> Date: Thu Nov 9 18:37:15 2017 +0100 opencl: fix previous backseat driving change "when using 'aux[3 * gid]' you have to remove the '.x', at this point it is a float array"
(In reply to Øyvind Kolås (pippin) from comment #16) > commit 9df4d1d580a991ae813180e98308589565df668d > Author: Massimo <sixtysix@inwind.it> > Date: Thu Nov 9 18:37:15 2017 +0100 > > opencl: fix previous backseat driving change > > "when using 'aux[3 * gid]' you have to remove the '.x', at this point > it is a float array" Tested in GIMP (Tools->Gegl Operation...) with pocl and NVidia OpenCL and I don't see the stripes I used to see before.