Repository navigation
Fix Color Filters - #806
Conversation
|
Starting to get really annoyed by tests randomly failing in different environments. The NET Core tests pass on my machine. |
Codecov Report
@@ Coverage Diff @@
## master #806 +/- ##
==========================================
+ Coverage 88.79% 88.83% +0.04%
==========================================
Files 1012 1014 +2
Lines 43313 43703 +390
Branches 3124 3124
==========================================
+ Hits 38459 38824 +365
- Misses 4156 4178 +22
- Partials 698 701 +3
Continue to review full report at Codecov.
|
antonfirsov
left a comment
antonfirsov
left a comment
There was a problem hiding this comment.
Good job! A few imprtant remarks:
- There are issues with math correctness for certain (unused!)
Matrix5x4operations. It's easy to solve them by simply dropping them :) - We should consider moving
Matrix5x4to SixLabors.Core (not necessarily in this PR) - We should probably add tests based on images in #802. For my naive eyes they look like important special cases.
| /// A structure encapsulating a 5x4 matrix. | ||
| /// </summary> | ||
| [StructLayout(LayoutKind.Sequential)] | ||
| public struct Matrix5x4 : IEquatable<Matrix5x4> |
There was a problem hiding this comment.
Shouldn't we move this to SixLabors.Core?
There was a problem hiding this comment.
I don't think it has any practical application outside of ImageSharp.
| /// <summary> | ||
| /// Gets the multiplicative identity matrix. | ||
| /// </summary> | ||
| public static Matrix5x4 Identity { get; } = |
There was a problem hiding this comment.
The term identity matrix is only valid for square matrices! We should drop this.
| { | ||
| #if NETCOREAPP2_1 | ||
| Matrix5x4 m = KnownFilterMatrices.CreateBrightnessFilter(.5F); | ||
| HashCode hash = default; |
There was a problem hiding this comment.
This test depends too strongly on the current implementation. IMO a unit test for GetHashCode() should be only responsible for ensuring that for equal objects the hash code is also equal. (The opposite is not necessarily true!)
There was a problem hiding this comment.
I just copied what they do with Numerics tests. I think it still adds value as it ensures we touch every field when calculating the hashcode
There was a problem hiding this comment.
Ok, let's keep it as is for now!
| /// <param name="value1">The first source matrix.</param> | ||
| /// <param name="value2">The second source matrix.</param> | ||
| /// <returns>The result of the multiplication.</returns> | ||
| public static Matrix5x4 operator *(Matrix5x4 value1, Matrix5x4 value2) |
There was a problem hiding this comment.
The result of this multiplication should be either a 5x5 or a 4x4 matrix, depending on how we define the operation.
As for non-square fixed size matrices the multiplication has usually no practical use, I suggest to drop this operation.
There was a problem hiding this comment.
As you touched upon below this does make sense if our struct is an optimized form of a Matrix5x5 in the manner that Matrix3x2 is an optimized Matrix3x3 which it is.
Most importantly it works!
There was a problem hiding this comment.
Missed that usage, thanks for pointing it out!
|
Re
|
I did consider that but it only really adds regression protection which we already have covered with our sample files. I could replace the samples though.
This did cross my mind also, I just like the conformity of naming it currently has with Numerics. It's a coin toss. |
If we keep the class within the bounds of ImageSharp, and only use it only for colorspace transformations, it makes the most sense to name it (The rest of my findings are obsolete.) |
|
Haha... I’ll rename it tomorrow then. |
2 participants
Prerequisites
Description
This PR replaces the incorrect
Matrix4x4filter implementation with a newColorMatrixtransform.All color matrices normally have a minimum of 5x4 indices which are used to transform the vector. While our implementation gave accurate output in fully opaque images, images with an alpha component were incorrect. In addition, the Hue filter was using an incorrect operator in its calculation which led to output error.
This PR.
ColorMatrixstruct modelled after theMatrix4x4struct. This contains all the properties required to implement color transforms.KnownFilterMatricesgeneration code to reflect the new struct layout and fix bugs. HueProcessor doesn't behave as expected #802