This bug report specifically deals with the transformation tools and the code in gimpdrawable-transform.c. There's another bug report (bug #433241) about scaling an image.

According to gg, the problem can be reproduced with all interpolation types. At the centre of a blank canvas, create a 10x10 "circle" - actually a stepped octogon - and rotate 6 x 15 degrees. The result is that it drops precisely one pixel.

More importantly the interpolation type None is behaving badly. It girates about some slightly off-centre point and ends up +4px in X after the 6 rotations. There is heavy and asymetrical distortion of the basic shape. Clearly any distortion should be symetrical.

It would be nice to have an example image that can be used to reproduce the problem attached to this bug report.

Created attachment 87041 [details]
four , 15 deg rotations with NONE interpolation

ugh

Created attachment 87042 [details]
six , 15 deg rotations with NONE interpolation

This image started as a 10 x 10 spot centred in the canvas.

It "centre" now as far as that can still have any meaning seems to be at 15,14 ie. a drop of 4px in y.

At this point in the repetition (180 degrees) the x offset has wheeled back on itself and is currently zero. See previous image to see that it is generally non-zero.

Created attachment 87043 [details]
screenshot of previous in gimp at 800%

Could you please attach the test image. The results can be easily reproduced, the test image is more important.

See also bug #317853 about artefacts introduced when scaling a drawable. This might be a similar or even the same problem. Note that bug #317853 talks about scaling, but the code path ends up using gimp_drawable_transform_affine(), not gimp_drawable_scale().

Created attachment 87836 [details]
test image - 10x10 black square on a 20x20 white canvas

I don't know if gg intends to attach his test image, but any image would do so here is mine.  It is a 10x10 black square in the middle of a 20x20 white canvas.  I also added very light green and red diagonals to make it obvious that the original square is well centered.

Applying several rotations (optionally holding Ctrl for multiples of 15 degrees) shows an offset in the center of rotation.

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

Created attachment 141571 [details] [review]
Quick hack

It seems that the code for affine tranformations
makes use of a coordinate system with origin at 
the top left corner of the top left pixel, whereas
the code performing pixels interpolation has the
origin at the center of that same pixel.

patch against file 'app/core/gimp-transform-region.c'
of release 2.6.7

Review of attachment 141571 [details] [review]:

We could keep doing quick hacks that seems to work, or we can built up a thorough set of regression tests so things stays fixed. Voting for the latter, setting to needs-work until the patch comes with a regression test. Thanks for the patch though, Massimo, you rock.

Created attachment 164756 [details]
Regression test

Here is my take for a regression test related to this bug.

The starting point is gimp has two ways to downscale a layer: 
gimp-layer-scale-full and gimp-drawable-transform-scale.

That is: one based on the affine transformations framework (scale tool)
and the other scaling the dimensions integer to integer
(menus "<Image>/Scale image" or "<Layer>/Scale layer").

Today the two methods produce different results even when asked 
to do the same work and with interpolation mode 'None'.

The objective is to have similar results and to choose which
one is correct.

So starting from a flat white image I scale it up of the factor I will
downscale it later. On the big image I draw the borders of the pixels
of the small version and then downscale it.

Today gimp-layer-scale-full samples from the white 
(the center of the area covered by the destination pixels) as expected,
whereas gimp-drawable-transform-scale samples from somewhere on 
the borders.

After applying the patch above, both behave similarly.



Put the attached script in its directory and launch gimp passing
on the command line

app/gimp-2.7 -b "(regression-test-000)"

Opening the undo history window you can rewind the steps executed.

<Ctrl>Q, apply the patch, make, and repeat.

HTH

Would it be possible to get this regression test in the form of a patch that will also run when doing make check?

Created attachment 164988 [details] [review]
Patch with a regression test

I added a new file test1.c and updated Makefile.am accordingly.

It is derived from test-ui.c, because it seems that to 
instantiate a GimpContext the initialization performed
in test-core.c is not enough.

Comments are appreciated.

Review of attachment 164988 [details] [review]:

A few initial comments:
* 'test1' is not a good name for a test program
* You should make the necessary changes to test-core to be able to add the test there
* You should use the helper macros to add tests
* Maybe there is no way, but isn't there a way to make a simpler test? Seems like a lot of code to test this, but maybe it's necessary

Created attachment 173016 [details] [review]
More regression tests

In the attached tar.gz there are a few new fixes with their 
regression tests added to test-core.c. 

The idea is to recycle the great amount of code among few tests 
verifying the basics of the code for affine transformations.

Please dup: Bug #636853 - Gimp: rotate 90 using tool off-by-one boundary error
and ideally tweak the subject line of this bug (I didn't find it during filing the other one).

Note easier replication:

Forwarded: https://launchpad.net/bugs/687737

  1. Open an image, select a 48x48 region
  2. Use the rotate tool and input "90.0 degrees"
  3. Note that the _preview_ is correct
  4. Click [Rotate]

Now the region actually rotated is a 47x47 selection, aligned to the
bottom-right of the original region. A pair of thin 1-pixel strips are left
empty.

https://bugs.launchpad.net/ubuntu/+source/gimp/+bug/687737/+attachment/1760582/+files/gimp-rotate90-off-by-one.png

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

Massimo, what about the patches here?

Massimo, ping, will you please update about patches ?

Tested today: the bug's still there, but, to avoid introduce regressions that
could delay the release, I'm waiting after 2.8 release before pushing  
these patches.

I lost track here. What about these patches? Does this also happen
with GEGL in master?

The fixes should still apply to the branch gimp-2-8, and while fixing
several minor issues, they do not touch the majors, the regression tests 
are useless (smoke in the eyes of naif contributors), and GEGL is affected
by the problem described here, comparing the output of:

<gegl>
  <gegl:crop width='20' height='20'/>
  <gegl:vector-stroke color='#2dd' width='1'
    d='M 8.5,5.5 L 11.5,5.5 L 14.5,8.5 L 14.5,11.5 L 11.5,14.5 L 8.5,14.5 L 5.5,11.5 L 5.5,8.5 Z'/>
  <gegl:color value='#d22'/>
</gegl>

with that of:

<gegl>
  <gegl:crop width='20' height='20'/>

  <gegl:rotate filter='linear' origin-x='10' origin-y='10' degrees='15'/><gegl:nop/>
  <gegl:rotate filter='linear' origin-x='10' origin-y='10' degrees='15'/><gegl:nop/>
  <gegl:rotate filter='linear' origin-x='10' origin-y='10' degrees='15'/><gegl:nop/>
  <gegl:rotate filter='linear' origin-x='10' origin-y='10' degrees='15'/><gegl:nop/>
  <gegl:rotate filter='linear' origin-x='10' origin-y='10' degrees='15'/><gegl:nop/>
  <gegl:rotate filter='linear' origin-x='10' origin-y='10' degrees='15'/>

  <gegl:crop width='20' height='20'/>
  <gegl:vector-stroke color='#2dd' width='1'
    d='M 8.5,5.5 L 11.5,5.5 L 14.5,8.5 L 14.5,11.5 L 11.5,14.5 L 8.5,14.5 L 5.5,11.5 L 5.5,8.5 Z'/>
  <gegl:color value='#d22'/>
</gegl>

it is possible to notice the one pixel drop described in comment 1

The scale tool and <Tools>/Gegl operation.../{scaleratio,scalesize} in                   
GIMP from master are probably still WIP: after halving the size of an opaque layer,
the projection is not completely cleared and remnants of the original size layer
are left on screen and anyway if you look at the first/last row/column of the
alpha channel you see that the layer is not extended/resampled symmetrically,
GEGL transforms pixel top-left corners, not pixel centers.

You seem to know what to do here, can you take care of fixing 2.8 please?
Master is obviously different.

The projection artifacts are unrelated.

Comment on attachment 173016 [details] [review]
More regression tests

Pushed something similar to these patches, except for the regression tests

Probably stating something everybody knows, but here is, just in case. (I've written this many times and in many places.)

The way to fix these issues to make a choice of image geometry convention, an apply consistently with everything and everywhere.

The old GIMP, if I remember correctly, used the "align corners" convention. There are many ways of explaining the convention, but when resizing, what this means is that the four outer corners of the four corner pixels of the image before and after resizing are mapped to each other by the geometrical transformation. Another way to explain the convention is that an MxN pixels is understood to have an area of MxN. If it is decided that pixel locations are defined by their indices, so that the top left corner has index and location (0,0) and the bottom right one is at index (M-1,N-1), this means that the image extends from (-1/2,-1/2) to (M-1/2,N-1/2), with two additional outer corners at (-1/2,N-1/2) and (M-1/2,-1/2). (It is also possible to implement align corners so that the top left corner is at (0,0), but then pixel indices do not match their location, which IMHO is more of a hassle than having to start at (-.5,-.5) and stopping at (M-.5,N-.5).)

This convention is better when reducing the size of an image because it reduces the contribution of the abyss, and it is also better if mipmap levels are used to zoom in and zoom out, because integer box filtering implicitly uses this convention.

What happens with affine and perspective transformations (including rotations) must then be set to that when these transformations can be understood as resizes, they are in agreement the above prescriptions.

Looking quickly, massimo's patch seems to be a step in the right direction.
This being said, the only way to really do things is to make sure that every single piece follows one single convention, which I am pretty sure should be "align corners" for GIMP (and GEGL).

Exchange on #gimp:

<massimo> I once suggested a one-liner fix for gegl:transform op
...
<massimo> I was told they had to ask nicolas, if you want to test it it is just a matter of add
<massimo> gegl_matrix3_originate (&inverse, -.5, -.5); 
<massimo> at line 688 after gegl_matrix3_invert
<massimo> in file operations/transform/transform-core.c

Now, I have not had time to look into how this links to the GIMP GUIs and other operations, but having looked at the code for gegl_matrix_originate this exactly sets things up so that the align corners convention with pixel locations at their indices holds.

The only thing that may be better is actually applying this to the original matrix (not the inverse) and letting things be propagated this way. The "magick" of using matrices to implement affine transforms together with the facts that (-.5,-.5) is the top left corner in both the input and output images allows that. But to make sure this does not break anything I'd have to follow the toolchain all the way back to the GUI and I don't have time for that now.

You also may have to change the GUI a bit so that it explains what's happening.

But in any case, the Grand and Highly Soapboxed Image Geometry Wizard and Poobah blesses this one line change.

If we do agree that the convention adopted by 'linear', 'cubic' and 
'lohalo' gegl samplers is valid (and the one to follow). 
The next step is to agree that the 'nearest' gegl-sampler is not 
following it:

when sampling at x = j + .75, and y = i (i and j integers)

with 'linear' should result

in a quarter of pixel[i][j] and 3 quarters of pixel[i][j+1]

whereas at x = j + .25, and y = i should result

in 3 quarters of pixel[i][j] and a quarter of pixel[i][j+1]


with 'nearest' at x = j + .75, and y = i (i and j integers)

it should be pixel[i][j+1]

whereas at x = j + .25, and y = i

it should be pixel[i][j]

that is (pseudocode)

1)   i = lrint (y); and j = lrint (x); 
  or i = floor (y + 0.5); and j = floor (x + 0.5);

2) *sampler->context_rect = { 0, 0, 2, 2 };



http://git.gnome.org/browse/gegl/tree/gegl/buffer/gegl-sampler-nearest.c#n72

The assumption that GIMP makes globally is that pixels are small rectangles,
so the top-left pixel would cover the area from (0, 0) to (1, 1),
and the bottom-right pixels covers the area from
(width-1, height-1) to (width, height).

GIMP passes the coordinates to GEGL like that.

(writing this is probably redundant, I just wanted to be sure).

Michael: I wrote the samplers I wrote based on the assumption that pixels are small squares of unit area, just like GIMP assumes. (Actually, this is not quite the issue: the issue is how geometrical transformations are adjusted to account for implicit image geometry.)

However, my understanding was that the GEGL convention was that the location of a pixel's center matches its C-style index. So, the center of the top left pixel is located at (0,0), so that the top left corner of the top left pixel is located at (-.5,-.5).

In the GIMP convention as you state it (and it matches my experience in terms of what I see in menus), the top left corner of the top left pixel is located at (0,0), so that the center of the top left pixel is located at (+.5,+.5). In other words, pixels are not centered at their index within the GIMP convention.

Flaming wars have been fought over which one's better.

The only difference between the two conventions is a shift. No more an no less. It's not that GIMP uses the "align corner pixels' corner" (image resizing) convention and GEGL uses the "align corner pixels' centers" (image resizing) convention. They use the same convention. It's just that the correspondence between indexes and position is different.

Now, this mismatch is not the end of the world, provided developers are aware of this difference: As long as GEGL is internally consistent, things will work as desired, because although I forgot to mention it earlier, when properly set up, the goal truly is to put the centers of the images in correct correspondence, and this just happens to be achieved, in the case of affine transformations, by Massimo's one line fix.

But of course GEGL could be rewritten to match the GIMP convention. So, what do you guys want: GEGL rewritten so that it uses the same convention w.r.t. pixel location and index as GIMP, or GEGL slightly fixed so it's consistent w.r.t. its own convention (position of pixel center = index)?

Now: It is possible that I misunderstood things when I started programming things into GEGL (and guiding my students doing the same), and that actually GEGL follows the GIMP convention everywhere EXCEPT in Lohalo and Linear, which I based on the existing Linear and Cubic. This is simple to fix, and I certainly can do that quickly.

... EXCEPT in Lohalo and Linear and Cubic ...

Massimo:

(In reply to comment #28)
> 1)   i = lrint (y); and j = lrint (x); 
>   or i = floor (y + 0.5); and j = floor (x + 0.5);
> 
> 2) *sampler->context_rect = { 0, 0, 2, 2 };

Indeed, nearest does not follow the same convention as lohalo, linear and cubic. The correct indices are indeed obtained by rounding, not by flooring.

If I wanted the rounding to be cheap, I'd use FAST_PSEUDO_FLOOR(x+0.5) instead of the above lrint and floor based calls. It was faster last I checked, and I assume it's still the case. (If not please let me know.) Exactly how midpoint values are rounded is not that important: There are pros and cons to pretty much every choice.

However, there is no need enlarge the number of pixels within the context rect to 2x2 unless there is something broken somewhere: Nearest neighbour definitely only needs one input pixel value per output pixel value.

Massimo:

It sounds like the program is actually to have every sampler follow the same convention as nearest. So, the nearest bug was a feature. And the other samplers' features were bugs.

I think that now all the work is done in gegl:transform,
GIMP creates a graph and then passes around GeglBuffers.

Both conventions are equally good (it is a matter of moving 
the second + (-.5, -.5) implied in gegl_matrix3_originate from 
the caller to the callee or v.v.).

I started assuming that I did not want to touch 'lohalo', and so
I assumed it was better to adjust 'nearest' to the others. The point
is that the meaning of context_rect and the coordinate system origin 
is not consistent for all samplers and it is therefore impossible to 
correctly fix 'get_bounding_box', 'get_required_for_output' and
'invalidated_by_change' gegl:transform members. 

It is important to know which convention is right, because the work
is split in many places and they must agree.

Created attachment 229271 [details] [review]
Fixing samplers so that the relationship between pixel indices and physical location matches GIMP's

RE: Fixing samplers so that the relationship between pixel indices and physical location matches GIMP's (28.54 KB, patch)

I have set things up in every single sampler (nearest, linear, cubic, lohalo) so that they all use the "pixel index is physical location of its top left corner" convention used, TTBOMK, by GIMP. Although there are still abyss and probably transformations issues, I am passing the torch to Massimo on those.
I moved FAST_PSEUDO_FLOOR to gegl-buffer-private.h so that it not be copied all over the place.
I did a partial clean up of the cubic sampler without changing it's behavior. I don't foresee doing a full clean up any time soon.
Cosmetic stuff: I also cleaned up the formatting and edited comments.
-----
Massimo: Note that the nearest and lohalo samplers anchor the context_rect at the input pixel closest to the sampling point, while the linear and cubic samplers anchor the context_rect at the nearest input pixel TO THE TOP LEFT. This does not mean that the samplers use different conventions. Just that their computation are normalized using context_rects that are set up with different relationships to the sampling points.
Trust that the context_rects I put there are correct. Feel free to contact me by email if things are not clear. (Use nicolas.robidoux@gmail.com)
It is not a coincidence that nearest and lohalo have widths and heights are are odd (1 and 5) and linear and cubic have widths and heights that are even: Because of this, the choices of anchor point stated above are expedient.

Created attachment 229296 [details] [review]
proposed patch

Following GIMP convention, the fix necessary to transform pixel
centres and not corners is in the attached patch.

Re: context_rect.
All samplers have to agree on where it is anchored, because
it is also used in gegl:transform in a generic way, that is
without knowing which sampler belongs to which group. The
alternative is to enlarge always the required source for
a given output rectangle.

Example: the alpha channel produced by the following gegl
composition. To compare with that obtained after setting
linear sampler context_rect to  { -1, -1, 3, 3 }

<gegl>
  <gegl:crop x='-512' y='-512' width='1024' height='1024'/>

  <gegl:add value='0.5' />
  <gegl:subtract>
    <gegl:checkerboard x='128' y='128' />
  </gegl:subtract>

  <gegl:scale filter='linear' x='2' y='-2'/>
  <gegl:nop/>
  <gegl:scale filter='linear' x='-2' y='2'/>
  <gegl:crop x='-260' y='-260' width='520' height='520'/>
  <gegl:checkerboard x='32' y='32'/>
</gegl>

Massimo:

If it is necessary to use { -1, -1, 3, 3 } for bilinear something is broken somehwere.

My guess would be that the "rectangle of needed pixels" is not aligned correctly or fattened enough. There is absolutely no need to use a 3x3 data square to compute bilinear.

It's going to take me a while sort through what you added.

(In reply to comment #38)
> Massimo:
> 
> If it is necessary to use { -1, -1, 3, 3 } for bilinear something is broken
> somehwere.
> 

It is simply the number of pixels whose tee-pee is non zero over
the range [j,j+1) and it is 3, when tee-pees are centred on the 
half integer and it is two when tee when tee-pees are centred
on the integers.

Massimo:

I do not understand your comment:

The 1D bilinear teepee is nonzero for at most two input pixels, never more (and sometimes less: only one right at an input pixel).

The 2D bilinear "teepee" is nonzero for at most four input pixels, never more (and sometimes less).

The only somewhat reasonable reason I can think of why using { -1, -1, 3, 3 } may give better results for bilinear assuming there is no bug or poor implementation would be that this ensures that the added abyss is almost symmetrical left to right and top to bottom. But this is a dirty-hacky way of making this happen, assuming it is even desirable.

If you'd rather not discuss this in public, my email is easy to find.

This looks like a miscommunication to me, because your comments RE: context_rect choices make little sense, and it's clear you know what you're doing.

However, an implementation of transform that requires { -1, -1, 3, 3 } for bilinear is something that, IMHO, should be written differently.

Review of attachment 229271 [details] [review]:

I redid the patch and pushed to master.

Review of attachment 229296 [details] [review]:

TTBOMK, this patch is correct.
I imagine that roi's are rectangles of pixel indices. The inverse transform has to use the right locations, however, and converting an index of a location is done by adding .5.
Thumbs up.

Review of attachment 229296 [details] [review]:

If Massimo's patch breaks something, it's something that needs fixing.

Review of attachment 141571 [details] [review]:

Obsolete.

Ok, now in masters gegl:transform transforms pixel centres
and samplers 'get' member functions work consistently.

(In reply to comment #40)
> Massimo:
> 
> I do not understand your comment:
> 
> The 1D bilinear teepee is nonzero for at most two input pixels, never more (and
> sometimes less: only one right at an input pixel).
> 

When you start from a function sampled on the integers and a half
   F[k] == f (k +.5)    it is:

double Teepee (double x, int k)
{
  return MAX (0., 1. - fabs (k + 0.5 - x));
}

and the piecewise linear reconstructed function is:
  
f(x) = sum/k  F[k] * Teepee (x, k)
  
  
in [j, j+1)

Teepee (j + 0.25, j-1) = MAX (0.0, 1 - |j - 1 + 0.5 - (j + .25)|) = 0.25
Teepee (j + 0.50, j  ) = MAX (0.0, 1 - |j     + 0.5 - (j + 0.5)|) = 1.0
Teepee (j + 0.75, j+1) = MAX (0.0, 1 - |j + 1 + 0.5 - (j + .75)|) = 0.25

whereas Teepee (x, j +/- n) == 0    for x in [j, j+1] and n > 1

so if I'm asked how many samples I need in order to resample somewhere
in [j, j+1), I answer at least 3: those indexed by j-1, j, j+1.


This is what 'get_required_for_output' is using 'context_rect' for, and
yes it is possible to always fatten there the required rectangle, or to
add there the knowledge of which anchoring convention each sampler is
using, but I think it is easier if each sampler rounds its context to
the nearest (integer sized and positioned) bounding rectangle, following
the same convention used in 'get'.
 
> This looks like a miscommunication to me, because your comments RE:
> context_rect choices make little sense, and it's clear you know what you're
> doing.
> 


Anyway, it is what the program outputs that should make sense.

<gegl> 
  <gegl:crop x='-512' y='-512' width='1024' height='1024'/>
  <gegl:scale filter='linear' x='-4' y='-4'/> 
  <gegl:crop x='-260' y='-260' width='520' height='520'/>
  <gegl:color value='#777' />
</gegl>

Massimo:
Could you please read http://en.wikipedia.org/wiki/Bilinear_interpolation and let me know if I need to explain more?

If this helps: The (cardinal) basis function of bilinear interpolation, namely the "Mexican hat function", has a support which is an OPEN interval of length 2. You cannot fit three integers in an open interval of length 2. The interval is open because the support is the set of points where the basis function is not zero. We do not need point values for locations with vanishing weight.

-----

The results do not necessarily speak for themselves. What may be going on (and what I think is going on) is that the information contained in context_rect is not propagated back through the system quite right.

Seeing that apparently there are edge issues, I did think of fixing things with the hack which consists of enlarging all the context rects by 1 all around (most likely, we can't fix things only at the right and bottom, B/C sooner or later transformations that "flip things" probably will break things at the other ends).
So, I'd rather fix the interaction between context_rects and buffers than artificially enlarge them to get a margin of safety.

(From memory,) John Cupitt of VIPS internally uses seven different coordinate systems to handle the geometry involved in making sure that tiles have just enough data (and a bit, to account for round off) to cover the need of an inverse transformed rectangle. It's really easy to lose half a pixel along the way. And one needs to account for the fact that round off error may "suddenly" push things just a nudge over where they would be safe in exact arithmetic.

Again: I may be wrong. I've not looked at things from beginning to end and top to bottom. But this is my hunch.

Some parts of GEGL used the "pixel index is location of its top left corner" and other parts used the "pixel index is location of its center". My guess is that this is still the case in parts I have not touched yet. To me, this is a much more likely explanation for the boundary artifacts.
In addition, I am pretty sure (still double checking: depends on what this is used for) that the bounding box computation in transform-core drops a pixel when max_x happens to land right at an integer.

-----

So yes: Adding extra pixels to context_rect is a quick hack that will paper over problems. I'd rather do things "right".

Massimo:

Now I understand what the misunderstanding is, and it is possible that I programmed things too fast. I'm going to double check ASAP.

Yes, if you put anchor points at the center of the pixels, to reconstruct the bilinear interpolant within the (1D) pixel you need three input pixels. You only use two at a time (the leftmost two on the left half of the pixel, and the rightmost two on the right half---the pixel center is in both groups).
However, the idea is to choose the "patch" over which the sampling point is deemed to belong to and which defines the context_rect so as to minimize the width of the context_rect (because the width of the context_rect impacts the number of requested pixels, and less requested pixels implies more tile reuse).

With (bi)linear, if you center the patch at a pixel corner instead of at a pixel center, you only need two values, not three.
Same with (bi)cubic: You then need four instead of five.
So, linear and cubic use patches centered at pixel corners, and nearest and lohalo use patches centered at pixel centers. And, as mentioned earlier, the context_rect with even widths are centered at pixel corners, and the context_rect with odd widths are centered at pixel centers.

Now, I'm going to make sure the code reflects my words.

The current code reflects my words.

Massimo:

If what I've written is not clear, please look at your own

Teepee (j + 0.25, j-1) = MAX (0.0, 1 - |j - 1 + 0.5 - (j + .25)|) = 0.25
Teepee (j + 0.50, j  ) = MAX (0.0, 1 - |j     + 0.5 - (j + 0.5)|) = 1.0
Teepee (j + 0.75, j+1) = MAX (0.0, 1 - |j + 1 + 0.5 - (j + .75)|) = 0.25

The fact that you are using MAX (0.0, ... says that you are sending weights to zero. Why not simply exclude the corresponding pixel values?

Created attachment 229390 [details]
test output

I don't know if we are seeing the same artifacts when 
rendering the composition above, so I'm attaching the
output that I obtain. I save that xml in a file
named test-scale.xml and run

gegl test-scale.xml -o tmp.png

The discontinuities in the alpha channel that I see are
simply caused by the fact that gegl:transform 
get_required_for_output is using context_rect to extend
the rectangle requested. Obviously since upsizing 4x we are
going to put 2 destination pixels at the left of the leftmost
source pixel centre and 2 pixels to the right of the rightmost
source pixel, we need to extend in both direction of the same
amount.

The only misunderstanding is that not existing a specification
of what context_rect is, we infer its meaning by its usage,
and I considered its usage in 'get_required_for_output', you probably
its usage between the sampler base class and each derived class.

I'm not denying that with bilinear you always use at most 2x2 pixels,
I'm only saying that its usage in 'get_required_for_ouput'
is incompatible with the tight and anchored definition { 0, 0, 2, 2 }.

massimo:
I ran your test and indeed there is something really fishy going on.
:(

I'm pretty sure I tracked the problem. Now to fix it cleanly.

massimo's bug is fixed by the following hack:

  output->x = (gint) floor ((double) min_x) - (gint) 1;
  output->y = (gint) floor ((double) min_y) - (gint) 1;
  output->width  = (gint) ceil ((double) max_x) - output->x + (gint) 2;
  output->height = (gint) ceil ((double) max_y) - output->y + (gint) 2;

in gegl_transform_bounding_box.

I do not know if this hack breaks something else.

I have also tried to systematize the use of the convention that says
that integer ranges are indices and floating point ranges are absolute
positions, which are shifted by 0.5 w.r.t. indices. The result of this
attempt is found in the attached transform-core.c.

However, I have the impression that whatever "cuts the image to size"
needs to be fixed, and I don't know where this is. Also, this may
break the interaction with GIMP.

I also suspect that round off error is not explicitly taken into
account in the system that manages buffers. Just remember that pixel
indices that fix when computed one way may stick out when computed
another way. This is a well known issue with floating point
computations of this type. For this reason, 1 extra pixel all around
HAS to be added on purpose to the source buffers. I just don't know
where this should be done (maybe it was done already).

Finally, a comment RE: context_rect.width and height:

If the width is, say, 3 pixels, making sure that there is enough
pixels in an elarged region adds 2 pixels to the width, not three,
because one of the 3 has to already belong to the region. Again, this
may have been taken care of earlier, but maybe not.

I am not pushing this yet because the first fix is a hack, and the
second may be the right thing to do, but does not fit, for some
reason, with what's there.

I'm seeing more strange stuff, which I'll discuss later.

Created attachment 229612 [details]
transform-core.c with attempt at systematic use of absolute and index coordinate systems

Created attachment 229613 [details]
transform-core.c with attempt at systematic use of absolute and index coordinate systems

I am pretty sure that what massimo's test shows is that the toolchain does not account for variations by 1 that arise from round off error going different ways 

This is weakly confirmed by tweaking things: The results are way too sensitive to changes in individual steps' values that are of the size of typical round off error. I'm sorry I can't be much more precise: this is a hard thing to pin down (obviously).

I can fix "massimo's bug example" a number of non-contrived ways, but then other results don't fall in line. For example, there are breaks of symmetry with respect to x and y.

I unfortunately do not grasp the entire toolchain from start to finish. (And need to move on.)

The minimal change that will fix "massimo's bug example" and (apparently) not break resizing in GIMP is the following:

Replace lines 377-380 in transform-core.c (the last four lines of gegl_transform_bounding_box) by

  output->x = (gint) floor ((double) min_x) - (gint) 1;
  output->y = (gint) floor ((double) min_y) - (gint) 1;
  output->width  = (gint) ceil ((double) max_x) - output->x + (gint) 1;
  output->height = (gint) ceil ((double) max_y) - output->y + (gint) 1;

I would push this change if it was not for the fact that it changes the results of the gegl composition tests in ways that suggest that the results not be as intended. In particular, the size of the result of transform.xml is not 400x400 anymore, it is 401x401 if the above change is done.

Since I can't really tell, I'll just leave this as my best suggestion so far. And won't push unless told to do so anyway.

-----

I see signs that resize and affine transformations in GEGL are not robust w.r.t. round off. I also would be quite surprised if the change to the GIMP's coordinate system does not throw a wrench into non-affine transformations, because the mapping is not translation invariant with, for example, perspective transformations, which means that you can't fix a bounding box computed with a shifted set of points by translating the result.

I believe that GIMP is better off with the above "minimal change".
For this reason, I am planning to push it sometime soon, unless told not to.
As mentioned, this "minimal change" leads to somewhat undesirable results in the GEGL composition regression tests. However, I'd change the references to these "undesirable results" in the hope that this spurs changes for the better.
At least at the surface, adding one extra pixel of marging of safety at the top and left, which is what the "minimal change" does, should not have some of the side effects I see.
I would guess that the code that "cuts" the result should be changed. But I don't know where this code is.

I think that to properly implement 'get_required_for_output'
a fixed 'context_rect' is not sufficient. I mean, for instance,
implementing a correct lanczos3 sampler, a fixed context_rect
with width 6 is correct only upscaling, when downscaling
it requires the source pixels in the area covered by 6x6 
destination pixels, that is, when downscaling 10:1 you'll need
~60x60 pixels surrounding the projected destination pixel center.

I mean interpolators like 'linear' and 'cubic', are not suitable to
downsample, unless you low pass the source in advance, and so it is 
not correct to design gegl-sampler's interface around these.

'lohalo', for example, has a context_rect with width set to 27, but 
probably when upsampling each width greater than 5 does not change 
anything. and 27 is only a reasonable upper bound.


IMO this means that each sampler should implement a member function
that answers the question: what is the source rectangle required
to sample at 'x', 'y', with a given 'inverse_jacobian'?

For now, either leave the situation as is, or the "minimal change"
is equally fine

(In reply to comment #60)
> I think that to properly implement 'get_required_for_output'
> a fixed 'context_rect' is not sufficient. I mean, for instance,
> implementing a correct lanczos3 sampler, a fixed context_rect
> with width 6 is correct only upscaling, when downscaling
> it requires the source pixels in the area covered by 6x6 
> destination pixels, that is, when downscaling 10:1 you'll need
> ~60x60 pixels surrounding the projected destination pixel center.

Oyvind (I believe) decided that context_rects would be constant, and I can imagine the programming nightmare + speed hit of not making them so. (Although I think that maybe VIPS has machinery that would accomodate that.)

The way to access large swatches of data is to use mipmaps.

> I mean interpolators like 'linear' and 'cubic', are not suitable to
> downsample, unless you low pass the source in advance, and so it is 
> not correct to design gegl-sampler's interface around these.

I am planning to remedy this fairly soon.

> 'lohalo', for example, has a context_rect with width set to 27, but 
> probably when upsampling each width greater than 5 does not change 
> anything. and 27 is only a reasonable upper bound.

As you may know already, it's more complicated than that: There are two different context_rects, one for each of the two mipmap levels that are used.
The context_rect width for the level 1 mipmap level is currently set to 47, which means that it will downsample reasonably well down to about a ratio of 1/100, at which point it will start resembling box filtering by a ratio of about 50 followed by nearest neighbour decimation.

On my further down TO DO list there is adding more mipmap levels to improve large ratio downsampling and speed up not so large ratio.

But yes: When upsampling, having a width of 27 for Lohalo, which only needs 5, is a waste, and it comes with a performance hit. This particular compromise comes from wanting small ratio downsampling to be fairly accurate. (I may have overdone it.)

> IMO this means that each sampler should implement a member function
> that answers the question: what is the source rectangle required
> to sample at 'x', 'y', with a given 'inverse_jacobian'?

In theory, this is the way to go. Not so sure in practice.
I think we need to fix the "fixed context_rect" system before thinking about that. I think this much simpler set up still needs quite a bit of love.

The reason why non-constant context_rects are a pain is that if you are doing non-affine transformations, they vary on a point by point basis, and the added accounting needed to keep track of this stuff will not be worth the gain.
I agree with Oyvind that grabbing more data through mipmaps when needed is a much more reasonable approach.
I kind of wish that I could access level 0 through more than one context_rect (one for upsampling and one for downsampling, say), but I again understand that this is adding complications with, most likely, little benefit.

correction: ... box filtering by a ratio of about 100 ...

massimo:
If you happen to know what would make it better (without using non-fixed context_rect), could you please state it or provided patches or push commits?

I fixed the offset problem in late November and/or December of 2012, when I brought GEGL's image geometry convention in line with "classic GIMP"'s.

Excellent work Nicolas, thanks for the effort. This kind of small low level bug looks negligible but can cause notable deterioration after multiple operations that are part of lengthy and complex image processing.