Merge pull request #3164 from SixLabors/js/fix-pbm-16bit-endianness

Fix 16-bit binary PBM sample byte order

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;
 /// </summary>
 internal class BinaryDecoder
 {
+    /// <summary>
+    /// The luminance value written for an unset bit in the black and white format.
+    /// </summary>
     private static L8 white = new(255);
+
+    /// <summary>
+    /// The luminance value written for a set bit in the black and white format.
+    /// </summary>
     private static L8 black = new(0);
 
     /// <summary>
     /// Decode the specified pixels.
     /// </summary>
-    /// <typeparam name="TPixel">The type of pixel to encode to.</typeparam>
+    /// <typeparam name="TPixel">The type of pixel to decode to.</typeparam>
     /// <param name="configuration">The configuration.</param>
-    /// <param name="pixels">The pixel array to encode into.</param>
+    /// <param name="pixels">The pixel buffer to decode into.</param>
     /// <param name="stream">The stream to read the data from.</param>
-    /// <param name="colorType">The ColorType to decode.</param>
-    /// <param name="componentType">Data type of the pixles components.</param>
-    /// <exception cref="InvalidImageContentException">
-    /// Thrown if an invalid combination of setting is requested.
-    /// </exception>
+    /// <param name="colorType">The color type of the encoded pixels.</param>
+    /// <param name="componentType">The data type of the pixel components.</param>
     public static void Process<TPixel>(Configuration configuration, Buffer2D<TPixel> pixels, BufferedReadStream stream, PbmColorType colorType, PbmComponentType componentType)
         where TPixel : unmanaged, IPixel<TPixel>
     {
@@ -59,6 +65,15 @@ internal class BinaryDecoder
         }
     }
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    /// <typeparam name="TPixel">The type of pixel to decode to.</typeparam>
+    /// <param name="configuration">The configuration.</param>
+    /// <param name="pixels">The pixel buffer to decode into.</param>
+    /// <param name="stream">The stream to read the data from.</param>
     private static void ProcessGrayscale<TPixel>(Configuration configuration, Buffer2D<TPixel> pixels, BufferedReadStream stream)
         where TPixel : unmanaged, IPixel<TPixel>
     {
@@ -85,6 +100,15 @@ internal class BinaryDecoder
         }
     }
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    /// <typeparam name="TPixel">The type of pixel to decode to.</typeparam>
+    /// <param name="configuration">The configuration.</param>
+    /// <param name="pixels">The pixel buffer to decode into.</param>
+    /// <param name="stream">The stream to read the data from.</param>
     private static void ProcessWideGrayscale<TPixel>(Configuration configuration, Buffer2D<TPixel> pixels, BufferedReadStream stream)
         where TPixel : unmanaged, IPixel<TPixel>
     {
@@ -102,6 +126,10 @@ internal class BinaryDecoder
                 return;
             }
 
+            // The binary format stores 16-bit samples most significant byte first,
+            // but L16 expects native (little-endian) byte order.
+            SwapSampleBytes(rowSpan);
+
             Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
             PixelOperations<TPixel>.Instance.FromL16Bytes(
                 configuration,
@@ -111,6 +139,15 @@ internal class BinaryDecoder
         }
     }
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    /// <typeparam name="TPixel">The type of pixel to decode to.</typeparam>
+    /// <param name="configuration">The configuration.</param>
+    /// <param name="pixels">The pixel buffer to decode into.</param>
+    /// <param name="stream">The stream to read the data from.</param>
     private static void ProcessRgb<TPixel>(Configuration configuration, Buffer2D<TPixel> pixels, BufferedReadStream stream)
         where TPixel : unmanaged, IPixel<TPixel>
     {
@@ -137,6 +174,15 @@ internal class BinaryDecoder
         }
     }
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    /// <typeparam name="TPixel">The type of pixel to decode to.</typeparam>
+    /// <param name="configuration">The configuration.</param>
+    /// <param name="pixels">The pixel buffer to decode into.</param>
+    /// <param name="stream">The stream to read the data from.</param>
     private static void ProcessWideRgb<TPixel>(Configuration configuration, Buffer2D<TPixel> pixels, BufferedReadStream stream)
         where TPixel : unmanaged, IPixel<TPixel>
     {
@@ -154,6 +200,10 @@ internal class BinaryDecoder
                 return;
             }
 
+            // The binary format stores 16-bit samples most significant byte first,
+            // but Rgb48 expects native (little-endian) byte order.
+            SwapSampleBytes(rowSpan);
+
             Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
             PixelOperations<TPixel>.Instance.FromRgb48Bytes(
                 configuration,
@@ -163,6 +213,30 @@ internal class BinaryDecoder
         }
     }
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    /// <param name="rowSpan">The row of big-endian sample data to convert in place.</param>
+    private static void SwapSampleBytes(Span<byte> rowSpan)
+    {
+        if (BitConverter.IsLittleEndian)
+        {
+            Span<ushort> samples = MemoryMarshal.Cast<byte, ushort>(rowSpan);
+            BinaryPrimitives.ReverseEndianness(samples, samples);
+        }
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    /// <typeparam name="TPixel">The type of pixel to decode to.</typeparam>
+    /// <param name="configuration">The configuration.</param>
+    /// <param name="pixels">The pixel buffer to decode into.</param>
+    /// <param name="stream">The stream to read the data from.</param>
     private static void ProcessBlackAndWhite<TPixel>(Configuration configuration, Buffer2D<TPixel> pixels, BufferedReadStream stream)
         where TPixel : unmanaged, IPixel<TPixel>
     {

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
 {
     /// <summary>
-    /// Decode pixels into the PBM binary encoding.
+    /// Encode pixels into the PBM binary encoding.
     /// </summary>
     /// <typeparam name="TPixel">The type of input pixel.</typeparam>
     /// <param name="configuration">The configuration.</param>
     /// <param name="stream">The byte stream to write to.</param>
     /// <param name="image">The input image.</param>
-    /// <param name="colorType">The ColorType to use.</param>
-    /// <param name="componentType">Data type of the pixels components.</param>
+    /// <param name="colorType">The color type to use.</param>
+    /// <param name="componentType">The data type of the pixel components.</param>
     /// <param name="cancellationToken">The token to monitor for cancellation requests.</param>
     /// <exception cref="ImageFormatException">
     /// Thrown if an invalid combination of setting is requested.
@@ -70,6 +72,15 @@ internal class BinaryEncoder
         }
     }
 
+    /// <summary>
+    /// Encodes 8-bit binary grayscale (PGM) pixel data.
+    /// Each pixel is written as a single byte that holds its luminance value.
+    /// </summary>
+    /// <typeparam name="TPixel">The type of input pixel.</typeparam>
+    /// <param name="configuration">The configuration.</param>
+    /// <param name="stream">The byte stream to write to.</param>
+    /// <param name="image">The input image.</param>
+    /// <param name="cancellationToken">The token to monitor for cancellation requests.</param>
     private static void WriteGrayscale<TPixel>(
         Configuration configuration,
         Stream stream,
@@ -100,6 +111,15 @@ internal class BinaryEncoder
         }
     }
 
+    /// <summary>
+    /// Encodes 16-bit binary grayscale (PGM) pixel data.
+    /// Each pixel is written as one 16-bit sample, most significant byte first.
+    /// </summary>
+    /// <typeparam name="TPixel">The type of input pixel.</typeparam>
+    /// <param name="configuration">The configuration.</param>
+    /// <param name="stream">The byte stream to write to.</param>
+    /// <param name="image">The input image.</param>
+    /// <param name="cancellationToken">The token to monitor for cancellation requests.</param>
     private static void WriteWideGrayscale<TPixel>(
         Configuration configuration,
         Stream stream,
@@ -127,10 +147,23 @@ internal class BinaryEncoder
                 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);
         }
     }
 
+    /// <summary>
+    /// Encodes 8-bit binary color (PPM) pixel data.
+    /// Each pixel is written as three bytes in red, green, blue order.
+    /// </summary>
+    /// <typeparam name="TPixel">The type of input pixel.</typeparam>
+    /// <param name="configuration">The configuration.</param>
+    /// <param name="stream">The byte stream to write to.</param>
+    /// <param name="image">The input image.</param>
+    /// <param name="cancellationToken">The token to monitor for cancellation requests.</param>
     private static void WriteRgb<TPixel>(
         Configuration configuration,
         Stream stream,
@@ -162,6 +195,15 @@ internal class BinaryEncoder
         }
     }
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    /// <typeparam name="TPixel">The type of input pixel.</typeparam>
+    /// <param name="configuration">The configuration.</param>
+    /// <param name="stream">The byte stream to write to.</param>
+    /// <param name="image">The input image.</param>
+    /// <param name="cancellationToken">The token to monitor for cancellation requests.</param>
     private static void WriteWideRgb<TPixel>(
         Configuration configuration,
         Stream stream,
@@ -189,10 +231,39 @@ internal class BinaryEncoder
                 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);
         }
     }
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    /// <param name="rowSpan">The row of native-endian sample data to convert in place.</param>
+    private static void SwapSampleBytes(Span<byte> rowSpan)
+    {
+        if (BitConverter.IsLittleEndian)
+        {
+            Span<ushort> samples = MemoryMarshal.Cast<byte, ushort>(rowSpan);
+            BinaryPrimitives.ReverseEndianness(samples, samples);
+        }
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    /// <typeparam name="TPixel">The type of input pixel.</typeparam>
+    /// <param name="configuration">The configuration.</param>
+    /// <param name="stream">The byte stream to write to.</param>
+    /// <param name="image">The input image.</param>
+    /// <param name="cancellationToken">The token to monitor for cancellation requests.</param>
     private static void WriteBlackAndWhite<TPixel>(
         Configuration
         configuration,

src/ImageSharp/Formats/Pbm/PbmDecoderCore.cs

@@ -53,9 +53,7 @@ internal sealed class PbmDecoderCore : ImageDecoderCore
     /// <param name="options">The decoder options.</param>
     public PbmDecoderCore(DecoderOptions options)
         : base(options)
-    {
-        this.configuration = options.Configuration;
-    }
+        => this.configuration = options.Configuration;
 
     /// <inheritdoc/>
     protected override Image<TPixel> Decode<TPixel>(BufferedReadStream stream, CancellationToken cancellationToken)
@@ -205,7 +203,7 @@ internal sealed class PbmDecoderCore : ImageDecoderCore
         where TPixel : unmanaged, IPixel<TPixel>
     {
         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));
     }
 

tests/ImageSharp.Tests/Formats/Pbm/PbmDecoderTests.cs

@@ -7,6 +7,7 @@ using SixLabors.ImageSharp.Formats.Pbm;
 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 class PbmDecoderTests
     [WithFile(RgbPlain, PixelTypes.Rgb24, "ppm")]
     [WithFile(RgbPlainNormalized, PixelTypes.Rgb24, "ppm")]
     [WithFile(RgbBinary, PixelTypes.Rgb24, "ppm")]
+    [WithFile(RgbBinaryWide, PixelTypes.Rgb48, "ppm")]
     public void DecodeReferenceImage<TPixel>(TestImageProvider<TPixel> provider, string extension)
         where TPixel : unmanaged, IPixel<TPixel>
     {
@@ -101,6 +104,65 @@ public class PbmDecoderTests
         image.CompareToReferenceOutput(provider, grayscale: isGrayscale);
     }
 
+    [Theory]
+    [WithFile(GrayscaleBinaryWide, PixelTypes.Rgb48)]
+    [WithFile(RgbBinaryWide, PixelTypes.Rgb48)]
+    public void Decode_WideBinary_MatchesReferenceDecoder<TPixel>(TestImageProvider<TPixel> provider)
+        where TPixel : unmanaged, IPixel<TPixel>
+    {
+        using Image<TPixel> 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<L16> image = Image.Load<L16>(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<Rgb48> image = Image.Load<Rgb48>(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<L8> image = Image.Load<L8>(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<L16> image = Image.Load<L16>(data);
+
+        Assert.Equal(65535, image[0, 0].PackedValue);
+    }
+
     [Theory]
     [WithFile(RgbPlain, PixelTypes.Rgb24)]
     public void PbmDecoder_Decode_Resize<TPixel>(TestImageProvider<TPixel> provider)

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 class PbmEncoderTests
     public void PbmEncoder_P6_Works<TPixel>(TestImageProvider<TPixel> provider)
         where TPixel : unmanaged, IPixel<TPixel> => 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<L16> 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<Rgb48> 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<TPixel>(
         TestImageProvider<TPixel> provider,
         PbmColorType colorType,

tests/ImageSharp.Tests/Formats/Pbm/PbmRoundTripTests.cs

@@ -54,6 +54,40 @@ public class PbmRoundTripTests
         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<L16> originalImage = Image.Load<L16>(stream);
+        using Image<L16> 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<Rgb48> originalImage = Image.Load<Rgb48>(stream);
+        using Image<Rgb48> encodedImage = this.RoundTrip(originalImage);
+
+        // Assert
+        Assert.NotNull(encodedImage);
+        ImageComparer.Exact.VerifySimilarity(originalImage, encodedImage);
+    }
+
     private Image<TPixel> RoundTrip<TPixel>(Image<TPixel> originalImage)
         where TPixel : unmanaged, IPixel<TPixel>
     {

tests/ImageSharp.Tests/TestImages.cs

@@ -1257,6 +1257,7 @@ public static class TestImages
         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";

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

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