Repository navigation
🐛 fix(crop): apply pixel aspect at every rotation - #165
Conversation
- 🐛 Defects:
- `_orientation_transform` built its viewport with the raw source width
for rotation 0 and 180 and applied `pixel_aspect` only in the 90/270
branch, while `MediaTransform.oriented_display_size` applies it to the
width for 0 and 180. `MediaTransform.scale` then fitted against a box
the mapping did not use, so `oriented_rect_to_source_rect` returned a
rectangle outside the frame: the oriented `CropSpec(8, 0, 8, 8)` on a
16x8 source at pixel aspect 2 mapped to `x=8 y=-4 w=8 h=16`, twice the
height of an 8 px frame and starting above it. Six of sixteen
rotation/pixel-aspect combinations were wrong, every one of them a 0 or
180 rotation with a pixel aspect other than 1 — so the crop rectangle
the user drags sat somewhere other than the region that was cut on an
anamorphic source without rotation.
- The viewport keeps the oriented display axes from
`MediaTransform.oriented_display_size` but rounds the pixel-aspect width
with the decoder's exact rule. Pixel aspect 1 is unchanged, which is what
the existing crop, staging and controller tests pin.
- Fractional display widths now use the decoder's round-half-up extent, so
a 720x480 source at pixel aspect 32/27 maps against 853 decoded pixels
rather than an 853.3333-pixel canvas.
- 🧪 Tests:
- Two matrices over rotation {0, 90, 180, 270} and pixel aspect
{1.0, 2.0, 1.5, 0.75} in `tests/core/test_crop.py`: the mapped
rectangle stays inside the source frame and round-trips back through the
display geometry, and `oriented_point_from_widget` round-trips through
`build_crop_geometry`. Twelve of the new cases fail without the fix. The
existing rotation-270/pixel-aspect-2 case is one of the ten combinations
that already passed, which is why the gap survived.
- A four-rotation fractional-aspect matrix asserts the viewport against
`_display_dimensions` itself rather than a written-out 853, so it fails
if either side's rounding drifts from the other.
- 🔧 Guardrails: third consecutive `fix:` touching `core/crop.py`, agreed
with the maintainer rather than taken silently. It is not a hardening
spiral: `a296a3b` bounded `fit_crop_aspect`, `cc9ec01` changed
`clamp_crop`, and this changes `_orientation_transform` — three
independent defects in three functions, and this one removes the
duplicated viewport expression instead of adding a guard. The round-half-up
rule does remain in two layers, because `core` may not import `jobs`; the
fractional-aspect test is what holds the two copies together.
Trigger: measured `oriented_rect_to_source_rect` over a synthetic 16x8
source at pixel aspect 2, rotation 0 — it returned a source rectangle at
y=-4 with height 16; a 720x480 source at pixel aspect 32/27 produced a
decoded width of 853 while the canvas extent was 853.3333.
Refs #164
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
- 🐛 Defects:
- `ui/crop_presentation.py` converts the source's exact `Fraction` pixel
aspect to a `float`, and `_orientation_transform` reconstructed it with
`Fraction(str(pixel_aspect))`, which recovers the float's decimal
expansion rather than the original rational. A product that is exactly
a half then lands just below it and rounds the wrong way: 1926 x 11/12
is 1765.5, so the decoder yields 1766 while the canvas yielded 1765,
and a rectangle in the rightmost column was painted partly outside the
displayed frame while masking the actual final column.
- The ratio is now recovered with `Fraction(...).limit_denominator(99)`.
Container pixel aspects are small rationals declared in the stream, so
this recovers the ratio the decoder holds rather than inventing one.
- The bound is 99, not 33: H.264 Table E-1 defines sixteen sample aspect
ratios and the largest denominator among them is 99, in 160:99. A bound
of 33 recovers fifteen of the sixteen and silently misses that one.
- 🧪 Tests:
- The rounded-extent matrix now covers both sides of the rounding
boundary and the boundary itself: 720x480 at 32/27 below .5, 640x480 at
11/12 above it, and 1926x1080 at 11/12 exactly on it, over all four
rotations.
- Two matrices over the whole of H.264 Table E-1 rather than a chosen
handful: every standard ratio survives the float round trip, and the
canvas extent equals `_display_dimensions` for each of them at every
rotation. Five cases fail at a bound of 33, all of them 160:99.
- 🔧 Choice: reconstructing the rational at the call site leaves the frozen
`core/geometry.py` float contract alone and is a smaller change than
carrying a `Fraction` through the presentation layer.
Trigger: repro — 1926x1080 coded at pixel aspect 11/12 produced a decoder
width of 1766 and a canvas width of 1765.
Refs #164
|
Follow-up commit
The ratio is now recovered with The bound is 99, not 33. The first attempt used 33 with a comment claiming container pixel aspects have no larger denominator. H.264 Table E-1 defines sixteen sample aspect ratios and the largest denominator among them is 99, in 160:99 — a bound of 33 recovers fifteen of the sixteen and silently misses that one. Measured, then pinned: the tests now cover the whole of Table E-1 rather than a chosen handful, and five cases fail at a bound of 33, all of them 160:99. Verification after the follow-up: ruff, mypy on all three platforms, guardrails, |
- 🔄 `_orientation_transform` used a second round-half-up implementation, `_round_fraction`, while `_rhu` twenty lines above it in the same module already did exactly that. Measured over 972 values spanning negatives and both sides of every half: `_rhu`, the removed helper and the decoder's own `_round_fraction` agree on all of them. - 🔄 A pull request whose subject is one expression existing twice is a poor place to introduce a second one. Reported by the review bot. Refs #164
|
Gitar's finding is correct and now fixed in Checked rather than assumed before removing it — SonarCloud: quality gate passed, 0 new issues. |
Confirmed — thanks for the fix and for verifying |
- 🐛 Defects:
- Two earlier attempts on this branch recomputed the display extent from
the float `pixel_aspect` after the decoder had already resolved it.
`Fraction(str(...))` recovered the float's decimal expansion instead of
the rational, so an exactly-half product rounded the wrong way; then
`limit_denominator(99)` guessed the rational back, which is only sound
for ratios near a standard one — a container declaring 18333/20000 was
snapped to 11/12, and the canvas used 1766 where the decoder used 1765.
- `SourceInfo.width` and `.height` are already the decoder's own
`_display_dimensions` output, and `CropPresentation` already carries
them. `_orientation_transform` now takes that extent rather than
deriving one, so there is nothing left to round, guess or keep in
step. `pixel_aspect` stays on `MediaTransform` for its own scaling.
- No fallback recreates the extent when display metadata is missing: a
fallback here would be the same guess under another name.
- 🧪 Tests:
- Two tests added with the previous attempt were hollow and are gone
rather than adjusted. One retyped the production expression inside the
test, so it exercised the standard library — reverting the production
arithmetic left all sixteen cases passing. The other swept the whole
H.264 aspect-ratio table at dimensions where no product lands on a
half, so all sixty-four cases passed with the arithmetic it was meant
to pin.
- What replaces them asserts against `_display_dimensions` itself: the
rotation x pixel-aspect matrices, the exact-half case, and the
18333/20000 counter-example that the guess got wrong.
Trigger: repro — coded 1926x1080 declaring pixel aspect 18333/20000 at
rotation 0; the decoder produced width 1765 while the canvas snapped the
ratio to 11/12 and used 1766.
Refs #164
|
Code Review ✅ Approved 1 resolved / 1 findingsFixes crop rectangle mapping for anamorphic sources by applying pixel aspect consistently across all rotations and rounding the display width to match the decoder's exact rule. The viewport now uses oriented display axes from ✅ 1 resolved✅ Quality: _round_fraction duplicates existing _rhu in the same module
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Important Your trial ends in 7 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
1 participant
The crop rectangle was painted in one place and cut in another on a source with
non-square pixels and no rotation.
_orientation_transform(core/crop.py) built its viewport with the rawsource width for rotation 0 and 180 and applied
pixel_aspectonly in the90/270 branch, while
MediaTransform.oriented_display_sizeapplies it to thewidth for 0 and 180.
MediaTransform.scaletherefore fitted against a box themapping did not use.
Measured
oriented_rect_to_source_rectfor the oriented rectangleCropSpec(2, 1, 4, 6)on a 16x8 source, over rotation {0, 90, 180, 270} and pixel aspect
{1.0, 2.0, 1.5, 0.75} — a rectangle chosen to stay inside the oriented frame in
all sixteen combinations. A cell counts as broken when the mapped rectangle
leaves the source frame, or when mapping it back through the display geometry
does not return the rectangle that went in:
Every broken cell is a 0 or 180 rotation with a pixel aspect other than 1. The
worst of them: at pixel aspect 2 the oriented
CropSpec(8, 0, 8, 8)mapped tox=8 y=-4 w=8 h=16— starting above an 8 px frame and twice its height.A second, smaller defect in the same property
Review of the first fix found that the canvas mapped with an unrounded
display width while the decoder produces a rounded one, so the two grids
disagree whenever
coded_width x pixel_aspectis not an integer.A 720x480 source at pixel aspect 32/27 — NTSC DVD 16:9 — decodes to 853
pixels wide (
_display_dimensions,jobs/source.py) while the canvas extentwas 853.3333. A rectangle near the right edge was painted and hit-tested
against a slightly different grid than the one the renderer cuts. Rotation
moves the discrepancy to the other axis rather than removing it.
The branch was unpushed, so this is folded into the same commit rather than
chasing it with a second one.
Effect
Anamorphic material without rotation: SD broadcast 16:9 and DVD-era sources,
where the coded frame is 720 wide and displays at 1024. Phone and screen
captures have square pixels, which is why this was never reported.
The change
The viewport keeps the oriented display axes from
oriented_display_sizeandrounds the pixel-aspect width with the decoder's exact round-half-up rule.
core/geometry.pyis frozen in the guardrail baseline and is untouched. Pixelaspect 1 is unchanged, which is what the existing crop, staging and controller
tests pin.
The viewport expression no longer exists twice. The rounding rule does remain
in two layers, because
coremay not importjobs; the fractional-aspect testbelow is what holds the two copies together — it asserts against
_display_dimensionsitself rather than a written-out 853, so it fails ifeither side drifts.
Tests
Two matrices over the sixteen rotation/pixel-aspect combinations in
tests/core/test_crop.py: the mapped rectangle stays inside the source frameand round-trips back through the display geometry, and
oriented_point_from_widgetround-trips throughbuild_crop_geometry. Plus afour-rotation fractional-aspect matrix for the 720x480, 32/27 repro.
Measured: 16 of 52 cases in that file fail with
core/crop.pyreverted tomain, and all 52 pass with the fix.tests/core/test_crop.py:63pinned rotation 270 with pixel aspect 2 — one ofthe ten combinations that already passed, which is why the gap survived. That
case is kept.
Guardrails
This is the third consecutive
fix:commit touchingcore/crop.py, whichdocs/engineering-guardrails.mdsays to stop and raise rather than takesilently. Raised and agreed. It is not a hardening spiral:
a296a3bboundedfit_crop_aspect,cc9ec01changedclamp_crop, and this changes_orientation_transform— three independent defects in three functions.Verification
Found while verifying the design for #80, which needs this mapping to paint
exclusion rectangles where they are applied.
Closes #164