Add Transform ICC Profile API

[AttilaTheFun]

I previously asked about adding support for transforming images between ICC profiles and it was suggested that suggested I make a pull request. I was able to get the vips_icc_transform function working with an ICC profile that I supplied externally. If you have a Mac, the built-in color profiles are located at /System/Library/ColorSync/Profiles/.

I was able to take an image with an embedded Display P3 color profile and transform it to both an sRGB version and a Display P3 version without breaking it.

Please let me know what you think and if you have any feedback!

[coveralls]

Coverage increased (+0.01%) to 76.513% when pulling 4fcb54f on AttilaTheFun:master into 6433b63 on davidbyttow:master.

[tonimelisma]

Hey @AttilaTheFun thanks so much for the PR. Looks good generally. Could we add just a little bit of error handling in the Go function, e.g. to ensure the file exists and is readable? Pure libvips errors aren't as well handled by govips.

Also, can you add unit tests and golden image tests for the provided functions? It would be ideal if we could keep test coverage of 80% (it's already dipped a bit below).

In any case, great work, thanks so much for the contribution!

[AttilaTheFun]

Hi @tonimelisma , sure thing! Are there instructions for adding additional test cases? Also are there any instructions for building from source? I can run make test but are there other options?

[tonimelisma]

Just check the existing unit tests and golden image tests for the previous ICC tests. Be sure to check for both positive as well as negative cases

[tonimelisma]

For building govips, make test is all that's needed

[tonimelisma]

One more thing - let's try and be cross-platform here if possible. You can see OS detection code in the golden image tests. I'd recommend using something like that. It's fine if you don't implement the logic on all OSs, just have a nice way to fail safely.

[AttilaTheFun]

Sounds good - I'm traveling this week but I'll get to that when I'm back.

[AttilaTheFun]

Hey @tonimelisma! Sorry this fell off of my radar and I'm picking it back up. Since I wrote this PR, this commit landed which added almost all of the functionality I needed: 6ba1ed3

The only missing piece is exposing the TransformICCProfile function as a public method on the vips image type. Since this change is purely additive and doesn't require any changes to existing code, hopefully it should be easier to land. Could you take a look and let me know what you think?

[tonimelisma]

Hey @AttilaTheFun thanks for the update! I think it looks fine. Not sure about the ergonomics - do we want to really supply a file path or should govips offer the embedded, built-in ICC profiles..?

Nah, I suppose this is a good approach. Can you also add unit tests? Good to merge after that I think.

[AttilaTheFun]

@tonimelisma Sure - could you point me at some of the existing unit tests that I could use as a reference? What are you thinking for a test? Maybe take a test image with a known color profile and convert it to another (e.g. Display P3 -> sRGB)? And how would we validate it's successful? Should we compare the hashes of the produced images or go byte by byte? Thanks!

[tonimelisma]

Hey @AttilaTheFun, unit tests are in the _test.go files always respective to the file with the functionality, so unit tests for image.go are in image_test.go. Most of them are pretty simple, just test positive (no error) as well as most typical negative scenarios (make sure you can reproduce all errors). Also check image_golden_test.go which tests for entire images being successfully transformed, it should be quite self-explanatory. The golden tests verify against a known good image - so no need to do hashing or byte-by-byte matching, a golden reference image is good enough. Running the tests produces the references images.

[AttilaTheFun]

Okay @tonimelisma, I added two golden tests, transforming an image without an ICC profile and transforming an image with an Adobe RGB profile both to sRGB IEC61966-2.1, as well as a unit test to ensure it doesn't produce an error. This is comparable to the test coverage of the OptimizeICCProfile method which calls the same underlying API albeit with different parameters. Is this now good to merge?

[tonimelisma]

Thanks for the new commit! However, the unit tests aren't working on the CI. If you check above, you should see the error log. Looks like it's not finding the profile.

[AttilaTheFun]

Ah @tonimelisma - I've seen this issue before. Apparently the core vips binaries have different behavior on macOS and linux when calling vips_icc_transform with images without an embedded profile. On linux, it throws an error if you don't supply an input profile that it can fall back on, while macOS just assumes it's standard RGB. I updated the function to pass an sRGB input profile that the function can fall back on when the input image is missing an embedded profile. Hopefully this fixes the CI issue.

[AttilaTheFun]

@tonimelisma Would you mind kicking off the CI workflow again so we can see if it passes?

[AttilaTheFun]

Hmm @tonimelisma so my tests passed this time:

=== RUN   TestImage_TransformICCProfile_RGB_No_Profile
...
--- PASS: TestImage_TransformICCProfile_RGB_No_Profile (0.03s)

=== RUN   TestImage_TransformICCProfile_RGB_Embedded
...
--- PASS: TestImage_TransformICCProfile_RGB_Embedded (0.15s)

=== RUN   TestImageRef_TransformICCProfile
...
--- PASS: TestImageRef_TransformICCProfile (0.01s)

But several other unrelated tests are failing:

=== RUN   TestImage_OptimizeICCProfile_CMYK
...
--- FAIL: TestImage_OptimizeICCProfile_CMYK (0.17s)

=== RUN   TestImage_OptimizeICCProfile_RGB_Embedded
...
--- FAIL: TestImage_OptimizeICCProfile_RGB_Embedded (0.16s)

=== RUN   TestImageRef_Orientation_Issue
...
--- FAIL: TestImageRef_Orientation_Issue (1.31s)

=== RUN   TestImage_AutoRotate_1
...
--- FAIL: TestImage_AutoRotate_1 (0.17s)

=== RUN   TestImage_AutoRotate_6
...
--- FAIL: TestImage_AutoRotate_6 (0.38s)

=== RUN   TestImage_AutoRotate_6__jpeg_to_webp
...
--- FAIL: TestImage_AutoRotate_6__jpeg_to_webp (1.18s)

=== RUN   TestImage_AutoRotate_6__heic_to_jpg
...
--- FAIL: TestImage_AutoRotate_6__heic_to_jpg (1.03s)

=== RUN   TestImage_Modulate
...
--- FAIL: TestImage_Modulate (0.67s)

=== RUN   TestImage_ModulateHSV
...
--- FAIL: TestImage_ModulateHSV (0.30s)

=== RUN   TestImage_ExtractArea
...
--- FAIL: TestImage_ExtractArea (0.08s)

=== RUN   TestImage_Rotate
...
--- FAIL: TestImage_Rotate (0.17s)

Note that none of these failing tests cover code modified by my PR. Could you verify that these tests are also failing in master, and assuming that's the case, can we go ahead and merge this?

It's also interesting that all of these tests pass on Ubuntu 20 but fail on macOS 11. I guess there must be some differences in the binaries implementations on macOS and Ubuntu.

[AttilaTheFun]

Hey @tonimelisma! Hope you had a nice holiday! I was wondering what I should do about these unrelated failing tests. Are they failing on master too? Thanks!

[tonimelisma]

Hey @AttilaTheFun David fixed the master branch unit tests. Can you rebase? Ping me when you've done that, I'll re-run the CI, it should pass now. Apologies for the back and forth and thanks again for the contribution!

[AttilaTheFun]

No worries @tonimelisma! I just rebased and pushed - can you kick off CI again? Thanks!

[AttilaTheFun]

Awesome, thanks @tonimelisma! How does the release schedule work? Do you know when this change will go out in a tagged release? (I just want to make myself a reminder to change the version from a commit hash to an actual version number.)

[tonimelisma]

Just pushed a new release! Have a happy new year! <3