libvips / php-vips

php binding for libvips
MIT License
606 stars 24 forks source link

Color distortion when using the thumbnail function with a linear parameter #169

Closed romanzks closed 1 year ago

romanzks commented 1 year ago

I noticed a strange image color distortion when using the thumbnail function with a linear parameter for image with a color profile.

In the examples below, I don't change the size of the image on purpose, as it makes it easier to see color differences. The bug is reproduced anyway.

input input.jpg - original image


$image = Vips\Image::thumbnail('input.jpg', 1920, [
    'linear' => true,
]);
$image->writeToFile('output.jpg');

output output.jpg


Removing the color profile does not solve the issue. The colors are slightly different from the previous version, but still distorted.

$image = Vips\Image::thumbnail('input.jpg', 1920, [
    'export_profile' => Vips\Interpretation::SRGB,
    'linear' => true,
]);
$image->writeToFile('output2.jpg', [
    'strip' => true,
]);

output output2.jpg


However, if I first remove the color profile, save the image, and apply the linear parameter to a new image, no color distortion occurs. The following example keeps the original colors.

$image = Vips\Image::thumbnail('input.jpg', 1920, [
    'export_profile' => Vips\Interpretation::SRGB,
]);
$image->writeToFile('output3.jpg', [
    'strip' => true,
]);

$image = Vips\Image::thumbnail('output3.jpg', 1920, [
    'linear' => true,
]);
$image->writeToFile('output4.jpg');

Unfortunately this is not an acceptable solution, because I do a double image conversion what affects the quality of the image and the speed of work.


jcupitt commented 1 year ago

Hi @romanzks, thanks for the report!

I was able to reproduce this with:

vips icc_import input.jpg t1.v --pcs xyz
vips icc_export t1.v output.jpg

But this works:

vips icc_import input.jpg t1.v
vips icc_export t1.v output.jpg

So it looks like it's a bug in ICC transforms with XYZ PCS. I'll open an issue on libvips.

jcupitt commented 1 year ago

.... I'll close this issue, let's discuss on the libvips one.

kleisauke commented 1 year ago

FWIW, I just created a standalone reproducer in PHP for this issue, see: https://gist.github.com/kleisauke/34ce2bf6fad1841296eab5316573b69d

The following response addresses the questions and API usage mentioned in https://github.com/libvips/libvips/issues/3159.

I couldn't find how to remove the metadata without saving the image or overwriting the memory. Maybe there is something like this?

I see, I thought you were using that writeToMemory()/newFromMemory() construction only for performance reasons. To remove a ICC profile from an image, you can use this instead:

if ($image->getType('icc-profile-data') != 0) {
    // Removing metadata, need to copy the image
    $image = $image->copy();

    // Remove the ICC profile
    $image->remove('icc-profile-data');
}
# it takes 41.83 seconds

Are you sure this script take 41 seconds? I guess it's around 4 seconds if you use random access. Opening the image with the sequential access hint should probably fix this performance degradation.

$image = $image->icc_transform(Vips\Interpretation::SRGB);

Since you are using the CYMK image in the linked issue, this call should set the optional embedded argument to true to ensure that the embedded ICC profile is transformed, rather than the built-in CMYK profile of libvips.

$image = $image->icc_import();
$image = $image->thumbnail_image(400);
$image = $image->icc_export();

Ditto, the icc_import() call does not use the optional embedded argument. Also, it seems that the icc_export() call is redundant here because the output_profile argument is not set and you don't use the optional export_profile argument of the thumbnail operation.

Note that the same ICC operations are also used when thumbnailing with the linear argument, so it's unlikely that this workaround would resolve this.

romanzks commented 1 year ago

@kleisauke Thanks for the clarification and help in resolving this issue. The more I research this topic, the more strange things I come across. Anyway, right now this is the best option for me:

$image = Vips\Image::newFromFile('input.jpg');
$image = $image->icc_transform(Vips\Interpretation::SRGB);
$image = Vips\Image::newFromMemory(
    $image->writeToMemory(), $image->width, $image->height, $image->bands, $image->format
);
$image = $image->thumbnail_image(400);
$image->writeToFile('output.jpg', ['Q' => 90, 'strip' => true]);

It provides good speed and avoids color distortion on all my test images. However, I noticed a strange thing. The first few runs of the script with a new test image show the worst time, then the conversion time becomes several times better. As if some kind of cache is starting to work. Maybe it's related to my local environment, although I can't imagine how that could be yet. I am using OPcache and JIT compiler. This is just for information 🙂

romanzks commented 1 year ago

@jcupitt yeah, it works, and it's faster than writing/reading from memory.

$image->remove('icc-profile-data');

Hmmm, although on large images it is still much faster to write and read from memory... 🤷

kleisauke commented 1 year ago

@romanzks Sorry, as mentioned earlier, I can't recommend your workaround. It's also a bit different behavior than what was originally posted in this issue, since you are no longer processing in linear space.

The first few runs of the script with a new test image show the worst time, then the conversion time becomes several times better.

This is probably due to libvips' operation cache, which I disabled in the above mentioned gist.

although on large images it is still much faster to write and read from memory.

As mentioned earlier, I think this can be fixed with opening the image with the sequential access hint. See the above mentioned gist for reference.