fdl_imaging warp-everywhere + cffi deadlock fix (on top of Adam's rounding)#2
Open
giardiello wants to merge 4 commits into
Open
fdl_imaging warp-everywhere + cffi deadlock fix (on top of Adam's rounding)#2giardiello wants to merge 4 commits into
giardiello wants to merge 4 commits into
Conversation
OwnedHandle.__del__ can fire during garbage collection while another thread is inside cffi.api.make_accessor resolving a different lib function. If the finalizer requested self._lib.fdl_doc_free via cffi's lazy-accessor path and that function had never been bound before, make_accessor would reenter and deadlock on cffi's internal lock. Capture lib.fdl_doc_free at construction time (when the stack is known-safe) and call it directly in close(). No behavior change in the happy path; eliminates the GC-vs-accessor deadlock surfaced by concurrent test workers. Made-with: Cursor
Replace the cut/resize/paste chain with ImageBufAlgo.warp so float anchors and dimensions propagate through the filter kernel without being truncated to integers at the OIIO boundary. Output buffer sizes are rounded up with math.ceil to match the frameline generator's display convention: every sub-pixel of a region is preserved; the last column/row may be a partial sample filtered against wrap="black". Fast path: when the source region is already integer-aligned (whole- pixel anchor + integer dims), fall back to ImageBufAlgo.cut. cut is a memcpy-style copy that skips the reconstruction filter, keeping pixel-perfect inputs (v1 FDLs, integer-producing templates) bit- identical through the pipeline instead of paying for a spurious lanczos pass. Refactored into _warp_crop + _warp_compose helpers. All 53 Scenarios_For_Implementers imaging tests pass against regenerated EXR goldens (see follow-up commit). Made-with: Cursor
Re-baseline all 53 Scenarios_For_Implementers RESULT EXRs (52 scenarios + 1 EdgeCases alignment combo) against the current pipeline: - native/core ef57e1f — Adam Davis's "split step 5 into 5a/5b/5c" rounding pass, which ceils effective once, preserves the anchor, and floor-shrinks effective when it would overflow the rounded canvas; inner (protection, framing) dims stay float and are only clamped down if they'd exceed the canvas; - fdl_imaging warp-everywhere — float source coordinates are now carried through ImageBufAlgo.warp instead of being truncated at cut/resize boundaries, with a bit-preserving cut fast path for integer-aligned sources. Generated with scripts/update_expected_exrs.py against native/bindings/python/tests/outputs/main. All 52 tests in test_templates_imaging.py and the 1 edge-case test pass. Pixel deltas vs the pre-ef57e1f6 goldens are sub-pixel edge anti-aliasing only — no layout regressions, confirmed by the Scenarios_For_Implementers FDL comparisons which continue to pass unchanged. Made-with: Cursor
…ding Re-baseline all 53 source_scene_*.png and 53 output_scene_*.png goldens (106 files across scen1..scen32) used by test_ui_scenarios.py. Pixel deltas are sub-pixel anti-aliasing jitter from Qt's rasterizer reacting to the new float-precision geometry — OIIO's strict 1% per-channel compare flagged them even though ImageMagick's perceptual -fuzz 1% compare reports <0.1% of pixels as truly different. Verified by visual inspection that no layout regressions are present. All 53 test_ui_scenarios.py tests now pass (~9 min wall time). Regenerated by copying the latest outputs from packages/fdl_viewer/tests/outputs/scenes/ into the corresponding expected/ directories. Made-with: Cursor
davisadam10
reviewed
Apr 26, 2026
| """ | ||
|
|
||
| __slots__ = ("_close_lock", "_closed") | ||
| __slots__ = ("_close_lock", "_closed", "_free_fn") |
Collaborator
There was a problem hiding this comment.
This all looks good apart from this, why did we need to do this here when its been fine for ages?
There could be an edge case, but if we need to do this fix, then it should be part of the codegen templates not had written in
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.
Summary
Four commits layered on top of Adam's
ef57e1f6("Split step 5 into 5a/5b/5c: floor-shrink effective, preserve anchor"):30065ee—OwnedHandlecffi reentrancy deadlock fix. Capturelib.fdl_doc_freeat construction time so__del__never hits cffi's lazy accessor path during GC and deadlocks on cffi's internal lock.e71596d—fdl_imagingwarp-everywhere. Replacecut/resize/pastewithImageBufAlgo.warpso float anchors/dims propagate through the filter kernel instead of being truncated at the OIIO boundary. Output sizesceil'd to match the frameline generator's display convention. Integer-aligned sources take a bit-preservingcutfast path.c53c8c9— Regenerate 53 RESULT EXR goldens for the new warp pipeline + Adam's rounding. Sub-pixel edge anti-aliasing only; no layout regressions (FDL comparisons still pass unchanged).8f84a43— Regenerate 106 viewer scene PNG goldens (53 source + 53 output acrossscen1..scen32). Same story: Qt rasterizer jitter from the float geometry, flagged by OIIO's strict 1%-per-channel compare even though ImageMagick's perceptual-fuzz 1%reports <0.1% truly different.Base
Based on
base/adam-ef57e1f6(= Adam'sef57e1f6). This is the real review surface — only my four commits. Once Adam's rounding fix lands onorigin/dev, this PR can be rebased and re-targeted there.Test plan
native/bindings/python/tests/— all pass (incl.test_optional_rounding.py,test_templates_parameterized.py)packages/fdl_imaging/tests/test_templates_imaging.py— 52/52 pass against regenerated EXRspackages/fdl_imaging/tests/test_image_processing.py— passpackages/fdl_viewer/tests/test_ui_scenarios.py— 53/53 pass against regenerated PNGs (~9 min)Notes for reviewer
91dd064"consolidate geometry_round" commit that sat onfork/wip/optional-rounding-snapshot-20260423-110024was intentionally discarded in favor of Adam'sef57e1f6semantics.backup/pre-adam-reset(local only) preserves the pre-reset HEAD (4dcb84c) if we ever need it.Made with Cursor