search

mozilla / mozjpeg

Improved JPEG encoder.
Other
5.47k stars 415 forks source link

Input PNG images with ICC profiles don't account for colour profile #191

Open njdoyle opened 8 years ago

njdoyle commented 8 years ago

If a PNG image is used as an input to cjpeg and that PNG image has an attached ICC profile, that profile will be stripped and the output JPEG will have incorrect colour.

kornelski commented 8 years ago

pngquant has code for it:

https://github.com/pornel/pngquant/blob/master/rwpng.c#L303

Would you like to add it to mozjpeg?

dwbuiten commented 8 years ago

Isn't this massively out of scope for cjpeg?

njdoyle commented 8 years ago

It's only out of scope as much as producing output images that reflect the input images is out of scope. Mozjpeg says it supports PNG input and attached ICC profiles are part of the PNG standard. ICC profiles must be considered important image data because the image can not be displayed properly without it. Possible options are to say mozjpeg only supports input of a subset of the PNG standard (unfortunate considering the number of PNG images with attached profiles), or support this common part of the PNG standard by either attaching the ICC profile to the output JPEG (what currently happens with input JPEGs with attached ICC profiles) or to do the colour conversion to sRGB.

dwbuiten commented 8 years ago

I've always been of the opinion that cjpeg is not supposed to be an all-in-one-tool, but rather an example/simple creation tool. It seems like it would be a better endeavour to expose any missing options in upstream libtools/libraries such as ImageMagick. Why build a completely new tool when the usual suspects already work with mozjpeg out of the box, today?

EDIT: I thought it was silly to add PNG support to cjpeg in the first place for this reason.

njdoyle commented 8 years ago

This isn't really an issue of cjpeg being an all in one tool. This is an issue of cjpeg not properly supporting an input format it says it supports. The tool already handles this case (in a non-breaking but not optimized for the web way) for input JPEG images so I believed it would be in scope.

dwbuiten commented 8 years ago

ICC profiles aren't actually part of the JPEG or JFIF spec, for what it's worth.

I'm not going to object to adding LCMS2 support, however I believe users probably should not be using cjpeg in production.

njdoyle commented 8 years ago

I understand ICC profiles aren't part of JPEG or JFIF proper (they might as well be from a de-facto standard point of view though). They are part of the PNG spec though and cjpeg says it supports PNG.

Obviously it's not my place to scope cjpeg but it is the only practical entry point for most users to mozjpeg today.

It's possible to not break output images without adding LCMS2 support. A simple passthrough of the uncompressed ICC profile data is sufficient. It's not optimal form a web optimization stand point but at least the output image isn't broken.

dwbuiten commented 8 years ago

Obviously it's not my place to scope cjpeg but it is the only practical entry point for most users to mozjpeg today.

mozjpeg is API and ABI compatible with libjpeg, and you can thus use it from any tool which supports it.

It's possible to not break output images without adding LCMS2 support. A simple passthrough of the uncompressed ICC profile data is sufficient. It's not optimal form a web optimization stand point but at least the output image isn't broken.

Every major browser vendor supports ICC profiles in JPEG, so this may be more reasonable than adding a (optional) dependency to make it work.

kornelski commented 8 years ago

I agree that tools like ImageMagick should adopt mozjpeg. Unfortunately, they haven't, and it's much easier to build just cjpeg than to build an entire custom ImageMagick package. Personally I do use cjpeg in production for quite important projects, and I'd prefer it to do the right thing for conversion.

ImageMagick isn't the silver bullet either. By default it'll just pass ICC through, and options such as -set colorspace sRGB and +profile sRGB don't actually convert pixels. Only -profile sRGB.icc seems to do the right thing (and you really have to have that file).

I have mixed feelings about passing ICC through. The upside is that it could be implemented without an LCMS dependency, but it seems inappropriate for a tool that's focused on compression performance. If cjpeg is compiled without LCMS I'd rather drop profile (so that user can notice something is wrong) than to generate bloated files.

I think the best solution for both accuracy and performance would be to always convert embedded ICC to sRGB and generate JPEGs with a minimal sRGB-like ICC embedded (such as the one that Facebook developed).

https://phabricator.wikimedia.org/T100976#1360867

paperboyo commented 8 years ago

I think the best solution for both accuracy and performance would be to always convert embedded ICC to sRGB and generate JPEGs with a minimal sRGB-like ICC embedded (such as the one that Facebook developed).

+1

In my humble opinion for a full-featured colour management the above is not enough, though. It leaves out input images without embedded profiles (they will undergo no colour conversion and will have no ICC profile embedded, so will behave unexpectedly depending on browser/OS/monitor and will behave differently to images with embedded profiles).

A complete solution would be to have options for:

In an ideal world there would also be an option for a whitelist of ICC profiles that are exempted from the above (standard minimal-size RGB profiles like e.g. AdobeRGB, maybe greyscale). That’s for when we would decide to trust browsers to do the final conversion from an image profile to the display profile completely (most current generation of both desktop and mobile browsers).

Regards m.