Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Original file line number Diff line number Diff line change
Expand Up @@ -700,6 +700,71 @@ public void Should_Return_BitsPerPixel()
Assert.Equal(32, bitmap.BitsPerPixel);
}

[TheoryWithAutomaticDisplayName]
[InlineData("ScanDev_BW.tif", 1)]
[InlineData("ScanDev_Gray.tif", 8)]
[InlineData("ScanDev_Color.tif", 24)]
public void DW_9_LoadImage_ShouldReturnOriginalBitsPerPixel(string fileName, int expectedBitsPerPixel)
{
string imagePath = GetRelativeFilePath(fileName);

var bitmap = AnyBitmap.FromFile(imagePath);

Assert.Equal(expectedBitsPerPixel, bitmap.BitsPerPixel);
}

[IgnoreOnUnixFact]
public void DW_9_LoadBlackAndWhiteTiff_ShouldReturnOriginalBitsPerPixel_AndAllowChangingBpp()
{
string imagePath = GetRelativeFilePath("tifimg.tif");

var bitmap = AnyBitmap.FromFile(imagePath);

Assert.Equal(1, bitmap.BitsPerPixel);
Assert.Equal(PixelFormat.Format1bppIndexed, new Bitmap(imagePath).PixelFormat);

var converted = bitmap.ChangeBitsPerPixel(24);

Assert.Equal(24, converted.BitsPerPixel);
Assert.Equal(bitmap.Width, converted.Width);
Assert.Equal(bitmap.Height, converted.Height);
}

[FactWithAutomaticDisplayName]
public void DW_9_LoadImage_NotPreservingOriginalFormat_ShouldReturn32BitsPerPixel()
{
string imagePath = GetRelativeFilePath("ScanDev_BW.tif");

var bitmap = AnyBitmap.FromFile(imagePath, preserveOriginalFormat: false);

Assert.Equal(32, bitmap.BitsPerPixel);
}

[TheoryWithAutomaticDisplayName]
[InlineData(8)]
[InlineData(24)]
[InlineData(32)]
public void DW_9_ChangeBitsPerPixel_ShouldReturnRequestedColorDepth(int targetBitsPerPixel)
{
string imagePath = GetRelativeFilePath("ScanDev_BW.tif");
var bitmap = AnyBitmap.FromFile(imagePath);

var converted = bitmap.ChangeBitsPerPixel(targetBitsPerPixel);

Assert.Equal(targetBitsPerPixel, converted.BitsPerPixel);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: Happy-path only. ChangeBitsPerPixel(32) here asserts a clone is 32bpp (it already was — proves nothing), and no test round-trips a converted bitmap through SaveAs/GetBytes and reloads — which is exactly why the non-durable-depth issue above is invisible. Suggest adding a round-trip test (it will currently fail, documenting the gap) and a Stride/Scan0-vs-BitsPerPixel consistency assertion so the intentional decoupling is locked against future re-coupling.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added DW_9_BitsPerPixel_IsIntentionallyDecoupledFromStrideAndScan0 (locks the decoupling) and DW_9_ChangeBitsPerPixel_DurabilityDependsOnEncoder (PNG→8, BMP/GetBytes→32).

Assert.Equal(bitmap.Width, converted.Width);
Assert.Equal(bitmap.Height, converted.Height);
}

[FactWithAutomaticDisplayName]
public void DW_9_ChangeBitsPerPixel_WithUnsupportedDepth_ShouldThrow()
{
string imagePath = GetRelativeFilePath("ScanDev_BW.tif");
var bitmap = AnyBitmap.FromFile(imagePath);

Assert.Throws<NotSupportedException>(() => bitmap.ChangeBitsPerPixel(16));
}

[TheoryWithAutomaticDisplayName()]
[InlineData("mountainclimbers.jpg", "image/jpeg", AnyBitmap.ImageFormat.Jpeg)]
[InlineData("watermark.deployment.png", "image/png", AnyBitmap.ImageFormat.Png)]
Expand Down
93 changes: 90 additions & 3 deletions IronSoftware.Drawing/IronSoftware.Drawing.Common/AnyBitmap.cs
Original file line number Diff line number Diff line change
Expand Up @@ -980,13 +980,56 @@ public static AnyBitmap LoadAnyBitmapFromRGBBuffer(byte[] buffer, int width, int

//cache
private int? _bitsPerPixel = null;

/// <summary>
/// The color depth (bits per pixel) of the original source image when it can be
/// determined from the source metadata (e.g. TIFF). SixLabors.ImageSharp has no
/// pixel format below 8bpp, so indexed/bilevel sources would otherwise misreport
/// their depth once decoded into memory (e.g. a 1bpp black &amp; white TIFF that is
/// decoded to a 32bpp Rgba32 image). When set, this is reported by <see cref="BitsPerPixel"/>.
/// </summary>
private int? _originalBitsPerPixel = null;

//cache of the bits per pixel of the in-memory (decoded) image
private int InMemoryBitsPerPixel => _bitsPerPixel ??= GetFirstInternalImage().PixelType.BitsPerPixel;

/// <summary>
/// Gets colors depth, in number of bits per pixel.
/// <para>When the image is loaded preserving its original format, this reports the
/// bits per pixel of the original source image (for example, 1 for a black &amp; white
/// image) rather than the bits per pixel of the in-memory decoded representation.</para>
/// <br/><para><b>Further Documentation:</b><br/>
/// <a href="https://ironsoftware.com/open-source/csharp/drawing/examples/get-color-depth/">
/// Code Example</a></para>
/// </summary>
public int BitsPerPixel => _bitsPerPixel ??= GetFirstInternalImage().PixelType.BitsPerPixel;
public int BitsPerPixel => _originalBitsPerPixel ?? InMemoryBitsPerPixel;

/// <summary>
/// Creates a new <see cref="AnyBitmap"/> with the pixel data converted to the requested
/// color depth, in number of bits per pixel. This is analogous to changing the
/// <c>PixelFormat</c> of a <see cref="System.Drawing.Bitmap"/>.
/// </summary>
/// <param name="bitsPerPixel">The target color depth. Supported values are
/// <c>8</c> (grayscale), <c>24</c> (RGB) and <c>32</c> (RGBA).</param>
/// <returns>A new <see cref="AnyBitmap"/> whose <see cref="BitsPerPixel"/> equals
/// <paramref name="bitsPerPixel"/>.</returns>
/// <exception cref="NotSupportedException">Thrown when <paramref name="bitsPerPixel"/>
/// is not one of the supported values.</exception>
public AnyBitmap ChangeBitsPerPixel(int bitsPerPixel)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Major: The converted depth only lives in the in-memory PixelType; it is not durable across serialization. new AnyBitmap(converted) has no Binary, so the first Binary/SaveAs/GetBytes access re-encodes via GetDefaultImageExportEncoderGetDefaultImageEncoder → a 32bpp BmpEncoder (AnyBitmap.cs:3335). So bmp.ChangeBitsPerPixel(8).SaveAs("x.bmp") produces a 32bpp file. For a feature billed as the System.Drawing ChangeBpp equivalent (where the point is to save at that depth), either make the exporter honor the converted pixel type, or document clearly that this only affects the in-memory representation.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verified empirically (PNG is durable → 8; default BMP/GetBytes → 32). Documented the durability behavior precisely in the XML doc and locked it with a round-trip test.

{
Image source = GetFirstInternalImage();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor: GetFirstInternalImage() converts only frame 0, so calling this on a multi-page TIFF (the very format this PR targets) silently drops frames 2..N. Document the single-frame behavior or map over all frames.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Documented single-frame behavior in the XML doc + inline comment.

Image converted = bitsPerPixel switch
{
8 => source.CloneAs<L8>(),
24 => source.CloneAs<Rgb24>(),
32 => source.CloneAs<Rgba32>(),
_ => throw new NotSupportedException(
$"Changing bits per pixel to {bitsPerPixel} is not supported. " +
$"Supported values are 8, 24 and 32.")
};

return new AnyBitmap(converted);
}

//cache
private int? _frameCount = null;
Expand Down Expand Up @@ -2610,8 +2653,17 @@ private void LoadImage(Stream stream, bool preserveOriginalFormat)
private void LoadImage(ReadOnlySpan<byte> span, bool preserveOriginalFormat)
{
Binary = span.ToArray();
if (Format is TiffFormat)
if (Format is TiffFormat)
{
// TIFFs are decoded into a 32bpp Rgba32 image (via LibTiff or ImageSharp), which
// loses the original color depth. When preserving the original format, capture the
// source bits per pixel from the TIFF metadata so BitsPerPixel reports it faithfully
// (e.g. 1 for a black & white image) instead of the decoded 32bpp value.
if (preserveOriginalFormat)
{
_originalBitsPerPixel = GetTiffBitsPerPixelFast();
}

if(GetTiffFrameCountFast() > 1)
{
_lazyImage = OpenTiffToImageSharp();
Expand Down Expand Up @@ -2825,6 +2877,39 @@ private int GetTiffFrameCountFast()
}
}

/// <summary>
/// Reads the original bits per pixel of the first frame of the loaded TIFF directly from
/// its metadata (BitsPerSample x SamplesPerPixel), without fully decoding the image.
/// </summary>
/// <returns>The original bits per pixel, or <c>null</c> if it cannot be determined.</returns>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor: For every TIFF load this is now a second Tiff.ClientOpen over the same Binary (plus a redundant static SetErrorHandler), in addition to GetTiffFrameCountFast and InternalLoadTiff. Cheap (metadata-only) but trivially mergeable — one open could return both the directory count and frame-0 bits.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Merged GetTiffFrameCountFast + GetTiffBitsPerPixelFast into one ReadTiffMetadataFast() that returns (FrameCount, BitsPerPixel) from a single open.

private int? GetTiffBitsPerPixelFast()
{
try
{
using var tiffStream = new MemoryStream(Binary);

// Disable error messages for fast check
Tiff.SetErrorHandler(new DisableErrorHandler());

using var tiff = Tiff.ClientOpen("in-memory", "r", tiffStream, new TiffStream());
if (tiff == null) return null;

FieldValue[] bitsPerSampleField = tiff.GetField(TiffTag.BITSPERSAMPLE);
FieldValue[] samplesPerPixelField = tiff.GetField(TiffTag.SAMPLESPERPIXEL);

// BitsPerSample defaults to 1 and SamplesPerPixel defaults to 1 per the TIFF spec.
int bitsPerSample = bitsPerSampleField != null ? bitsPerSampleField[0].ToInt() : 1;
int samplesPerPixel = samplesPerPixelField != null ? samplesPerPixelField[0].ToInt() : 1;

int bitsPerPixel = bitsPerSample * samplesPerPixel;
return bitsPerPixel > 0 ? bitsPerPixel : (int?)null;
}
catch
{
return null; // Fall back to the in-memory pixel depth on any error
}
}

private Lazy<IReadOnlyList<Image>> OpenTiffToImageSharp()
{
return new Lazy<IReadOnlyList<Image>>(() =>
Expand Down Expand Up @@ -3114,7 +3199,9 @@ private int GetStride(Image source = null)
{
if (source == null)
{
return 4 * (((Width * BitsPerPixel) + 31) / 32);
// Use the in-memory pixel depth (not the reported original BitsPerPixel) so the
// stride stays consistent with the decoded pixel data exposed by GetFirstPixelData.
return 4 * (((Width * InMemoryBitsPerPixel) + 31) / 32);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Major: This decoupling is correct internally (Stride stays 32-based, consistent with the 32bpp BGRA buffer from GetFirstPixelData), but it creates a public-API inconsistency. After this PR a 1bpp TIFF reports BitsPerPixel == 1 while Stride and Scan0 still describe 32bpp data — System.Drawing never does that (a Format1bppIndexed bitmap has 1bpp BitsPerPixel, Stride, and Scan0, all consistent). Any external caller sizing a Scan0 buffer as Height * Width * BitsPerPixel / 8 now under-allocates 32x. Either expose a separate OriginalBitsPerPixel and leave BitsPerPixel reporting in-memory depth, or document the divergence explicitly as a breaking change and audit downstream consumers.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Documented the divergence explicitly on BitsPerPixel, Stride, and Scan0 XML docs (including "use Stride, not BitsPerPixel, for buffer math"). Did not take your suggested alternative (revert BitsPerPixel to in-memory depth + add OriginalBitsPerPixel) because the DW-9 acceptance criteria explicitly require BitsPerPixel == 1.

}
else
{
Expand Down
Loading