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 PngWriteDefines to Magick.NET #1661

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

Conversation

cordeiro-rubens
Copy link
Contributor

Description

This pull request introduces the PngWriteDefines class to the Magick.NET project. The PngWriteDefines class includes several enums (PngChunkFlags, PngCompressionFilter, PngCompressionStrategy),

Changes Proposed:

Added the PngWriteDefines class with the following properties:

BitDepth - Specifies the bit depth for the PNG image.
ColorType - Specifies the color type for the PNG image.
CompressionFilter - Specifies the compression filter for the PNG image using the PngCompressionFilter enum.
CompressionLevel - Specifies the compression level for the PNG image.
CompressionStrategy - Specifies the compression strategy for the PNG image using the PngCompressionStrategy enum.
ExcludeChunks - Specifies the chunks to be excluded using the PngChunkFlags enum.
IncludeChunks - Specifies the chunks to be included using the PngChunkFlags enum.
IgnoreCrc - Indicates whether the PNG decoder should ignore the CRC.
PreserveICCP - Indicates whether to preserve the ICC profile.
PreserveColormap - Indicates whether to preserve the colormap.

@cordeiro-rubens cordeiro-rubens changed the title Add PngWriteDefines to Magick.NET for PNG Chunk Management #225 Add PngWriteDefines to Magick.NET #225 Jun 27, 2024
@cordeiro-rubens cordeiro-rubens changed the title Add PngWriteDefines to Magick.NET #225 Add PngWriteDefines to Magick.NET Jun 27, 2024
…arnings

- Updated XML comments for PreserveColorMap property to resolve SA1623.
- Removed an empty line to resolve SA1507.
- Added copyright notice to the header that was previously missing.
- Added `#if NETSTANDARD == false` to hide `[Flags]` attribute for non-.NET Standard frameworks.
   This resolves build failures on Windows ARM64 processors.
- Added justification to SuppressMessage in PngChunkFlags
"The lowercase names are consistent with the naming conventions of PNG chunk types as defined in the PNG specification."
- Removed bitwise shift operators and used explicit int values for flag definitions.
- Resolved StyleCop error SA1137: Elements should have the same indentation.
Copy link
Owner

@dlemstra dlemstra left a comment

Choose a reason for hiding this comment

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

Thanks for helping me with this. I left some review comments.

src/Magick.NET/Formats/Png/PngChunkFlags.cs Show resolved Hide resolved
yield return new MagickDefine(Format, "bit-depth", BitDepth.Value);

if (ColorType.HasValue)
yield return new MagickDefine(Format, "color-type", ColorType.Value);
Copy link
Owner

Choose a reason for hiding this comment

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

This should be a numeric value and not the name of the color type:

%               png:color-type can be 0, 2, 3, 4, or 6.
%
%               When png:color-type is 0 (Grayscale), png:bit-depth can
%               be 1, 2, 4, 8, or 16.
%
%               When png:color-type is 2 (RGB), png:bit-depth can
%               be 8 or 16.
%
%               When png:color-type is 3 (Indexed), png:bit-depth can
%               be 1, 2, 4, or 8.  This refers to the number of bits
%               used to store the index.  The color samples always have
%               bit-depth 8 in indexed PNG files.
%
%               When png:color-type is 4 (Gray-Matte) or 6 (RGB-Matte),
%               png:bit-depth can be 8 or 16.

This also means we probably need a new enumeration for that.

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 have created a new PngColorType enum with values for Grayscale, RGB, Indexed, GrayMatte, and RGBMatte. Additionally, I implemented a private method GetPngColorTypeValue to convert these values appropriately, as well as a private method ValidateBitDepth to ensure the bit depth is valid based on the color type.

Here are the key changes:

Enum Creation: Introduced PngColorType enum for clarity and type safety.
Conversion Method: Added GetPngColorTypeValue to map the PngColorType enum to the correct numeric values.
Validation Method: Implemented ValidateBitDepth to check the bit depth against the color type, throwing an ArgumentException for invalid combinations.

I will commit these changes. If any modifications are needed, please let me know. I'm more than happy to make any necessary corrections.

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we need the complexity to check for invalid combinations. This might change in ImageMagick in the future. We should leave this up to ImageMagick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback. I've removed the validation as suggested.

if (CompressionLevel.HasValue)
yield return new MagickDefine(Format, "compression-level", CompressionLevel.Value);

if (CompressionFilter.HasValue)
Copy link
Owner

Choose a reason for hiding this comment

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

Please put this in alphabetical order.

yield return new MagickDefine(Format, "compression-filter", CompressionFilter.Value);

if (CompressionStrategy.HasValue)
yield return new MagickDefine(Format, "compression-strategy", CompressionStrategy.Value);
Copy link
Owner

Choose a reason for hiding this comment

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

We also need a numeric value here.

if (IncludeChunks.HasValue)
yield return new MagickDefine(Format, "include-chunks", EnumHelper.ConvertFlags(IncludeChunks.Value));

if (IgnoreCrc)
Copy link
Owner

Choose a reason for hiding this comment

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

This option is only used when reading a PNG file.

}

[Fact]
public void ShouldSetTheDefineNull()
Copy link
Owner

Choose a reason for hiding this comment

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

ShouldNotSetTheDefineWhenNull

And please change this in the other files also.

using var image = new MagickImage();
image.Settings.SetDefines(new PngWriteDefines
{
BitDepth = 8u,
Copy link
Owner

Choose a reason for hiding this comment

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

Please use an uppercase U.

public class ThePreserveColorMapPropety
{
[Fact]
public void ShouldSetTheDefine()
Copy link
Owner

Choose a reason for hiding this comment

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

ShouldSetTheDefineWhenSetToTrue

  - Alphabetical ordering for CompressionLevel and CompressionFilter.
  - Converted CompressionFilter and CompressionStrategy to numeric values.
- This option is only used when reading a PNG.
- Removed TheIncludeChunksProperty class from PngWriteDefinesTests
…roperty.

- Updated the class name to follow the naming convention for property test classes.
…nNull PngWriteDefinesTests.

  - TheBitDepthProperty
  - TheColorTypeProperty
  - TheCompressionFilterProperty
  - TheExcludeChunksProperty
  - TheCompressionStrategyProperty
- Updated tests to use numeric values for properties in
  - TheColorTypeProperty
  - TheCompressionFilterProperty
  - ThreCompressionStrategyProperty
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.

None yet

2 participants