From 8ca712019e9d0bade8feaf3cf4adf192f4269f42 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Sat, 8 Aug 2026 21:01:51 +1000 Subject: [PATCH] Fix 16-bit binary PBM sample byte order The binary PGM and PPM formats store 16-bit samples most significant byte first. The decoder and encoder read and wrote them in native little-endian order, so every wide sample was byte swapped. This corrects the swizzled colors reported for 16-bit PPM images. - Reverse the sample byte order in BinaryDecoder and BinaryEncoder. - Fix integer division in the max pixel value upscale factor. - Add exact-value, reference-decoder, upscale, and round-trip tests. - Add a 16-bit PPM test image and its Magick-anchored reference PNG. - Document the binary decoder and encoder in full. --- src/ImageSharp/Formats/Pbm/BinaryDecoder.cs | 88 +++++++++++++++++-- src/ImageSharp/Formats/Pbm/BinaryEncoder.cs | 77 +++++++++++++++- src/ImageSharp/Formats/Pbm/PbmDecoderCore.cs | 6 +- .../Formats/Pbm/PbmDecoderTests.cs | 62 +++++++++++++ .../Formats/Pbm/PbmEncoderTests.cs | 40 +++++++++ .../Formats/Pbm/PbmRoundTripTests.cs | 34 +++++++ tests/ImageSharp.Tests/TestImages.cs | 1 + ...deReferenceImage_Rgb48_rgb_binary_wide.png | 3 + tests/Images/Input/Pbm/rgb_binary_wide.ppm | 3 + 9 files changed, 300 insertions(+), 14 deletions(-) create mode 100644 tests/Images/External/ReferenceOutput/PbmDecoderTests/DecodeReferenceImage_Rgb48_rgb_binary_wide.png create mode 100644 tests/Images/Input/Pbm/rgb_binary_wide.ppm diff --git a/src/ImageSharp/Formats/Pbm/BinaryDecoder.cs b/src/ImageSharp/Formats/Pbm/BinaryDecoder.cs index ce7e379fc5..a465907cf0 100644 --- a/src/ImageSharp/Formats/Pbm/BinaryDecoder.cs +++ b/src/ImageSharp/Formats/Pbm/BinaryDecoder.cs @@ -2,6 +2,8 @@ // Licensed under the Six Labors Split License. using System.Buffers; +using System.Buffers.Binary; +using System.Runtime.InteropServices; using SixLabors.ImageSharp.IO; using SixLabors.ImageSharp.Memory; using SixLabors.ImageSharp.PixelFormats; @@ -13,21 +15,25 @@ namespace SixLabors.ImageSharp.Formats.Pbm; /// internal class BinaryDecoder { + /// + /// The luminance value written for an unset bit in the black and white format. + /// private static L8 white = new(255); + + /// + /// The luminance value written for a set bit in the black and white format. + /// private static L8 black = new(0); /// /// Decode the specified pixels. /// - /// The type of pixel to encode to. + /// The type of pixel to decode to. /// The configuration. - /// The pixel array to encode into. + /// The pixel buffer to decode into. /// The stream to read the data from. - /// The ColorType to decode. - /// Data type of the pixles components. - /// - /// Thrown if an invalid combination of setting is requested. - /// + /// The color type of the encoded pixels. + /// The data type of the pixel components. public static void Process(Configuration configuration, Buffer2D pixels, BufferedReadStream stream, PbmColorType colorType, PbmComponentType componentType) where TPixel : unmanaged, IPixel { @@ -59,6 +65,15 @@ public static void Process(Configuration configuration, Buffer2D } } + /// + /// Decodes 8-bit binary grayscale (PGM) pixel data. + /// Each pixel is a single byte that holds its luminance value. + /// When the stream ends early, the rows that were not read keep their default value. + /// + /// The type of pixel to decode to. + /// The configuration. + /// The pixel buffer to decode into. + /// The stream to read the data from. private static void ProcessGrayscale(Configuration configuration, Buffer2D pixels, BufferedReadStream stream) where TPixel : unmanaged, IPixel { @@ -85,6 +100,15 @@ private static void ProcessGrayscale(Configuration configuration, Buffer } } + /// + /// Decodes 16-bit binary grayscale (PGM) pixel data. + /// Each pixel is one 16-bit sample, stored most significant byte first. + /// When the stream ends early, the rows that were not read keep their default value. + /// + /// The type of pixel to decode to. + /// The configuration. + /// The pixel buffer to decode into. + /// The stream to read the data from. private static void ProcessWideGrayscale(Configuration configuration, Buffer2D pixels, BufferedReadStream stream) where TPixel : unmanaged, IPixel { @@ -102,6 +126,10 @@ private static void ProcessWideGrayscale(Configuration configuration, Bu return; } + // The binary format stores 16-bit samples most significant byte first, + // but L16 expects native (little-endian) byte order. + SwapSampleBytes(rowSpan); + Span pixelSpan = pixels.DangerousGetRowSpan(y); PixelOperations.Instance.FromL16Bytes( configuration, @@ -111,6 +139,15 @@ private static void ProcessWideGrayscale(Configuration configuration, Bu } } + /// + /// Decodes 8-bit binary color (PPM) pixel data. + /// Each pixel is three bytes in red, green, blue order. + /// When the stream ends early, the rows that were not read keep their default value. + /// + /// The type of pixel to decode to. + /// The configuration. + /// The pixel buffer to decode into. + /// The stream to read the data from. private static void ProcessRgb(Configuration configuration, Buffer2D pixels, BufferedReadStream stream) where TPixel : unmanaged, IPixel { @@ -137,6 +174,15 @@ private static void ProcessRgb(Configuration configuration, Buffer2D + /// Decodes 16-bit binary color (PPM) pixel data. + /// Each pixel is three 16-bit samples in red, green, blue order, stored most significant byte first. + /// When the stream ends early, the rows that were not read keep their default value. + /// + /// The type of pixel to decode to. + /// The configuration. + /// The pixel buffer to decode into. + /// The stream to read the data from. private static void ProcessWideRgb(Configuration configuration, Buffer2D pixels, BufferedReadStream stream) where TPixel : unmanaged, IPixel { @@ -154,6 +200,10 @@ private static void ProcessWideRgb(Configuration configuration, Buffer2D return; } + // The binary format stores 16-bit samples most significant byte first, + // but Rgb48 expects native (little-endian) byte order. + SwapSampleBytes(rowSpan); + Span pixelSpan = pixels.DangerousGetRowSpan(y); PixelOperations.Instance.FromRgb48Bytes( configuration, @@ -163,6 +213,30 @@ private static void ProcessWideRgb(Configuration configuration, Buffer2D } } + /// + /// Reverses the byte order of each 16-bit sample in the given row when the host is little-endian. + /// The binary PGM and PPM formats store multi-byte samples most significant byte first. + /// + /// The row of big-endian sample data to convert in place. + private static void SwapSampleBytes(Span rowSpan) + { + if (BitConverter.IsLittleEndian) + { + Span samples = MemoryMarshal.Cast(rowSpan); + BinaryPrimitives.ReverseEndianness(samples, samples); + } + } + + /// + /// Decodes binary black and white (PBM) pixel data. + /// Each byte holds eight pixels, most significant bit first, and a set bit means black. + /// Each row starts on a byte boundary, so the last byte of a row can hold unused bits. + /// When the stream ends early, the pixels that were not read keep their default value. + /// + /// The type of pixel to decode to. + /// The configuration. + /// The pixel buffer to decode into. + /// The stream to read the data from. private static void ProcessBlackAndWhite(Configuration configuration, Buffer2D pixels, BufferedReadStream stream) where TPixel : unmanaged, IPixel { diff --git a/src/ImageSharp/Formats/Pbm/BinaryEncoder.cs b/src/ImageSharp/Formats/Pbm/BinaryEncoder.cs index 8b379e4d76..e86b703314 100644 --- a/src/ImageSharp/Formats/Pbm/BinaryEncoder.cs +++ b/src/ImageSharp/Formats/Pbm/BinaryEncoder.cs @@ -2,6 +2,8 @@ // Licensed under the Six Labors Split License. using System.Buffers; +using System.Buffers.Binary; +using System.Runtime.InteropServices; using SixLabors.ImageSharp.Memory; using SixLabors.ImageSharp.PixelFormats; @@ -13,14 +15,14 @@ namespace SixLabors.ImageSharp.Formats.Pbm; internal class BinaryEncoder { /// - /// Decode pixels into the PBM binary encoding. + /// Encode pixels into the PBM binary encoding. /// /// The type of input pixel. /// The configuration. /// The byte stream to write to. /// The input image. - /// The ColorType to use. - /// Data type of the pixels components. + /// The color type to use. + /// The data type of the pixel components. /// The token to monitor for cancellation requests. /// /// Thrown if an invalid combination of setting is requested. @@ -70,6 +72,15 @@ public static void WritePixels( } } + /// + /// Encodes 8-bit binary grayscale (PGM) pixel data. + /// Each pixel is written as a single byte that holds its luminance value. + /// + /// The type of input pixel. + /// The configuration. + /// The byte stream to write to. + /// The input image. + /// The token to monitor for cancellation requests. private static void WriteGrayscale( Configuration configuration, Stream stream, @@ -100,6 +111,15 @@ private static void WriteGrayscale( } } + /// + /// Encodes 16-bit binary grayscale (PGM) pixel data. + /// Each pixel is written as one 16-bit sample, most significant byte first. + /// + /// The type of input pixel. + /// The configuration. + /// The byte stream to write to. + /// The input image. + /// The token to monitor for cancellation requests. private static void WriteWideGrayscale( Configuration configuration, Stream stream, @@ -127,10 +147,23 @@ private static void WriteWideGrayscale( rowSpan, width); + // The binary format stores 16-bit samples most significant byte first, + // but ToL16Bytes produces native (little-endian) byte order. + SwapSampleBytes(rowSpan); + stream.Write(rowSpan); } } + /// + /// Encodes 8-bit binary color (PPM) pixel data. + /// Each pixel is written as three bytes in red, green, blue order. + /// + /// The type of input pixel. + /// The configuration. + /// The byte stream to write to. + /// The input image. + /// The token to monitor for cancellation requests. private static void WriteRgb( Configuration configuration, Stream stream, @@ -162,6 +195,15 @@ private static void WriteRgb( } } + /// + /// Encodes 16-bit binary color (PPM) pixel data. + /// Each pixel is written as three 16-bit samples in red, green, blue order, most significant byte first. + /// + /// The type of input pixel. + /// The configuration. + /// The byte stream to write to. + /// The input image. + /// The token to monitor for cancellation requests. private static void WriteWideRgb( Configuration configuration, Stream stream, @@ -189,10 +231,39 @@ private static void WriteWideRgb( rowSpan, width); + // The binary format stores 16-bit samples most significant byte first, + // but ToRgb48Bytes produces native (little-endian) byte order. + SwapSampleBytes(rowSpan); + stream.Write(rowSpan); } } + /// + /// Reverses the byte order of each 16-bit sample in the given row when the host is little-endian. + /// The binary PGM and PPM formats store multi-byte samples most significant byte first. + /// + /// The row of native-endian sample data to convert in place. + private static void SwapSampleBytes(Span rowSpan) + { + if (BitConverter.IsLittleEndian) + { + Span samples = MemoryMarshal.Cast(rowSpan); + BinaryPrimitives.ReverseEndianness(samples, samples); + } + } + + /// + /// Encodes binary black and white (PBM) pixel data. + /// Each byte holds eight pixels, most significant bit first, and a set bit means black. + /// A pixel with a luminance value less than 128 is written as black. + /// Each row starts on a byte boundary, so the last byte of a row can hold unused bits. + /// + /// The type of input pixel. + /// The configuration. + /// The byte stream to write to. + /// The input image. + /// The token to monitor for cancellation requests. private static void WriteBlackAndWhite( Configuration configuration, diff --git a/src/ImageSharp/Formats/Pbm/PbmDecoderCore.cs b/src/ImageSharp/Formats/Pbm/PbmDecoderCore.cs index 5451d3d461..938be62408 100644 --- a/src/ImageSharp/Formats/Pbm/PbmDecoderCore.cs +++ b/src/ImageSharp/Formats/Pbm/PbmDecoderCore.cs @@ -53,9 +53,7 @@ internal sealed class PbmDecoderCore : ImageDecoderCore /// The decoder options. public PbmDecoderCore(DecoderOptions options) : base(options) - { - this.configuration = options.Configuration; - } + => this.configuration = options.Configuration; /// protected override Image Decode(BufferedReadStream stream, CancellationToken cancellationToken) @@ -205,7 +203,7 @@ private void ProcessUpscaling(Image image) where TPixel : unmanaged, IPixel { int maxAllocationValue = this.componentType == PbmComponentType.Short ? 65535 : 255; - float factor = maxAllocationValue / this.maxPixelValue; + float factor = maxAllocationValue / (float)this.maxPixelValue; image.Mutate(x => x.Brightness(factor)); } diff --git a/tests/ImageSharp.Tests/Formats/Pbm/PbmDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Pbm/PbmDecoderTests.cs index 5c52c9785d..778ad75897 100644 --- a/tests/ImageSharp.Tests/Formats/Pbm/PbmDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Pbm/PbmDecoderTests.cs @@ -7,6 +7,7 @@ using SixLabors.ImageSharp.PixelFormats; using SixLabors.ImageSharp.Tests.TestUtilities; using SixLabors.ImageSharp.Tests.TestUtilities.ImageComparison; +using SixLabors.ImageSharp.Tests.TestUtilities.ReferenceCodecs; using static SixLabors.ImageSharp.Tests.TestImages.Pbm; // ReSharper disable InconsistentNaming @@ -26,6 +27,7 @@ public class PbmDecoderTests [InlineData(RgbPlain, PbmColorType.Rgb, PbmComponentType.Byte)] [InlineData(RgbPlainMagick, PbmColorType.Rgb, PbmComponentType.Byte)] [InlineData(RgbBinary, PbmColorType.Rgb, PbmComponentType.Byte)] + [InlineData(RgbBinaryWide, PbmColorType.Rgb, PbmComponentType.Short)] public void ImageLoadCanDecode(string imagePath, PbmColorType expectedColorType, PbmComponentType expectedComponentType) { // Arrange @@ -91,6 +93,7 @@ public void ImageLoadRgb24CanDecode(string imagePath) [WithFile(RgbPlain, PixelTypes.Rgb24, "ppm")] [WithFile(RgbPlainNormalized, PixelTypes.Rgb24, "ppm")] [WithFile(RgbBinary, PixelTypes.Rgb24, "ppm")] + [WithFile(RgbBinaryWide, PixelTypes.Rgb48, "ppm")] public void DecodeReferenceImage(TestImageProvider provider, string extension) where TPixel : unmanaged, IPixel { @@ -101,6 +104,65 @@ public void DecodeReferenceImage(TestImageProvider provider, str image.CompareToReferenceOutput(provider, grayscale: isGrayscale); } + [Theory] + [WithFile(GrayscaleBinaryWide, PixelTypes.Rgb48)] + [WithFile(RgbBinaryWide, PixelTypes.Rgb48)] + public void Decode_WideBinary_MatchesReferenceDecoder(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + using Image image = provider.GetImage(PbmDecoder.Instance); + image.CompareToOriginal(provider, ImageComparer.Exact, new MagickReferenceDecoder(PbmFormat.Instance)); + } + + [Fact] + public void Decode_WideBinaryGrayscale_SamplesAreBigEndian() + { + // Per the Netpbm specification, 16-bit samples store the most significant byte first. + byte[] header = Encoding.ASCII.GetBytes("P5\n2 1\n65535\n"); + byte[] samples = [0x80, 0x00, 0x00, 0x80]; + byte[] data = [.. header, .. samples]; + + using Image image = Image.Load(data); + + Assert.Equal(0x8000, image[0, 0].PackedValue); + Assert.Equal(0x0080, image[1, 0].PackedValue); + } + + [Fact] + public void Decode_WideBinaryRgb_SamplesAreBigEndian() + { + // Per the Netpbm specification, 16-bit samples store the most significant byte first. + byte[] header = Encoding.ASCII.GetBytes("P6\n1 1\n65535\n"); + byte[] samples = [0x81, 0xB5, 0x84, 0x91, 0x86, 0x71]; + byte[] data = [.. header, .. samples]; + + using Image image = Image.Load(data); + + Assert.Equal(new Rgb48(0x81B5, 0x8491, 0x8671), image[0, 0]); + } + + [Fact] + public void Decode_NonStandardByteMaxPixelValue_UpscalesToFullRange() + { + byte[] data = Encoding.ASCII.GetBytes("P2\n1 1\n100\n100"); + + using Image image = Image.Load(data); + + Assert.Equal(255, image[0, 0].PackedValue); + } + + [Fact] + public void Decode_NonStandardShortMaxPixelValue_UpscalesToFullRange() + { + byte[] header = Encoding.ASCII.GetBytes("P5\n1 1\n1000\n"); + byte[] samples = [0x03, 0xE8]; + byte[] data = [.. header, .. samples]; + + using Image image = Image.Load(data); + + Assert.Equal(65535, image[0, 0].PackedValue); + } + [Theory] [WithFile(RgbPlain, PixelTypes.Rgb24)] public void PbmDecoder_Decode_Resize(TestImageProvider provider) diff --git a/tests/ImageSharp.Tests/Formats/Pbm/PbmEncoderTests.cs b/tests/ImageSharp.Tests/Formats/Pbm/PbmEncoderTests.cs index 0fea65f6ea..5ef075404a 100644 --- a/tests/ImageSharp.Tests/Formats/Pbm/PbmEncoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Pbm/PbmEncoderTests.cs @@ -31,6 +31,7 @@ public class PbmEncoderTests { GrayscaleBinaryWide, PbmColorType.Grayscale }, { GrayscalePlain, PbmColorType.Grayscale }, { RgbBinary, PbmColorType.Rgb }, + { RgbBinaryWide, PbmColorType.Rgb }, { RgbPlain, PbmColorType.Rgb }, }; @@ -122,6 +123,45 @@ public void PbmEncoder_P3_Works(TestImageProvider provider) public void PbmEncoder_P6_Works(TestImageProvider provider) where TPixel : unmanaged, IPixel => TestPbmEncoderCore(provider, PbmColorType.Rgb, PbmEncoding.Binary); + [Fact] + public void PbmEncoder_WideBinaryGrayscale_WritesBigEndianSamples() + { + // Per the Netpbm specification, 16-bit samples store the most significant byte first. + using Image image = new(2, 1); + image[0, 0] = new L16(0x8000); + image[1, 0] = new L16(0x1234); + + using MemoryStream memStream = new(); + image.Save(memStream, new PbmEncoder + { + ColorType = PbmColorType.Grayscale, + ComponentType = PbmComponentType.Short, + Encoding = PbmEncoding.Binary + }); + + byte[] encoded = memStream.ToArray(); + Assert.Equal(new byte[] { 0x80, 0x00, 0x12, 0x34 }, encoded[^4..]); + } + + [Fact] + public void PbmEncoder_WideBinaryRgb_WritesBigEndianSamples() + { + // Per the Netpbm specification, 16-bit samples store the most significant byte first. + using Image image = new(1, 1); + image[0, 0] = new Rgb48(0x8000, 0x1234, 0x00FF); + + using MemoryStream memStream = new(); + image.Save(memStream, new PbmEncoder + { + ColorType = PbmColorType.Rgb, + ComponentType = PbmComponentType.Short, + Encoding = PbmEncoding.Binary + }); + + byte[] encoded = memStream.ToArray(); + Assert.Equal(new byte[] { 0x80, 0x00, 0x12, 0x34, 0x00, 0xFF }, encoded[^6..]); + } + private static void TestPbmEncoderCore( TestImageProvider provider, PbmColorType colorType, diff --git a/tests/ImageSharp.Tests/Formats/Pbm/PbmRoundTripTests.cs b/tests/ImageSharp.Tests/Formats/Pbm/PbmRoundTripTests.cs index 6524b35065..6fca91e0d0 100644 --- a/tests/ImageSharp.Tests/Formats/Pbm/PbmRoundTripTests.cs +++ b/tests/ImageSharp.Tests/Formats/Pbm/PbmRoundTripTests.cs @@ -54,6 +54,40 @@ public void PbmColorImageCanRoundTrip(string imagePath) ImageComparer.Exact.VerifySimilarity(originalImage, encodedImage); } + [Theory] + [InlineData(GrayscaleBinaryWide)] + public void PbmWideGrayscaleImageCanRoundTrip(string imagePath) + { + // Arrange + TestFile testFile = TestFile.Create(imagePath); + using MemoryStream stream = new(testFile.Bytes, false); + + // Act + using Image originalImage = Image.Load(stream); + using Image encodedImage = this.RoundTrip(originalImage); + + // Assert + Assert.NotNull(encodedImage); + ImageComparer.Exact.VerifySimilarity(originalImage, encodedImage); + } + + [Theory] + [InlineData(RgbBinaryWide)] + public void PbmWideColorImageCanRoundTrip(string imagePath) + { + // Arrange + TestFile testFile = TestFile.Create(imagePath); + using MemoryStream stream = new(testFile.Bytes, false); + + // Act + using Image originalImage = Image.Load(stream); + using Image encodedImage = this.RoundTrip(originalImage); + + // Assert + Assert.NotNull(encodedImage); + ImageComparer.Exact.VerifySimilarity(originalImage, encodedImage); + } + private Image RoundTrip(Image originalImage) where TPixel : unmanaged, IPixel { diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 2622a0fb9b..c0071e9062 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -1257,6 +1257,7 @@ public static class Pbm public const string GrayscalePlainNormalized = "Pbm/grayscale_plain_normalized.pgm"; public const string GrayscalePlainMagick = "Pbm/grayscale_plain_magick.pgm"; public const string RgbBinary = "Pbm/00000_00000.ppm"; + public const string RgbBinaryWide = "Pbm/rgb_binary_wide.ppm"; public const string RgbBinaryPrematureEof = "Pbm/00000_00000_premature_eof.ppm"; public const string RgbPlain = "Pbm/rgb_plain.ppm"; public const string RgbPlainNormalized = "Pbm/rgb_plain_normalized.ppm"; diff --git a/tests/Images/External/ReferenceOutput/PbmDecoderTests/DecodeReferenceImage_Rgb48_rgb_binary_wide.png b/tests/Images/External/ReferenceOutput/PbmDecoderTests/DecodeReferenceImage_Rgb48_rgb_binary_wide.png new file mode 100644 index 0000000000..a1b407528f --- /dev/null +++ b/tests/Images/External/ReferenceOutput/PbmDecoderTests/DecodeReferenceImage_Rgb48_rgb_binary_wide.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:2bb443147c06e4d712421695f5cda1c9823c75ac8be7fbf6ce067c51d01445bf +size 5182 diff --git a/tests/Images/Input/Pbm/rgb_binary_wide.ppm b/tests/Images/Input/Pbm/rgb_binary_wide.ppm new file mode 100644 index 0000000000..20f11554f6 --- /dev/null +++ b/tests/Images/Input/Pbm/rgb_binary_wide.ppm @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:10cb6013ed7f17fd29b857189e864b11057fc34cdbf6d719c14470f7f2c34743 +size 5235