Skip to content
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

Open
wants to merge 188 commits into
base: main
Choose a base branch
from
Open

Conversation

ynse01
Copy link
Contributor

@ynse01 ynse01 commented Dec 29, 2023

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

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.

@@ -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));
Copy link
Member

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?

Copy link
Contributor Author

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 !

Copy link
Member

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)
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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(...)
  {
    ...
  }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah exactly. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants