I don't have any idea about IM. I've run unit tests. Getting following error for all provider for ::testKeepColorProfileStripMeta()

identify: unknown image property "%[profile:icc]" @ warning/property.c/InterpretImageProperties/4238.

It's no help with the todos (sorry about that) but I was just thinking if we maybe could change the driver name to "imagick" for this and keep the old "im" driver? Then it wouldn't be a breaking change, but more important: the preferred name would be a lot better.

• Help needed: Cannot figure out why ICC profile isn't preserved

I have some doubts about calling stripImage() together with profileImage().

A short test for demonstration (not a solution):

function doTest(string $file, string $ext) {
    $image = new Imagick($file);

    // get the ICC profile before stripping
    $profiles = $image->getImageProfiles('icc', true);

    // strip all metadata
    $image->stripImage();

    // re-apply the ICC profile
    if ($icc = $profiles['icc'] ?? null) {
        $image->profileImage('icc', $icc);
    }

    // save the image
    file_put_contents('./output-wrong.' . $ext, $image);
    // destroy the image
    $image->destroy();


    // re-open the output image and apply the ICC profile without calling stripImage
    $image = new Imagick('./output-wrong.' . $ext);
    if ($icc = $profiles['icc'] ?? null) {
        $image->profileImage('icc', $icc);
    }

    // save the image
    file_put_contents('./output-correct.' . $ext, $image);
    // destroy the image
    $image->destroy();
}

doTest('./png-adobe-rgb-gps.png', 'png');
doTest('./onigiri-adobe-rgb-gps.jpg', 'jpg');

Execute this in a folder with the png-adobe-rgb-gps.png and onigiri-adobe-rgb-gps.jpg images. You should end up with two sets of output-wrong.* a output-correct.* files. The "output-wrong" files do what you're doing in Kirby, the "output-correct" files are made by opening the "output-wrong" files and only re-adding the ICC profile.

The ICC profiles being present in:

I know the top comment in the PHP guide about stripImage() suggests doing exactly this, but it doesn't seem to work, at least not for PNG files.

Imho this looks like a bug in either Imagick or ImageMagick, because other file formats seem to work ootb.

@rasteiner Thanks for sharing your insights, much appreciated.

So to make our use case work (keep color profile but strip all other metadata) we would have to write the thumb basically twice - at least for PNG.

So to make our use case work (keep color profile but strip all other metadata) we would have to write the thumb basically twice - at least for PNG.

I don't know if that can be a valid workaround, encoding the image twice surely isn't very optimal for a CMS...

FYI this seems to be an issue in ImageMagick, not the imagick extension. The same behaviour issue also shows with the command line:

identify -format %[profile:icc]\\n png-adobe-rgb-gps.png # --> Adobe RGB (1998)
convert png-adobe-rgb-gps.png extracted.icc
convert png-adobe-rgb-gps.png -strip -profile extracted.icc clean.png
identify -format %[profile:icc]\\n clean.png # --> unknown image property "%[profile:icc]"

I've also posted this here:
https://github.com/ImageMagick/ImageMagick/discussions/7944

Anyway, if it's really a bug in imagemagick, I don't know if it can be fixed in a useful timespan since it's 2 steps upstream (Kirby -> Imagick -> Magick C API) which is then compiled who knows when by unknown hosting providers :/

So unless we find a better workaround (maybe the discussion in ImageMagick leads to something), maybe encoding it twice might actually be the only solution, in that case we should only do it when writing a png file with a profile: as converting a PNG with profile to JPG/WEBP seems to work fine. Saving to avif also seems to skip the profile, but I'm not sure if that's the same problem

@rasteiner yea I guess we cannot expect a fix there but have to work around it ourselves. @afbora has pushed a working version that avoids writing to the filesystem but making use of getImageBloc/readImageBlob.

@rasteiner Seems like I eventually found a way with mixing ::removeImageProfile() and ::deleteImageProperty().

@distantnative I didn't even consider not using stripImage!
At this point I'm wondering if it makes sense to even care about the color profiles at all or just do the ::deleteImageProperty() part: I mean the "sensistive information part" would be in there, and do we actually want to remove any kind of color profile?

@rasteiner Could be - I honestly don't feel that versed with what Imagick/ImageMagick considers to be part of ::getImageProfile() to know if not stripping parts of this could have a negative side effect.

@distantnative sure, me neither; imagick and imagemagick has always been a bit arcane.

...And you're probably right: the "profiles" seem to actually include most informations (not just color profiles), at least if we want to believe wordpress:

	/*
	 * Protect a few profiles from being stripped for the following reasons:
	 *
	 * - icc:  Color profile information
	 * - icm:  Color profile information
	 * - iptc: Copyright data
	 * - exif: Orientation data
	 * - xmp:  Rights usage data
	 */
	$protected_profiles = array(
		'icc',
		'icm',
		'iptc',
		'exif',
		'xmp',
	);

	try {
		// Strip profiles.
		foreach ( $this->image->getImageProfiles( '*', true ) as $key => $value ) {
			if ( ! in_array( $key, $protected_profiles, true ) ) {
				$this->image->removeImageProfile( $key );
			}
		}
	} catch ( Exception $e ) {
		return new WP_Error( 'image_strip_meta_error', $e->getMessage() );
	}

Maybe we could have something similar where "protected_profiles" in Kirby would be a public static, containing by default ['icc', 'icm']. So that if someone wants to preserve copyright infos they could have a little plugin with

// keep copyright - lightroom seems to put the same copyright info both here and in xmp
Imagick::$protected_profiles[] = 'iptc';

Which depending on the situation could either be important to keep or sensitive infomation (lie someone's name) to remove.

@rasteiner I like that. Added it as suggested.

Couldn't we add the profiles as a config option for the driver?

@bastianallgeier changed it

The code looks really good to me know, but I've just tested it in the demokit environment of the sandbox and cropping and resizing leads to stretched images in my case.

imagick

gd

@bastianallgeier Does it work for you as well with the fix I pushed?

Cropping and resizing works great now. There seems to be an issue with the focus point though. If I set the focus point right in the center of the banana product in the shop, I get this …

gd

imagick

Focus point support works like a charm now.

@distantnative do you think it's solid enough? Since it's a secondary option so far, I would give it a go if you agree.

@bastianallgeier I think so. I cannot rule out that there are some use cases where we still have a bug, but we'll find these out probably best with users testing it in there setups. As fixing it on there end is just resetting the config option for now, I think that's an acceptable option.