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

Support resize without preserving aspect ratio #192

Merged
merged 1 commit into from
Apr 16, 2015
Merged

Support resize without preserving aspect ratio #192

merged 1 commit into from
Apr 16, 2015

Conversation

skedastik
Copy link
Contributor

Goal

Add an ignoreAspectRatio method to support resizing of images to arbitrary aspect ratios, as requested in #118.

Behavior

Fixed width and height:

sharp('foo.jpg').ignoreAspectRatio().resize(300, 300);

Fixed width (leaving height unchanged):

sharp('foo.jpg').ignoreAspectRatio().resize(300);

Fixed height (leaving width unchanged):

sharp('foo.jpg').ignoreAspectRatio().resize(null, 300);

sharp's behavior is otherwise unchanged.

Code paths affected

  • src/resize.cc
    • Canvas
      • Additional member IGNORE_ASPECT
    • ResizeWorker::Execute
      • Logic is mostly unchanged. Add separate x and y variables for factor, shrink, and residual.
      • Move small amount of logic into private class methods:
        • CalculateShrink
        • CalculateResidual
      • Use average of xresidual and yresidual to compute sigma for pre-affine-reduction Gaussian blur (see Progress on #118 (ignore aspect ratio) #191).
  • index.js
    • New method Sharp.prototype.ignoreAspectRatio
      • Sets this.options.canvas to "ignore_aspect"
  • test/unit/resize.js

@skedastik skedastik changed the title Support resize without preserving aspect ratio #118 Support resize without preserving aspect ratio Apr 14, 2015
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 97.31% when pulling 1f4e1c1 on skedastik:judgement into 3810f64 on lovell:judgement.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 97.31% when pulling 1f4e1c1 on skedastik:judgement into 3810f64 on lovell:judgement.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 97.31% when pulling 1f4e1c1 on skedastik:judgement into 3810f64 on lovell:judgement.

@lovell
Copy link
Owner

lovell commented Apr 15, 2015

Hi @jsongHBO @bkw @blowsie, as requestors of #118 are you able to help test @skedastik's great work on the feature you need?

@bkw
Copy link
Contributor

bkw commented Apr 15, 2015

Great work, @skedastik !
I noticed something odd with the size calculations, though:
Resizing a 610x600 image to 256x256 with ignoreAspectRatio() consistently results in a 256x260 image for me.

@skedastik
Copy link
Contributor Author

Thanks, @bkw.
I'm having a hard time reproducing the bug on my end. Running vips-7.42.1 under OSX. I'm using a 610x600 input image with combinations of JPEG/PNG/TIFF inputs and outputs and getting a 256x256 result consistently. I'll keep you posted...

@bkw
Copy link
Contributor

bkw commented Apr 15, 2015

I just noticed I was a bit behind on my checkout of libvips. rebuilding with 7.42 now, will report back if it makes a difference.

@bkw
Copy link
Contributor

bkw commented Apr 15, 2015

I can still reproduce it with this:

var sharp = require('sharp'),
    request = require('request'),
    resizer = sharp().ignoreAspectRatio().resize(256,256);

request('http://i.imgur.com/dLKxTCk.jpg')
  .pipe(resizer)
  .pipe(sharp().metadata(console.log));

This gives the following output for me:

null { format: 'jpeg',
  width: 256,
  height: 260,
  space: 'srgb',
  channels: 3,
  hasProfile: false,
  hasAlpha: false }

Strange but true: Adding .gamma() produces the correct height of 256...

@skedastik
Copy link
Contributor Author

Curiouser and curioser. Reproduced on my system. Looking into it...

@lovell
Copy link
Owner

lovell commented Apr 15, 2015

Quick thought: possibly related to the use of gamma correction preventing JPEG shrink-on-load.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 97.31% when pulling 968736c on skedastik:judgement into 3810f64 on lovell:judgement.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 97.31% when pulling 968736c on skedastik:judgement into 3810f64 on lovell:judgement.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 97.31% when pulling 968736c on skedastik:judgement into 3810f64 on lovell:judgement.

@skedastik
Copy link
Contributor Author

Right again, @lovell. There was a typo in the JPEG shrink-on-load conditional. In fixing it I realized we could retain the shrink-on-load optimization even if the aspect ratio changes. An improvement all around!

Nice work exposing the bug @bkw.

Fix has been squashed and pushed.

@bkw bkw mentioned this pull request Apr 16, 2015
@bkw
Copy link
Contributor

bkw commented Apr 16, 2015

I can confirm that for me it works as expected now. Thanks for the quick response, @skedastik !

assert.strictEqual(320, info.height);
done();
});
});

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you able to add a couple of tests for upscaling width but downscaling height and vice versa?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 97.31% when pulling f72435c on skedastik:judgement into 3810f64 on lovell:judgement.

@skedastik
Copy link
Contributor Author

More unit tests: done!

lovell added a commit that referenced this pull request Apr 16, 2015
Add support for ignoreAspectRatio option when resizing
@lovell lovell merged commit 3dfc7be into lovell:judgement Apr 16, 2015
@lovell
Copy link
Owner

lovell commented Apr 16, 2015

Fantastic, thank you Alaric. This will be included in the forthcoming v0.10.0 release.

@skedastik skedastik deleted the judgement branch April 16, 2015 15:01
@lovell lovell added this to the v0.10.0 milestone Apr 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants