Code review changes

Directory.Build.targets

@@ -24,7 +24,7 @@
   <ItemGroup>
     <PackageReference Update="BenchmarkDotNet" Version="0.11.5" />
     <PackageReference Update="Colourful" Version="2.0.2" />
-    <PackageReference Update="Magick.NET-Q16-AnyCPU" Version="7.12.0" />
+    <PackageReference Update="Magick.NET-Q16-AnyCPU" Version="7.14.4" />
     <PackageReference Update="Microsoft.Net.Compilers.Toolset" Version="3.3.1" />
     <PackageReference Update="Microsoft.NET.Test.Sdk" Version="15.9.0" />
     <PackageReference Update="Moq" Version="4.10.0" />

src/ImageSharp/Formats/Tga/ITgaEncoderOptions.cs

@@ -16,6 +16,6 @@ namespace SixLabors.ImageSharp.Formats.Tga
         /// <summary>
         /// Gets a value indicating whether run length compression should be used.
         /// </summary>
-        bool Compress { get; }
+        TgaCompression Compression { get; }
     }
 }

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
+{
+    /// <summary>
+    /// Indicates if compression is used.
+    /// </summary>
+    public enum TgaCompression
+    {
+        /// <summary>
+        /// No compression is used.
+        /// </summary>
+        None,
+
+        /// <summary>
+        /// Run length encoding is used.
+        /// </summary>
+        RunLength,
+    }
+}

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;

src/ImageSharp/Formats/Tga/TgaEncoder.cs

@@ -19,9 +19,9 @@ namespace SixLabors.ImageSharp.Formats.Tga
         public TgaBitsPerPixel? BitsPerPixel { get; set; }
 
         /// <summary>
-        /// 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.
         /// </summary>
-        public bool Compress { get; set; }
+        public TgaCompression Compression { get; set; }
 
         /// <inheritdoc/>
         public void Encode<TPixel>(Image<TPixel> image, Stream stream)

src/ImageSharp/Formats/Tga/TgaEncoderCore.cs

@@ -43,7 +43,12 @@ namespace SixLabors.ImageSharp.Formats.Tga
         /// <summary>
         /// Indicates if run length compression should be used.
         /// </summary>
-        private readonly bool useCompression;
+        private readonly TgaCompression compression;
+
+        /// <summary>
+        /// Vector for converting pixel to gray value.
+        /// </summary>
+        private static readonly Vector4 Bt709 = new Vector4(.2126f, .7152f, .0722f, 0.0f);
 
         /// <summary>
         /// Initializes a new instance of the <see cref="TgaEncoderCore"/> 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;
         }
 
         /// <summary>
@@ -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
         /// <param name="vector">The vector to get the luminance from.</param>
         [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));
     }
 }

src/ImageSharp/Formats/Tga/TgaImageType.cs

@@ -12,7 +12,6 @@ namespace SixLabors.
     {
         /// <summary>
         /// No image data included.
-        /// Not sure what this is used for.
         /// </summary>
         NoImageData = 0,
 

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<TPixel>(TestImageProvider<TPixel> 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<TPixel> => TestTgaEncoderCore(provider, bitsPerPixel, useCompression: true, useExactComparer: false, compareTolerance: 0.03f);
+            where TPixel : struct, IPixel<TPixel> => TestTgaEncoderCore(provider, bitsPerPixel, TgaCompression.None, useExactComparer: false, compareTolerance: 0.03f);
 
         [Theory]
         [WithFile(Bit32, PixelTypes.Rgba32)]
         public void Encode_Bit16_Works<TPixel>(TestImageProvider<TPixel> provider, TgaBitsPerPixel bitsPerPixel = TgaBitsPerPixel.Pixel16)
-            where TPixel : struct, IPixel<TPixel> => TestTgaEncoderCore(provider, bitsPerPixel, useCompression: false, useExactComparer: false);
+            where TPixel : struct, IPixel<TPixel> => TestTgaEncoderCore(provider, bitsPerPixel, TgaCompression.None, useExactComparer: false);
 
         [Theory]
         [WithFile(Bit32, PixelTypes.Rgba32)]
         public void Encode_Bit24_Works<TPixel>(TestImageProvider<TPixel> provider, TgaBitsPerPixel bitsPerPixel = TgaBitsPerPixel.Pixel24)
-            where TPixel : struct, IPixel<TPixel> => TestTgaEncoderCore(provider, bitsPerPixel);
+            where TPixel : struct, IPixel<TPixel> => TestTgaEncoderCore(provider, bitsPerPixel, TgaCompression.None);
 
         [Theory]
         [WithFile(Bit32, PixelTypes.Rgba32)]
         public void Encode_Bit32_Works<TPixel>(TestImageProvider<TPixel> provider, TgaBitsPerPixel bitsPerPixel = TgaBitsPerPixel.Pixel32)
-            where TPixel : struct, IPixel<TPixel> => TestTgaEncoderCore(provider, bitsPerPixel);
+            where TPixel : struct, IPixel<TPixel> => TestTgaEncoderCore(provider, bitsPerPixel, TgaCompression.None);
 
         [Theory]
         [WithFile(Bit32, PixelTypes.Rgba32)]
         public void Encode_Bit8_WithRunLengthEncoding_Works<TPixel>(TestImageProvider<TPixel> 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<TPixel> => TestTgaEncoderCore(provider, bitsPerPixel, useCompression: true, useExactComparer: false, compareTolerance: 0.03f);
+            where TPixel : struct, IPixel<TPixel> => TestTgaEncoderCore(provider, bitsPerPixel, TgaCompression.RunLength, useExactComparer: false, compareTolerance: 0.03f);
 
         [Theory]
         [WithFile(Bit32, PixelTypes.Rgba32)]
         public void Encode_Bit16_WithRunLengthEncoding_Works<TPixel>(TestImageProvider<TPixel> provider, TgaBitsPerPixel bitsPerPixel = TgaBitsPerPixel.Pixel16)
-            where TPixel : struct, IPixel<TPixel> => TestTgaEncoderCore(provider, bitsPerPixel, useCompression: true, useExactComparer: false);
+            where TPixel : struct, IPixel<TPixel> => TestTgaEncoderCore(provider, bitsPerPixel, TgaCompression.RunLength, useExactComparer: false);
 
         [Theory]
         [WithFile(Bit32, PixelTypes.Rgba32)]
         public void Encode_Bit24_WithRunLengthEncoding_Works<TPixel>(TestImageProvider<TPixel> provider, TgaBitsPerPixel bitsPerPixel = TgaBitsPerPixel.Pixel24)
-            where TPixel : struct, IPixel<TPixel> => TestTgaEncoderCore(provider, bitsPerPixel, true);
+            where TPixel : struct, IPixel<TPixel> => TestTgaEncoderCore(provider, bitsPerPixel, TgaCompression.RunLength);
 
         [Theory]
         [WithFile(Bit32, PixelTypes.Rgba32)]
         public void Encode_Bit32_WithRunLengthEncoding_Works<TPixel>(TestImageProvider<TPixel> provider, TgaBitsPerPixel bitsPerPixel = TgaBitsPerPixel.Pixel32)
-            where TPixel : struct, IPixel<TPixel> => TestTgaEncoderCore(provider, bitsPerPixel, true);
+            where TPixel : struct, IPixel<TPixel> => TestTgaEncoderCore(provider, bitsPerPixel, TgaCompression.RunLength);
 
         private static void TestTgaEncoderCore<TPixel>(
             TestImageProvider<TPixel> provider,
             TgaBitsPerPixel bitsPerPixel,
-            bool useCompression = false,
+            TgaCompression compression = TgaCompression.None,
             bool useExactComparer = true,
             float compareTolerance = 0.01f)
             where TPixel : struct, IPixel<TPixel>
         {
             using (Image<TPixel> image = provider.GetImage())
             {
-                var encoder = new TgaEncoder { BitsPerPixel = bitsPerPixel, Compress = useCompression};
+                var encoder = new TgaEncoder { BitsPerPixel = bitsPerPixel, Compression = compression};
 
                 using (var memStream = new MemoryStream())
                 {

tests/ImageSharp.Tests/ImageSharp.Tests.csproj

@@ -14,7 +14,7 @@
   </PropertyGroup>
 
   <ItemGroup>
-    <PackageReference Include="Magick.NET-Q16-AnyCPU" Version="7.14.4" />
+    <PackageReference Include="Magick.NET-Q16-AnyCPU" />
     <PackageReference Include="Moq" />
     <PackageReference Include="System.Drawing.Common" />
     <PackageReference Include="xunit.runner.console" />