GNOME Bugzilla – Bug 777775
grey: Use memmove, not memcpy
Last modified: 2017-02-02 18:42:26 UTC
The actual gegl:gray logic is encoded in the Babl conversion to "Y". It is guaranteed that the input buffer is already encoded as "Y", so we can just pass it along instead of actually copying the pixels. Even if we really do want to copy, I am not sure that memcpy is the right choice. Being a OperationPointFilter sub-class, gegl:gray has want_in_place=TRUE and dont_cache=TRUE. That means the input and output pointers might be pointing to the same memory location. In practice, though, I haven't been able to reproduce this. Something or the other (a gegl:buffer-source, or gegl_node_get_cache) ends up setting the input buffer as 'has forked', and we end up not processing it in-place.
Created attachment 344290 [details] [review] operations/common/grey: Pass the buffer through, don't copy the pixels
From #gegl on GIMPNet: 18:43 <pippin> rishi: how relevant are your gegl:gray bugs (and their titles) with babl from git master? 10:10 <rishi> pippin: They are still relevant with Babl master. The persistent caching definitely helps with the 'first run' time. 10:11 <rishi> For subsequent runs, without my patches, I saw some speedups with Babl master on my older x220, but not on my X1. 10:12 <rishi> But those patches definitely make things a lot faster regardless of Babl versions. 10:14 <rishi> pippin: But regardless of Babl and speed, this patch still makes sense, doesn't it: https://bugzilla.gnome.org/show_bug.cgi?id=777775 ? 12:41 <pippin> rishi: yep, that one makes sense - not sure if the forced copy in this op is the remnant of an old no longer needed workaround or not 17:12 <rishi> pippin: I see. Do you think that I should commit the patch in bug 777775, or should I wait? 17:30 <pippin> rishi: just push it 17:30 <pippin> probably the best way to figure out if any existing assumptions are broken
Review of attachment 344290 [details] [review]: ::: operations/common/grey.c @@ +54,1 @@ + gegl_operation_context_take_object (context, "output", g_object_ref (input)); This won't work because GeglOperation:process doesn't go through any Babl conversions. Those are only done just before GeglOperationPointFilter:process.
Review of attachment 344290 [details] [review]: Reverted.
Created attachment 344748 [details] [review] operations/common/grey: Use memmove, in case we're processing in-place
Comment on attachment 344748 [details] [review] operations/common/grey: Use memmove, in case we're processing in-place Pushed to master after talking to pippin in person at Wilber Week.