Fix contact sheet silently dropping frames on large rolls#600
Merged
marcinz606 merged 1 commit intoJul 22, 2026
Conversation
render_display_array is documented as producing a small tile and takes target_long_px, but only used it to derive scale_factor — the returned array was the full-resolution render (the GPU path's _readback_downsampled reads the texture back at native size despite its name). Measured on a 24MP source with target_long_px=1200: the tile came back 6000x4000, 72MB, instead of 1200x800, 2.9MB. run_contact_sheet holds every tile in memory until it composites, so peak cost scaled with frames x full resolution — ~1.4GB for a 20-frame 24MP roll, ~2.7GB at 45MP. When an allocation failed, the blanket except in render_display_array swallowed it and returned None, and the worker dropped the tile with no error emitted. Progress is emitted before the render, so the dialog still counted every file: a 20-frame run could report 20 processed and produce a sheet with a single image. Downsample the tile to target_long_px (after the display transform, where the compositor also resamples, so it looks identical) and report a dropped tile through the existing error signal so failures surface as "N failed" instead of silently shrinking the sheet. Per-tile processor.cleanup() was considered and rejected: the texture pool stays flat across a same-size roll and the source cache holds one entry, so the existing per-batch evacuation is correct and rebuilding the pool every frame would only cost time.
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
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.
The bug
Reported on Windows: a contact sheet export processed 20+ frames according to the progress dialog but produced a sheet containing only a single image, with no error shown.
Root cause
render_display_arrayis documented as rendering "to a small sRGB uint8 RGB array for tiling" and takestarget_long_px— but it never downsamples.target_long_pxis only used to derivescale_factor(a pipeline hint for blur radii); the GPU path's_readback_downsampledreads the texture back at its native size despite the name, and the CPU path likewise returns full res.Measured directly on a 24MP source with
target_long_px=1200:run_contact_sheetaccumulates every tile in a list before compositing, so peak memory scaled withframes × full resolution:The full resolution bought nothing:
_paste_resizeddownsamples each tile to the 600px cell at composite time anyway.When an allocation eventually failed,
render_display_array's blanketexcept Exceptioncaught it (includingMemoryError), logged it, and returnedNone; the worker then discarded the tile with noerror.emit. Sinceprogress.emit(i + 1, total, name)fires before the render, the dialog faithfully counted every file regardless. Hence: "processed 20+", sheet built from however few survived. Machine-dependent, which is why it showed up on the reporter's box and not on a high-RAM dev machine.Changes
target_long_pxinrender_display_array, via a new_downsample_to_long_edgehelper. Applied after the display transform — the same point where the compositor resamples — so the tile is visually identical, just smaller.errorsignal, which already feeds_export_failuresand surfaces as "— N failed" in the HUD, matchingrun_batch's behaviour. A short sheet no longer looks like a clean success.Per-tile
processor.cleanup()was considered and rejected: the existingfinallyblock documents a deliberate once-per-batch evacuation to avoid rebuilding the texture pool each frame, and I measured_tex_cachestaying flat (86 textures) across a full same-size roll while the source cache holds a single entry. Nothing accumulates, so per-tile cleanup would cost time for no memory benefit.Test plan
tests/test_contact_sheet_tiles.py(10 tests): the_downsample_to_long_edgehelper (shrink, aspect ratio, never upscale, no-op on non-positive target); the real render path honouringtarget_long_pxon both the CPU and GPU paths against an on-disk source; failed tiles emitting one error each; a clean run emitting none and writing one sheet; cleanup staying once-per-batch.ruff check+ruff formatclean.