Fix target_dimensions=0 corrupting pad_to_maximum alignment on the non-driving axis - #56
Open
giardiello wants to merge 4 commits into
Open
Conversation
Per spec 7.4.7, for fit_method "width"/"height" the non-fit axis of target_dimensions "should be automatically calculated by the application and not predefined by the user." The reference implementation ignored this for the alignment/pad step: it fed the raw, literal target_dimensions value for that axis straight into fdl_alignment_shift() unconditionally, so any producer submitting a placeholder (0, 1, or any non-matching value) for "the axis I'm not fitting by" corrupted alignment/padding whenever alignment_method wasn't "center" (center algebraically cancels the term already). Resolve that axis to the actual output extent (out_w/out_h) before alignment_shift runs, except when preserve_from_source_canvas diverges from fit_source — per spec 7.4.9, in that case target_dimensions is a genuine, load-bearing relative-target rectangle on both axes, not a placeholder, and must be used as literally authored (this preserves Scen_32-shape templates, e.g. "eff_tl_to_br", where target intentionally differs from maximum_dimensions to control how much preserved padding is added). Verified: 134/134 native CTest, 457/457 Python binding tests (including all 53 official Scenarios_For_Implementers parameterized cases), 59/59 fdl_imaging pixel-level render tests all still pass. Additionally confirmed a zero/placeholder target_dimensions height (fit_method=width, non-center alignment, pad_to_maximum=true) now produces byte-identical output to a correctly-authored control, matching independently-verified correct behavior (FLAPI's engine).
…out_w/out_h
Correction to the previous commit. Per spec 7.4.11, pad_to_maximum always
center-aligns the "virtual" target rectangle within maximum_dimensions,
and per 7.4.8 alignment_method only positions fit_source *inside* that
virtual rectangle — it does not move the virtual rectangle itself against
the padded canvas edge. My first fix incorrectly resolved the non-fit axis
to the padded output size (out_w/out_h), which made alignment_method
behave as if it aligned against the outer canvas edge — wrong per the
literal spec text, and it required a special-case carve-out for templates
where preserve_from_source_canvas diverges from fit_source (e.g. Scen_32)
to avoid regressing them.
The correct "virtual" size for the non-fit axis is the scaled,
preserve-extended canvas immediately after scaling and before padding —
already captured in this function as `scaled_bounding_box`
(== geometry.canvas_dims post-scale, pre-pad). It equals the scaled
fit_source size when preserve is absent/non-diverging, and the larger
preserve-extended size when it diverges (spec 7.4.9), so no case-split is
needed: the same one-line resolution is correct for every
preserve/fit_source combination.
Practical effect: for fit_method width/height with a non-center
alignment_method and pad_to_maximum=true, alignment_method now has no
effect at all unless preserve_from_source_canvas genuinely extends beyond
fit_source (the only way a real gap exists between the virtual rectangle
and the fit content) — content is otherwise always centered within
maximum_dimensions, regardless of a placeholder or literal value in
target_dimensions on that axis. This matches FLAPI's independently-computed
behavior exactly, verified for Scen_16, Scen_17, and Scen_32 templates.
Regenerated (all self-referential / non-independently-verified, produced
by calling this same reference implementation):
- native/core/tests/vectors/template/template_vectors.json
(left_top_alignment, right_bottom_alignment)
- resources/FDL/Scenarios_For_Implementers/Scen_{16,17}.../Results/*
(A/B/C variants)
- resources/FDL/Scenarios_For_Implementers/Scen_32.../Results/*
(Scen_32's expected fixture actually lives under
resources/FDL/EdgeCases/alignment_combos/ — a Netflix-fork addition,
not an ASC-upstream artifact — only its Results/ output is filed under
the Scenarios_For_Implementers/Scen_32... folder)
Verified: 134/134 native CTest, 457/457 Python binding tests (including
all 53 parameterized scenarios), 59/59 fdl_imaging pixel-level render
tests all pass against the regenerated goldens.
fit_all/fill are not a distinct sizing model, just an automatic choice of which axis drives the scale ratio (calculate_scale_ratio's w_ratio/h_ratio comparison). Once that axis is chosen, the other axis's target_dimensions should be exactly as non-authoritative as it would be had the user picked that axis's fit_method explicitly — so the target_dims_resolved logic from the previous commit needed to apply there too, not just to explicit width/height. However, naively applying it unconditionally broke fill's crop-alignment behavior (Scen_12/13 and camera variants): fill deliberately overflows the non-driving axis past target_dimensions, and that scenario is a genuine crop, not a pad. In a crop, target_dimensions is the real, load-bearing crop-window size — alignment_method determines which portion of the overflow is kept — so replacing it with the (overflowing) scaled size collapses the gap to zero and silently disables alignment for e.g. "right"/"bottom" (they degenerate to flush-left/top). Fix: only resolve the non-driving axis to scaled_bounding_box when that axis is NOT overflowing maximum_dimensions (i.e. it would pad, or need no adjustment) — leave it as the literal, authored value when it overflows (i.e. it would crop). This is symmetric with fit_method width/height too: an explicit width/height fit can also cause the other axis to overflow maximum_dimensions given a large enough preserve_from_source_canvas extension, and that case needs the same literal-value treatment. Verified: 134/134 CTest, 457/457 Python binding tests (all scenarios, including fill/fit_all crop cases restored to their original correct behavior with no golden regeneration needed, and Scen_16/17/32's padding-case fix from the previous commit unaffected).
…etection
Make target_dimensions.{width,height} == 0 a schema-legal sentinel for
"not predefined by the user, calculated by the application" (spec
7.4.4/7.4.7), instead of the schema forbidding 0 outright. A new
target_dimensions_int def (minimum: 0) replaces the shared dimensions_int
(exclusiveMinimum: 0) for target_dimensions only; every other dimensions
field is untouched.
fdl_template.cpp resolves the 0 sentinel to the scaled, preserve-extended
canvas size (scaled_bounding_box) on the non-driving axis before calling
alignment_shift(), gated on that axis padding rather than cropping (crop
windows keep target_dimensions literal and load-bearing). Nonzero values
are always used literally.
Also simplifies width_drives_scale to reuse calculate_scale_ratio()'s
already-computed result for fit_all/fill instead of re-deriving the
w_ratio/h_ratio comparison.
Regenerates template_vectors.json and the official Scen_16/17/32 goldens,
which are self-generated by this same reference implementation.
giardiello
requested review from
davidbkhall and
davisadam10
as code owners
September 10, 2026 16:46
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.
Summary
target_dimensionson the axis that does not drive the scale ratio (e.g.heightunderfit_method: "width") may legitimately be left "not predefined by the user" per spec 7.4.4/7.4.7, calculated by the application instead. The schema had no legal way to express that:dimensions_intrequires bothwidth/heightto be> 0, so producers are forced to submit an arbitrary placeholder (observed in the wild as1, since0was schema-invalid).alignment_shift()forpad_to_maximumtemplates, corrupting the pad/alignment math on the non-driving axis — e.g.TopLeft/BottomLeftpad_to_maximumtemplates producing large, wrong vertical shifts instead of the expected center-then-align-within-max behavior.target_dimensions_intschema def (minimum: 0, only fortarget_dimensions) so0is now the legal, explicit sentinel for "not predefined, calculate automatically." Every other dimensions field (maximum_dimensions, etc.) is untouched and still requires> 0.fdl_template.cppresolves that0sentinel to the scaled, preserve-extended canvas size (scaled_bounding_box) on the non-driving axis before callingalignment_shift(), gated on that axis padding rather than cropping — crop windows (e.g.filloverflowing the non-driving axis) keeptarget_dimensionsfully literal and load-bearing, since alignment there genuinely selects which part of the overflow is visible.fit_all/fill: they're not a distinct sizing model, just an automatic choice of which axis drives the scale (reusingcalculate_scale_ratio()'s already-computed result rather than re-deriving it).target_dimensionsvalue continues to be used exactly as authored — this only changes behavior for the explicit0sentinel.Test plan
ctestinnative/core/build: 134/134 passed (regeneratedtemplate_vectors.jsonfor the new behavior)pytest native/bindings/python/tests: 457 passed, 4 skippedpytest packages/fdl_imaging/tests: 59/59 passedScen_16/Scen_17/Scen_32Results/*.fdl+*.exrgoldens (self-generated by this same reference implementation, so regeneration after an intentional behavior change is expected)fit_method(width/fit_all) ×pad_to_maximum× target height (0,1,20,4000, ...) × alignment (top/center/bottom) confirming:target=0centers correctly; nonzero targets (including small ones like20) are respected as literal virtual boundaries that alignment flushes against;fit_alland explicitwidthproduce identical results once the driving axis matches.