fix(pdfx): render cropRect directly into a region-sized bitmap - #649
Open
Fury76 wants to merge 3 commits into
Open
fix(pdfx): render cropRect directly into a region-sized bitmap#649Fury76 wants to merge 3 commits into
Fury76 wants to merge 3 commits into
Conversation
… page `Page.render` rasterized the page into a `width` x `height` bitmap and then called `Bitmap.createBitmap(bitmap, cropX, cropY, cropW, cropH)`, so asking for a small region of a heavily scaled page still allocated (and rendered) the full page. `PdfPage.render` documents `cropRect` as "render only the necessary part of the image", and the Windows backend already implements it that way in `Page::render` (it sizes the FPDF bitmap to the crop and passes a negative start_x/start_y to FPDF_RenderPageBitmap). Android was the odd one out. Allocate the bitmap at the region size and let PdfRenderer do the placement: the transform scales the page onto the `width` x `height` raster and translates by -cropX / -cropY so the crop origin lands at (0, 0). This is the same matrix the plugin already builds for the texture path in `Messages.kt#updateTexture`. Output pixels are unchanged. Peak memory now follows the region rather than the page: rendering a viewport-sized tile of an A4 page at 13x drops from ~87M pixels (~349 MB) to ~3.6M pixels (~14 MB), which is the difference between working and OutOfMemoryError on a zoomable viewer. One behaviour change, in an error path only: a cropRect reaching outside the page raster used to throw IllegalArgumentException from Bitmap.createBitmap and now renders background there instead.
…le page
Same problem as the Android backend: `Page.render` allocated a `width` x
`height` bitmap context, drew the whole page into it, and then dropped
everything outside the crop with `cgImage.cropping(to:)`. Asking for a small
region of a heavily scaled page therefore cost the full page in memory, which is
what a zoomable viewer does on every pinch.
Split the two sizes that were conflated in `bitmapSize`: `pageSize` stays the
raster the page is laid out on and keeps driving `getDrawingTransform` and the
rotation fixups, while `outputSize` (the crop, when there is one) is what the
CGContext and its backing buffer are allocated at. The region is then positioned
by translating before the page transform is concatenated -- `crop` is in image
coordinates with a top-left origin and the context draws bottom-up, hence
`-(pageSize.height - crop.origin.y - crop.size.height)` for y.
Output pixels and the reported DataResult size are unchanged; `cropping(to:)`
and its force-unwrap are gone. This is the behaviour the Windows backend already
has in `Page::render`, and what `PdfPage.render` documents cropRect to mean
("render only the necessary part of the image").
Regression from ScerIO#541 (7d4db5c, "Update Messages.kt"): while moving rendering onto a coroutine the crop parameters were re-typed and cropW ended up reading `message.width` instead of `message.cropWidth`. Every cropRect request on Android therefore produced a bitmap as wide as the whole page (only the height was cropped), and with the old full-page-then-crop implementation any cropX > 0 threw from Bitmap.createBitmap because x + width exceeded the source bitmap. iOS reads `message.cropWidth` / `message.cropHeight`; Android now does too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes Android crop-width decoding and makes Android/iOS/macOS render directly into the requested region, instead of allocating a full-page raster before cropping. Related to #353.
Problem
Android
Messages.ktreadsmessage.widthintocropW, rather thanmessage.cropWidth. This regression was introduced in #541 (7d4db5c). For a crop narrower and shorter than the page raster, the old implementation attempts to crop a full-width bitmap; a positivecropXthen exceeds the source bitmap bounds.Separately, Android and iOS/macOS allocate and render the entire
width × heightpage before cropping.PdfPage.renderdocumentscropRectas “render only the necessary part of the image”; Windows already allocates the crop-sized bitmap and uses negative render offsets.For example, a 595 × 842-point page rendered at 13× has a 7735 × 10946 raster: 84,668,810 pixels, approximately 323 MiB for one RGBA buffer. A 1080 × 1920 tile needs approximately 7.91 MiB for one RGBA buffer. These are buffer-size calculations, not measured total process memory.
Changes
message.cropWidth; allocate the requested region and use a scale/translation matrix to position the page at-cropX/-cropY. Keep the existing non-crop rendering path and print/display mode selection.cgImage.cropping(to:)call.darwin/pdfx/Sources/pdfx/Document.swiftfile on current main.Reproduction
With a portrait A4 page (595 × 842 points):
Decode
cropped.bytesto check its real dimensions: expected 476 × 674. Compare its pixels against the same rectangle of a full render at 1190 × 1684. On the current Android implementation the call throws due to the incorrect crop width.For the high-zoom case, use
width: 7735,height: 10946, and an in-bounds 1080 × 1920 crop. Only a region-sized destination buffer should be allocated.Validation
The original patches were verified in a downstream application with a synthetic A4 PDF containing a 7pt grid, diagonals, concentric rectangles and location markers. The handoff records an Android mean absolute luma difference of 0.02 between the region render and the corresponding full-render subregion; a 1080 × 1920 tile at 13× returned in approximately 185 ms. These are earlier downstream measurements, not a new benchmark of the rebased branch. The application owner also reports successful Android/iOS/HarmonyOS device checks.
For this submission, the three commits were rebased onto
452979e49fa00df9d964422ff1bc9f1f6b122d16.git range-diffconfirms unchanged Android patches and only the upstream file relocation for the Swift patch. Both native source files retain CRLF line endings; the diff is 3 files, +61/-40.Checks on the rebased branch with Flutter 3.35.7:
flutter test --no-pub: all 7 existing tests passed.flutter analyze --no-pub --fatal-infos: no issues.dart format --output=none --set-exit-if-changed .: 49 files, 0 changes.The existing Dart tests mock platform calls and do not validate native raster output. Native device tests were not rerun after rebasing, and macOS runtime rendering has not been independently verified.
Scope / edge cases
The pixel comparison covers positive, integer, in-bounds crop rectangles on the synthetic portrait page. Fractional and out-of-bounds rectangles are not covered by those measurements. Rendering directly into the destination also changes some out-of-bounds behavior previously delegated to bitmap cropping; this PR does not introduce a cross-platform validation policy for invalid rectangles.
The one-line Android field correction is a separate commit and can be split into a separate PR if preferred.