Skip to content

Use source length as bounds when unpacking RGB planes #2599

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.

Sign up for GitHub

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

Merged
JimBobSquarePants merged 2 commits into from
Dec 1, 2023
Merged

Use source length as bounds when unpacking RGB planes #2599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
291e9e7
Use source length as bounds
JimBobSquarePants Dec 1, 2023
d9ac175
Merge branch 'main' into js/fix-2595
JimBobSquarePants Dec 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/ImageSharp/PixelFormats/PixelImplementations/PixelOperations/Rgb24.PixelOperations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
16 changes: 13 additions & 3 deletions src/ImageSharp/PixelFormats/PixelOperations{TPixel}.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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>
Expand All @@ -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;

Expand All @@ -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)
{
Expand Down
36 changes: 30 additions & 6 deletions tests/ImageSharp.Tests/Formats/Jpg/JpegEncoderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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)]
Copy link
Member Author

@JimBobSquarePants JimBobSquarePants Dec 1, 2023

Choose a reason for hiding this comment

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

I'm explicitly using a real file here rather than a generated one to make it easier to do visual verification.

[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>
Expand Down Expand Up @@ -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
Expand Down