DW-9: Load image using original BPP and support changing color depth#149
Conversation
mee-ironsoftware
left a comment
There was a problem hiding this comment.
Approving — the metadata-reading core is sound and well-modeled on the existing GetTiffFrameCountFast, and decoupling GetStride from the reported BitsPerPixel shows the seam was considered. A few things to address before/after merge (none blocking):
Findings outside the diff
AnyBitmap.cs:1319— theScan0XML doc still states it returns "the first 32bpp BGRA pixel data". After this PR that directly contradictsBitsPerPixelreporting 1 for a B&W TIFF. Update the doc and the release notes to call out thatBitsPerPixelis now decoupled fromScan0/Stride, and grep the downstream consumers (IronOCR/IronPDF) for.BitsPerPixelpaired withScan0buffer math.- Checklist: "run all unit tests on Linux" is unchecked, and the headline repro test is
[IgnoreOnUnixFact]. The non-Unix-ignoredDW_9_LoadImage_ShouldReturnOriginalBitsPerPixeltheory should be confirmed green on Linux/LibTiff before merge.
Verdict: fix-then-ship.
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| /// <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) |
There was a problem hiding this comment.
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 GetDefaultImageExportEncoder → GetDefaultImageEncoder → 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.
There was a problem hiding this comment.
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.
| /// is not one of the supported values.</exception> | ||
| public AnyBitmap ChangeBitsPerPixel(int bitsPerPixel) | ||
| { | ||
| Image source = GetFirstInternalImage(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Documented single-frame behavior in the XML doc + inline comment.
| /// 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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Merged GetTiffFrameCountFast + GetTiffBitsPerPixelFast into one ReadTiffMetadataFast() that returns (FrameCount, BitsPerPixel) from a single open.
|
|
||
| var converted = bitmap.ChangeBitsPerPixel(targetBitsPerPixel); | ||
|
|
||
| Assert.Equal(targetBitsPerPixel, converted.BitsPerPixel); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Added DW_9_BitsPerPixel_IsIntentionallyDecoupledFromStrideAndScan0 (locks the decoupling) and DW_9_ChangeBitsPerPixel_DurabilityDependsOnEncoder (PNG→8, BMP/GetBytes→32).
Description
AnyBitmap.BitsPerPixelpreviously reported 32 bpp for every TIFF, including 1‑bpp black & white scans, because TIFFs are decoded into a 32‑bppRgba32image (via LibTiff / ImageSharp) and the property simply returned the in‑memory pixel depth. SixLabors.ImageSharp has no pixel format below 8 bpp, so the original color depth was lost.This PR makes
BitsPerPixelreport the original color depth of the source image when it is loaded preserving its original format (theFromFiledefault), matching whatSystem.Drawing.Bitmapreports (e.g.Format1bppIndexed→ 1).What changed (
AnyBitmap.cs):_originalBitsPerPixelfield, populated duringLoadImagefor TIFFs (whenpreserveOriginalFormat == true) by readingBitsPerSample × SamplesPerPixelstraight from the TIFF metadata via a new lightweightGetTiffBitsPerPixelFast()helper (modeled on the existingGetTiffFrameCountFast()— no full decode required).BitsPerPixelnow returns_originalBitsPerPixel ?? InMemoryBitsPerPixel. Non‑TIFF formats andpreserveOriginalFormat == falseare unchanged (still report the decoded depth, e.g. 32).GetStride()now uses the in‑memory pixel depth rather than the reportedBitsPerPixel, so stride stays consistent with the 32‑bpp pixel data exposed byGetFirstPixelData().ChangeBitsPerPixel(int)— theSystem.DrawingChangeBppequivalent. Returns a newAnyBitmapconverted to 8 (grayscale), 24 (RGB) or 32 (RGBA); throwsNotSupportedExceptionfor unsupported depths.Type of change
How Has This Been Tested?
Added unit tests in
AnyBitmapFunctionality.csusing the attached sample TIFFs (added to the testDatafolder):DW_9_LoadImage_ShouldReturnOriginalBitsPerPixel—ScanDev_BW.tif→1,ScanDev_Gray.tif→8,ScanDev_Color.tif→24DW_9_LoadBlackAndWhiteTiff_ShouldReturnOriginalBitsPerPixel_AndAllowChangingBpp— reproduces the exact reported scenario withtifimg.tif:BitsPerPixel == 1, cross‑checked againstnew System.Drawing.Bitmap(path).PixelFormat == Format1bppIndexed, plusChangeBitsPerPixel(24)→ 24DW_9_LoadImage_NotPreservingOriginalFormat_ShouldReturn32BitsPerPixel—preserveOriginalFormat: false→ 32DW_9_ChangeBitsPerPixel_ShouldReturnRequestedColorDepth— 8 / 24 / 32DW_9_ChangeBitsPerPixel_WithUnsupportedDepth_ShouldThrow—NotSupportedExceptionLocal results: full
AnyBitmapFunctionalitysuite passes — 103/103 (net8.0) and 101/101 (net48), no regressions.Checklist:
Additional Context
Behavior change for
BitsPerPixelon TIFFs loaded withpreserveOriginalFormat = true(theFromFiledefault):tifimg.tif(B&W)ScanDev_BW.tifScanDev_Gray.tifScanDev_Color.tif