From 2df831f1964079d99d8b4da4a99f555ac71b015a Mon Sep 17 00:00:00 2001 From: Sheyne Anderson Date: Tue, 1 Oct 2019 16:28:33 +0100 Subject: [PATCH 1/5] Allow inferring of some PngEncoderOptions --- src/ImageSharp/Formats/Png/PngEncoderCore.cs | 2 +- .../Formats/Png/PngEncoderOptionsHelpers.cs | 75 ++++++++++++++++++- 2 files changed, 73 insertions(+), 4 deletions(-) diff --git a/src/ImageSharp/Formats/Png/PngEncoderCore.cs b/src/ImageSharp/Formats/Png/PngEncoderCore.cs index 09575bb288..efe0f20694 100644 --- a/src/ImageSharp/Formats/Png/PngEncoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngEncoderCore.cs @@ -146,7 +146,7 @@ namespace SixLabors.ImageSharp.Formats.Png ImageMetadata metadata = image.Metadata; PngMetadata pngMetadata = metadata.GetFormatMetadata(PngFormat.Instance); - PngEncoderOptionsHelpers.AdjustOptions(this.options, pngMetadata, out this.use16Bit, out this.bytesPerPixel); + PngEncoderOptionsHelpers.AdjustOptions(this.options, pngMetadata, out this.use16Bit, out this.bytesPerPixel); IQuantizedFrame quantized = PngEncoderOptionsHelpers.CreateQuantizedFrame(this.options, image); this.bitDepth = PngEncoderOptionsHelpers.CalculateBitDepth(this.options, image, quantized); diff --git a/src/ImageSharp/Formats/Png/PngEncoderOptionsHelpers.cs b/src/ImageSharp/Formats/Png/PngEncoderOptionsHelpers.cs index e3f2948864..1b6330a3c8 100644 --- a/src/ImageSharp/Formats/Png/PngEncoderOptionsHelpers.cs +++ b/src/ImageSharp/Formats/Png/PngEncoderOptionsHelpers.cs @@ -20,16 +20,19 @@ namespace SixLabors.ImageSharp.Formats.Png /// The PNG metadata. /// if set to true [use16 bit]. /// The bytes per pixel. - public static void AdjustOptions( + public static void AdjustOptions( PngEncoderOptions options, PngMetadata pngMetadata, out bool use16Bit, out int bytesPerPixel) + where TPixel : struct, IPixel { // Always take the encoder options over the metadata values. options.Gamma = options.Gamma ?? pngMetadata.Gamma; - options.ColorType = options.ColorType ?? pngMetadata.ColorType; - options.BitDepth = options.BitDepth ?? pngMetadata.BitDepth; + + options.ColorType = options.ColorType ?? SuggestColorType() ?? pngMetadata.ColorType; + options.BitDepth = options.BitDepth ?? SuggestBitDepth() ?? pngMetadata.BitDepth; + options.InterlaceMethod = options.InterlaceMethod ?? pngMetadata.InterlaceMethod; use16Bit = options.BitDepth == PngBitDepth.Bit16; @@ -148,5 +151,71 @@ namespace SixLabors.ImageSharp.Formats.Png return use16Bit ? 8 : 4; } } + + /// + /// Comes up with the appropriate PngColorType for some kinds of + /// IPixel. This is not exhaustive because not all options have + /// reasonable defaults + /// + private static PngColorType? SuggestColorType() + where TPixel : struct, IPixel + { + Type tPixel = typeof(TPixel); + + if (tPixel == typeof(Alpha8)) + { + return PngColorType.GrayscaleWithAlpha; + } + if (tPixel == typeof(Argb32)) + { + return PngColorType.RgbWithAlpha; + } + if (tPixel == typeof(Rgb24)) + { + return PngColorType.Rgb; + } + if (tPixel == typeof(Gray16)) + { + return PngColorType.Grayscale; + } + if (tPixel == typeof(Gray8)) + { + return PngColorType.Grayscale; + } + return default; + } + + /// + /// Comes up with the appropriate PngBitDepth for some kinds of + /// IPixel. This is not exhaustive because not all options have + /// reasonable defaults + /// + private static PngBitDepth? SuggestBitDepth() + where TPixel : struct, IPixel + { + Type tPixel = typeof(TPixel); + + if (tPixel == typeof(Alpha8)) + { + return PngBitDepth.Bit8; + } + if (tPixel == typeof(Argb32)) + { + return PngBitDepth.Bit8; + } + if (tPixel == typeof(Rgb24)) + { + return PngBitDepth.Bit8; + } + if (tPixel == typeof(Gray16)) + { + return PngBitDepth.Bit16; + } + if (tPixel == typeof(Gray8)) + { + return PngBitDepth.Bit8; + } + return default; + } } } From d304c7721471fdf0899ff8b386da5eaf7c0bb50e Mon Sep 17 00:00:00 2001 From: Sheyne Anderson Date: Thu, 3 Oct 2019 12:28:22 +0100 Subject: [PATCH 2/5] Fix StyleCop issues --- src/ImageSharp/Formats/Png/PngEncoderOptionsHelpers.cs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/ImageSharp/Formats/Png/PngEncoderOptionsHelpers.cs b/src/ImageSharp/Formats/Png/PngEncoderOptionsHelpers.cs index 1b6330a3c8..a5a45a684d 100644 --- a/src/ImageSharp/Formats/Png/PngEncoderOptionsHelpers.cs +++ b/src/ImageSharp/Formats/Png/PngEncoderOptionsHelpers.cs @@ -166,22 +166,27 @@ namespace SixLabors.ImageSharp.Formats.Png { return PngColorType.GrayscaleWithAlpha; } + if (tPixel == typeof(Argb32)) { return PngColorType.RgbWithAlpha; } + if (tPixel == typeof(Rgb24)) { return PngColorType.Rgb; } + if (tPixel == typeof(Gray16)) { return PngColorType.Grayscale; } + if (tPixel == typeof(Gray8)) { return PngColorType.Grayscale; } + return default; } @@ -199,22 +204,27 @@ namespace SixLabors.ImageSharp.Formats.Png { return PngBitDepth.Bit8; } + if (tPixel == typeof(Argb32)) { return PngBitDepth.Bit8; } + if (tPixel == typeof(Rgb24)) { return PngBitDepth.Bit8; } + if (tPixel == typeof(Gray16)) { return PngBitDepth.Bit16; } + if (tPixel == typeof(Gray8)) { return PngBitDepth.Bit8; } + return default; } } From 9c48edd685d43d17263ac8acf4739f61448bf62b Mon Sep 17 00:00:00 2001 From: Sheyne Anderson Date: Thu, 3 Oct 2019 15:22:03 +0100 Subject: [PATCH 3/5] Add a test for PngEncoder inference --- .../Formats/Png/PngEncoderTests.cs | 43 +++++++++++++++++++ .../TestUtilities/PixelTypes.cs | 2 + 2 files changed, 45 insertions(+) diff --git a/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs index 2584391bb7..04503330e9 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs @@ -195,6 +195,49 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png } } + [Theory] + // does the following make sense? Or is it supposed to encode a 16bpp with two 8bit channels? + [WithBlankImages(1, 1, PixelTypes.Alpha8, PngColorType.GrayscaleWithAlpha, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.Argb32, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] + // [WithBlankImages(1, 1, PixelTypes.Bgr565, Can't reasonably be inferred)] + // [WithBlankImages(1, 1, PixelTypes.Bgra4444, Can't reasonably be inferred)] + // [WithBlankImages(1, 1, PixelTypes.Byte4, I'm not sure)] + // [WithBlankImages(1, 1, PixelTypes.HalfSingle, I'm not sure)] + // [WithBlankImages(1, 1, PixelTypes.HalfVector2, I'm not sure)] + // [WithBlankImages(1, 1, PixelTypes.HalfVector4, I'm not sure)] + // [WithBlankImages(1, 1, PixelTypes.NormalizedByte2, I'm not sure)] + // [WithBlankImages(1, 1, PixelTypes.NormalizedByte4, I'm not sure)] + // [WithBlankImages(1, 1, PixelTypes.NormalizedShort4, I'm not sure)] + // [WithBlankImages(1, 1, PixelTypes.Rg32, I'm not sure)] + // [WithBlankImages(1, 1, PixelTypes.Rgba1010102, I'm not sure)] + // [WithBlankImages(1, 1, PixelTypes.Rgba32, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] + // [WithBlankImages(1, 1, PixelTypes.Rgba64, PngColorType.RgbWithAlpha, PngBitDepth.Bit16)] + // [WithBlankImages(1, 1, PixelTypes.RgbaVector, I'm not sure)] + // [WithBlankImages(1, 1, PixelTypes.Short2, I'm not sure)] + // [WithBlankImages(1, 1, PixelTypes.Short4, I'm not sure)] + [WithBlankImages(1, 1, PixelTypes.Rgb24, PngColorType.Rgb, PngBitDepth.Bit8)] + // [WithBlankImages(1, 1, PixelTypes.Bgr24, I'm not sure)] + // [WithBlankImages(1, 1, PixelTypes.Bgra32, I'm not sure)] + [WithBlankImages(1, 1, PixelTypes.Rgb48, PngColorType.Rgb, PngBitDepth.Bit16)] + // [WithBlankImages(1, 1, PixelTypes.Bgra5551, I'm not sure)] + [WithBlankImages(1, 1, PixelTypes.Gray8, PngColorType.Grayscale, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.Gray16, PngColorType.Grayscale, PngBitDepth.Bit8)] + public void InfersColorTypeAndBitDepth(TestImageProvider provider, PngColorType pngColorType, PngBitDepth pngBitDepth) + where TPixel : struct, IPixel + { + Stream stream = new MemoryStream(); + PngEncoder encoder = new PngEncoder(); + encoder.Encode(provider.GetImage(), stream); + + stream.Seek(0, SeekOrigin.Begin); + + PngDecoder decoder = new PngDecoder(); + + Image image = decoder.Decode(Configuration.Default, stream); + + Assert.True(image is Image); + } + [Theory] [WithFile(TestImages.Png.Palette8Bpp, nameof(PaletteLargeOnly), PixelTypes.Rgba32)] public void PaletteColorType_WuQuantizer(TestImageProvider provider, int paletteSize) diff --git a/tests/ImageSharp.Tests/TestUtilities/PixelTypes.cs b/tests/ImageSharp.Tests/TestUtilities/PixelTypes.cs index 78431f31aa..36c08a8486 100644 --- a/tests/ImageSharp.Tests/TestUtilities/PixelTypes.cs +++ b/tests/ImageSharp.Tests/TestUtilities/PixelTypes.cs @@ -62,6 +62,8 @@ namespace SixLabors.ImageSharp.Tests Gray8 = 1 << 23, + Gray16 = 1 << 24, + // TODO: Add multi-flag entries by rules defined in PackedPixelConverterHelper // "All" is handled as a separate, individual case instead of using bitwise OR From bc1dde64c82122e4378cd530a5ff760c566fe323 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Mon, 27 Jan 2020 23:58:36 +1100 Subject: [PATCH 4/5] Update options helper to use switch and complete tests --- .../Formats/Png/PngEncoderOptionsHelpers.cs | 136 +++++++----------- .../Formats/Png/PngEncoderTests.cs | 65 +++++---- .../TestUtilities/PixelTypes.cs | 10 +- 3 files changed, 94 insertions(+), 117 deletions(-) diff --git a/src/ImageSharp/Formats/Png/PngEncoderOptionsHelpers.cs b/src/ImageSharp/Formats/Png/PngEncoderOptionsHelpers.cs index a5a45a684d..6196792480 100644 --- a/src/ImageSharp/Formats/Png/PngEncoderOptionsHelpers.cs +++ b/src/ImageSharp/Formats/Png/PngEncoderOptionsHelpers.cs @@ -14,7 +14,7 @@ namespace SixLabors.ImageSharp.Formats.Png internal static class PngEncoderOptionsHelpers { /// - /// Adjusts the options. + /// Adjusts the options based upon the given metadata. /// /// The options. /// The PNG metadata. @@ -28,12 +28,12 @@ namespace SixLabors.ImageSharp.Formats.Png where TPixel : struct, IPixel { // Always take the encoder options over the metadata values. - options.Gamma = options.Gamma ?? pngMetadata.Gamma; + options.Gamma ??= pngMetadata.Gamma; - options.ColorType = options.ColorType ?? SuggestColorType() ?? pngMetadata.ColorType; - options.BitDepth = options.BitDepth ?? SuggestBitDepth() ?? pngMetadata.BitDepth; + options.ColorType ??= SuggestColorType() ?? pngMetadata.ColorType; + options.BitDepth ??= SuggestBitDepth() ?? pngMetadata.BitDepth; - options.InterlaceMethod = options.InterlaceMethod ?? pngMetadata.InterlaceMethod; + options.InterlaceMethod ??= pngMetadata.InterlaceMethod; use16Bit = options.BitDepth == PngBitDepth.Bit16; bytesPerPixel = CalculateBytesPerPixel(options.ColorType, use16Bit); @@ -132,100 +132,68 @@ namespace SixLabors.ImageSharp.Formats.Png /// Bytes per pixel. private static int CalculateBytesPerPixel(PngColorType? pngColorType, bool use16Bit) { - switch (pngColorType) + return pngColorType switch { - case PngColorType.Grayscale: - return use16Bit ? 2 : 1; - - case PngColorType.GrayscaleWithAlpha: - return use16Bit ? 4 : 2; - - case PngColorType.Palette: - return 1; - - case PngColorType.Rgb: - return use16Bit ? 6 : 3; + PngColorType.Grayscale => use16Bit ? 2 : 1, + PngColorType.GrayscaleWithAlpha => use16Bit ? 4 : 2, + PngColorType.Palette => 1, + PngColorType.Rgb => use16Bit ? 6 : 3, // PngColorType.RgbWithAlpha - default: - return use16Bit ? 8 : 4; - } + _ => use16Bit ? 8 : 4, + }; } /// - /// Comes up with the appropriate PngColorType for some kinds of - /// IPixel. This is not exhaustive because not all options have - /// reasonable defaults + /// Returns a suggested for the given + /// This is not exhaustive but covers many common pixel formats. /// private static PngColorType? SuggestColorType() - where TPixel : struct, IPixel + where TPixel : struct, IPixel { - Type tPixel = typeof(TPixel); - - if (tPixel == typeof(Alpha8)) - { - return PngColorType.GrayscaleWithAlpha; - } - - if (tPixel == typeof(Argb32)) - { - return PngColorType.RgbWithAlpha; - } - - if (tPixel == typeof(Rgb24)) - { - return PngColorType.Rgb; - } - - if (tPixel == typeof(Gray16)) - { - return PngColorType.Grayscale; - } - - if (tPixel == typeof(Gray8)) - { - return PngColorType.Grayscale; - } - - return default; + return typeof(TPixel) switch + { + Type t when t == typeof(A8) => PngColorType.GrayscaleWithAlpha, + Type t when t == typeof(Argb32) => PngColorType.RgbWithAlpha, + Type t when t == typeof(Bgr24) => PngColorType.Rgb, + Type t when t == typeof(Bgra32) => PngColorType.RgbWithAlpha, + Type t when t == typeof(L8) => PngColorType.Grayscale, + Type t when t == typeof(L16) => PngColorType.Grayscale, + Type t when t == typeof(La16) => PngColorType.GrayscaleWithAlpha, + Type t when t == typeof(La32) => PngColorType.GrayscaleWithAlpha, + Type t when t == typeof(Rgb24) => PngColorType.Rgb, + Type t when t == typeof(Rgba32) => PngColorType.RgbWithAlpha, + Type t when t == typeof(Rgb48) => PngColorType.Rgb, + Type t when t == typeof(Rgba64) => PngColorType.RgbWithAlpha, + Type t when t == typeof(RgbaVector) => PngColorType.RgbWithAlpha, + _ => default(PngColorType?) + }; } /// - /// Comes up with the appropriate PngBitDepth for some kinds of - /// IPixel. This is not exhaustive because not all options have - /// reasonable defaults + /// Returns a suggested for the given + /// This is not exhaustive but covers many common pixel formats. /// private static PngBitDepth? SuggestBitDepth() - where TPixel : struct, IPixel + where TPixel : struct, IPixel { - Type tPixel = typeof(TPixel); - - if (tPixel == typeof(Alpha8)) - { - return PngBitDepth.Bit8; - } - - if (tPixel == typeof(Argb32)) - { - return PngBitDepth.Bit8; - } - - if (tPixel == typeof(Rgb24)) - { - return PngBitDepth.Bit8; - } - - if (tPixel == typeof(Gray16)) - { - return PngBitDepth.Bit16; - } - - if (tPixel == typeof(Gray8)) - { - return PngBitDepth.Bit8; - } - - return default; + return typeof(TPixel) switch + { + Type t when t == typeof(A8) => PngBitDepth.Bit8, + Type t when t == typeof(Argb32) => PngBitDepth.Bit8, + Type t when t == typeof(Bgr24) => PngBitDepth.Bit8, + Type t when t == typeof(Bgra32) => PngBitDepth.Bit8, + Type t when t == typeof(L8) => PngBitDepth.Bit8, + Type t when t == typeof(L16) => PngBitDepth.Bit16, + Type t when t == typeof(La16) => PngBitDepth.Bit8, + Type t when t == typeof(La32) => PngBitDepth.Bit16, + Type t when t == typeof(Rgb24) => PngBitDepth.Bit8, + Type t when t == typeof(Rgba32) => PngBitDepth.Bit8, + Type t when t == typeof(Rgb48) => PngBitDepth.Bit16, + Type t when t == typeof(Rgba64) => PngBitDepth.Bit16, + Type t when t == typeof(RgbaVector) => PngBitDepth.Bit16, + _ => default(PngBitDepth?) + }; } } } diff --git a/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs index cacb3e42fe..1fa131c914 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs @@ -202,46 +202,51 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png } [Theory] - // does the following make sense? Or is it supposed to encode a 16bpp with two 8bit channels? - [WithBlankImages(1, 1, PixelTypes.Alpha8, PngColorType.GrayscaleWithAlpha, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.A8, PngColorType.GrayscaleWithAlpha, PngBitDepth.Bit8)] [WithBlankImages(1, 1, PixelTypes.Argb32, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] - // [WithBlankImages(1, 1, PixelTypes.Bgr565, Can't reasonably be inferred)] - // [WithBlankImages(1, 1, PixelTypes.Bgra4444, Can't reasonably be inferred)] - // [WithBlankImages(1, 1, PixelTypes.Byte4, I'm not sure)] - // [WithBlankImages(1, 1, PixelTypes.HalfSingle, I'm not sure)] - // [WithBlankImages(1, 1, PixelTypes.HalfVector2, I'm not sure)] - // [WithBlankImages(1, 1, PixelTypes.HalfVector4, I'm not sure)] - // [WithBlankImages(1, 1, PixelTypes.NormalizedByte2, I'm not sure)] - // [WithBlankImages(1, 1, PixelTypes.NormalizedByte4, I'm not sure)] - // [WithBlankImages(1, 1, PixelTypes.NormalizedShort4, I'm not sure)] - // [WithBlankImages(1, 1, PixelTypes.Rg32, I'm not sure)] - // [WithBlankImages(1, 1, PixelTypes.Rgba1010102, I'm not sure)] - // [WithBlankImages(1, 1, PixelTypes.Rgba32, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] - // [WithBlankImages(1, 1, PixelTypes.Rgba64, PngColorType.RgbWithAlpha, PngBitDepth.Bit16)] - // [WithBlankImages(1, 1, PixelTypes.RgbaVector, I'm not sure)] - // [WithBlankImages(1, 1, PixelTypes.Short2, I'm not sure)] - // [WithBlankImages(1, 1, PixelTypes.Short4, I'm not sure)] + [WithBlankImages(1, 1, PixelTypes.Bgr565, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.Bgra4444, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.Byte4, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.HalfSingle, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.HalfVector2, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.HalfVector4, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.NormalizedByte2, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.NormalizedByte4, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.NormalizedShort4, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.Rg32, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.Rgba1010102, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.Rgba32, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.RgbaVector, PngColorType.RgbWithAlpha, PngBitDepth.Bit16)] + [WithBlankImages(1, 1, PixelTypes.Short2, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.Short4, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] [WithBlankImages(1, 1, PixelTypes.Rgb24, PngColorType.Rgb, PngBitDepth.Bit8)] - // [WithBlankImages(1, 1, PixelTypes.Bgr24, I'm not sure)] - // [WithBlankImages(1, 1, PixelTypes.Bgra32, I'm not sure)] + [WithBlankImages(1, 1, PixelTypes.Bgr24, PngColorType.Rgb, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.Bgra32, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] [WithBlankImages(1, 1, PixelTypes.Rgb48, PngColorType.Rgb, PngBitDepth.Bit16)] - // [WithBlankImages(1, 1, PixelTypes.Bgra5551, I'm not sure)] - [WithBlankImages(1, 1, PixelTypes.Gray8, PngColorType.Grayscale, PngBitDepth.Bit8)] - [WithBlankImages(1, 1, PixelTypes.Gray16, PngColorType.Grayscale, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.Rgba64, PngColorType.RgbWithAlpha, PngBitDepth.Bit16)] + [WithBlankImages(1, 1, PixelTypes.Bgra5551, PngColorType.RgbWithAlpha, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.L8, PngColorType.Grayscale, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.L16, PngColorType.Grayscale, PngBitDepth.Bit16)] + [WithBlankImages(1, 1, PixelTypes.La16, PngColorType.GrayscaleWithAlpha, PngBitDepth.Bit8)] + [WithBlankImages(1, 1, PixelTypes.La32, PngColorType.GrayscaleWithAlpha, PngBitDepth.Bit16)] public void InfersColorTypeAndBitDepth(TestImageProvider provider, PngColorType pngColorType, PngBitDepth pngBitDepth) where TPixel : struct, IPixel { - Stream stream = new MemoryStream(); - PngEncoder encoder = new PngEncoder(); - encoder.Encode(provider.GetImage(), stream); + using (Stream stream = new MemoryStream()) + { + var encoder = new PngEncoder(); + encoder.Encode(provider.GetImage(), stream); - stream.Seek(0, SeekOrigin.Begin); + stream.Seek(0, SeekOrigin.Begin); - PngDecoder decoder = new PngDecoder(); + var decoder = new PngDecoder(); - Image image = decoder.Decode(Configuration.Default, stream); + Image image = decoder.Decode(Configuration.Default, stream); - Assert.True(image is Image); + PngMetadata metadata = image.Metadata.GetPngMetadata(); + Assert.Equal(pngColorType, metadata.ColorType); + Assert.Equal(pngBitDepth, metadata.BitDepth); + } } [Theory] diff --git a/tests/ImageSharp.Tests/TestUtilities/PixelTypes.cs b/tests/ImageSharp.Tests/TestUtilities/PixelTypes.cs index c795563134..eb8860eb6f 100644 --- a/tests/ImageSharp.Tests/TestUtilities/PixelTypes.cs +++ b/tests/ImageSharp.Tests/TestUtilities/PixelTypes.cs @@ -1,4 +1,4 @@ -// Copyright (c) Six Labors and contributors. +// Copyright (c) Six Labors and contributors. // Licensed under the Apache License, Version 2.0. using System; @@ -62,11 +62,15 @@ namespace SixLabors.ImageSharp.Tests L8 = 1 << 23, - Gray16 = 1 << 24, + L16 = 1 << 24, + + La16 = 1 << 25, + + La32 = 1 << 26, // TODO: Add multi-flag entries by rules defined in PackedPixelConverterHelper // "All" is handled as a separate, individual case instead of using bitwise OR All = 30 } -} \ No newline at end of file +} From 02199fdd2b86ca09ca094cc1929b59bf8d7751c1 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Tue, 28 Jan 2020 00:32:22 +1100 Subject: [PATCH 5/5] Fix options calculation precedence --- src/ImageSharp/Formats/Png/PngEncoderCore.cs | 2 +- .../Formats/Png/PngEncoderOptionsHelpers.cs | 14 ++++++++------ src/ImageSharp/Formats/Png/PngMetadata.cs | 4 ++-- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/ImageSharp/Formats/Png/PngEncoderCore.cs b/src/ImageSharp/Formats/Png/PngEncoderCore.cs index 7f4b5b93b3..69a80e024e 100644 --- a/src/ImageSharp/Formats/Png/PngEncoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngEncoderCore.cs @@ -144,7 +144,7 @@ namespace SixLabors.ImageSharp.Formats.Png this.height = image.Height; ImageMetadata metadata = image.Metadata; - PngMetadata pngMetadata = metadata.GetFormatMetadata(PngFormat.Instance); + PngMetadata pngMetadata = metadata.GetPngMetadata(); PngEncoderOptionsHelpers.AdjustOptions(this.options, pngMetadata, out this.use16Bit, out this.bytesPerPixel); IQuantizedFrame quantized = PngEncoderOptionsHelpers.CreateQuantizedFrame(this.options, image); this.bitDepth = PngEncoderOptionsHelpers.CalculateBitDepth(this.options, image, quantized); diff --git a/src/ImageSharp/Formats/Png/PngEncoderOptionsHelpers.cs b/src/ImageSharp/Formats/Png/PngEncoderOptionsHelpers.cs index 6196792480..b494c164f5 100644 --- a/src/ImageSharp/Formats/Png/PngEncoderOptionsHelpers.cs +++ b/src/ImageSharp/Formats/Png/PngEncoderOptionsHelpers.cs @@ -30,8 +30,10 @@ namespace SixLabors.ImageSharp.Formats.Png // Always take the encoder options over the metadata values. options.Gamma ??= pngMetadata.Gamma; - options.ColorType ??= SuggestColorType() ?? pngMetadata.ColorType; - options.BitDepth ??= SuggestBitDepth() ?? pngMetadata.BitDepth; + // Use options, then check metadata, if nothing set there then we suggest + // a sensible default based upon the pixel format. + options.ColorType ??= pngMetadata.ColorType ?? SuggestColorType(); + options.BitDepth ??= pngMetadata.BitDepth ?? SuggestBitDepth(); options.InterlaceMethod ??= pngMetadata.InterlaceMethod; @@ -148,7 +150,7 @@ namespace SixLabors.ImageSharp.Formats.Png /// Returns a suggested for the given /// This is not exhaustive but covers many common pixel formats. /// - private static PngColorType? SuggestColorType() + private static PngColorType SuggestColorType() where TPixel : struct, IPixel { return typeof(TPixel) switch @@ -166,7 +168,7 @@ namespace SixLabors.ImageSharp.Formats.Png Type t when t == typeof(Rgb48) => PngColorType.Rgb, Type t when t == typeof(Rgba64) => PngColorType.RgbWithAlpha, Type t when t == typeof(RgbaVector) => PngColorType.RgbWithAlpha, - _ => default(PngColorType?) + _ => PngColorType.RgbWithAlpha }; } @@ -174,7 +176,7 @@ namespace SixLabors.ImageSharp.Formats.Png /// Returns a suggested for the given /// This is not exhaustive but covers many common pixel formats. /// - private static PngBitDepth? SuggestBitDepth() + private static PngBitDepth SuggestBitDepth() where TPixel : struct, IPixel { return typeof(TPixel) switch @@ -192,7 +194,7 @@ namespace SixLabors.ImageSharp.Formats.Png Type t when t == typeof(Rgb48) => PngBitDepth.Bit16, Type t when t == typeof(Rgba64) => PngBitDepth.Bit16, Type t when t == typeof(RgbaVector) => PngBitDepth.Bit16, - _ => default(PngBitDepth?) + _ => PngBitDepth.Bit8 }; } } diff --git a/src/ImageSharp/Formats/Png/PngMetadata.cs b/src/ImageSharp/Formats/Png/PngMetadata.cs index 87a2080f0f..341fc53edf 100644 --- a/src/ImageSharp/Formats/Png/PngMetadata.cs +++ b/src/ImageSharp/Formats/Png/PngMetadata.cs @@ -44,12 +44,12 @@ namespace SixLabors.ImageSharp.Formats.Png /// Gets or sets the number of bits per sample or per palette index (not per pixel). /// Not all values are allowed for all values. /// - public PngBitDepth BitDepth { get; set; } = PngBitDepth.Bit8; + public PngBitDepth? BitDepth { get; set; } /// /// Gets or sets the color type. /// - public PngColorType ColorType { get; set; } = PngColorType.RgbWithAlpha; + public PngColorType? ColorType { get; set; } /// /// Gets or sets a value indicating whether this instance should write an Adam7 interlaced image.