NodeBB
NodeBB copied to clipboard
NodeBB
Saved JPEG quality is severly degraded
I upload JPEGs with quality 97 looking wonderfully. Stripped-EXIF JPEGs saved by NodeBB contain a TON more artifacts and look really poorly (quality 85 I would say). I think the way image processing / EXIF stripping is done needs to be rethought. The image data should not be touched at all. All tags apart from orientation should be stripped. Degradation of quality can be avoided if the imaging data is not touched.
Independently, I believe that NodeBB is using a default JPEG quality level to save processed images. In case image resizing is enabled, it should take care that the same quality level is used for the resized output as was used in the uploaded JPEG.
It is really sad to upload nice quality images and see the result completely garbled - sorry for the rant ;)
Not sure if EXIF is replaced though...
https://github.com/lovell/sharp/issues/2910
Added the workaround from the above ticket. I think it should actually work ...
No I think it will not work, we need to use a command line tool like exiv2.
I have found the correct command using exiftool. The PR should work provided that exiftool is available on the system.
How should exiftool be added to the continuous integration Docker image?
The default jpeg output quality is 80: https://sharp.pixelplumbing.com/api-output#jpeg
That might be something we want to change or make configurable.
Your PR is never going to fly as is. You probably only saw a quality improvement because you're no longer going through sharp at all.
Ok it's terribly low but regardless of the quality, every re-compression with JPEG degrades quality. Especially that with Exif-Stripping + resizing those operations are currently chained !!! This could be worked around to re-compress only once but even so, for a photography forum such as mine, one recompression is already one too many. I strongly advocate in favor of exiftool for EXIF stripping. As implemented in PR #10447. It works perfectly - WAY better than sharp. And only requires a well established tool to be present on the system.
Sharp will probably never be able to achieve what exiftool is already doing without any problem whatsoever. Please let me know how the PR should be modified in order to "fly" ;)
You can implement just exif stripping in a plugin like https://github.com/NodeBB/nodebb-plugin-imagemagick if that's what you really want.
But I think there's some weird stuff going on in our image library. It does appear that we might be running images through sharp multiple times.
@baris do you mind taking a look?
As for what to do about your PR: I suggest changing it to just set the quality to 100 for jpegs and pngs when stripping exif data. See resize for an example of detecting the image type.
Wouldn't it be better to detect if exiftool is available and use it if possible?
Fixing quality to 100 will cause quite a lot of bloat when uploading other images with intentionally lower quality. It will get ugly fast. The problem is really that stripEXIF is not doing as advertised and not just stripping EXIF but recompressing the image.
Also stripEXIF does not seem to support a hook, so a plugin currently cannot be implemented.
You'd turn off exif stripping in the ACP and do it in your plugin instead.
I doubt that setting quality to 100 will bloat the images, but I can't tell you for sure without experimenting.
But which hook should I use then in the plugin?
Setting quality to 100 does blow up images. An example image I just tried saved at 97 has 600 KB. That image saved then at 80 has 200 KB and then the 80-image saved back as 100 has 500KB. So agreed it does not blow back even to the level of original 97 but it does blow up at least 2.5 times and that's for a relatively small image - 1400x1080.
I think we can make some improvements. Adding a hook into stripExif is one of them.
Also it seems right now if stripExif is turned on and the image is too big it goes through sharp twice. This can be changed so that it only goes through sharp once. Because it uses the same code.
stripExif does sharp(buffer).rotate().toFile() and then resize does sharp(buffer).rotate().resize().jpeg().toFile()
So it seems we don't need the call to stripExif here https://github.com/NodeBB/NodeBB/blob/master/src/controllers/uploads.js#L67 if image is already going to be resized by sharp.
I think a processImage function would be better to combine the two. Something like
const sharpImage = sharp(buffer, { failOnError: true});
sharpImage.rotate(); // auto-orients based on exif data
if (shouldResize) {
sharpImage.resize(width, height));
}
sharpImage.jpeg()
await sharpImage.toFile(path)
@sadaszewski https://github.com/NodeBB/NodeBB/commit/b8765df5f48b638517daac041964c7443dd52fde
The stripEXIF hook is welcome. If a plugin is indeed the only way to go...
Another alternative - although unmaintained - could be to use piexifjs. In this kind of code I would not expect many changes to be necessary over time - if any at all. What do you think?
Detecting the quality of an input jpeg is a hard problem: https://gist.github.com/FranckFreiburger/d8e7445245221c5cf38e69a88f22eeeb
But we could do something like this to try to output a jpeg at the same quality of the input jpeg.
You could "just strip exif data but orientation" but that won't work if you need to resize the image anyways, and support for exif orientation in browsers isn't great.
You could "just strip exif data but orientation" but that won't work if you need to resize the image anyways, and support for exif orientation in browsers isn't great.
I haven't seen any browser where it would not be supported in the last years...
Resizing is not something I am interested in but I agree that the correct way to do this would be to try to estimate similar image quality to the original.
Did you try disabling resizing? Nodebb stores the original image and then resizes and saves that to filename-resized.jpg. The resized image is displayed in the browser and when clicked should open the original. I tried with resizing disable and got the same image even with stripExif enabled.
I never had resizing enabled, only stripEXIF. And JPG images definitely come out garbled every single time. You can see the artifacts from low quality compression / recompression with but a slight zoom. It would be wrong to get the same image with stripEXIF enabled. After all the EXIF is supposed to be stripped.
I have created the plugin : https://github.com/sadaszewski/nodebb-plugin-exiftool