Avoid warning from imagecreatefrompng when loading png with obscure iCCP profiles

@someshver

Were you able to fix this?
I still occasionally come across this message ever since I upgraded to php 8.1

Thanks.

This is a patch to core

Upload an image for reproducing this warning. Meanwhile, I think this should be a core issue. But not sure if it's good to use `@` to ignore warnings in core completely.

I'm getting this on php 8 and most recent upgrade to Drupal core 9.3.12

Glad to see I'm not the only one apparently getting this issue. It was hard for me to reproduce (and unlike mentioned above, I wasn't uploading images, though my site is on php 8.0.16), but seems to perhaps happen when certain image style files are being generated (perusing the pages of /admin/content/media on my local build with stage_file_proxy enabled was a good way to generate more instances of the warning).

I'm additionally getting a few instances of Warning: imagecreatefrompng(): gd-png: libpng warning: Interlace handling should be turned on when using png_read_image in Drupal\system\Plugin\ImageToolkit\GDToolkit->load() (line 203 of /var/www/web/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php), which I don't know if that should be its own ticket, but I'm also not entirely sure how to characterize one warning vs. the other. Might not be a hard-and-fast rule, but the latter warning seems to be limited to some (but not all) .png files that clearly started their life as .jpgs as well as some the "generic media" icons, e.g. /sites/default/files/styles/thumbnail/public/media-icons/generic/audio.png.

I also got this warning "Warning: imagecreatefrompng(): gd-png: libpng warning: iCCP: known incorrect sRGB profile in Drupal\system\Plugin\ImageToolkit\GDToolkit->load() (line 201 of core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php)." when i was uploading media as a document in pdf form in version 9.5x , so i have created a patch to fix this warning for version 9.5x,, kindly review patch.

Exact same problem here as in #8. Warning is from uploading a pdf document as a media item.

I can confirm this issue on PHP 7.4, latest Drupal 9 and latest libpng. Check out https://stackoverflow.com/questions/22745076/libpng-warning-iccp-known-i... for more details. The patch resolves the warning.

I'm running into this issue as soon as I view the media library widget in node/x/edit.

We were encountering the same error on one of our websites. This was happening on the LIVE site, without stage_file_proxy running.

I tested the patch in comment 8 on a local build, and the warnings stopped.
- drupal core 9.3.22
- php 7.4
- stage_file_proxy enabled

I tested by running the site without, and with all image style derivatives removed. I then loaded pages with images that generated the errors. I could reproduce the errors by deleting all image style derivatives and reloading the pages.

Next I applied the patch and retested with the process above. NO errors were produced.

I've noted the files affected and will test on our production server next.

I can confirm that #8 removes the warning for me.

Drupal version: 9.4.8
PHP version: 8.0.23

I Applied Patch #8 and it working successfully removes the warning for me on Drupal 10.0.x. Adding screenshots to refer after and before patch.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Could you please provide steps/configuration taken to recreate this.
This will need a test case showing the issue.

Thanks

I was able to recreate this by uploading a .webp image into an image/media reference field, and then rendering it using an image style.

I was able to extract the broken color profile from the image and apply it to another one and trigger that error in tests. The broken color profile called sRGB IEC61966-2.1. I'm attaching it as well, so you can test it by yourself on any image (by injecting it using CLI tools or image editors like GIMP, Krita, etc).

I'm also switch versions to 10.1.x, because patch from #8 won’t apply on 10.0.x branch. And I focus initially on future branch, if it's needed it can be backported later on.

Updated issue summary with steps to reproduce this problem.

Added cspell ignore for iCCP

Sorry for noise, patch in #20 contains .icc profile by accident. This one is clean patches.

Gosh 🤯

Anyway, since I need to create patches again, I generated a test image using convert instead of GIMP. It is smaller, because there is no GIMP metadata in it.

🤞

Personally, I would not create tests for suppression operators. Such tests are too tricky.
Also, I think it needs a comment explaining what kind of errors could happen without the suppression.

Added comment for suppression call.

Disabled tests, because the only difference from #22 is comment.

Re-rolled patch #3 to make it work with 9.5.5

#26 works! no more warning on d 9.5.5/php 8.1.1
thank you!

The issue title is used as the commit message and it should state what this issue resolves instead of being a long error message.

Do we really want to suppress all warnings?

Backporting #8 to D7

Better title.

Do we really want to suppress all warnings?

I had the same question, but I'm not sure what options we're really given. I suspect this is probably OK though because real problems will throw and exception which gets caught and logged. At least in the 10.x versions of this patch.

That said, the block we're adjusting is generalized for all image formats but the comment clearly only addresses the impact on PNGs. Very bikesheddy but maybe we should probably adjust the documentation to match the scope of the code not the scope of this issue. Something like this?

      // Suppress warnings from image creation because there is a possibility
      // that source images contain data that would trigger non-fatal warnings
      // we can't handle. For example, a PNG saved with 'sRGB IEC61966-2.1'
      // color profile has 'iCCP' chunk data which is not recognized on most
      // systems and will cause libpng to trigger warnings but otherwise do not
      // affect image processing.

Looking at #25 and should the test go into ToolkitGdTest?

Also reading the last comment 31 think expanding to match makes sense. Least I can't think of a reason not to.

Rerolled the last patch against 11.x.

Rerolled the last patch against 10.1.2

I confirm the patch in 34 works for Drupal 10.

Based on the comment from #35, I'm changing the status to RTBC.

Based on the rest of the comments and the lack of tests don't think this is actually ready. There aren't even tests on the last couple patches.

re: #32 what the heck is that test. Its not in the system module, its in the wrong location based on the namespace of the class, the file name doesn't align with the tested class? Something weird going on there but it does look like that's where existing tests are.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

1 small feedback item.

Hiding patches for clarity.

@smustgrave I disagree, the dictionary already has all the wierd words ...

Which is actively trying to be cleaned up

Question: is it possible to resolve threads ?

There was 1 error:
1) Drupal\KernelTests\Core\Image\ToolkitGdTest::testIncorrectIccpSrgbProfile with data set "PNG with iCCP profile" ('core/tests/fixtures/files/ima...le.png')
PHPUnit\Framework\Exception: PHP Warning:  imagecreatefrompng(): gd-png: libpng warning: iCCP: known incorrect sRGB profile in /builds/issue/drupal-3261924/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php on line 271
Warning: imagecreatefrompng(): gd-png: libpng warning: iCCP: known incorrect sRGB profile in /builds/issue/drupal-3261924/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php on line 271
/builds/issue/drupal-3261924/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3261924/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3261924/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/issue/drupal-3261924/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/builds/issue/drupal-3261924/vendor/phpunit/phpunit/src/TextUI/Command.php:97
ERRORS!
Tests: 97, Assertions: 1197, Errors: 1.

Verified test coverage using test-only feature.

All feedback has been addressed.

@Taran2L as far as threads, as the opener of the thread I use to be able to close myself but that no longer seems to be the case. Think only the person who opened the MR can resolve threads now.

I'm triaging RTBC issues. I read the IS, the comments and the MR. Thanks for the issue summary. As someone who doesn't work with images it was very clear and easy to understand the problem. I didn't find any unanswered questions.

I read the MR (not a code review) and there is minor work to do on the comments.

Oh sorry, @neclimdul, thank you for answering my question in #31.

Rerolled patch from #34 for Drupal 10.2

Feedback has been addressed

Also, attaching a static patch from the latest changes in the MR (for composer patching purposes)

MR appears to have failures.

Feedback appears to have been addressed

Attaching static version of MR for composer patch.

The test doesn't work for me locally. With or without the change to GDToolkit I get:

Testing Drupal\KernelTests\Core\Image\ToolkitGdTest
..E                                                                 3 / 3 (100%)

Time: 00:03.934, Memory: 4.00 MB

There was 1 error:

1) Drupal\KernelTests\Core\Image\ToolkitGdTest::testIncorrectIccpSrgbProfile with data set "PNG with iCCP profile" ('core/tests/fixtures/files/ima...le.png')
PHPUnit\Framework\Exception: libpng warning: iCCP: known incorrect sRGB profile

Not entirely convinced we need the test here, we are only testing PHP's error suppression. The comment is probably good enough?

@longwave this is very weird - it does not work on my local as well ... but it seems it does work on CI, weird

I think the value of the test is that error is being suppressed.

Hello again. I see some folks not very happy with silencing all the errors from that call. I understand that as well, it can be helpful sometimes. Without it, I won't be found this issue as well. Maybe we wrap this call $image = @$function($this->getSource()); by a condition?

We have already configuration `system.logging` with `error_level` setting. Maybe we will do it like:

if ($error_level === 'verbose') {
  $image = $function($this->getSource());
}
else {
  $image = @$function($this->getSource());
}

This allows a developer to find out that there is a problem, but it won't be logged and showed on production instances. It might be looking ugly, but it solves most of the concerns here. Currently, as I remember correctly (I've already fixed that images), this will be logged on production on every over page with a broken image when it's processed for the first time (if no style yet created). In some cases, it can create a lot of useless noise in the logging system.

It IS possible to handle warnings, and therefore forego suppressing them all, but it would require setting up an error handler to do it. That said, if we are worried about particular image profiles, is there a way to whitelist some and check to see if the image is what we want? Is that too process intensive?

Trying to throw some fresh ideas at this that I didn't see in the comments. I do like Niklan's thoughts though.

I think the concerns might be a bit overstated. Actual problems will trigger real exceptions that get logged. This just avoids warnings that shouldn't be triggered. If I remember correctly they're warnings triggered by the underlying libraries so they don't actually follow PHP's documentation for the method.

Resolved MR conflict and added patches for D10 and D11.

Just curious about Patch 65. What's all the garbage text at the end of the file?

That's a new file... png based on the diff.

diff --git a/core/tests/fixtures/files/image-test-iccp-profile.png b/core/tests/fixtures/files/image-test-iccp-profile.png
new file mode 100644
index 0000000000000000000000000000000000000000..29cb8945f8e4c1ae40de24bf888715e4d870a164
--- /dev/null
+++ b/core/tests/fixtures/files/image-test-iccp-profile.png
@@ -0,0 +1,24 @@
+‰PNG

Pull the latest updates into MR.

I encountered the same issue again, and there was a lot of noise in the logs about it. If you just want to fix images and avoid patching the Drupal core, you can use the following script (adapt it to your needs):

#!/usr/bin/env bash
# Fixes iCCP: known incorrect sRGB profile
cd web/sites/default/files || exit
find . -type f -name '*.png' -exec mogrify \{\} \;

This tool will go through all your PNG images and re-save them with fixes for any issues found. However, it fixes more than just the iCCP profile, so use it with caution. I haven't faced any issues yet.

If you know exactly which images have problems, it's best to run the command directly on those files, like this (you can play with broken files from that issue):

mogrify filename.png

I recommend testing this tool locally before applying it to your live files. Alternatively, you could run the entire process locally and then sync the changes using rsync.

Please note that this command may take some time to complete, because it will open and save each image, regardless of whether it has any issues or not. The more and bigger in size files you have, the slower the process will be.

It utilizes the Mogrify binary provided with the ImageMagick package.

10.3 update

Fixes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

Have not reviewed but MR appears to have errors.

Marked this ticket as possibly related: https://www.drupal.org/project/drupal/issues/3424941#comment-15760118

Confirming this issue still exists and I agree muting it would make sense, as users can't do anything about it in most cases.

Appears to have test failures.

There is one more warning occurring in image system https://www.drupal.org/project/drupal/issues/3474274

I am also getting error during test run locally

PHPUnit\Framework\Exception: PHP Deprecated: image_type_to_extension(): Passing null to parameter #1 ($image_type) of type int is deprecated in web/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Resize.php on line 63

Green again, I think this can go in ?

Ran test-only feature

1) Drupal\KernelTests\Core\Image\ToolkitGdTest::testIncorrectIccpSrgbProfile with data set "PNG with iCCP profile" ('core/tests/fixtures/files/ima...le.png')
PHPUnit\Framework\Exception: PHP Warning:  imagecreatefrompng(): gd-png: libpng warning: iCCP: known incorrect sRGB profile in /builds/issue/drupal-3261924/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php on line 194
Warning: imagecreatefrompng(): gd-png: libpng warning: iCCP: known incorrect sRGB profile in /builds/issue/drupal-3261924/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php on line 194
ERRORS!
Tests: 96, Assertions: 1185, Errors: 1.
Exiting with EXIT_CODE=2

Shows current coverage.

Hiding the patches so it's clear just the MR, and resolved all threads except the one. Since we are suppressing with '@' not sure a CR would be needed? Will let committer decide that one.

But rest of the feedback appears to be addressed.

When will merged?

+1

patch in #72 no longer works in D10.4.1

MR looks good to me, but unfortunately needs a rebase.

Merged in the latest from origin.

Rebase seems fine, random javascript error.

Committed/pushed to 11.x, thanks!

Did my best with commit credit but this is a long issue with a lot of commenters on it so apologies if someone was overlooked.