EyalAr / lwip

Light Weight Image Processor for NodeJS
MIT License
2.36k stars 230 forks source link

PNG format seems broken since 0.0.8 #194

Closed gautaz closed 9 years ago

gautaz commented 9 years ago

Hello,

I'm using lwip to generate image thumbnails. In order to test my code, I'm comparing a generated thumbnail to a standard one in PNG format. Everything was fine since 0.0.8 on which my test raises an error.

In order to diagnose the issue, I have tried to compare the generated image with the standard one by using ImageMagick, I've got the following error:

11:17 $ LANG=C compare old.png new.png diff.png
compare: improper image header `new.png' @ error/png.c/ReadPNGImage/3921.
11:27 $ compare --version
Version: ImageMagick 6.9.2-3 Q16 x86_64 2015-10-02 http://www.imagemagick.org

new.png is the newly created thumbnail produced by lwip 0.0.8.

This might be related to https://github.com/EyalAr/lwip/pull/165 but I am not sure whether this is an ImageMagick issue or an lwip one.

EyalAr commented 9 years ago

Can you open new.png in another software?

gautaz commented 9 years ago

new.png can still be opened in any image viewer. I'm just concerned that the latest changes in lwip might not comply with the PNG standard. As said before, the issue might be an ImageMagick issue...

EyalAr commented 9 years ago

You are correct that #165 changes the way PNGs are encoded, but the additional code only runs if metadata is actually passed in. If you are not passing in metadata I don't see any reason why the encoding would be different.

Another thing that changed in 0.0.8 is the version of pnglib, which could somehow be related to this.

This will require more thorough checks to see if the problem is LWIP, ImageMagick or the new version of pnglib.

I'll install ImageMagick on my machine to check.

But before I do - Does this happen with every image? Can you upload here an image on which it does happen? Can you verify you are not setting any metadata? Can you verify this does not happen with LWIP 0.0.7?

Thanks.

gautaz commented 9 years ago

In the meantime, I have reinstalled lwip 0.0.7 to work around the issue and reinstalled lwip 0.0.8 back again to reproduce the failing sample. And now, I can just not reproduce the issue. What is bugging me is that before reinstalling, the issue was reproducible. I still have this difference between the old PNG sample and the new one but now ImageMagick accepts it without complaining.

The original image I'm using to operate my tests is available here.

Here are both thumbnails produced by lwip 0.0.7 and 0.0.8: thumbnail3-0 0 7 thumbnail3-0 0 8

And here is the difference that ImageMagick is now accepting to produce: diff

The source code I'm using to produce thumbnails:

var storeImageAsIcon = function (buffer, format, callback) {
    lwip.open(buffer, format, function (err, image) {
        if (err) { return callback(err); }
        var width = Math.ceil(50 * image.width() / image.height());

        image.resize(width, 50, function (err, image) {
            if (err) { return callback(err); }

            image.toBuffer('png', function (err, buffer) {
                if (err) { return callback(err); }
                var writestream = gfs().createWriteStream({
                    metadata: { type: 'icon' },
                    chunk_size: 1024,
                    content_type: 'image/png'
                });
                writestream.on('close', function (file) {
                    log.info('icon', file._id, 'stored');
                    return callback(null, file._id);
                });
                streamifier.createReadStream(buffer).pipe(writestream);
            });
        });
    });
};

As you can see, the only metadata I'm manipulating is for gridfs and not for lwip.

I have tried with a bunch of other images and I still cannot reproduce the issue, sorry. If you cannot reproduce on your side, I will just close the issue...

EyalAr commented 9 years ago

I'll close it for now. But please let me know if the issue comes back.

A possible explanation is that when you first upgraded from 0.0.7 to 0.0.8, somehow the old binaries were not cleaned properly and were used with the new JS.