From 8b0eaf8dfdeb655767337be1237e85cad0c55dbc Mon Sep 17 00:00:00 2001 From: Brian Popow Date: Thu, 7 Nov 2019 20:49:12 +0100 Subject: [PATCH] Code review changes --- Directory.Build.targets | 2 +- .../Formats/Tga/ITgaEncoderOptions.cs | 2 +- src/ImageSharp/Formats/Tga/TgaCompression.cs | 21 ++++++++++++ src/ImageSharp/Formats/Tga/TgaDecoderCore.cs | 33 ++++++++++++++----- src/ImageSharp/Formats/Tga/TgaEncoder.cs | 4 +-- src/ImageSharp/Formats/Tga/TgaEncoderCore.cs | 21 +++++++----- src/ImageSharp/Formats/Tga/TgaImageType.cs | 1 - .../Formats/Tga/TgaEncoderTests.cs | 22 ++++++------- .../ImageSharp.Tests/ImageSharp.Tests.csproj | 2 +- 9 files changed, 74 insertions(+), 34 deletions(-) create mode 100644 src/ImageSharp/Formats/Tga/TgaCompression.cs diff --git a/Directory.Build.targets b/Directory.Build.targets index 1dc081782..01c1f1039 100644 --- a/Directory.Build.targets +++ b/Directory.Build.targets @@ -24,7 +24,7 @@ - + diff --git a/src/ImageSharp/Formats/Tga/ITgaEncoderOptions.cs b/src/ImageSharp/Formats/Tga/ITgaEncoderOptions.cs index ef1fecc93..49983d236 100644 --- a/src/ImageSharp/Formats/Tga/ITgaEncoderOptions.cs +++ b/src/ImageSharp/Formats/Tga/ITgaEncoderOptions.cs @@ -16,6 +16,6 @@ namespace SixLabors.ImageSharp.Formats.Tga /// /// Gets a value indicating whether run length compression should be used. /// - bool Compress { get; } + TgaCompression Compression { get; } } } diff --git a/src/ImageSharp/Formats/Tga/TgaCompression.cs b/src/ImageSharp/Formats/Tga/TgaCompression.cs new file mode 100644 index 000000000..cc6e005ed --- /dev/null +++ b/src/ImageSharp/Formats/Tga/TgaCompression.cs @@ -0,0 +1,21 @@ +// Copyright (c) Six Labors and contributors. +// Licensed under the Apache License, Version 2.0. + +namespace SixLabors.ImageSharp.Formats.Tga +{ + /// + /// Indicates if compression is used. + /// + public enum TgaCompression + { + /// + /// No compression is used. + /// + None, + + /// + /// Run length encoding is used. + /// + RunLength, + } +} diff --git a/src/ImageSharp/Formats/Tga/TgaDecoderCore.cs b/src/ImageSharp/Formats/Tga/TgaDecoderCore.cs index 2d7014335..c5a4df3f9 100644 --- a/src/ImageSharp/Formats/Tga/TgaDecoderCore.cs +++ b/src/ImageSharp/Formats/Tga/TgaDecoderCore.cs @@ -106,16 +106,31 @@ namespace SixLabors.ImageSharp.Formats.Tga } int colorMapPixelSizeInBytes = this.fileHeader.CMapDepth / 8; - var palette = new byte[this.fileHeader.CMapLength * colorMapPixelSizeInBytes]; - this.currentStream.Read(palette, this.fileHeader.CMapStart, palette.Length); - - if (this.fileHeader.ImageType is TgaImageType.RleColorMapped) - { - this.ReadPalettedRle(this.fileHeader.Width, this.fileHeader.Height, pixels, palette, colorMapPixelSizeInBytes, inverted); - } - else + int colorMapSizeInBytes = this.fileHeader.CMapLength * colorMapPixelSizeInBytes; + using (IManagedByteBuffer palette = this.memoryAllocator.AllocateManagedByteBuffer(colorMapSizeInBytes, AllocationOptions.Clean)) { - this.ReadPaletted(this.fileHeader.Width, this.fileHeader.Height, pixels, palette, colorMapPixelSizeInBytes, inverted); + this.currentStream.Read(palette.Array, this.fileHeader.CMapStart, colorMapSizeInBytes); + + if (this.fileHeader.ImageType is TgaImageType.RleColorMapped) + { + this.ReadPalettedRle( + this.fileHeader.Width, + this.fileHeader.Height, + pixels, + palette.Array, + colorMapPixelSizeInBytes, + inverted); + } + else + { + this.ReadPaletted( + this.fileHeader.Width, + this.fileHeader.Height, + pixels, + palette.Array, + colorMapPixelSizeInBytes, + inverted); + } } return image; diff --git a/src/ImageSharp/Formats/Tga/TgaEncoder.cs b/src/ImageSharp/Formats/Tga/TgaEncoder.cs index 85b4fadfc..52a300f2e 100644 --- a/src/ImageSharp/Formats/Tga/TgaEncoder.cs +++ b/src/ImageSharp/Formats/Tga/TgaEncoder.cs @@ -19,9 +19,9 @@ namespace SixLabors.ImageSharp.Formats.Tga public TgaBitsPerPixel? BitsPerPixel { get; set; } /// - /// Gets or sets a value indicating whether run length compression should be used. + /// Gets or sets a value indicating whether no compression or run length compression should be used. /// - public bool Compress { get; set; } + public TgaCompression Compression { get; set; } /// public void Encode(Image image, Stream stream) diff --git a/src/ImageSharp/Formats/Tga/TgaEncoderCore.cs b/src/ImageSharp/Formats/Tga/TgaEncoderCore.cs index 139cc13fd..8022a0636 100644 --- a/src/ImageSharp/Formats/Tga/TgaEncoderCore.cs +++ b/src/ImageSharp/Formats/Tga/TgaEncoderCore.cs @@ -43,7 +43,12 @@ namespace SixLabors.ImageSharp.Formats.Tga /// /// Indicates if run length compression should be used. /// - private readonly bool useCompression; + private readonly TgaCompression compression; + + /// + /// Vector for converting pixel to gray value. + /// + private static readonly Vector4 Bt709 = new Vector4(.2126f, .7152f, .0722f, 0.0f); /// /// Initializes a new instance of the class. @@ -54,7 +59,7 @@ namespace SixLabors.ImageSharp.Formats.Tga { this.memoryAllocator = memoryAllocator; this.bitsPerPixel = options.BitsPerPixel; - this.useCompression = options.Compress; + this.compression = options.Compression; } /// @@ -74,14 +79,14 @@ namespace SixLabors.ImageSharp.Formats.Tga TgaMetadata tgaMetadata = metadata.GetFormatMetadata(TgaFormat.Instance); this.bitsPerPixel = this.bitsPerPixel ?? tgaMetadata.BitsPerPixel; - TgaImageType imageType = this.useCompression ? TgaImageType.RleTrueColor : TgaImageType.TrueColor; + TgaImageType imageType = this.compression is TgaCompression.RunLength ? TgaImageType.RleTrueColor : TgaImageType.TrueColor; if (this.bitsPerPixel == TgaBitsPerPixel.Pixel8) { - imageType = this.useCompression ? TgaImageType.RleBlackAndWhite : TgaImageType.BlackAndWhite; + imageType = this.compression is TgaCompression.RunLength ? TgaImageType.RleBlackAndWhite : TgaImageType.BlackAndWhite; } // If compression is used, set bit 5 of the image descriptor to indicate an left top origin. - byte imageDescriptor = (byte)(this.useCompression ? 32 : 0); + byte imageDescriptor = (byte)(this.compression is TgaCompression.RunLength ? 32 : 0); var fileHeader = new TgaFileHeader( idLength: 0, @@ -91,7 +96,7 @@ namespace SixLabors.ImageSharp.Formats.Tga cMapLength: 0, cMapDepth: 0, xOffset: 0, - yOffset: this.useCompression ? (short)image.Height : (short)0, // When run length encoding is used, the origin should be top left instead of the default bottom left. + yOffset: this.compression is TgaCompression.RunLength ? (short)image.Height : (short)0, // When run length encoding is used, the origin should be top left instead of the default bottom left. width: (short)image.Width, height: (short)image.Height, pixelDepth: (byte)this.bitsPerPixel.Value, @@ -106,7 +111,7 @@ namespace SixLabors.ImageSharp.Formats.Tga stream.Write(buffer, 0, TgaFileHeader.Size); - if (this.useCompression) + if (this.compression is TgaCompression.RunLength) { this.WriteRunLengthEndcodedImage(stream, image.Frames.RootFrame); } @@ -351,6 +356,6 @@ namespace SixLabors.ImageSharp.Formats.Tga /// The vector to get the luminance from. [MethodImpl(InliningOptions.ShortMethod)] public static int GetLuminance(ref Vector4 vector) - => (int)MathF.Round(((.2126F * vector.X) + (.7152F * vector.Y) + (.0722F * vector.Y)) * (256 - 1)); + => (int)MathF.Round(Vector4.Dot(vector, Bt709) * (256 - 1)); } } diff --git a/src/ImageSharp/Formats/Tga/TgaImageType.cs b/src/ImageSharp/Formats/Tga/TgaImageType.cs index cf0eda93c..491fd3ea7 100644 --- a/src/ImageSharp/Formats/Tga/TgaImageType.cs +++ b/src/ImageSharp/Formats/Tga/TgaImageType.cs @@ -12,7 +12,6 @@ namespace SixLabors. { /// /// No image data included. - /// Not sure what this is used for. /// NoImageData = 0, diff --git a/tests/ImageSharp.Tests/Formats/Tga/TgaEncoderTests.cs b/tests/ImageSharp.Tests/Formats/Tga/TgaEncoderTests.cs index 5dd49f4fa..e946729a1 100644 --- a/tests/ImageSharp.Tests/Formats/Tga/TgaEncoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Tga/TgaEncoderTests.cs @@ -60,7 +60,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Tga { var options = new TgaEncoder() { - Compress = true + Compression = TgaCompression.RunLength }; TestFile testFile = TestFile.Create(imagePath); @@ -83,55 +83,55 @@ namespace SixLabors.ImageSharp.Tests.Formats.Tga [WithFile(Bit32, PixelTypes.Rgba32)] public void Encode_Bit8_Works(TestImageProvider provider, TgaBitsPerPixel bitsPerPixel = TgaBitsPerPixel.Pixel8) // using tolerant comparer here. The results from magick differ slightly. Maybe a different ToGrey method is used. The image looks otherwise ok. - where TPixel : struct, IPixel => TestTgaEncoderCore(provider, bitsPerPixel, useCompression: true, useExactComparer: false, compareTolerance: 0.03f); + where TPixel : struct, IPixel => TestTgaEncoderCore(provider, bitsPerPixel, TgaCompression.None, useExactComparer: false, compareTolerance: 0.03f); [Theory] [WithFile(Bit32, PixelTypes.Rgba32)] public void Encode_Bit16_Works(TestImageProvider provider, TgaBitsPerPixel bitsPerPixel = TgaBitsPerPixel.Pixel16) - where TPixel : struct, IPixel => TestTgaEncoderCore(provider, bitsPerPixel, useCompression: false, useExactComparer: false); + where TPixel : struct, IPixel => TestTgaEncoderCore(provider, bitsPerPixel, TgaCompression.None, useExactComparer: false); [Theory] [WithFile(Bit32, PixelTypes.Rgba32)] public void Encode_Bit24_Works(TestImageProvider provider, TgaBitsPerPixel bitsPerPixel = TgaBitsPerPixel.Pixel24) - where TPixel : struct, IPixel => TestTgaEncoderCore(provider, bitsPerPixel); + where TPixel : struct, IPixel => TestTgaEncoderCore(provider, bitsPerPixel, TgaCompression.None); [Theory] [WithFile(Bit32, PixelTypes.Rgba32)] public void Encode_Bit32_Works(TestImageProvider provider, TgaBitsPerPixel bitsPerPixel = TgaBitsPerPixel.Pixel32) - where TPixel : struct, IPixel => TestTgaEncoderCore(provider, bitsPerPixel); + where TPixel : struct, IPixel => TestTgaEncoderCore(provider, bitsPerPixel, TgaCompression.None); [Theory] [WithFile(Bit32, PixelTypes.Rgba32)] public void Encode_Bit8_WithRunLengthEncoding_Works(TestImageProvider provider, TgaBitsPerPixel bitsPerPixel = TgaBitsPerPixel.Pixel8) // using tolerant comparer here. The results from magick differ slightly. Maybe a different ToGrey method is used. The image looks otherwise ok. - where TPixel : struct, IPixel => TestTgaEncoderCore(provider, bitsPerPixel, useCompression: true, useExactComparer: false, compareTolerance: 0.03f); + where TPixel : struct, IPixel => TestTgaEncoderCore(provider, bitsPerPixel, TgaCompression.RunLength, useExactComparer: false, compareTolerance: 0.03f); [Theory] [WithFile(Bit32, PixelTypes.Rgba32)] public void Encode_Bit16_WithRunLengthEncoding_Works(TestImageProvider provider, TgaBitsPerPixel bitsPerPixel = TgaBitsPerPixel.Pixel16) - where TPixel : struct, IPixel => TestTgaEncoderCore(provider, bitsPerPixel, useCompression: true, useExactComparer: false); + where TPixel : struct, IPixel => TestTgaEncoderCore(provider, bitsPerPixel, TgaCompression.RunLength, useExactComparer: false); [Theory] [WithFile(Bit32, PixelTypes.Rgba32)] public void Encode_Bit24_WithRunLengthEncoding_Works(TestImageProvider provider, TgaBitsPerPixel bitsPerPixel = TgaBitsPerPixel.Pixel24) - where TPixel : struct, IPixel => TestTgaEncoderCore(provider, bitsPerPixel, true); + where TPixel : struct, IPixel => TestTgaEncoderCore(provider, bitsPerPixel, TgaCompression.RunLength); [Theory] [WithFile(Bit32, PixelTypes.Rgba32)] public void Encode_Bit32_WithRunLengthEncoding_Works(TestImageProvider provider, TgaBitsPerPixel bitsPerPixel = TgaBitsPerPixel.Pixel32) - where TPixel : struct, IPixel => TestTgaEncoderCore(provider, bitsPerPixel, true); + where TPixel : struct, IPixel => TestTgaEncoderCore(provider, bitsPerPixel, TgaCompression.RunLength); private static void TestTgaEncoderCore( TestImageProvider provider, TgaBitsPerPixel bitsPerPixel, - bool useCompression = false, + TgaCompression compression = TgaCompression.None, bool useExactComparer = true, float compareTolerance = 0.01f) where TPixel : struct, IPixel { using (Image image = provider.GetImage()) { - var encoder = new TgaEncoder { BitsPerPixel = bitsPerPixel, Compress = useCompression}; + var encoder = new TgaEncoder { BitsPerPixel = bitsPerPixel, Compression = compression}; using (var memStream = new MemoryStream()) { diff --git a/tests/ImageSharp.Tests/ImageSharp.Tests.csproj b/tests/ImageSharp.Tests/ImageSharp.Tests.csproj index 1f6b8b4d9..1ac5f8085 100644 --- a/tests/ImageSharp.Tests/ImageSharp.Tests.csproj +++ b/tests/ImageSharp.Tests/ImageSharp.Tests.csproj @@ -14,7 +14,7 @@ - +