Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

colour: use cmsFLAGS_NOOPTIMIZE to keep all precision #3368

Closed
wants to merge 1 commit into from

Conversation

kleisauke
Copy link
Member

@kleisauke kleisauke commented Mar 1, 2023

Resolves: #3150.

Note: this PR targets the 8.14 branch.

@kleisauke
Copy link
Member Author

Using the images from libvips/php-vips#169 and libvips/php-vips#192, I see:

8.14 branch:

Details
$ /usr/bin/time -f %M:%e vips icc_import php-vips-169.jpg t1.v --pcs xyz
55512:0.10
$ /usr/bin/time -f %M:%e vips icc_import php-vips-169.jpg t2.v --pcs lab
56604:0.12
$ /usr/bin/time -f %M:%e vips icc_export t1.v output-xyz.v
36312:0.06
$ /usr/bin/time -f %M:%e vips icc_export t2.v output-lab.v
37084:0.06
$ vips avg output-xyz.v
59.424550
$ vips avg output-lab.v
69.385891

$ /usr/bin/time -f %M:%e vips icc_import php-vips-192.jpg t1.v --pcs xyz
51372:0.10
$ /usr/bin/time -f %M:%e vips icc_import php-vips-192.jpg t2.v --pcs lab
50728:0.11
$ /usr/bin/time -f %M:%e vips icc_export t1.v output-xyz.v
34512:0.06
$ /usr/bin/time -f %M:%e vips icc_export t2.v output-lab.v
35140:0.06
$ vips avg output-xyz.v
60.507960
$ vips avg output-lab.v
67.984691

This PR:

Details
$ /usr/bin/time -f %M:%e vips icc_import php-vips-169.jpg t1.v --pcs xyz
54864:0.12
$ /usr/bin/time -f %M:%e vips icc_import php-vips-169.jpg t2.v --pcs lab
54844:0.16
$ /usr/bin/time -f %M:%e vips icc_export t1.v output-xyz.v
36072:0.08
$ /usr/bin/time -f %M:%e vips icc_export t2.v output-lab.v
39672:0.09
$ vips avg output-xyz.v
69.553215
$ vips avg output-lab.v
69.553283

$ /usr/bin/time -f %M:%e vips icc_import php-vips-192.jpg t1.v --pcs xyz
50316:0.10
$ /usr/bin/time -f %M:%e vips icc_import php-vips-192.jpg t2.v --pcs lab
49556:0.12
$ /usr/bin/time -f %M:%e vips icc_export t1.v output-xyz.v
34524:0.06
$ /usr/bin/time -f %M:%e vips icc_export t2.v output-lab.v
34572:0.07
$ vips avg output-xyz.v
67.716544
$ vips avg output-lab.v
67.715953

So, it produces similar images using a profile connection space with either LAB or XYZ. Previously, it produced a visible colour difference when using XYZ as PCS.

I think the timings are reasonable for these particular images. Note that this flag also disables all optimizations of the fast-float plugin available in LCMS and released under the GPL (though, that also required an explicit opt-in, see e.g. commit kleisauke@a1a122e).

This is ready for review now.

@kleisauke kleisauke marked this pull request as ready for review March 3, 2023 12:16
@lovell
Copy link
Member

lovell commented Mar 3, 2023

Do we know which of the (4?) builtin lcms optimisations is causing this?

https://github.com/mm2/Little-CMS/blob/d0752309ff93f62be246ee9bf18338107f30d485/src/cmsopt.c#L1804-L1807

Could it be something to do with cmsFLAGS_CLUT_PRE_LINEARIZATION (and its post equivalent)?

Should we make the addition of this flag conditional on using XYZ as it suffers more greatly?

@kleisauke
Copy link
Member Author

Good points, I didn't investigate that.

It looks like using the TYPE_Lab_FLT / TYPE_XYZ_FLT APIs would also fix this. See for example this changeset:
8.14...kleisauke:lcms-simplify

With that I see:

$ vips icc_import php-vips-169.jpg t1.v --pcs xyz
$ vips icc_import php-vips-169.jpg t2.v --pcs lab
$ vips icc_export t1.v output-xyz.v
$ vips icc_export t2.v output-lab.v
$ vips avg output-xyz.v
69.553215
$ vips avg output-lab.v
69.553287

And:

$ vips icc_import php-vips-192.jpg t1.v --pcs xyz
$ vips icc_import php-vips-192.jpg t2.v --pcs lab
$ vips icc_export t1.v output-xyz.v
$ vips icc_export t2.v output-lab.v
$ vips avg output-xyz.v
67.716544
$ vips avg output-lab.v
67.716544
$ sha256sum output-{xyz,lab}.v
1bf5a9392fe67f1948302c6d864f180a3d242391a665f829fb61f5af7dddb32b  output-xyz.v
1bf5a9392fe67f1948302c6d864f180a3d242391a665f829fb61f5af7dddb32b  output-lab.v

Though, I had to raise a test threshold for that (see commit kleisauke@5608c82).

@lovell
Copy link
Member

lovell commented Mar 3, 2023

Three builtin optimisations are skipped when using floats, so I guess it was one of these causing the difference.

https://github.com/mm2/Little-CMS/blob/d0752309ff93f62be246ee9bf18338107f30d485/src/cmsopt.c#L661-L662
https://github.com/mm2/Little-CMS/blob/d0752309ff93f62be246ee9bf18338107f30d485/src/cmsopt.c#L1050-L1051
https://github.com/mm2/Little-CMS/blob/d0752309ff93f62be246ee9bf18338107f30d485/src/cmsopt.c#L1391-L1392

If the performance is similar then using floats would probably be simpler overall, plus not prevent optimisations that are possible.

kleisauke added a commit to kleisauke/libvips that referenced this pull request Mar 4, 2023
Avoids the need to pack the buffer of floats into lcms's fixed-point
formats and ensures full precision internally in lcms.

Resolves: libvips#3150.
Supersedes: libvips#3368.
@kleisauke
Copy link
Member Author

Closing in favor of #3373. I confirm the performance on that PR is similar to this PR.

@kleisauke kleisauke closed this Mar 4, 2023
@kleisauke kleisauke deleted the lcms-nooptimize branch March 4, 2023 13:30
kleisauke added a commit that referenced this pull request Mar 13, 2023
Avoids the need to pack the buffer of floats into lcms's fixed-point
formats and ensures full precision internally in lcms.

Resolves: #3150.
Supersedes: #3368.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants