Conversation
…nto heic-support
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
30 file(s) reviewed, 62 comment(s)
Edit PR Review Bot Settings | Greptile
| // Copyright (c) Six Labors. | ||
| // Licensed under the Six Labors Split License. | ||
|
|
||
| using SixLabors.ImageSharp.Formats.Heif.Av1.Transform; |
There was a problem hiding this comment.
style: Transform namespace is imported but not used
|
|
||
| public void Reset() => this.BitPosition = 0; | ||
|
|
||
| public void Skip(int bitCount) => this.BitPosition += bitCount; |
There was a problem hiding this comment.
logic: no bounds checking on Skip() - could advance beyond buffer end
| int byteOffset = Av1Math.DivideBy8Floor(this.BitPosition); | ||
| byte shift = (byte)(7 - Av1Math.Modulus8(this.BitPosition)); | ||
| this.BitPosition++; | ||
| return (uint)((this.data[byteOffset] >> shift) & 0x01); |
There was a problem hiding this comment.
logic: ReadBit() needs bounds check on byteOffset against data.Length
|
|
||
| ulong value = 0; | ||
| length = 0; | ||
| for (int i = 0; i < 56; i += 7) |
There was a problem hiding this comment.
style: magic number 56 should be defined as constant with descriptive name
| DebugGuard.IsTrue(Av1Math.Modulus8(this.BitPosition) == 0, "Reading of Little Endian value only allowed on byte alignment"); | ||
|
|
||
| uint t = 0; | ||
| for (int i = 0; i < 8 * n; i += 8) |
There was a problem hiding this comment.
logic: potential integer overflow if n > int.MaxValue/8
|
|
||
| namespace SixLabors.ImageSharp.Formats.Heif.Av1.OpenBitstreamUnit; | ||
|
|
||
| internal class ObuFrameHeader |
There was a problem hiding this comment.
style: class should be sealed since it's not designed for inheritance
|
|
||
| namespace SixLabors.ImageSharp.Formats.Heif.Av1.OpenBitstreamUnit; | ||
|
|
||
| internal class ObuFrameSize |
There was a problem hiding this comment.
style: Consider making this class sealed since it's not designed for inheritance and is internal
| internal int FrameWidth { get; set; } | ||
|
|
||
| internal int FrameHeight { get; set; } |
There was a problem hiding this comment.
logic: Consider adding validation to ensure width/height are non-negative
|
|
||
| internal int FrameHeight { get; set; } | ||
|
|
||
| internal int SuperResolutionDenominator { get; set; } |
There was a problem hiding this comment.
logic: SuperResolutionDenominator should be greater than or equal to 9 per AV1 spec
| internal class ObuFrameSize | ||
| { | ||
| internal int FrameWidth { get; set; } | ||
|
|
||
| internal int FrameHeight { get; set; } | ||
|
|
||
| internal int SuperResolutionDenominator { get; set; } | ||
|
|
||
| internal int SuperResolutionUpscaledWidth { get; set; } | ||
|
|
||
| internal int RenderWidth { get; set; } | ||
|
|
||
| internal int RenderHeight { get; set; } | ||
| } |
There was a problem hiding this comment.
style: XML documentation missing for class and properties to explain their purpose and constraints
4 participants
Prerequisites
Description
This PR is an attempt to start with SixLabors#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.
Greptile Summary
This PR adds initial support for HEIF-based image formats (HEIC, HIF, AVIF) by implementing AV1 codec components and HEIF container handling.
src/ImageSharp/Formats/Heif/Av1/including bitstream handling, frame buffers, and color format support.heic,.hif, and.aviffile formats in.gitattributesConfiguration.csviaHeifConfigurationModule