DW-40: Carry original BitsPerPixel through derived AnyBitmap operations and preserve depth on resize#154
Conversation
| // Resize a clone of the already-decoded image so its pixel type (and therefore its color | ||
| // depth) is preserved. | ||
| var image = original.GetFirstInternalImage().Clone(img => img.Resize(width, height)); | ||
|
|
||
| _lazyImage = new Lazy<IReadOnlyList<Image>>(() => | ||
| //update Binary | ||
| using (var memoryStream = new MemoryStream()) | ||
| { | ||
|
|
||
| var image = Image.Load<Rgba32>(Binary); | ||
| image.Mutate(img => img.Resize(width, height)); | ||
|
|
||
| //update Binary | ||
| using var memoryStream = new MemoryStream(); | ||
| image.Save(memoryStream, GetDefaultImageEncoder(image.Width, image.Height)); | ||
| Binary = memoryStream.ToArray(); | ||
| } | ||
|
|
||
| return [image]; | ||
| }); | ||
| _lazyImage = new Lazy<IReadOnlyList<Image>>(() => [image]); |
There was a problem hiding this comment.
actually we should move this code to be inside Lazy
//but this method call ForceLoadLazyImage(); anyway so it's not a deal breaker
There was a problem hiding this comment.
Good call, thanks. I have moved the clone, resize and encode work back inside the Lazy so the lazy-loading pattern is preserved. To keep it safe against the original's lifetime (what the old Binary = original.Binary line guarded), I capture the source's decoded image reference up front and then do the work inside the Lazy. Pushed. All AnyBitmapFunctionality tests still pass (123/123 net8.0, 121/121 net48).
abbf129 to
b08be16
Compare
mee-ironsoftware
left a comment
There was a problem hiding this comment.
Correct and well-tested for its stated scope (propagating the DW-9 source depth through derived operations and preserving depth on resize). Traced each path end-to-end: GetAllFrames per-frame alignment holds (thumbnails skipped in both lists), Clone() re-captures depth naturally, and resize preserves native pixel type. All findings below are minor/nit — the only one worth an explicit decision is #1 (multi-page TIFF resize).
| image.Mutate(img => img.Resize(width, height)); | ||
| // Resize a clone of the already-decoded image so its pixel type (and therefore its | ||
| // color depth) is preserved. | ||
| var image = sourceImage.Clone(img => img.Resize(width, height)); |
There was a problem hiding this comment.
Minor (behavior change, untested): cloning GetFirstInternalImage() takes page 0 only — for a multi-page TIFF InternalLoadTiff stores one Image per page, so images[0] is page 0. The old path Image.Load<Rgba32>(original.Binary) decoded all pages as frames before resizing, so a resized multi-page TIFF's FrameCount/GetAllFrames changes from N to 1. Narrow scope (only ImageSharp-decodable TIFFs; the saved BMP Binary only ever held page 0 anyway), but please confirm this is acceptable and note it in the resize doc, or clone across GetInternalImages() if multi-frame resize must be preserved.
There was a problem hiding this comment.
Fixed. LoadAndResizeImage now resizes across GetInternalImages() instead of just GetFirstInternalImage(), so a multi-page TIFF keeps its frame count (FrameCount and GetAllFrames stay N).
As a bonus it now also works for LibTiff-only TIFFs that the old Image.Load<Rgba32>(original.Binary) path could not decode, since it resizes the already-decoded pages.
Resize stays honest on depth (each resized page reports its in-memory depth), so _originalBitsPerPixel is intentionally not carried here. Updated the resize constructor doc to note that every page is resized, and added DW_40_Resize_ShouldPreserveFrameCountForMultiPageTiff.
| var result = new AnyBitmap(image); | ||
| // Redacting a region preserves the source color depth, so carry the original depth over | ||
| // (otherwise it would fall back to the decoded 32bpp value). | ||
| result._originalBitsPerPixel = bitmap._originalBitsPerPixel; |
There was a problem hiding this comment.
Minor (rationale inconsistency): this unconditionally carries _originalBitsPerPixel, but Redact(rect, Color.Red) on a 1bpp image produces pixels that can't exist in bilevel, yet BitsPerPixel still reports 1. That contradicts the "honesty" reasoning the PR applies to resize. The test only uses Color.Black (bilevel-safe), so it's untested. Low severity since BitsPerPixel is an explicitly decoupled in-memory label and re-encode drops it — either accept it (and drop the honesty framing as resize-specific) or skip carry-over for a non-monochrome redaction color. (RotateFlip is genuinely lossless, so carrying there is correct.)
There was a problem hiding this comment.
Accepted, and reworded the rationale. Redact still carries the source depth (it is the reported use case and is typically Color.Black), but the comment no longer claims the operation preserves color depth. It now states that Redact fills a region while leaving the rest untouched, so it carries the source's declared, decoupled BitsPerPixel label, and that this is a label only which is dropped on re-encode. It also calls out that resize is the exception because it resamples the whole image.
I considered skipping carry-over for a non-monochrome color, but a correct "is this color representable at N bpp" check is fuzzy across depths (1bpp bilevel vs 8bpp grayscale vs 24bpp), so I kept the simpler decoupled-label behavior and made the reasoning explicit instead.
| var result = new AnyBitmap(cloned); | ||
| // Cropping preserves the source color depth, so carry the original depth over (otherwise | ||
| // it would fall back to the decoded 32bpp value). | ||
| result._originalBitsPerPixel = _originalBitsPerPixel; |
There was a problem hiding this comment.
Nit: Clone(Rectangle), RotateFlip, and Redact copy the scalar _originalBitsPerPixel but not _framesOriginalBitsPerPixel, so calling GetAllFrames on a cropped/rotated/redacted multi-page TIFF falls back to the frame-0 depth for every frame (mixed-depth info lost). Edge-of-edge case — noting for completeness, not asking for a fix.
There was a problem hiding this comment.
Addressed for Clone(Rectangle). It now also carries _framesOriginalBitsPerPixel, since the crop maps 1:1 over GetInternalImages() (thumbnails already skipped in both), so alignment is guaranteed. Added DW_40_CloneRectangle_ShouldPreservePerFrameDepthForMultiPageTiff, which asserts a cropped mixed-depth TIFF still reports [1, 8, 24] per frame.
I deliberately did not copy the per-frame list in RotateFlip and Redact: those re-decode via Image.Load(bitmap.Binary), where ImageSharp's frame/thumbnail ordering is not guaranteed to match LibTiff's _framesOriginalBitsPerPixel, so copying could misalign. They keep the scalar fallback.
| var result = new AnyBitmap(image); | ||
| // Rotating/flipping preserves the source color depth, so carry the original depth over | ||
| // (otherwise it would fall back to the decoded 32bpp value). | ||
| result._originalBitsPerPixel = bitmap._originalBitsPerPixel; |
There was a problem hiding this comment.
Nit: the var result = new AnyBitmap(image); result._originalBitsPerPixel = ...; pattern repeats at three sites (here, Clone(Rectangle), Redact). A private AnyBitmap(Image image, int? originalBpp) overload would be cleaner and keep the cast operator AnyBitmap(Image) untouched — the separation the PR says it wants. Functionally equivalent; skip if you prefer the smaller diff.
There was a problem hiding this comment.
Done. Added a private AnyBitmap(Image image, int? originalBitsPerPixel) overload and used it in RotateFlip and Redact, so the cast operator AnyBitmap(Image) stays free of any assumed original depth. Clone(Rectangle) keeps its explicit assignment because it goes through the IEnumerable<Image> constructor and also carries the per-frame list.
…lip/Redact and preserve depth on resize
… Clone, add Image/originalBpp ctor, clarify Redact rationale
b08be16 to
4c66813
Compare
Description
Follow-up to DW-9. DW-9 made
AnyBitmap.BitsPerPixelreport a TIFF's true source color depth (for example 1 for black and white) instead of the decoded 32bpp value, but only for the object returned directly byAnyBitmap.FromFile/FromStream. Several public APIs build a newAnyBitmapfrom an already in-memory image and did not carry that captured depth over, so they silently fell back to reporting 32.This PR carries the original source depth through those derived operations, and fixes the resize path so it no longer flattens every image to 32bpp.
nuspec release notes (not yet pushed). The
NuGet/IronSoftware.Drawing.nuspecrelease notes update is intentionally not part of this branch yet. To be added when the change set is finalized:Fixes DW-40
Root cause. DW-9 stores the captured depth in a private
_originalBitsPerPixelfield that is only populated insideLoadImage.BitsPerPixelreturns_originalBitsPerPixel ?? InMemoryBitsPerPixel. Each derived API constructs its result through an internal constructor (or the ImageSharp cast operator) that never sets_originalBitsPerPixel, so the getter fell back to the in memory decoded depth (32). The resize path additionally hard codedImage.Load<Rgba32>, which forced 32bpp for all inputs.Changes in
AnyBitmap.cs:Clone(Rectangle),RotateFlip(...),Redact(...): these operations preserve the source color depth, so the result now copies_originalBitsPerPixelfrom the source. Propagation is done explicitly in each API (not in the sharedAnyBitmap(Image)constructor, because that same constructor is the ImageSharp cast operator, where there is no original source depth to assume).GetAllFrames: now reports each frame's own source depth. A multi page TIFF can legally mix depths per page (for example page 1 bilevel, page 2 color), so a single frame-0 value would be wrong for later pages. Per frame depth is captured duringInternalLoadTiffinside the same loop that decodes frames and skips thumbnails, guaranteeing index alignment with the decoded frames. Frames only receive an original depth when the object itself captured one (a TIFF loaded preserving its original format); otherwise they report the in memory depth as before.AnyBitmap(AnyBitmap, int, int)(LoadAndResizeImage): now resizes a clone of the already decoded image, preserving its pixel type, instead of re-decoding asRgba32. This fixes two problems: 24bpp is no longer upsampled to 32bpp, and 64bpp (Rgba64) is no longer flattened to 32bpp with loss of color precision (16 bits per channel down to 8). Resizing resamples (blends) pixels, so a source whose original depth is below 8bpp (for example 1bpp, which has no in memory representation below 8bpp) is honestly reported at its decoded depth rather than claiming to still be 1bpp. The original low depth is intentionally not carried over for resize.ReadDirectoryBitsPerPixel(Tiff), shared byReadTiffMetadataFast(frame 0) and the new per frame capture inInternalLoadTiff.Documentation: updated the resize constructor XML doc to state that resizing resamples and may change
BitsPerPixel, consistent with the existingBitsPerPixel/Stride/Scan0decoupling notes added in DW-9.Type of change
How Has This Been Tested?
Added six unit tests in
AnyBitmapFunctionality.cs, run on net48 and net8.0:DW_40_GetAllFrames_ShouldReturnOriginalBitsPerPixelPerFrame: a single frame 1bpp TIFF reports 1, and a mixed depth multi page TIFF reports[1, 8, 24]per frame.DW_40_CloneRectangle_ShouldPreserveOriginalBitsPerPixel: cropping a 1bpp TIFF reports 1.DW_40_RotateFlip_ShouldPreserveOriginalBitsPerPixel: rotating a 1bpp TIFF reports 1.DW_40_Redact_ShouldPreserveOriginalBitsPerPixel: redacting a 1bpp TIFF reports 1.DW_40_Resize_ShouldReportHonestDepthForSubEightBppSource: resizing a 1bpp TIFF honestly reports 32.DW_40_Resize_ShouldPreserveDepthForRepresentableFormats: resizing a 24bpp image reports 24, and resizing a 64bpp image reports 64.New test asset
Data/DW-40 MixedDepthMultiPage.tif: a synthesized 3 page TIFF (page 0 = 1bpp, page 1 = 8bpp grayscale, page 2 = 24bpp RGB) so the per frameGetAllFramesbehavior is verified against genuinely mixed depths.Results: full test project passes locally, 414 of 414 on net8.0 and 400 of 400 on net48, no regressions.
Checklist:
Additional Context
Behavior change summary for a source loaded preserving its original format:
GetAllFrames(per page)Clone(Rectangle)RotateFlip(...)Redact(...)Design decision (resize). Resize was intentionally kept honest for sub 8bpp sources rather than propagating the original depth, because resampling blends black and white into gray, so the resized image genuinely is no longer bilevel. For 8/24/32/64bpp the depth is preserved by resizing at the native pixel type.
Durability note. As with
BitsPerPixelon the loaded object, a carried original depth is an in memory label. Saving through the default encoder (BMP at 32bpp) or callingGetBytes()re-encodes and the reduced depth is not preserved in the saved bytes. This is unchanged behavior and is already documented onBitsPerPixel.