Comment on attachment 242017 [details] [review]
Use GEGL_ABYSS_CLAMP instead of GEGL_ABYSS_NONE

Well, my patch is ok only for the scale op... It breaks all the others...

This was tried already in November 2012 and reverted back because, although it makes "whole image" resizing better, it creates "radiating extensions" of the boundary when used with the transform tools (including resize), which is not the desired behavior according to developer and user consensus:
https://git.gnome.org/browse/gegl/commit/?id=9ed4db80bf7e6e884e5e9e56ca2246f181922db6
What needs to be done instead: Have calls to such operations (from, e.g., GIMP) set the abyss policy as part of the call. Then, Image -> Scale Image can use CLAMP, and Tools -> Transform Tools can use NONE.

Sorry about that, I must be blind...

However, I disagree with the idea of setting the abyss policy as a parameter of the function. I see two reason :

- The user have to guess what policy to use, and it doesn't looks like to be a trivial question.
- Transformations with _NONE will still have artifacts on edges, e.g perspectives.

As long as you need values outside of the input extent where the resulting interpolation is inside the extent, you can't use _NONE.

What I suggest is to use _NONE only where the output of the sampler is outside of the extent. In others cases, use _CLAMP.

I have quickly made a working patch illustrating the idea I suggest. That way we solve the problem in the general case, and without adding arbitrary parameters.

Two comments:

- The two samplers are here to avoid caching problems
- It's probably not the best way to do that (e.g, it adds conditionals expression in the critical part of the loop, ...). It's just a draft.


Opinions?

Created attachment 242071 [details] [review]
The patch above-mentionned

(In reply to comment #3)
Quick opinions RE: your proposal (I may be wrong). (I don't have time to try the patch today.)
...
> However, I disagree with the idea of setting the abyss policy as a parameter of
> the function. I see two reason :
> 
> - The user have to guess what policy to use, and it doesn't looks like to be a
> trivial question.
The user does not have to guess: The parameter that specifies which abyss policy is required can be hardwired inside GIMP, but outside GEGL, in the code that maps GUI buttons to GEGL calls. Even better, the tool could give a choice, with a default appropriately presented: CLAMP for Image -> ..., NONE for Tools -> ...
People will quickly figure which one does what, by pushing buttons. And better names from a UI viewpoint than "CLAMP" and "NONE" (e.g., "Extend" and "Transparent") could be given to the options.
Also:
There are situations in which one would want to use CLAMP with, say, perspective or warping transforms. An example is correcting for hourglass or barrel distortion. If you use NONE, you will have to crop the result inward more than if you use CLAMP. Since digital photographers are annoyed at having to crop the scene more than appears necessary to them, having the option of switching the Tools -> ... to CLAMP would be a score, because it would give them a slightly larger usable image.
Just saying that having abyss policy options with intelligent defaults would be a big plus IMHO. (But I don't have time to touch this.)
> - Transformations with _NONE will still have artifacts on edges, e.g
> perspectives.
Now we are talking!
> 
> As long as you need values outside of the input extent where the resulting
> interpolation is inside the extent, you can't use _NONE.
Not necessarily: The user may not prefer "hard edges" to the region, because they give a transition which is "nearest neighbour"-like.
> What I suggest is to use _NONE only where the output of the sampler is outside
> of the extent. In others cases, use _CLAMP.
I totally agree that what you describe is likely to be preferable in general than what's happening now. When I have a bit of time, I'll try the patch is someone else has not already.
In order to give digital photographers the extra extent with Tools behaving like you specify, all that is needed is something that allows a user to extend the image, pre-transform, with the CLAMP abyss policy (or something smarter, that increasingly blurs as one moves away from the original boundary).

Teo:
I replied before fully understanding.
This:
> What I suggest is to use _NONE only where the output of the sampler is outside
> of the extent. In others cases, use _CLAMP.
is a very good solution.
With that above mentioned, separate, tool that allows one to "extrapolate" an image outward (with CLAMP, say), I think that this solution removes the need from passing abyss arguments around.
Mucho kudos.

... at least, it removes the need for abyss policy setting as far as GIMP is concerned. (Although who am I to know what people will wish to do?)

Thanks Nicolas for your quick answers!


I will think about it more... 

And I figured out that there is another problem with the solution suggested: Antialiasing at the edges of the "output" (This problem seems independent from what we talked above).

Created attachment 242466 [details] [review]
Add abyss policy as a parameter of gegl:transform

As said on IRC, just let the user choose what policy to use.

- Default value : GEGL_ABYSS_NONE (as it was before)
- No plan to implement hard edges and antialiasing yet.

The patch submitted is ready for review and inclusion.

Comment on attachment 242466 [details] [review]
Add abyss policy as a parameter of gegl:transform

Patch obsolete, it should prevent consecutive matrices with different abysses from collapsing. Will provide another patch later

Created attachment 258779 [details] [review]
add an abyss parameter

Here is patch rebased on top of master, handling properly the collapsing of matrices.

I don't know if it's the right approach yet, because it's currently usable only for a scale-like transformation, though all affine (and other) transformations should be able to get hard edges.

Created attachment 258784 [details] [review]
add an abyss-policy parameter

Here is the patch rebased on top of master, handling properly the collapsing of matrices.

I don't know if it's the right approach yet, because it's currently usable only for a scale-like transformation, though all affine (and other) transformations should be able to get hard edges.

Created attachment 260040 [details] [review]
missing test case

While playing with it, I noticed a test case was not properly updated, the new patch fixes that.

Created attachment 321958 [details] [review]
Rebased change on master

Added an updated version of patch, that applies to master.

This still has problems for all other uses of the transform ops than scale; and scale only works in the circumstances where the bounding-box/extent is respected. An antialiasing mask of some kind is still needed to yield for instance sharp rotated+scaled edges.

As a probably silly suggestion, as removing the alpha channel before scaling seems to allow nohalo scaling to work without edge artifacts, would it work if the nohalo scaling code removed the alpha channel, scaled the layer, scaled the alpha channel separately, and then put the alpha channel back on the layer?

*** Bug 696090 has been marked as a duplicate of this bug. ***

For now, let's just add an abyss-policy property to the scale ops only, which defaults to NONE but can be set by GIMP to CLAMP for whole-image/layer scales, and call it fixed.  If there's ever a need for such a property in the rest of the transform ops, we can always move it to the base OpTransform class without breaking anything.

Author: Ell <ell_se@yahoo.com>
Date:   Thu Mar 22 16:03:51 2018 -0400

    transform: add OpTransform::get_abyss_policy() virtual function
    
    Based on a patch by Téo Mazars.
    
    Add a get_abyss_policy() virtual function to OpTransform, which can
    be used by subclasses to control the abyss policy used when
    sampling the input.  When not overridden, ABYSS_NONE is used, as
    before.

 operations/transform/transform-core.c | 41 +++++++++++++++++++++++++++++------------
 operations/transform/transform-core.h |  5 +++--
 2 files changed, 32 insertions(+), 14 deletions(-)

commit bff1364593a3aa9541c49ed76b6a940aef22d34f
Author: Ell <ell_se@yahoo.com>
Date:   Thu Mar 22 16:12:56 2018 -0400

    Bug 698468 - Add abyss-policy property to scale operations
    
    Add an OpScale subclass of OpTransform, and use it as a base class
    for the scale ops: scale-ratio, scale-size, and scale-size-
    keepaspect.
    
    OpScale provides an abyss-policy property, and overrides
    OpTransform::get_abyss_policy() to return its value.  The point is
    that we want to be able to control the abyss policy of scale ops
    (in particular, to avoid "leaking transparency" into the image when
    upscaling the entire image), while not providing this option for
    other transform ops, for which it makes little sense, at least for
    now.

 operations/transform/Makefile.am             |   2 ++
 operations/transform/scale-ratio.c           |   6 ++---
 operations/transform/scale-size-keepaspect.c |   6 ++---
 operations/transform/scale-size.c            |   6 ++---
 operations/transform/scale.c                 | 156 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 operations/transform/scale.h                 |  35 ++++++++++++++++++++++++++
 po/POTFILES.in                               |   1 +
 7 files changed, 203 insertions(+), 9 deletions(-)