-
-
Notifications
You must be signed in to change notification settings - Fork 851
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
Add support for HEIF based images #2633
base: main
Are you sure you want to change the base?
Conversation
…nto heic-support
@@ -35,51 +68,71 @@ public static void TransformSse(ref Vector128<int> input, ref Vector128<int> out | |||
Vector128<int> v0, v1, v2, v3; | |||
|
|||
int endidx = 3 * columnNumber; | |||
s0 = Sse41.Add(input, Unsafe.Add(ref input, endidx)); | |||
s3 = Sse41.Subtract(input, Unsafe.Add(ref input, endidx)); | |||
s0 = Sse2.Add(input, Unsafe.Add(ref input, endidx)); |
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.
If possible, we should look to use Vector128
opreators and methods here that should abstract the instrinsic support.
I'm a little nervous also about the lack of contraints here. Are they strictly defined in the caller?
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.
The constraints are handled inside the Av1BlockDecoder.DeriveBlockPointers
method. Please note that the Av1FrameBuffer
is over allocating, such that the last blocks still fit in entirely (even if the resulting image is smaller).
Please note, the Transform
method will be called twice for every 16 pixels, for each color channel. In other words, this is bound to become a performance hotspot !
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.
The operators and methods are hardware accelerated though and provide additional support for AdvSimd and PackedSimd
output1 = step2; | ||
output2 = step1; | ||
output3 = step3; | ||
} | ||
|
||
public void TransformAvx2(ref Vector256<int> input, ref Vector256<int> output, int cosBit, int columnNumber) |
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 would try to normalize these. Having specific public Avx2/SSe methods is something we want to remove from the codebase.
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 intend to leave 2 code paths, the scalar path and an AVX2 path. Thinking out loud for putting in an exception for the 4x4 case, as it operates on a very narrow strip of memory.
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.
My concern here is with implementation detail leaking into the API shape. Im slowly refactoring the library to ensure internal method callers have no requirement to know the level of hardware support present. The runtime is moving that way also.
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 guess you mean to have something like this:
internal class Av1Dct4ForwardTransformer : IAv1ForwardTransformer
{
public void Transform(...)
{
if (Vector128.IsHardwareAccelerated)
{
TransformSse(...);
}
else
{
TransformScalar();
}
}
private void TransformScalar(...)
{
...
}
private void TransformSse(...)
{
...
}
}
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.
Yeah exactly. Thanks!
Prerequisites
Description
This PR is an attempt to start with #1320
Implemented decoding and encoding of images based on a HEIF (ISO/IEC 23008-12:2022) container. These include (amongst others): HEIC, HIF and AVIF.
Please note this PR does NOT add any new compression algorithm to ImageSharp. For now it will take a JPEG thumbnail as its pixel source.
Needless to say, much more work is needed after this PR to reach the goal. I'm try to help there also.
Please do let me know any comments, also on whether to support the HEIF file format in the first place.