WIP Colorspace Transforms #664
Conversation
|
@antonfirsov I've been revisiting this PR and the method you suggested and I cannot see how adding it will give use any advantage. As it stands we loop through the span and perform conversion + adaptation in a single loop. If we introduce that method and attempt to integrate it into a bulk conversion we have to loop through and create the unadapted destination span, then loop through again to create the adapted version. |
There was a problem hiding this comment.
@JimBobSquarePants it will enable bulk variants for converter API-s which could be SIMD-optimized and conditions like this or this are evaluated only once per bulk loop.
whitePoint is invariant (could be readonly) in a ColorSpaceConverter instance.
If we introduce that method and attempt to integrate it into a bulk conversion we have to loop through and create the unadapted destination span, then loop through again to create the adapted version.
I think I fail to understand this. But if you mean that it's an issue to create a temporary buffer to call the bulk variant of IChromaticAdaptation.Adapt(...) before calling bulk conversion on the adapted data -it shouldn't be. A MemoryAllocator could quickly allocate it, and with the bulk optimization, the execution will be faster.
Nevertheless, it looks like my main findigs are addressed now. The bulk adaptation API could be added later, we just need to make sure it's possible to do it without breaking changes.
| private CieXyzAndLmsConverter cachedCieXyzAndLmsConverter; | ||
| // Options. | ||
| private Matrix4x4 lmsAdaptationMatrix; | ||
| private CieXyz whitePoint; |
There was a problem hiding this comment.
All white point members could be readonly!
There was a problem hiding this comment.
Thought I'd got these... Will update.
@antonfirsov What I mean it that for many, if not most conversions, the Adapt method comes after the initial conversion which takes place inside the loop. In pseudo code. (for var i = 0 etc){
var d = ToDWithAdapt(source[i]);
}In order to do the actual optimization we'd have to stop calling the single methods and inline their code. That dramatically decreases our code reuse and increases the tests we need to write. (I already have to write so, so many conversion tests). Possible of course but so much work... Once inlined I don't think we'll need an allocater so I'll add the method to the interface now and we can add a chore optimization task to the tracker. |
|
I guess we mean the same: Initial conversion could be done in a bulk way into a temporary buffer. With |
…crease tolerance because of failing test case
|
@JimBobSquarePants hope you are happy with the changes I just pushed. |
| @@ -14,7 +14,7 @@ namespace SixLabors.ImageSharp.ColorSpaces.Conversion.Implementation.RgbColorSap | |||
| /// <see href="http://www.brucelindbloom.com/index.html?Eqn_RGB_to_XYZ.html"/> | |||
| /// <see href="http://www.brucelindbloom.com/index.html?Eqn_XYZ_to_RGB.html"/> | |||
| /// </remarks> | |||
| internal class SRgbCompanding : ICompanding | |||
| public sealed class SRgbCompanding : ICompanding | |||
There was a problem hiding this comment.
We have the same code in Vector4Extensions. Might make sense to have a unified & expressive common API.
| { | ||
| /// <summary> | ||
| /// Pair of companding functions for <see cref="RgbWorkingSpace"/>. | ||
| /// Used for conversion to <see cref="CieXyz"/> and backwards. | ||
| /// See also: <seealso cref="RgbWorkingSpace.Companding"/> | ||
| /// </summary> | ||
| internal interface ICompanding | ||
| public interface ICompanding |
There was a problem hiding this comment.
I wonder why do we have interfaces which are not used through the abstraction. I think they shouldn't be there, and the methods should be moved to static utilities. If they were being actually used, it would be very expensive to do non-inlinable virtual interface calls to convert only one float value.
There was a problem hiding this comment.
I agree. I’m glad you brought this up as they’ve never sat well with me. Will do that tomorrow
|
Right... let’s get this merged. |
Prerequisites
Description
I've been slowly but surely refactoring the colorspace transformation code to make it usable and performant. This PR reintroduces the transform code into the codebase.
Span<T>based bulk transforms.