Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

experiment with round to nearest for image size change #491

Closed
jcupitt opened this issue Jul 23, 2016 · 36 comments
Closed

experiment with round to nearest for image size change #491

jcupitt opened this issue Jul 23, 2016 · 36 comments
Milestone

Comments

@jcupitt
Copy link
Member

jcupitt commented Jul 23, 2016

vips always does int() on image dimensions for a size change. The aim is to prevent fractional pixels at image edges. If you want to size to a specific number of pixels, you need to add a small positive offset to the target size to prevent rounding errors producing a smaller image than you wanted.

This seems ugly. Investigate switching to round to nearest rather than strict round down. This will affect at least resize, affine, reduce, reduceh, reducev, shrink.

See https://github.com/jcupitt/libvips/pull/490

@ioquatix
Copy link
Member

One option to consider is whether using floating point values for scale factors is a good idea.

Alternatives include:

  • Fixed point ala libfreetype.
  • Rationals eg numerator and denominator data type.
  • Explicitly provide width and height to function rather than scale factor.

Personally I think if you stick with a scale factor you need to use appropriate rounding.

Alternatively explicitly specify width and height is a good idea too.

Keep in mind not all resizing is integral.. Not sure if you have captured that in the API but it might also be worth considering..

@ioquatix
Copy link
Member

Also wrt your assertion regarding preventing fractional pixels, how is round any different? It still produce integral values, just ones that are probably what the user was expecting, rather than truncation.

@jcupitt
Copy link
Member Author

jcupitt commented Jul 23, 2016

I use vips for medical imaging, I need to be able to resize images so that matching pixels line up. For example, if I have a pair of slices from CT scans taken at different resolutions, I need to be able to upsize the smaller one using the ratio of the resolutions so that the two images can be overlaid. Resize therefore must take a float scale factor.

On fractional pixels: for example, suppose you have a 100 pixel across image and you scale by 0.995. Now the output size will be 99.5, if you round-to-nearest, that's back to 100 again. The right-most column of pixels in the output will only be half a pixel wide.

Probably the simplest fix is just to add another column of pixels to the right edge of the input, but I suppose you could also (arguably) make it 50% transparent. Anyway, strict round down means you don't have to deal with these problems.

A related issue is corner vs. centre convention. vips is corner, but with extra parameters which let you give an input offset to produce centre if you wish. I'm a bit unclear if that might need revisiting too.

@jcupitt jcupitt added this to the 8.4 milestone Aug 14, 2016
jcupitt referenced this issue Aug 15, 2016
seems to all work, test it a bit more

see https://github.com/jcupitt/libvips/issues/491
@jcupitt
Copy link
Member Author

jcupitt commented Aug 15, 2016

I've had a go in this branch:

https://github.com/jcupitt/libvips/tree/try-round-to-nearest

It seems to pass the basic tests, I'll try it on some of my workloads.

Any other testing would be great! @felixbuenemann, you should be able to remove your 0.1 hack, I think, though it's also fine to leave it in.

@jcupitt
Copy link
Member Author

jcupitt commented Aug 15, 2016

@lovell, I meant to ask you about this too, do you have any size rounding hacks in sharp? This could make them removable.

@lovell
Copy link
Member

lovell commented Aug 16, 2016

Some previous versions of sharp contained the +0.1 hacks, but now it always recalculates as it goes. I'll try the sharp test suite using libvips from this branch.

@felixbuenemann
Copy link
Collaborator

@jcupitt Thanks, I did a quick check and it works fine for the common cases.

I found a weird edge case though, where the result was unexpected:

# 100x1 black image
original = Vips::Image.black(100, 1)
# resize to 50x1
scale = 50 / original.width.to_f
=> 0.5
resized = original.resize(scale)
resized.width
=> 1 # expected 50
resized.height
=> 1 # ok

So instead of rounding the height from 0.5 to 1 pixel it instead resized the width to 1 px.

Note that in vips 8.3.3 this fails with:

GLib-GObject-WARNING **:value "inf" of type 'gdouble' is invalid or out of range for property 'yshrink' of type 'gdouble'

@lovell
Copy link
Member

lovell commented Aug 18, 2016

@jcupitt Do the C++ bindings for shrink/reduce/v/h need updating on the try-round-to-nearest branch to reflect the xshrink to hshrink etc. parameter name changes?

@jcupitt
Copy link
Member Author

jcupitt commented Aug 18, 2016

Those changes should be invisible I think @lovell, they are not exposed in the API, except as names in prototype declarations.

@jcupitt
Copy link
Member Author

jcupitt commented Aug 18, 2016

Oh argh, you're right @lovell, they do need updating, sorry about that.

@jcupitt
Copy link
Member Author

jcupitt commented Aug 18, 2016

@felixbuenemann I tried in python and it seems to work. I see:

>>> import gi
>>> gi.require_version('Vips', '8.0')
>>> from gi.repository import Vips
>>> x = Vips.Image.black(100,1)
>>> y = x.resize(0.5)
Traceback (most recent call last):
...
gi.overrides.Vips.Error: Error calling operator resize.
  reducev: image has shrunk to nothing
>>> y = x.resize(0.51)
>>> y.width
51

I'll have a go with ruby-vips.

@felixbuenemann
Copy link
Collaborator

Yeah, this somehow behaves differently on ruby-vips vs. pyvips8. I tried in python and am getting the same error as you posted above.

@lovell
Copy link
Member

lovell commented Aug 18, 2016

@jcupitt Thanks for updating the C++ bindings - this change looks good from sharp's perspective.

@jcupitt
Copy link
Member Author

jcupitt commented Aug 18, 2016

Yes, looks like a ruby-vips problem. I've fixed a few things and pushed 1.0.3. I now see:

$ irb
irb(main):001:0> require 'vips'
=> true
irb(main):002:0> x = Vips::Image.black 100,1
=> #<Vips::Image:0x2eedbb0 ptr=0x2ed8010>
irb(main):003:0> y = x.resize 0.51
=> #<Vips::Image:0x2fcdee0 ptr=0x2ed81a0>
irb(main):004:0> y.width
=> 51
irb(main):005:0> y = x.resize 0.5
Vips::Error: reducev: image has shrunk to nothing

    from /home/john/packages/gems/gems/ruby-vips-1.0.3/lib/vips/call.rb:184:in `build'
    from /home/john/packages/gems/gems/ruby-vips-1.0.3/lib/vips/call.rb:249:in `invoke'
    from /home/john/packages/gems/gems/ruby-vips-1.0.3/lib/vips/call.rb:278:in `call_base'
    from /home/john/packages/gems/gems/ruby-vips-1.0.3/lib/vips/image.rb:395:in `method_missing'
    from (irb):5
    from /usr/bin/irb:11:in `<main>'
irb(main):006:0> 

Which I think looks correct.

@felixbuenemann
Copy link
Collaborator

@jcupitt Yeah, I'm getting the same error now in ruby-vips.

However in my calculations (1*0.5).round should be one not zero, so why is it rounding down?

@jcupitt
Copy link
Member Author

jcupitt commented Aug 19, 2016

I think that's just float arithmetic, it's never exact. 0.5 is not precisely 0.5 in binary. I experimented:

irb(main):013:0> y = x.resize 0.50000000000000001
Vips::Error: reducev: image has shrunk to nothing

    from /home/john/packages/gems/gems/ruby-vips-1.0.3/lib/vips/call.rb:184:in `build'
    from /home/john/packages/gems/gems/ruby-vips-1.0.3/lib/vips/call.rb:249:in `invoke'
    from /home/john/packages/gems/gems/ruby-vips-1.0.3/lib/vips/call.rb:278:in `call_base'
    from /home/john/packages/gems/gems/ruby-vips-1.0.3/lib/vips/image.rb:395:in `method_missing'
    from (irb):13
    from /usr/bin/irb:11:in `<main>'
irb(main):014:0> y = x.resize 0.5000000000000001
=> #<Vips::Image:0x141b760 ptr=0x1cbe7f0>
irb(main):015:0> 

So the first line is rounding down, the second is rounding up, and they differ in just 0.0000000000000001.

@ioquatix
Copy link
Member

@jcupitt 0.5 can most definitely be represented accurately in IEEE float and double data types.

Floating point numbers are in base 2, which applies to the decimal part. So, a floating point number consists of a sequence of sums of 2**-n where for all n is the bit position - so, 2**-1 = 0.5, 2**-2 = 0.25, therefore 0.25, 0.5 and 0.75 could all be represented accurately as floating point numbers.

@ioquatix
Copy link
Member

@felixbuenemann did you look at the documentation for round? for x.y, where y < 0.5, round(x.y) -> x, otherwise round(x.y) -> x+1

@jcupitt
Copy link
Member Author

jcupitt commented Aug 19, 2016

@ioquatix Oh, true, I was thinking of 0.1 perhaps. Anyway, that 0.5 argument to resize goes through a long chain of processing, you would expect it to drift a little.

@felixbuenemann
Copy link
Collaborator

@jcupitt The problem I have with the rounding differences is this:

If I don't know how the resize internal rounding differs from normal rounding, then how should I calculate the correct scale value?

@jcupitt
Copy link
Member Author

jcupitt commented Aug 19, 2016

@felixbuenemann because it's now round-to-nearest, you have a 0.5 margin of error, so it doesn't matter. Just do:

scale = target_width.to_f / original_width

And it'll be correct.

I think your corner-case is a different issue: what should we do if an axis length rounds down to zero? We could add a new rules preventing this, I suppose, but it would need a little thought.

If you make a 100 x 2 pixel black, your problem goes away:

irb(main):026:0> x = Vips::Image.black 100,2
=> #<Vips::Image:0x1c8bb98 ptr=0x1cea190>
irb(main):027:0> y = x.resize 0.49
=> #<Vips::Image:0x141b288 ptr=0x1e771a0>
irb(main):028:0> y.size
=> [49, 1]
irb(main):029:0> y = x.resize 0.51
=> #<Vips::Image:0x1ea0500 ptr=0x1d96960>
irb(main):030:0> y.size
=> [51, 1]

@felixbuenemann
Copy link
Collaborator

@felixbuenemann because it's now round-to-nearest, you have a 0.5 margin of error, so it doesn't matter.

scale = target_width.to_f / original_width

And it'll be correct.

Except it isn't correct, as the discussion above shows, that resize() rounds down in cases where it should round up, so I need to have some additional logic to determine when that happens.

Your commit showed a similar issue:

1450 * 0.33
=> 478.5
x = Vips::Image.black(1450, 1450)
y = x.resize(0.33)
y.width
=> 478 # expected 479

So apparently resize() always rounds down for 0.5 when it should round up.

Regarding the round to zero case: As long as the rounding is predictable, I think the exception is ok.

@jcupitt
Copy link
Member Author

jcupitt commented Aug 19, 2016

I'll see if I can improve the rounding behaviour, maybe there's a little more precision to find.

Your example would work if you calculated the scale factor:

irb(main):002:0> x = Vips::Image.black 1450, 1450
=> #<Vips::Image:0x190a000 ptr=0x18f0000>
irb(main):003:0> target_width = 479
=> 479
irb(main):004:0> original_width = 1450
=> 1450
irb(main):005:0> scale = target_width.to_f / original_width
=> 0.3303448275862069
irb(main):007:0> y = x.resize scale
=> #<Vips::Image:0x1a155a8 ptr=0x18f0190>
irb(main):008:0> y.width
=> 479

So I'm not sure this is an issue in practice. You can't expect exact results with float arithmetic.

@jcupitt
Copy link
Member Author

jcupitt commented Aug 19, 2016

I actually traced it back to the C rint() function. This is a bit odd:

/* compile with
 *
 *      gcc -Wall rint.c -lm
 *
 */

#include <math.h>
#include <stdio.h>

int 
main( int argc, char **argv )
{
        double x = 0.5;

        printf( "x = %g\n", x ); 
        printf( "rint( x ) = %g\n", rint( x ) ); 
        printf( "(int) (x + 0.5) = %d\n", (int) (x + 0.5) ); 

        return( 0 );
}

Output for me is:

$ ./a.out 
x = 0.5
rint( x ) = 0
(int) (x + 0.5) = 1

So I guess vips is no worse than libm at least.

@jcupitt
Copy link
Member Author

jcupitt commented Aug 19, 2016

Ah, got it, it depends if the number is odd or even. 1.5 is not exactly representable as binary float, it's stored as 1.50000000000001. So rint(0.5) == 0, but rint(1.5) == 2.

Anyway, floating point number madness, don't worry about it, IMO.

@ioquatix
Copy link
Member

ioquatix commented Aug 19, 2016

Where are you using rint? My PR used round. rint does this to ensure that statistical averages make sense when aggregating a lot of numbers.

If you read the documentation for rint you'll see it rounds to the nearest even number.

@ioquatix
Copy link
Member

Just FYI, the second answer of this is correct: http://mathematica.stackexchange.com/questions/2116/why-round-to-even-integers

@ioquatix
Copy link
Member

So, just to clarify, you don't want to use rint for rounding the size of the image. It won't work as you expect it to.

@jcupitt
Copy link
Member Author

jcupitt commented Aug 20, 2016

That's interesting, I didn't know about Banker's Rounding, thanks! I've changed it to round() where appropriate and I now see:

irb(main):002:0> x = Vips::Image.black 100, 1
=> #<Vips::Image:0x12cde80 ptr=0x12bc010>
irb(main):003:0> y = x.resize 0.5
=> #<Vips::Image:0x13ae1d8 ptr=0x12bc1a0>
irb(main):004:0> y.size
=> [50, 1]

@jcupitt
Copy link
Member Author

jcupitt commented Sep 5, 2016

OK, merged to master, this will be in 8.4. Thank you everyone.

@jcupitt jcupitt closed this as completed Sep 5, 2016
@aarti
Copy link

aarti commented Sep 12, 2016

@jcupitt Is it possible to get this fix in a dot release? and/or curious about the 8.4 release date. Thanks for a great library.

@felixbuenemann
Copy link
Collaborator

This is a change in behavior rather than a simple bugfix, so I don't think it belongs in a patch release.

Then again I'm no maintainer, so John might have a different point of view.

@aarti Is there a particular reason you can't use the current master?

@aarti
Copy link

aarti commented Sep 12, 2016

It's ok, will wait for 8.4, using a workaround for now.

@Meekohi
Copy link

Meekohi commented Oct 13, 2016

Should we start a new issue about the option of giving explicit integers when you know the exact width or height you want? It is pretty awkward to use this on the command line tool when you, for example, want to down-sample a bunch of images of different sizes to the same width.

@felixbuenemann
Copy link
Collaborator

@Meekohi I would suggest a new issue, this issue is closed and deals with rounding issues, while your concern is mostly with usability.

@jcupitt
Copy link
Member Author

jcupitt commented Oct 14, 2016

Sure, please open an issue.

I would use vipsthumbnail for this task from the command-line, it should do what you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants