Sincere request to remove the sRGB profile error

[ghost]

Hello Glenn,

Was just curious if you guys are comfortable in removing this if statement. It's causing a lot of headache throughout the communities. Some examples are here and here.

I understand that the warning is important to you, however I'm a really big user of the Godot engine. And our contributors are denying my request to remove it, as they are not allowed to touch upstream sources.

If you guys can remove that if statement in a new version, they will most likely update to the latest version, and thus, it will be removed. The line in your codebase is here, at #2350.

However, I don't expect you guys to listen nor care at all, but it's worth a shot! For the record, I'm not a Godot contributor, just a heavy user of the engine. Also, libpng has been great within Godot and honestly, this would relieve quite a bit of stress.

Kind Regards,

-Nick

[glennrp]

I doubt that we will be removing the test from libpng. However, at the application level you can avoid the test by including

   png_set_option(png_ptr, PNG_SKIP_sRGB_CHECK_PROFILE,
       PNG_OPTION_ON);

in your application source code as explained in the libpng manual.

[DominikDeak]

Personally, I am against the idea of removing that sRGB profile check. Colour spaces is a complicated subject and many software authors get it wrong, or enforce bad habits, such as embedding broken or terrible profiles into images. Tools that perform such checks is a service to the community. The solution is simple: Stop embedding bad profiles into images!

[andrewla]

While it is true that people should stop embedding bad profiles, this warning appears on the read side, where we have no control over the profiles used to generate the images that are being consumed. At that point there is no action that can possibly be taken to fix the issue. It's hard to see the value in showing the warning at all. At the very least, could we consider disabling the warning by default and allowing writers and producers to enable it as part of their sanity checks?

[jbowler]

Please comment on this @ProgramMax

https://github.com/ProgramMax/ProgramMax is the current chair of the W3C PNG specification working group, so far as I am aware no one involved with libpng is currently a member of that group. The warning message is there because a bad sRGB profile was erroneously released into the wild by someone (not me or, indeed, anyone ever involved in libpng).

libpng currently handles the erroneous profile and the correct profile by treating them as sRGB but this can be fixed; sRGB is well established and special handling for the sRGB profile is not a part of libpng functionality so can and should be removed at this point. The correct handling has been documented for a great many years.

@ctruta: pinging you since this issue is way out of date and this involves the removal of dubious code and a bug-farm warning message.

[bghira]

we see this image emitted millions of times to the logs when training on large web-scale datasets. there's no way to modify how middleware interacts with libpng, eg. cv2

libpng should offer some kind of env var to disable this warning so that users do not endlessly pile issue reports onto trackers around GitHub.

[jbowler]

we see this image emitted millions of times to the logs when training on large web-scale datasets. there's no way to modify how middleware interacts with libpng, eg. cv2

libpng should offer some kind of env var to disable this warning so that users do not endlessly pile issue reports onto trackers around GitHub.

libpng is written in strict ANSI-C; it has no way to read an environment variable. It's necessary for the caller of libpng, something which has access to the png_struct, to disable the error message either by filtering it out or, better, by not outputting "warning" messages at all. Note that it is an error message but it is converted into a PNG warning message because it is in a chunk which is not required to read the PNG.

Better design for the future is to drop ancillary chunks with detectable errors silently unless the caller switches warnings on.

Even better design would be for the various libraries involved, there are a lot, not to simply spurt warning messages onto stderr! The default behavior of png_warning and png_error is just for hackers; it allows us to write a few lines of C to read or write a PNG. The default implementation should not be used in higher level libraries but is desirable for hacks. The problem is libpng doesn't know whether it is part of a hack or a serious piece of code!

[bghira]

https://github.com/pnggroup/libpng/assets/59658056/8e5e412d-3963-4080-9725-c4b56f49d8a6

yes. because this is currently a poor design offloading that decision/requirement to downstream users of the library

[jbowler]

@ctruta: by design. Can be switched off when building libpng using the extensively documented setting sRGB_PROFILE_CHECKS. (See scripts/pnglibconf.dfa.)

Can be disabled by the png_set_option Glenn described.

The specific problem is apps that don't handle warnings in any way; warnings (diagnostics) are there for a reason and I'm 110% in agreement with @DominikDeak

In this specific case the profile was erroneously distributed by Microsoft. It is a test profile provided by HP which uses the original von Kries transformation. The result is no better than using the cHRM chunk in the most basic way possible; in other words using cHRM in place of the profile will produce no worse results and they will typically be better.

It's sad that the W3C cared not to respond. Chris Lilley, who is on that committee, is a colour scientist (at least that is what he lead me to believe many years ago on another W3C committee).

My own opinion, which is on topic here, is that the problem is not the profile but that an app might blindly use the profile and thus produce worse results than would be obtained by not using it at all!

Time to close this bug.

[andrewla]

The default behavior of png_warning and png_error is just for hackers

If that is the case, then why is it the default? Let hackers set a flag to enable verbose warnings while hacking, and spare the console spam.

But that's a broader issue; this particular warning is very spammy and unactionable. Nothing is ever going to get fixed by this warning because it is an issue with the image file itself, which a consumer has no control over. The behavior here should be inverted; we should only switch this on when requested.

[bghira]

in an effort to improve visibility in this issue, i have made cv2 the default image loading method for simpletuner which has introduced thousands more people to this awesome warning that prints out for, in some cases, absolutely every png image in the dataset. this happens because we train from large read-only sets where we cannot modify the image, and we're not modifying cv2 to disable this warning, and libpng is the one emitting it by default, ultimately. 🥳 🎉

[jbowler]

There are more than enough controls to allow this or, indeed, any warning to be turned off. Indeed this warning has its own run-time png_set_option setting and there is a libpng build time option if you are not using the system libpng (like FireFox).

[bghira]

yes but the pre-packaged options are not doing this. and instead of making it a cv2 issue, we make it a libpng one, since this is the common denominator among all platforms like cv2 that reach into libpng.

[jbowler]

yes but the pre-packaged options are not doing this. and instead of making it a cv2 issue, we make it a libpng one, since this is the common denominator among all platforms like cv2 that reach into libpng.

I doubt that. Well written code handles the warning and error messages in a way appropriate to the application or library. Removing one particular warning message because your image test set has been written with the bogus profile and therefore removing the warning from all other libraries and applications is wrong, particularly as it is a one-line change to the OpenCV source code (png_create_read_struct is only called in one place in the whole code base.)

The OpenCV implementation appears to be around 25 years old. It is a complete hack; it does the absolute minimum error handling (it returns 'false' on an error and that is it). So this problem exists with all error and warning messages; an app programmer cannot handle any of them because the codec does not return them.

It also doesn't do any of the expected things like gamma correction; it doesn't even read gAMA and it certainly does not use iCCP or, indeed, the results of the checks.

If it were to use the simplified API it would get these things for free and the code in OpenCV (which appears to be statically linked into multiple DLLs) would go down to a few lines, but for some reason it is reading eXIf. In any case if it simply listed the ancillary chunks it actually uses (see the code in pngread.c for PNG_SKIP_CHUNKS) then it would get faster and the problem of errors in unused chunks would disappear.

It needs fixing.

[jbowler]

For those who don't know how to find the code in the simplified API do this:

png_set_keep_unknown_chunks(png_ptr, PNG_HANDLE_CHUNK_NEVER, NULL, -1);
png_set_keep_unknown_chunks(png_ptr, PNG_HANDLE_CHUNK_AS_DEFAULT, "eXIf\0", 1);

The code in pngread.c is more portable (it doesn't depend on ASCII) but the above works for most people. To generalise list the chunks you want handled in the second line terminated by "\0" characters and give the number of chunks (sizeof list/5).

The first line causes all the chunks except the PNG (not MNG) critical chunks and tRNS to be skipped - no checks or anything - the second line adds whatever you want back in. You can do the second line multiple times and you can also use the API to skip specific chunks (rather than skipping all of the ancillary chunks then adding things you want back.) See png.h for more description.

Doing this will speed up your PNG reading code, potentially quite a lot if you have big iCCP profiles on small images (since the iCCP check decompresses the data).