Memory improvements + better image hook integration#8
Open
laurens-pilot wants to merge 38 commits into
Open
Conversation
The lazy adapter's slice paths now share one gate that refuses uncompressed HEIF (and AVIF) instead of silently falling back to a full decode plus copy, so a future layout-probe change cannot reintroduce a hidden owned-RGBA fallback; the hook constructor's eager fallback keeps such inputs decodable. A synthetic decodable uncompressed rgb3 fixture pins both halves: eager APIs and the hook fallback decode it, every lazy path rejects it as Unsupported.
Share one implementation for the 8/16-bit halves of the owned grid RGBA decode, the coded-item slice decodes, the HEIF slice dispatch, the family-detection preamble, the paste buffer-length validation, and the image adapters' storage/byte-count helpers. LazyHeifImageDecoder gets a manual Debug impl so it no longer dumps the entire encoded input.
fmt and clippy verdicts no longer drift with the runner image's preinstalled stable; both jobs install and default to the pinned version (matching the locally verified toolchain).
The RGBA grid decode paths zero-filled their output canvas, so descriptor pixels not covered by any tile decoded as transparent black. libheif (and this crate's plane-canvas grid path) composes grids on a zero-filled YUV canvas and converts the whole canvas afterwards, which renders those gap pixels as the converted all-zero YUV sample: opaque, green-tinted for limited-range color. Match that exactly by converting a single zero sample through the same tile conversion function and filling the canvas with it before pasting, in both the owned grid path and the image-hook slice path. The fill only runs when the uniform tile lattice does not cover the descriptor, so fully-covered grids (the normal case) keep the plain zero fill.
read_seekable_input_to_vec sized Vec::with_capacity directly from the length reported by seek(SeekFrom::End(0)) before reading a single byte. That length is untrusted: a lying or corrupt reader (or a huge sparse file under the default unbounded guardrails) triggered a multi-gigabyte allocation, a capacity-overflow panic, or an uncatchable allocator abort inside the library. The previous eager hook never allocated from the reported length. Clamp the speculative pre-allocation to 64 MiB — read_to_end still grows the buffer for genuinely larger, guardrail-permitted inputs — and route both input-size checks through the existing enforce_input_bytes helper instead of two hand-built InputTooLarge constructions.
The lazy image-hook layout probe took its bit depth from the av1C box's twelve_bit/high_bitdepth flags, while read_image's slice decode validates its output depth against the rav1d-decoded frame and the direct API trusts only the decoded frame. An AVIF whose av1C flags disagree with the coded stream (spec-invalid but craftable) therefore failed through image::ImageReader while decode_bytes_to_rgba succeeded on the same bytes. Parse the actual AV1 sequence header instead, mirroring how the HEIC probe reads the SPS: rav1d's dav1d_parse_sequence_header scans the av1C configOBUs (present in practice, ~100 bytes), with a fallback to the primary item payload, and a new MissingSequenceHeader error when neither carries one — a file the decode path could not decode either.
The lazy hook decoder implemented only dimensions/color_type/icc_profile, so image-crate callers always saw Orientation::NoTransforms: the image crate derives ImageDecoder::orientation() from exif_metadata(), whose default returns None. Since decode deliberately does not bake EXIF orientation into pixels, an EXIF-only-rotated HEIC rendered sideways for anyone using the standard trait surface, even though the crate parses that orientation internally for its own hint helpers. Override exif_metadata() to return the primary item's cdsc-linked EXIF block, TIFF-aligned as Orientation::from_exif_chunk expects. The cdsc walk is shared with the existing orientation hint path, and the TIFF offset candidates are shared with the orientation parser.
Every lazy hook decode loop (coded HEIC, AVIF 8/16-bit, uncompressed, grid paste, grid alpha seeding) calls map_destination_pixel or map_source_pixel once per pixel. For the common no-irot/imir/clap case each call still performed two usize-to-u32 conversions, range checks, an empty step walk, and Result wrapping — tens of millions of redundant operations per multi-megapixel hook decode that the owned eager paths avoid with linear row loops. Short-circuit both mapping functions when the plan has no steps (which by construction implies destination dimensions equal source dimensions). Also drop the per-pixel checked index chains in the coded-HEIC and grid paste loops in favor of the plain in-bounds indexing their AVIF and uncompressed twins already use: the mapped coordinates are in-bounds by construction and the validated sample counts fit usize, so the checked variants only constructed unreachable error values per pixel.
decode_heif_bytes_to_rgba16_native_endian_bytes inlined a step-for-step copy of decode_heif_bytes_to_rgba_slice's grid/coded dispatch (parse transforms, extract primary item, resolve auxiliary alpha, match Grid/Coded) while the RGBA8 twin already called the shared helper. Any future change to the dispatch would have had to be applied twice, with the 16-bit hook path silently keeping the old behavior when missed. The 16-bit slice functions write native-endian u16 samples into a byte slice, so they instantiate the shared dispatch at T = u8 directly.
The cap only bounds the speculative up-front allocation sized from the untrusted seek-reported length; larger guardrail-permitted inputs still load fully via read_to_end growth. 128 MiB gives headroom for very large camera files while keeping a lying reader's claimed length harmless.
The lazy layout probe extracted the primary item's full coded payload and re-copied it NAL-by-NAL into a length-prefixed decoder stream just to parse one SPS, then discarded the stream; read_image repeats the extraction and assembly when pixels are actually decoded. The SPS normally sits in the hvcC parameter-set arrays, so the probe can read geometry and bit depth without touching a single payload byte. Probe coded (hvc1/hev1) primaries via the preflight properties parse: take the SPS from the hvcC arrays and keep the existing SPS-vs-ispe geometry validation. hev1 items whose parameter sets live only in-stream fall back to an in-place scan of the length-prefixed payload (same NAL order as the assembled stream, still no copies), and any other miss falls through to the unchanged extraction path so errors stay identical to the decode path's. The grid probe reads the first tile's SPS the same way instead of assembling the tile's stream. The single-SPS parse is shared with the stream scanner via hevc_metadata_from_sps_nal, and the payload NAL walk is shared with stream assembly via walk_length_prefixed_payload_nals, so the probe introduces no parallel SPS interpretation that could drift from decode. Full verify corpus: 272 files, 219 pixel-exact with hook/direct parity, 0 failures. bench-image aggregate adapter/direct ratio 1.12x -> 1.10x; non-grid coded HEIC 1.22x -> 1.04x.
laurens-pilot
changed the title
The lazy hook decoder returned the EXIF block unconditionally, so for files carrying both an irot/imir transform (which decode bakes into the pixels) and an EXIF orientation tag, generic image-crate callers using decoder.orientation() + apply_orientation would rotate the already rotated pixels a second time. Override orientation() to report NoTransforms exactly when the primary item has an orientation transform, mirroring ExifOrientationHint::should_apply_exif_orientation; the EXIF block itself stays exposed through exif_metadata.
Hook decodes buffer the whole encoded input since the lazy-decoder rework, so the previously harmless unbounded default became a multi-gigabyte allocation risk for oversized inputs (e.g. motion-photo HEICs). register_image_decoder_hooks() now applies DEFAULT_HOOK_MAX_INPUT_BYTES; explicit-guardrail registration keeps full control. Also reunite the read_seekable_input_to_vec doc comment with its function (a const had been inserted between them).
The lazy hook decoder supersedes it: it decodes into the caller's buffer instead of holding a full owned RGBA copy, and it exposes EXIF metadata/orientation, which the eager adapter structurally could not (it retained only decoded pixels). Keeping both meant two ImageDecoder impls whose orientation reporting disagreed for the same file. Direct callers keep the decode_*_to_rgba functions; image-crate callers use the registered hooks.
For grid primaries (and hev1 items with in-stream parameter sets) the lazy hook's layout probe fell through to extract_primary_heic_item_data_with_grid, copying every tile payload out of the container only to read the grid descriptor and first-tile SPS, and read_image then ran the identical extraction again. The probe now returns the extraction it performed inside an RgbaLayoutProbe; the decoder stores it and read_image consumes it, so all coded bytes are copied out of the container at most once per hook decode.
The caller-buffer grid decode cleared the whole output and, with an auxiliary alpha plane, seeded alpha for every source pixel — work the tile paste then rewrote sample-for-sample whenever the tiles cover the grid descriptor (the common case). Both passes now run only in the gap-pixel branch; the alpha-plane validation stays unconditional since the paste indexes the plane directly. The now-unused RgbaSampleOutput::fill default method is removed.
uncompressed_output_bit_depth_from_properties re-implemented the decoder's cmpd/uncC component-mapping loop with slightly different error variants, and skipped its format/depth split messages and multi-Y bit-depth rule. Extract the decoder's spec construction into uncompressed_component_layout_from_properties and its channel folding into resolve_uncompressed_channel_map, and build the probe from those two helpers plus select_uncompressed_output_bit_depth, so probe and decode resolve bit depth from one code path.
The decode/validate/colr-override/convert/origin prologue was copy-pasted between paste_heic_grid_tiles_to_rgba and paste_heic_grid_tiles_to_transformed_rgba_slice, and paste_decoded_heic_grid_tile re-inlined the consistency checks and origin math that validate_decoded_heic_grid_tile_reference and heic_grid_tile_origin already implement. Extract for_each_heic_grid_tile_rgba as the single per-tile driver (both paste paths keep only their paste bodies) and have the plane-canvas path call the shared validators through a from_output_canvas reference.
crop_heic_by_clean_aperture kept an inline copy of the clap rounding/clamp/empty-check math that clean_aperture_crop_bounds implements for the RGBA transform path — and the chroma-phase guard that runs immediately before every crop already calls the shared helper, so the bounds were computed twice per crop from two copies of the same libheif-mirroring semantics. Delegate to the helper and map its error into the tile-specific DecodeHeicError.
Their only consumers are the feature-gated lazy-adapter tests, so default-feature --all-targets builds flagged them as dead code.
The layout probe's coded-item fallback (hvcC without an SPS, i.e. hev1 in-stream parameter sets) called decode_primary_heic_stream_and_metadata_from_coded_item_data and discarded the assembled stream — a full length-prefix-normalized copy of the coded payload built just to parse one SPS. Use decode_hevc_metadata_from_hvcc_or_payload (already used by the grid probe arm) with the same preflight id cross-check and ispe geometry validation, now shared via parse_and_validate_heic_coded_item_preflight.
decoded_avif_to_rgba8_slice and decoded_avif_to_rgba16_output were ~120-line near-copies differing only in source sample type, output sink, and scaling. Collapse them onto decoded_avif_to_rgba_output, generic over the source plane sample type and RgbaSampleOutput sink (the same parameterization the HEIC twin uses), with the bit-depth gates left in two thin wrappers. The mono-verbatim fast path keeps its bit_depth == 8 condition, which is constant-false in the 16-bit instantiation. ChromaPlanesU8/U16 and prepare_chroma_u8/u16 fold into a generic ChromaPlanes<S> + prepare_chroma with aliases and wrappers so the owned conversion paths stay unchanged.
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
2 participants
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.
No description provided.