After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 789554 - shadows-highlights: Passthrough when properties evaluate to NOP
shadows-highlights: Passthrough when properties evaluate to NOP
Status: RESOLVED FIXED
Product: GEGL
Classification: Other
Component: operations
git master
Other All
: Normal normal
: ---
Assigned To: Default Gegl Component Owner
Default Gegl Component Owner
Depends on:
Blocks: 788201
 
 
Reported: 2017-10-27 11:16 UTC by Debarshi Ray
Modified: 2017-11-09 17:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
shadows-highlights: Passthrough when properties evaluate to NOP (6.35 KB, patch)
2017-10-27 11:18 UTC, Debarshi Ray
none Details | Review
shadows-highlights: Passthrough when properties evaluate to NOP (6.28 KB, patch)
2017-10-27 17:01 UTC, Debarshi Ray
committed Details | Review
operation-meta: Break property redirections when the child is gone (6.36 KB, patch)
2017-10-27 17:02 UTC, Debarshi Ray
committed Details | Review

Description Debarshi Ray 2017-10-27 11:16:39 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.
Comment 1 Debarshi Ray 2017-10-27 11:18:42 UTC
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:

  • #0 gegl_node_copy_property_property
    at gegl-operation-meta.c line 142
  • #1 gegl_operation_meta_property_changed
    at gegl-operation-meta.c line 231
  • #2 g_closure_invoke
  • #3 signal_emit_unlocked_R
  • #4 g_signal_emit_valist
  • #5 g_signal_emit
  • #6 g_object_dispatch_properties_changed
  • #7 g_object_notify_queue_thaw
  • #8 g_object_thaw_notify
  • #9 gegl_node_set_valist
    at gegl-node.c line 1416

Comment 2 Debarshi Ray 2017-10-27 17:01:25 UTC
(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.
Comment 3 Debarshi Ray 2017-10-27 17:01:58 UTC
Created attachment 362423 [details] [review]
shadows-highlights: Passthrough when properties evaluate to NOP
Comment 4 Debarshi Ray 2017-10-27 17:02:28 UTC
Created attachment 362424 [details] [review]
operation-meta: Break property redirections when the child is gone
Comment 5 Debarshi Ray 2017-11-01 08:12:44 UTC
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).
Comment 6 Debarshi Ray 2017-11-06 13:56:31 UTC
The patches are also available in the gegl:wip/rishi/binding-shadhi branch.
Comment 7 Massimo 2017-11-06 15:59:46 UTC
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
Comment 8 Debarshi Ray 2017-11-06 16:29:23 UTC
(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.
Comment 9 Massimo 2017-11-07 08:46:10 UTC
(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
Comment 10 Debarshi Ray 2017-11-09 10:27:02 UTC
(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.
Comment 11 Øyvind Kolås (pippin) 2017-11-09 11:20:23 UTC
(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.
Comment 12 Øyvind Kolås (pippin) 2017-11-09 11:29:42 UTC
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
Comment 13 Øyvind Kolås (pippin) 2017-11-09 11:31:32 UTC
(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.
Comment 14 Debarshi Ray 2017-11-09 11:41:36 UTC
(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.
Comment 15 Massimo 2017-11-09 16:23:31 UTC
(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
Comment 16 Øyvind Kolås (pippin) 2017-11-09 17:41:47 UTC
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"
Comment 17 Massimo 2017-11-09 17:52:07 UTC
(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.