Uh oh!
There was an error while loading. Please reload this page.
PDF-2231: Per-page multi-page TIFF (mixed orientation) + resolution/Rgb24 writer fixes - #150
Conversation
mee-ironsoftware
left a comment
There was a problem hiding this comment.
Approve with comments. Core mixed-orientation fix is correct, tested, and lives at the right layer (Drawing, so other products inherit it) — nothing blocking. Two Medium follow-ups recommended before merge (inline). Note: companion IronPdf #956 reviewed alongside — its -ci/prerelease pins are fine on a develop base, but PR #956's integration tests assert orientation only, so the per-page DPI round-trip is unverified end-to-end (see Medium on the resolution line here).
| case SixLabors.ImageSharp.Metadata.PixelResolutionUnit.PixelsPerMeter: | ||
| output.SetField(TiffTag.XRESOLUTION, image.Metadata.HorizontalResolution * 100); | ||
| output.SetField(TiffTag.YRESOLUTION, image.Metadata.VerticalResolution * 100); | ||
| output.SetField(TiffTag.XRESOLUTION, image.Metadata.HorizontalResolution / 100); |
There was a problem hiding this comment.
Medium: This / 100 fix is correct (TIFF has no per-metre unit; with RESOLUTIONUNIT = CENTIMETER, px/m / 100 = px/cm — the old * 100 wrote a value ~10^4x too large). But no test actually exercises this branch: CreateMultiFrameTiffBytes_Preserves_Per_Page_Dimensions_And_Resolution builds its bitmaps with PixelsPerInch, so it runs the unchanged Inch branch above. The PR description's claim that that test "proves the unit-conversion fix" isn't accurate, and the fix has no regression guard. Please add a case that sets PixelsPerMeter on the source and asserts the read-back DPI via the existing ToDotsPerInch helper — that test would fail under the old * 100.
There was a problem hiding this comment.
Added CreateMultiFrameTiffBytes_PreservesResolution_FromPixelsPerMeterSource, which builds a source with PixelResolutionUnit.PixelsPerMeter (300 DPI expressed as px/m), runs it through CreateMultiFrameTiffBytes, and asserts the read-back resolution via ToDotsPerInch ≈ 300. I verified it fails under the old * 100 (got 2999994).
| // Bgr24 is likewise 3 bytes/pixel; convert to Rgba32 for the | ||
| // same reason as Rgb24 above (this also yields the correct | ||
| // R,G,B sample order under PHOTOMETRIC.RGB). | ||
| using var rgba = imageAsFormat.CloneAs<Rgba32>(); |
There was a problem hiding this comment.
Low/Nit: Good fix here (and on the Rgb24 case) — the 3-bpp source packed into the 4-bpp buffer was misaligning every scanline, and CloneAs<Rgba32> also gives the correct R,G,B sample order. Out-of-scope follow-up: the sibling Abgr32 / Argb32 / Bgra32 cases just below still CopyPixelDataTo directly, but the writer emits PHOTOMETRIC.RGB with sample order R,G,B,A — so e.g. Bgra32 (bytes B,G,R,A) would land channel-swapped if ever hit. Pre-existing and the new public API avoids it entirely (it CloneAs<Rgba32> upfront), but worth a ticket to normalise these or confirm they're unreachable.
There was a problem hiding this comment.
Normalized all three to CloneAs<Rgba32>() (matching the Rgb24/Bgr24 pattern), so the writer always gets correct R,G,B,A order under PHOTOMETRIC.RGB.
| if (images == null) | ||
| throw new ArgumentNullException(nameof(images)); | ||
| var frames = new List<Image>(); |
There was a problem hiding this comment.
Low: Nice touch forcing every frame through CloneAs<Rgba32> so the writer's default (correct-channel-order) branch is always taken — a clean correctness firewall. One memory note: the clones are eagerly materialised into this List<Image> before any write, so for a moment both the caller's pages and the full set of Rgba32 clones are alive (transient ~2x page memory). Fine for typical use and no worse than the old IronPdf path, but for very large / high-DPI documents a lazy Select(... CloneAs<Rgba32> ...) with per-frame dispose after WriteDirectory would lower the peak. Not blocking.
There was a problem hiding this comment.
The suggested lazy + per-frame-dispose can't be done safely in InternalSaveAsMultiPageTiff because it's shared with CreateMultiFrameTiff(IEnumerable<AnyBitmap>) / (IEnumerable<string>), which pass borrowed images (the (Image)anyBitmap cast returns the AnyBitmap's internal frame), disposing those inside the writer would corrupt the source bitmaps. Intentionally left as-is.
| foreach (var page in pages) | ||
| { | ||
| ToDotsPerInch(page.XResolution, page.ResolutionUnit) |
There was a problem hiding this comment.
Low/Nit: This DPI assertion only ever sees the INCH branch because CreateSolidBitmap sets PixelsPerInch. The ToDotsPerInch helper already handles CENTIMETER, so adding a PixelsPerMeter source here would extend this same test to cover the actual resolution fix (see the Medium on AnyBitmap.cs).
0cf21c5 to
9abef96CompareUh oh!
There was an error while loading. Please reload this page.
Description
Adds per-page multi-page TIFF support to
AnyBitmapand fixes two latent bugs in the existing multi-page TIFF writer (InternalSaveAsMultiPageTiff).This is the IronSoftware.Drawing side of PDF-2231:
PdfDocument.ToMultiPageTiffStream()(in IronPdf) normalized every page of a mixed-orientation PDF to a single uniform size/orientation. The root cause is that a multi-page TIFF was being assembled as a single ImageSharpImage<T>, which requires all frames to share one size. This PR provides a path that keeps each page's native dimensions/orientation/resolution.New public APIs:
AnyBitmap.CreateMultiFrameTiffStream(IEnumerable<AnyBitmap>)→MemoryStreamAnyBitmap.CreateMultiFrameTiffBytes(IEnumerable<AnyBitmap>)→byte[]Both reuse the existing per-page LibTiff writer (one IFD per page) instead of round-tripping through a single ImageSharp image (the lossy step in the old
CreateMultiFrameTiff/FromStreampath). Each page is converted toRgba32first so its byte layout matches the writer regardless of source format.Bug fixes in
InternalSaveAsMultiPageTiff:PixelsPerMeterbranch multiplied by 100 when converting pixels-per-metre → pixels-per-centimetre; it must divide by 100. TIFF has no per-metre unit, so the value was previously written ~10,000× too large.Rgb24/Bgr243-bpp stride corruption: The writer fills a 4-samples/pixel (RGBA) buffer, but 3-byteRgb24/Bgr24pixels were copied tightly-packed, shifting every row and scrambling the page. These cases now convert toRgba32before copying.Type of change
How Has This Been Tested?
Added unit tests in
AnyBitmapFunctionality.cs:CreateMultiFrameTiffStream_Preserves_Mixed_Orientation: Portrait + landscape pages keep their own dimensions (read back per-IFD with LibTiff).CreateMultiFrameTiffBytes_Preserves_Per_Page_Dimensions_And_Resolution: Per-page sizes preserved and DPI round-trips to the source value (proves the unit-conversion fix).CreateMultiFrameTiffStream_Empty_Sequence_Throws: Argument validation.CreateMultiFrameTiff_Preserves_Rgb24_Pixels: A JPEG (decodes toRgb24) round-trips through the writer with sampled pixels matching the source (proves the 3-bpp stride fix). Verified this test fails with the fix reverted.Checklist: