Merge pull request #2599 from SixLabors/js/fix-2595

Use source length as bounds when unpacking RGB planes

Author: JimBobSquarePants

src/ImageSharp/PixelFormats/PixelImplementations/PixelOperations/Rgb24.PixelOperations.cs

@@ -40,6 +40,9 @@ internal override void UnpackIntoRgbPlanes(
            Span<float> greenChannel,
            Span<float> blueChannel,
            ReadOnlySpan<Rgb24> source)
-           => SimdUtils.UnpackToRgbPlanes(redChannel, greenChannel, blueChannel, source);
+        {
+            GuardUnpackIntoRgbPlanes(redChannel, greenChannel, blueChannel, source);
+            SimdUtils.UnpackToRgbPlanes(redChannel, greenChannel, blueChannel, source);
+        }
     }
 }

src/ImageSharp/PixelFormats/PixelOperations{TPixel}.cs

@@ -165,7 +165,7 @@ public virtual void To<TDestinationPixel>(
     }
 
     /// <summary>
-    /// Bulk operation that packs 3 seperate RGB channels to <paramref name="destination"/>.
+    /// Bulk operation that packs 3 separate RGB channels to <paramref name="destination"/>.
     /// The destination must have a padding of 3.
     /// </summary>
     /// <param name="redChannel">A <see cref="ReadOnlySpan{T}"/> to the red values.</param>
@@ -198,7 +198,7 @@ internal virtual void PackFromRgbPlanes(
 
     /// <summary>
     /// Bulk operation that unpacks pixels from <paramref name="source"/>
-    /// into 3 seperate RGB channels. The destination must have a padding of 3.
+    /// into 3 separate RGB channels.
     /// </summary>
     /// <param name="redChannel">A <see cref="ReadOnlySpan{T}"/> to the red values.</param>
     /// <param name="greenChannel">A <see cref="ReadOnlySpan{T}"/> to the green values.</param>
@@ -210,7 +210,9 @@ internal virtual void UnpackIntoRgbPlanes(
         Span<float> blueChannel,
         ReadOnlySpan<TPixel> source)
     {
-        int count = redChannel.Length;
+        GuardUnpackIntoRgbPlanes(redChannel, greenChannel, blueChannel, source);
+
+        int count = source.Length;
 
         Rgba32 rgba32 = default;
 
@@ -227,6 +229,14 @@ internal virtual void UnpackIntoRgbPlanes(
         }
     }
 
+    [MethodImpl(InliningOptions.ShortMethod)]
+    internal static void GuardUnpackIntoRgbPlanes(Span<float> redChannel, Span<float> greenChannel, Span<float> blueChannel, ReadOnlySpan<TPixel> source)
+    {
+        Guard.IsTrue(greenChannel.Length == redChannel.Length, nameof(greenChannel), "Channels must be of same size!");
+        Guard.IsTrue(blueChannel.Length == redChannel.Length, nameof(blueChannel), "Channels must be of same size!");
+        Guard.IsTrue(source.Length <= redChannel.Length, nameof(source), "'source' span should not be bigger than the destination channels!");
+    }
+
     [MethodImpl(InliningOptions.ShortMethod)]
     internal static void GuardPackFromRgbPlanes(ReadOnlySpan<byte> greenChannel, ReadOnlySpan<byte> blueChannel, Span<TPixel> destination, int count)
     {

tests/ImageSharp.Tests/Formats/Jpg/JpegEncoderTests.cs

@@ -3,6 +3,7 @@
 
 using SixLabors.ImageSharp.Formats.Jpeg;
 using SixLabors.ImageSharp.PixelFormats;
+using SixLabors.ImageSharp.Processing;
 using SixLabors.ImageSharp.Tests.TestUtilities;
 using SixLabors.ImageSharp.Tests.TestUtilities.ImageComparison;
 
@@ -87,7 +88,7 @@ public void EncodeBaseline_NonInterleavedMode<TPixel>(TestImageProvider<TPixel>
     {
         using Image<TPixel> image = provider.GetImage();
 
-        var encoder = new JpegEncoder
+        JpegEncoder encoder = new()
         {
             Quality = quality,
             ColorType = colorType,
@@ -164,8 +165,8 @@ public void EncodeBaseline_WorksWithDiscontiguousBuffers<TPixel>(TestImageProvid
     [InlineData(JpegEncodingColor.YCbCrRatio444)]
     public async Task Encode_IsCancellable(JpegEncodingColor colorType)
     {
-        var cts = new CancellationTokenSource();
-        using var pausedStream = new PausedStream(new MemoryStream());
+        CancellationTokenSource cts = new();
+        using PausedStream pausedStream = new(new MemoryStream());
         pausedStream.OnWaiting(s =>
         {
             // after some writing
@@ -181,14 +182,37 @@ public async Task Encode_IsCancellable(JpegEncodingColor colorType)
             }
         });
 
-        using var image = new Image<Rgba32>(5000, 5000);
+        using Image<Rgba32> image = new(5000, 5000);
         await Assert.ThrowsAsync<TaskCanceledException>(async () =>
         {
-            var encoder = new JpegEncoder() { ColorType = colorType };
+            JpegEncoder encoder = new() { ColorType = colorType };
             await image.SaveAsync(pausedStream, encoder, cts.Token);
         });
     }
 
+    // https://github.com/SixLabors/ImageSharp/issues/2595
+    [Theory]
+    [WithFile(TestImages.Jpeg.Baseline.ForestBridgeDifferentComponentsQuality, PixelTypes.Bgra32)]
+    [WithFile(TestImages.Jpeg.Baseline.ForestBridgeDifferentComponentsQuality, PixelTypes.Rgb24)]
+    public static void Issue2595<TPixel>(TestImageProvider<TPixel> provider)
+        where TPixel : unmanaged, IPixel<TPixel>
+    {
+        using Image<TPixel> image = provider.GetImage();
+        image.Mutate(x => x.Crop(132, 1606));
+
+        int[] quality = new int[] { 100, 50 };
+        JpegEncodingColor[] colors = new[] { JpegEncodingColor.YCbCrRatio444, JpegEncodingColor.YCbCrRatio420 };
+        for (int i = 0; i < quality.Length; i++)
+        {
+            int q = quality[i];
+            for (int j = 0; j < colors.Length; j++)
+            {
+                JpegEncodingColor c = colors[j];
+                image.VerifyEncoder(provider, "jpeg", $"{q}-{c}", new JpegEncoder() { Quality = q, ColorType = c }, GetComparer(q, c));
+            }
+        }
+    }
+
     /// <summary>
     /// Anton's SUPER-SCIENTIFIC tolerance threshold calculation
     /// </summary>
@@ -225,7 +249,7 @@ private static void TestJpegEncoderCore<TPixel>(TestImageProvider<TPixel> provid
     {
         using Image<TPixel> image = provider.GetImage();
 
-        var encoder = new JpegEncoder
+        JpegEncoder encoder = new()
         {
             Quality = quality,
             ColorType = colorType