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

Image corruption when resizing and converting to jpg #249

Closed
compeak opened this issue Aug 7, 2015 · 16 comments
Closed

Image corruption when resizing and converting to jpg #249

compeak opened this issue Aug 7, 2015 · 16 comments
Labels

Comments

@compeak
Copy link

compeak commented Aug 7, 2015

When I try to resize and convert a png to jpg sharp outputs a corrupted image.
Input:

$ pngcheck test.png 
OK: test.png (1280x877, 32-bit RGB+alpha, non-interlaced, 78.5%).
$ file test.png 
test.png: PNG image data, 1280 x 877, 8-bit/color RGBA, non-interlaced

test

convert-resize.js

var sharp = require('sharp');

var width   = 1000;
var height  = 1000;
var quality = 75;
var format  = 'jpeg';

var newImage = sharp('./test.png')
                .resize(width, height)
                .max()
                .withoutEnlargement()
                .quality(quality)
                .toFormat(format)
                .toFile('./test.jpg');

Output:

$ file test.jpg 
test.jpg: JPEG image data, JFIF standard 1.01, aspect ratio, density 1x1, segment length 16, baseline, precision 8, 1000x685, frames 3

test

This seems to happen every time a png gets converted to jpg and resized to a smaller size.

vipsthumbnail test.png -o tn_%s.jpg --size 1000 produces correct output.

I used sharp 0.11.0 and vips 7.40.6 on Debian 8.1.

@lovell
Copy link
Owner

lovell commented Aug 7, 2015

Hello, the saturated output you're experiencing looks like it could be related to the (un)pre-multiplication stage added in v0.11.0. This is designed to improve the quality of transparent image resizing and allow alpha compositing and is applied for any input image with an alpha channel.

libvips 8.1.0+ provides a native (un)premultiply operation for us and for earlier versions sharp uses it's own (un)pre-multiplication logic (thanks to @gasi).

I've tried your sample image with both libvips' and sharp's version of this logic and both work for me with the latest libvips master code.

My best guess is that we may have run into something that's been fixed within the last year in libvips itself since 7.40.6. Are you able to test using a more recent version of libvips to help narrow this down?

A possible quick fix here might be to use flatten if you know you're converting to JPEG as this seems to avoid sharp's (un)pre-multiplication logic (but the presence of this shortcut is probably itself a bug!).

@lovell lovell added the triage label Aug 7, 2015
@compeak
Copy link
Author

compeak commented Aug 10, 2015

Using flatten, the image is converted correctly.

It seems like I can't get a working build of libvips 8 on Debian 8.

@compeak compeak closed this as completed Aug 10, 2015
@lovell
Copy link
Owner

lovell commented Aug 10, 2015

@compeak Glad to hear the quick fix worked for you.

I'm still a little concerned there may be a problem with sharp's premultiply code on older versions of libvips.

If anyone else should run into this, please add more examples here and I'll re-open the issue.

@compeak
Copy link
Author

compeak commented Aug 10, 2015

@lovell Thank you for your help.

@gasi
Copy link

gasi commented Aug 10, 2015

A possible quick fix here might be to use flatten if you know you're converting to JPEG as this seems to avoid sharp's (un)pre-multiplication logic (but the presence of this shortcut is probably itself a bug!).

Hmmm, shouldn’t (un)premultiplication only happen for compositing images with alpha channel? In this case, @compeak is only resizing and converting from PNG to JPEG.

@gasi
Copy link

gasi commented Aug 10, 2015

Hmmm, shouldn’t (un)premultiplication only happen for compositing images with alpha channel?

I take that back. Looks like it happens for any affine transform:

sharp/src/pipeline.cc

Lines 495 to 500 in dee9ca3

bool shouldAffineTransform = xresidual != 0.0 || yresidual != 0.0;
bool shouldBlur = baton->blurSigma != 0.0;
bool shouldSharpen = baton->sharpenRadius != 0;
bool shouldTransform = shouldAffineTransform || shouldBlur || shouldSharpen;
bool hasOverlay = !baton->overlayPath.empty();
bool shouldPremultiplyAlpha = HasAlpha(image) && image->Bands == 4 && (shouldTransform || hasOverlay);

@lovell
Copy link
Owner

lovell commented Aug 11, 2015

Thanks @gasi, I think the use of premultiply with affine is the correct behaviour.

In addition, thanks to the investigation into this problem, I may have discovered another relating to the use of normalisation and/or gamma correction with alpha channels.

@gasi
Copy link

gasi commented Aug 11, 2015

Thanks @gasi, I think the use of premultiply with affine is the correct behaviour.

Agreed, I believe it is too.

In addition, thanks to the investigation into this problem, I may have discovered another relating to the use of normalisation and/or gamma correction with alpha channels.

Sounds promising. Based on my (limited) experience working with (un)premultiplication, errors were usually subtle especially if images were close to fully opaque or fully transparent. Sometimes errors were big, but that generally meant the sign of some multiplication was wrong and that would be an obvious error we’d see more frequently.

Good luck on your investigation!

@lovell
Copy link
Owner

lovell commented Aug 11, 2015

@gasi see 36ac882

@gasi
Copy link

gasi commented Aug 11, 2015

@lovell Wow, nice find and fix! 👍

@baoshan
Copy link

baoshan commented Sep 2, 2015

So @lovell has fixed the problem in 0.11.1? I'm running on 0.11.2, but the problem is still there.

Any suggestions? Thanks.

@lovell
Copy link
Owner

lovell commented Sep 2, 2015

@baoshan Which version of libvips are you using? I think the summary was that 7.40.6 can fail but 8.0.2 is OK. The workaround for earlier versions is to explicitly add the flatten() option.

@baoshan
Copy link

baoshan commented Sep 2, 2015

flatten works great! Thanks.

Baoshans-MacBook-Pro:contributor Baoshan$ brew install vips
Warning: homebrew/science/vips-8.0.2 already installed

Does that mean I have 8.0.2? But still requires flatten.

@lovell
Copy link
Owner

lovell commented Sep 2, 2015

@baoshan Sorry, my mistake, I meant to say that libvips v8.1.0 is (or will be) OK. Glad to hear flatten works for now.

@baoshan
Copy link

baoshan commented Sep 2, 2015

Thanks. Sorry for didn't get the summary correct at first.
Wonderful work!

@jyounus
Copy link

jyounus commented Sep 18, 2015

I've had the same issue.

brew install vips
Warning: homebrew/science/vips-8.0.2 already installed

Using flatten() solved the issue for me as well.

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

No branches or pull requests

5 participants