"-distort Barrrel" gives wrong centre pixel

[snibgo]

The effect occurs with real-world examples, but here is a simple test:

Code: Select all

convert -size 100x100 plasma: plasma.png

convert plasma.png ^
  -distort Barrel 0,0,0,1 ^
  plasma_debarrel.png

The "Barrel" shouldn't change anything, but the pixel at (49,49) becomes almost white. Other values (eg 0.999 or 1.001 instead of 1) show the bad behaviour.

The bad behaviour doesn't occur when the input is a solid colour.

IM 6.6.0-8 Q16 on Windows 7.

[snibgo]

I guess that in distort.c around line 2222 the lines

Code: Select all

            else { /* Special handling to avoid divide by zero when r=0 */
              s.x=s.y=0.0;

should be

Code: Select all

            else { /* Special handling to avoid divide by zero when r=0 */
              s.x=coeff[8];
              s.y=coeff[9];

(But don't take my word for it.)

[anthony]

I guess that in distort.c around line 2222 the lines

Code: Select all

            else { /* Special handling to avoid divide by zero when r=0 */
              s.x=s.y=0.0;

should be

Code: Select all

            else { /* Special handling to avoid divide by zero when r=0 */
              s.x=coeff[8];
              s.y=coeff[9];

(But don't take my word for it.)

hmmm except the line is 2234 in the latest version of IM. But yes that does appear to be the error.

However that line should simply be removed or commented out.
Their is an initial s = d at the top that will take care of things.

However what is stranger is that you see the error at all on a 100x100 pixel image
In that case the center should be at image coordinates 50,50 but the nearest pixels
to that center is at 49.5,49.5

A pixel should not match the 'center' of the image unless the image has a odd pixel size!
that is the image was say 99x99 not 100x100!

Checking further...
Yeap center calculations was assuming 'pixel' coordinates, which is what the 'verbose FX equivalent' was doing it in, as it is simpler to encode. But the actual distortion code has to do it in image coordinates as that is what the 'd' (destination coordinate) is supplied in, and what 's' (source coordinate for resampling lookup) is expected to be in. That is the 'center' (coefficients 8 and 9) of the barrel distortion was half a pixel out.

See Image Coordinates vs Pixel Coordinates
http://www.imagemagick.org/Usage/distor ... oordinates

God is in the details!!!!

testing -- then will submit to SVN for release of IM v6.6.1-5

[anthony]

Hmmm while I am at it lets see about fixing the stupid 'd' argument required bug...

Code: Select all

convert plasma.png -distort Barrel 0,0,0 show:
convert: invalid argument for option Barrel : number of arguments @ error/distort.c/GenerateCoefficients/1248

The fourth 'd' argument should automatically calculate to be 1.0.

that is input arguments should be in one of these forms...

A,B,C
A,B,C,D
A,B,C X,Y
A,B,C,D X,Y
Ax,Bx,Cx Ay,By,Cy
Ax,Bx,Cx,Dx Ay,By,Cy,Dy
Ax,Bx,Cx,Dx Ay,By,Cy,Dy X,Y

with any missing 'D' (scale adjust) argument being calculated appropriately.

[anthony]

Done...
* Fix for special case handling in Barrel distortion (pixel at image center)
* Fix calculation of image for Barrel distortion (pixel vs image coords)
* Fix argument input (handling optional 'd' argument) for Barrel distortion

[snibgo]

Thanks, Anthony.

Sorry about the line number; I was looking at 6.5.9-6 sources.

The code in 6.5.9-6 wouldn't be able to distinguish between the cases

A,B,C,D X,Y
Ax,Bx,Cx Ay,By,Cy

as they both have 6 arguments. Doubtless you are addressing this.

[anthony]

Hmmm yes. 6 arguments. you can't have that. That will introduce a new bug!

Checking and removing the Ax,Bx,Cx, Ay,By,Cy version

Fixes are now for IM v6.6.1-6

[fmw42]

can't you distinguish by the number of commas, if you are strict on enforcing the comma delimiters?

A,B,C,D X,Y <--- 3 commas then a space and then one comma
Ax,Bx,Cx Ay,By,Cy <--- 2 commas, then a space and then 2 commas

Fred

[snibgo]

Commas aren't currently enforced -- space and comma are interchangable. Enforcing commas now could break scripts.

But I suppose it could work something like: "if and only if pattern is 2 commas, space, 2 commas, then assume Ax,Bx,Cx Ay,By,Cy."

[anthony]

It is a little more complex than that...
A B
is two argument, as is
A,B
A, B
A , B

BUT multiple commas delimit separate arguments, where multiple spaces do not.
A,,B
is actually three arguments, the middle argument is undefined, though I believe is
usually set to zero. However this aspect is currently not used or properly supported.

The other aspect is it isn't just space but whitespace. That is space, tabs, newlines, and returns are all acceptable argument separators. This can be vital for the 'human reading' of
arguments, especially when a lot of coordinates become involved. For example...

Code: Select all

 ...  -distort Shepards '
         19,8, 29,8
         19,27 29,27
         26,34 36,34
         33,37 43,37
         36,37 46,37
         53,37 63,37
         58,25 68,25
         13,20 13,20
         17,28 17,28
         25,36 25,36
         35,39 35,39
         46,40 46,40
         50,43 50,43
    ' ......

This also means you should be able to read arguments from a file with all the values in columns, perhaps generated by some other program, or exported from a spreadsheet.

Code: Select all

  .... -distort Shepards '@coordinates.txt' ...

This should become very important when using perspective to distort photos into a multi-iamge 'panarama' image.

Aside for -distort and -sparse-color you can also use percent escapes in the generation of the arguments! For example...
Generate a diagonal gradient from one corner to another corner of ANY sized image.

Code: Select all

    convert -size 600x60 xc: -sparse-color barycentric \
             '0,%h black  %w,0 skyblue  0,-%h skyblue' diagonal_gradient.jpg