Color conversion with ICC profiles #1567
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems
Vector4.Transformwas performing Vector × Matrix not Matrix × Vector? My matrix maths is rusty, but in lieu of some kind ofMatrix4x4.Transform(matrix, vector)function it sounds like transposing the matrix has the same effect as changing the order of multiplication.This issue wasn't caught yet because none of the ICC conversion tests use a profile of CMYK data ⇒ XYZ PCS. And even if it did, it would only get caught if using a non-identity matrix? As always, I'll be surprised if we can find a profile that meets these conditions, though we can include it in testing if we do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I'm not sure. @saucecontrol could I borrow your brain here please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vector4.Transformis a dot (inner) product, which is commutative between a vector and matrix1.I can work up some profiles to test some of these edge cases if you'd like. I swear one of these days I'm going to get around to finishing my implementation of LUT profile transforms, and I'll need them then anyway.
Footnotes
Well, not in general, but in this case it's a square matrix and a vector with length equal to the length of each matrix dimension, so it is here. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we want a matrix multiplication M×V which isn't commutative, but doing V⋅MT effectively calculates M×V in this particular case.
Is that an OK assumption to make given we're restricted to
Matrix4x4andVector4? Is there a more suitableSystem.Numericsway to do this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't want M×V, you want M⋅V, and I promise it's commutative.
The reason transposing gives you the correct result is that
Vector4.Transformassumes the matrix is row-major, i.e. the V is a 1x4 matrix. Most of ICC docs describe the matrices as column-major, which would mean your V is a 4x1 matrix.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also see from the implementation of
Vector4.Transformwhy it's done row-major -- that enables SIMD acceleration per row because of the memory layout. For perf reasons, you'll want to normalize to row major on the edges of the API.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some of my assumptions have muddied the waters a bit, so I'm going to try to summarise, and if I'm still making a mess I'll have to leave this to someone else.
System.Numericsto know if there's a more conventional way to "achieve the effect of matrix multiplication" than transposing the 4x4 matrix - @JimBobSquarePants perhaps any 4x4 matrices just need to be initialised row-major?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also commutative, as it is also a dot product. The only difference is whether you compute the dot product of the vector with each row of the matrix or with each column of the matrix.
There is no fast way to compute it column-wise, because it means gathering the non-adjacent values in memory. In fact, transposing the matrix with SIMD and then performing a row-wise dot product with SIMD is the fast way to do this, although it is much less fast than normalizing your matrix to row-major when reading the profile and skipping the transpose step during processing.
Exactly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure the maths explanation is clear:
M⋅V where M is 4x4 can be summarized as VMatrix1⋅VColor + VMatrix2⋅VColor + VMatrix3⋅VColor + VMatrix4⋅VColor where VMatrix represents either the rows (VColor is 1x4) or columns (VColor is 4x1) of the matrix.
That result is the same as V⋅M, which would be VColor⋅VMatrix1 + VColor⋅VMatrix2 + VColor⋅VMatrix3 + VColor⋅VMatrix4, because V⋅V is commutative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'm referring to matrix multiplication in general.
I'm not sure how feasible initialising row-major is when we also need the inverse of the matrix for reverse transforms, will see.