From 8720d95d98f5e0bbf1cefc64ac18c10e4806a097 Mon Sep 17 00:00:00 2001 From: smb Date: Sat, 12 Sep 2026 07:43:35 +0200 Subject: [PATCH 1/4] =?UTF-8?q?=F0=9F=90=9B=20fix(crop):=20apply=20pixel?= =?UTF-8?q?=20aspect=20at=20every=20rotation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ๐Ÿ› 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 --- src/matteloop/core/crop.py | 24 +++++++--- tests/core/test_crop.py | 89 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 5 deletions(-) diff --git a/src/matteloop/core/crop.py b/src/matteloop/core/crop.py index 8b4d671..b47b905 100644 --- a/src/matteloop/core/crop.py +++ b/src/matteloop/core/crop.py @@ -3,6 +3,7 @@ from __future__ import annotations import math +from dataclasses import replace from fractions import Fraction from matteloop.core.geometry import ( @@ -255,15 +256,23 @@ def _orientation_transform( source_width: int, source_height: int, rotation: int, pixel_aspect: float ) -> MediaTransform: _validate_dimensions(source_width, source_height) - return MediaTransform( + transform = MediaTransform( source_size=SizeF(source_width, source_height), - viewport=SizeF( - source_height if rotation in {90, 270} else source_width, - source_width * pixel_aspect if rotation in {90, 270} else source_height, - ), + viewport=SizeF(source_width, source_height), rotation=rotation, pixel_aspect=pixel_aspect, ) + display_width = max( + 1, + _round_fraction(Fraction(source_width) * Fraction(str(pixel_aspect))), + ) + return replace( + transform, + viewport=SizeF( + source_height if rotation in {90, 270} else display_width, + display_width if rotation in {90, 270} else source_height, + ), + ) def _orientation_transform_from_media(transform: MediaTransform) -> MediaTransform: @@ -279,6 +288,11 @@ def _rounded_delta(value: float) -> int: return math.floor(value + 0.5) if value >= 0 else math.ceil(value - 0.5) +def _round_fraction(value: Fraction) -> int: + quotient, remainder = divmod(value.numerator, value.denominator) + return quotient + int(remainder * 2 >= value.denominator) + + def _validate_dimensions(source_width: int, source_height: int) -> None: if ( not isinstance(source_width, int) diff --git a/tests/core/test_crop.py b/tests/core/test_crop.py index 7851fe8..7730870 100644 --- a/tests/core/test_crop.py +++ b/tests/core/test_crop.py @@ -2,7 +2,10 @@ from fractions import Fraction +import pytest + from matteloop.core.crop import ( + _orientation_transform, centered_crop_for_aspect, clamp_crop, crop_from_drag, @@ -13,12 +16,14 @@ ) from matteloop.core.geometry import ( CropGeometryState, + MediaTransform, PointF, RectF, SizeF, build_crop_geometry, ) from matteloop.core.specs import CropSpec +from matteloop.jobs.source import _display_dimensions def test_dragging_crop_moves_and_clamps_in_oriented_source_pixels() -> None: @@ -84,6 +89,90 @@ def test_oriented_crop_geometry_maps_rotation_and_pixel_aspect_once() -> None: assert oriented_point_from_widget(geometry, PointF(0, 0)) == PointF(0, 0) +@pytest.mark.parametrize("rotation", [0, 90, 180, 270]) +def test_oriented_crop_mapping_uses_rounded_decoder_extent(rotation: int) -> None: + """720x480 at pixel aspect 32/27 (NTSC DVD 16:9) decodes to 853, not 853.33. + + Asserting against ``_display_dimensions`` rather than a written-out 853 + is the point: the canvas and the decoder each round the same product, and + this is the assertion that fails if either one drifts from the other. + """ + decoded = _display_dimensions(720, 480, Fraction(32, 27), rotation) + + transform = _orientation_transform(720, 480, rotation, 32 / 27) + + assert transform.viewport == SizeF(*decoded) + + +@pytest.mark.parametrize("rotation", [0, 90, 180, 270]) +@pytest.mark.parametrize("pixel_aspect", [1.0, 2.0, 1.5, 0.75]) +def test_oriented_crop_mapping_round_trips_inside_source_frame( + rotation: int, pixel_aspect: float +) -> None: + source_size = SizeF(16, 8) + crop = CropSpec(4, 0, 4, 8) + display_size = MediaTransform( + source_size=source_size, + viewport=SizeF(1, 1), + rotation=rotation, + pixel_aspect=pixel_aspect, + ).oriented_display_size + + mapped = oriented_rect_to_source_rect( + crop, + source_width=16, + source_height=8, + rotation=rotation, + pixel_aspect=pixel_aspect, + ) + + assert mapped.x >= 0 + assert mapped.y >= 0 + assert mapped.right <= source_size.width + assert mapped.bottom <= source_size.height + + restored = MediaTransform( + source_size=source_size, + viewport=display_size, + rotation=rotation, + pixel_aspect=pixel_aspect, + ).source_rect_to_widget(mapped) + + assert restored.x == pytest.approx(crop.x) + assert restored.y == pytest.approx(crop.y) + assert restored.width == pytest.approx(crop.width) + assert restored.height == pytest.approx(crop.height) + + +@pytest.mark.parametrize("rotation", [0, 90, 180, 270]) +@pytest.mark.parametrize("pixel_aspect", [1.0, 2.0, 1.5, 0.75]) +def test_oriented_point_mapping_round_trips_through_display_geometry( + rotation: int, pixel_aspect: float +) -> None: + source_size = SizeF(16, 8) + display_size = MediaTransform( + source_size=source_size, + viewport=SizeF(1, 1), + rotation=rotation, + pixel_aspect=pixel_aspect, + ).oriented_display_size + geometry = build_crop_geometry( + state=CropGeometryState( + source_size=source_size, + crop=RectF(0, 0, 8, 8), + rotation=rotation, + pixel_aspect=pixel_aspect, + ), + viewport=display_size, + dpr=1, + ) + + restored = oriented_point_from_widget(geometry, PointF(2, 3)) + + assert restored.x == pytest.approx(2) + assert restored.y == pytest.approx(3) + + def test_fit_crop_aspect_on_a_corner_keeps_the_opposite_corner_fixed() -> None: fitted = fit_crop_aspect( CropSpec(2, 3, 6, 5), From 3d5d83e5718ac231f0ef37e23a696240ef91c8a6 Mon Sep 17 00:00:00 2001 From: smb Date: Sat, 12 Sep 2026 11:38:35 +0200 Subject: [PATCH 2/4] =?UTF-8?q?=F0=9F=90=9B=20fix(crop):=20recover=20the?= =?UTF-8?q?=20exact=20aspect=20ratio?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ๐Ÿ› 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 --- src/matteloop/core/crop.py | 12 ++++++- tests/core/test_crop.py | 65 ++++++++++++++++++++++++++++++++++---- 2 files changed, 69 insertions(+), 8 deletions(-) diff --git a/src/matteloop/core/crop.py b/src/matteloop/core/crop.py index b47b905..3a5aa82 100644 --- a/src/matteloop/core/crop.py +++ b/src/matteloop/core/crop.py @@ -15,6 +15,8 @@ ) from matteloop.core.specs import CropSpec +_MAX_ASPECT_DENOMINATOR = 99 + _HANDLES = frozenset( { "north_west", @@ -262,9 +264,17 @@ def _orientation_transform( rotation=rotation, pixel_aspect=pixel_aspect, ) + # The presentation layer hands this in as a float, so the decoder's exact + # ratio has to be recovered before it is rounded -- 1926 * 11/12 is exactly + # 1765.5, and the float product 1765.4999999999998 rounds the other way. + # H.264 Table E-1 defines sixteen sample aspect ratios whose largest + # denominator is 99 (160:99); a bound of 33 silently misses that one. + pixel_aspect_fraction = Fraction(pixel_aspect).limit_denominator( + _MAX_ASPECT_DENOMINATOR + ) display_width = max( 1, - _round_fraction(Fraction(source_width) * Fraction(str(pixel_aspect))), + _round_fraction(Fraction(source_width) * pixel_aspect_fraction), ) return replace( transform, diff --git a/tests/core/test_crop.py b/tests/core/test_crop.py index 7730870..6fa06d9 100644 --- a/tests/core/test_crop.py +++ b/tests/core/test_crop.py @@ -5,6 +5,7 @@ import pytest from matteloop.core.crop import ( + _MAX_ASPECT_DENOMINATOR, _orientation_transform, centered_crop_for_aspect, clamp_crop, @@ -90,16 +91,30 @@ def test_oriented_crop_geometry_maps_rotation_and_pixel_aspect_once() -> None: @pytest.mark.parametrize("rotation", [0, 90, 180, 270]) -def test_oriented_crop_mapping_uses_rounded_decoder_extent(rotation: int) -> None: - """720x480 at pixel aspect 32/27 (NTSC DVD 16:9) decodes to 853, not 853.33. +@pytest.mark.parametrize( + ("source_width", "source_height", "pixel_aspect"), + [ + pytest.param(720, 480, Fraction(32, 27), id="below-half"), + pytest.param(640, 480, Fraction(11, 12), id="above-half"), + pytest.param(1926, 1080, Fraction(11, 12), id="exact-half"), + ], +) +def test_oriented_crop_mapping_uses_rounded_decoder_extent( + rotation: int, source_width: int, source_height: int, pixel_aspect: Fraction +) -> None: + """The canvas uses the decoder's rounded extent on both sides of .5. - Asserting against ``_display_dimensions`` rather than a written-out 853 - is the point: the canvas and the decoder each round the same product, and - this is the assertion that fails if either one drifts from the other. + Asserting against ``_display_dimensions`` rather than written-out widths + is the point: the canvas and decoder each round the same product, and this + is the assertion that fails if either one drifts from the other. """ - decoded = _display_dimensions(720, 480, Fraction(32, 27), rotation) + decoded = _display_dimensions( + source_width, source_height, pixel_aspect, rotation + ) - transform = _orientation_transform(720, 480, rotation, 32 / 27) + transform = _orientation_transform( + source_width, source_height, rotation, float(pixel_aspect) + ) assert transform.viewport == SizeF(*decoded) @@ -298,3 +313,39 @@ def test_clamp_crop_leaves_a_crop_that_already_fits_unchanged() -> None: crop = CropSpec(5, 5, 50, 50) assert clamp_crop(crop, 100, 100) == crop + + +# H.264 Table E-1: every standard sample aspect ratio, largest denominator 99. +_H264_SAMPLE_ASPECT_RATIOS = [ + (1, 1), (12, 11), (10, 11), (16, 11), (40, 33), (24, 11), (20, 11), + (32, 11), (80, 33), (18, 11), (15, 11), (64, 33), (160, 99), (4, 3), + (3, 2), (2, 1), +] + + +@pytest.mark.parametrize(("numerator", "denominator"), _H264_SAMPLE_ASPECT_RATIOS) +def test_every_standard_sample_aspect_ratio_survives_the_float_round_trip( + numerator: int, denominator: int +) -> None: + """The presentation layer hands the mapping a float, so the exact ratio has + to be recoverable for every ratio a real container can declare -- not only + the handful used as examples. A denominator bound below 99 loses 160:99. + """ + exact = Fraction(numerator, denominator) + + recovered = Fraction(float(exact)).limit_denominator(_MAX_ASPECT_DENOMINATOR) + + assert recovered == exact + + +@pytest.mark.parametrize(("numerator", "denominator"), _H264_SAMPLE_ASPECT_RATIOS) +@pytest.mark.parametrize("rotation", [0, 90, 180, 270]) +def test_canvas_extent_matches_the_decoder_for_every_standard_ratio( + numerator: int, denominator: int, rotation: int +) -> None: + exact = Fraction(numerator, denominator) + + decoded = _display_dimensions(1926, 1080, exact, rotation) + transform = _orientation_transform(1926, 1080, rotation, float(exact)) + + assert transform.viewport == SizeF(*decoded) From 082720e15ba93e884c2be1442a0e16c399adaede Mon Sep 17 00:00:00 2001 From: smb Date: Sat, 12 Sep 2026 13:33:53 +0200 Subject: [PATCH 3/4] =?UTF-8?q?=F0=9F=94=84=20refactor(crop):=20reuse=20th?= =?UTF-8?q?e=20rounding=20helper?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ๐Ÿ”„ `_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 --- src/matteloop/core/crop.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/matteloop/core/crop.py b/src/matteloop/core/crop.py index 3a5aa82..1729081 100644 --- a/src/matteloop/core/crop.py +++ b/src/matteloop/core/crop.py @@ -274,7 +274,7 @@ def _orientation_transform( ) display_width = max( 1, - _round_fraction(Fraction(source_width) * pixel_aspect_fraction), + _rhu(Fraction(source_width) * pixel_aspect_fraction), ) return replace( transform, @@ -298,10 +298,6 @@ def _rounded_delta(value: float) -> int: return math.floor(value + 0.5) if value >= 0 else math.ceil(value - 0.5) -def _round_fraction(value: Fraction) -> int: - quotient, remainder = divmod(value.numerator, value.denominator) - return quotient + int(remainder * 2 >= value.denominator) - def _validate_dimensions(source_width: int, source_height: int) -> None: if ( From dcf14a685d7c0fb8f19ad8f1bfb87d7aa7bad021 Mon Sep 17 00:00:00 2001 From: smb Date: Sat, 12 Sep 2026 14:02:22 +0200 Subject: [PATCH 4/4] =?UTF-8?q?=F0=9F=90=9B=20fix(crop):=20use=20the=20dec?= =?UTF-8?q?oder's=20display=20extent?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ๐Ÿ› 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 --- src/matteloop/core/crop.py | 60 ++++++++--------- src/matteloop/ui/crop_canvas.py | 17 ++++- tests/core/test_crop.py | 111 ++++++++++++++++---------------- 3 files changed, 100 insertions(+), 88 deletions(-) diff --git a/src/matteloop/core/crop.py b/src/matteloop/core/crop.py index 1729081..3a1a4ef 100644 --- a/src/matteloop/core/crop.py +++ b/src/matteloop/core/crop.py @@ -3,7 +3,6 @@ from __future__ import annotations import math -from dataclasses import replace from fractions import Fraction from matteloop.core.geometry import ( @@ -15,8 +14,6 @@ ) from matteloop.core.specs import CropSpec -_MAX_ASPECT_DENOMINATOR = 99 - _HANDLES = frozenset( { "north_west", @@ -99,12 +96,19 @@ def oriented_rect_to_source_rect( *, source_width: int, source_height: int, + display_width: int, + display_height: int, rotation: int, pixel_aspect: float, ) -> RectF: """Map oriented crop pixels into the raw coordinates used by geometry.""" transform = _orientation_transform( - source_width, source_height, rotation, pixel_aspect + source_width, + source_height, + display_width, + display_height, + rotation, + pixel_aspect, ) return transform.widget_rect_to_source( RectF(crop.x, crop.y, crop.width, crop.height) @@ -112,14 +116,20 @@ def oriented_rect_to_source_rect( def oriented_point_from_widget( - geometry: InteractionGeometry, point: PointF + geometry: InteractionGeometry, + point: PointF, + *, + display_width: int, + display_height: int, ) -> PointF: """Map a widget point through one crop geometry snapshot to oriented space.""" transform = geometry.transform if not isinstance(transform, MediaTransform): raise ValueError("crop geometry must use a media transform") raw = geometry.widget_to_source(point) - return _orientation_transform_from_media(transform).source_to_widget(raw) + return _orientation_transform_from_media( + transform, display_width=display_width, display_height=display_height + ).source_to_widget(raw) def fit_crop_aspect( @@ -255,40 +265,31 @@ def _resize_crop( def _orientation_transform( - source_width: int, source_height: int, rotation: int, pixel_aspect: float + source_width: int, + source_height: int, + display_width: int, + display_height: int, + rotation: int, + pixel_aspect: float, ) -> MediaTransform: _validate_dimensions(source_width, source_height) - transform = MediaTransform( + _validate_dimensions(display_width, display_height) + return MediaTransform( source_size=SizeF(source_width, source_height), - viewport=SizeF(source_width, source_height), + viewport=SizeF(display_width, display_height), rotation=rotation, pixel_aspect=pixel_aspect, ) - # The presentation layer hands this in as a float, so the decoder's exact - # ratio has to be recovered before it is rounded -- 1926 * 11/12 is exactly - # 1765.5, and the float product 1765.4999999999998 rounds the other way. - # H.264 Table E-1 defines sixteen sample aspect ratios whose largest - # denominator is 99 (160:99); a bound of 33 silently misses that one. - pixel_aspect_fraction = Fraction(pixel_aspect).limit_denominator( - _MAX_ASPECT_DENOMINATOR - ) - display_width = max( - 1, - _rhu(Fraction(source_width) * pixel_aspect_fraction), - ) - return replace( - transform, - viewport=SizeF( - source_height if rotation in {90, 270} else display_width, - display_width if rotation in {90, 270} else source_height, - ), - ) -def _orientation_transform_from_media(transform: MediaTransform) -> MediaTransform: +def _orientation_transform_from_media( + transform: MediaTransform, *, display_width: int, display_height: int +) -> MediaTransform: return _orientation_transform( int(transform.source_size.width), int(transform.source_size.height), + display_width, + display_height, transform.rotation, transform.pixel_aspect, ) @@ -298,7 +299,6 @@ def _rounded_delta(value: float) -> int: return math.floor(value + 0.5) if value >= 0 else math.ceil(value - 0.5) - def _validate_dimensions(source_width: int, source_height: int) -> None: if ( not isinstance(source_width, int) diff --git a/src/matteloop/ui/crop_canvas.py b/src/matteloop/ui/crop_canvas.py index 75e915b..1f079e6 100644 --- a/src/matteloop/ui/crop_canvas.py +++ b/src/matteloop/ui/crop_canvas.py @@ -169,8 +169,14 @@ def mousePressEvent(self, event: QMouseEvent) -> None: return self._focused_target = target self._dragged = target - self._drag_crop = self._presentation - self._drag_start = oriented_point_from_widget(geometry, position) + presentation = self._presentation + self._drag_crop = presentation + self._drag_start = oriented_point_from_widget( + geometry, + position, + display_width=presentation.width, + display_height=presentation.height, + ) self.setFocus(Qt.FocusReason.MouseFocusReason) self._rebuild_geometry(focused=target, dragged=target) event.accept() @@ -244,7 +250,10 @@ def _emit_drag(self, position: QPointF) -> None: if geometry is None or presentation is None or self._dragged is None: return current = oriented_point_from_widget( - geometry, PointF(position.x(), position.y()) + geometry, + PointF(position.x(), position.y()), + display_width=presentation.width, + display_height=presentation.height, ) crop = crop_from_drag( presentation.crop, @@ -289,6 +298,8 @@ def _rebuild_geometry( presentation.crop, source_width=presentation.coded_width, source_height=presentation.coded_height, + display_width=presentation.width, + display_height=presentation.height, rotation=presentation.rotation, pixel_aspect=presentation.pixel_aspect, ) diff --git a/tests/core/test_crop.py b/tests/core/test_crop.py index 6fa06d9..74e8694 100644 --- a/tests/core/test_crop.py +++ b/tests/core/test_crop.py @@ -5,7 +5,6 @@ import pytest from matteloop.core.crop import ( - _MAX_ASPECT_DENOMINATOR, _orientation_transform, centered_crop_for_aspect, clamp_crop, @@ -68,10 +67,13 @@ def test_crop_keyboard_nudge_uses_ten_source_pixels_with_shift_step() -> None: def test_oriented_crop_geometry_maps_rotation_and_pixel_aspect_once() -> None: crop = CropSpec(0, 0, 8, 8) + display_width, display_height = _display_dimensions(16, 8, Fraction(2), 270) raw_crop = oriented_rect_to_source_rect( crop, source_width=16, source_height=8, + display_width=display_width, + display_height=display_height, rotation=270, pixel_aspect=2, ) @@ -82,12 +84,17 @@ def test_oriented_crop_geometry_maps_rotation_and_pixel_aspect_once() -> None: rotation=270, pixel_aspect=2, ), - viewport=SizeF(8, 32), + viewport=SizeF(display_width, display_height), dpr=1, ) assert geometry.visual["crop"] == RectF(0, 0, 8, 8) - assert oriented_point_from_widget(geometry, PointF(0, 0)) == PointF(0, 0) + assert oriented_point_from_widget( + geometry, + PointF(0, 0), + display_width=display_width, + display_height=display_height, + ) == PointF(0, 0) @pytest.mark.parametrize("rotation", [0, 90, 180, 270]) @@ -113,32 +120,40 @@ def test_oriented_crop_mapping_uses_rounded_decoder_extent( ) transform = _orientation_transform( - source_width, source_height, rotation, float(pixel_aspect) + source_width, + source_height, + decoded[0], + decoded[1], + rotation, + float(pixel_aspect), ) assert transform.viewport == SizeF(*decoded) @pytest.mark.parametrize("rotation", [0, 90, 180, 270]) -@pytest.mark.parametrize("pixel_aspect", [1.0, 2.0, 1.5, 0.75]) +@pytest.mark.parametrize( + "pixel_aspect", + [Fraction(1), Fraction(2), Fraction(3, 2), Fraction(3, 4)], +) def test_oriented_crop_mapping_round_trips_inside_source_frame( - rotation: int, pixel_aspect: float + rotation: int, pixel_aspect: Fraction ) -> None: source_size = SizeF(16, 8) crop = CropSpec(4, 0, 4, 8) - display_size = MediaTransform( - source_size=source_size, - viewport=SizeF(1, 1), - rotation=rotation, - pixel_aspect=pixel_aspect, - ).oriented_display_size + display_width, display_height = _display_dimensions( + 16, 8, pixel_aspect, rotation + ) + display_size = SizeF(display_width, display_height) mapped = oriented_rect_to_source_rect( crop, source_width=16, source_height=8, + display_width=display_width, + display_height=display_height, rotation=rotation, - pixel_aspect=pixel_aspect, + pixel_aspect=float(pixel_aspect), ) assert mapped.x >= 0 @@ -150,7 +165,7 @@ def test_oriented_crop_mapping_round_trips_inside_source_frame( source_size=source_size, viewport=display_size, rotation=rotation, - pixel_aspect=pixel_aspect, + pixel_aspect=float(pixel_aspect), ).source_rect_to_widget(mapped) assert restored.x == pytest.approx(crop.x) @@ -160,29 +175,35 @@ def test_oriented_crop_mapping_round_trips_inside_source_frame( @pytest.mark.parametrize("rotation", [0, 90, 180, 270]) -@pytest.mark.parametrize("pixel_aspect", [1.0, 2.0, 1.5, 0.75]) +@pytest.mark.parametrize( + "pixel_aspect", + [Fraction(1), Fraction(2), Fraction(3, 2), Fraction(3, 4)], +) def test_oriented_point_mapping_round_trips_through_display_geometry( - rotation: int, pixel_aspect: float + rotation: int, pixel_aspect: Fraction ) -> None: source_size = SizeF(16, 8) - display_size = MediaTransform( - source_size=source_size, - viewport=SizeF(1, 1), - rotation=rotation, - pixel_aspect=pixel_aspect, - ).oriented_display_size + display_width, display_height = _display_dimensions( + 16, 8, pixel_aspect, rotation + ) + display_size = SizeF(display_width, display_height) geometry = build_crop_geometry( state=CropGeometryState( source_size=source_size, crop=RectF(0, 0, 8, 8), rotation=rotation, - pixel_aspect=pixel_aspect, + pixel_aspect=float(pixel_aspect), ), viewport=display_size, dpr=1, ) - restored = oriented_point_from_widget(geometry, PointF(2, 3)) + restored = oriented_point_from_widget( + geometry, + PointF(2, 3), + display_width=display_width, + display_height=display_height, + ) assert restored.x == pytest.approx(2) assert restored.y == pytest.approx(3) @@ -315,37 +336,17 @@ def test_clamp_crop_leaves_a_crop_that_already_fits_unchanged() -> None: assert clamp_crop(crop, 100, 100) == crop -# H.264 Table E-1: every standard sample aspect ratio, largest denominator 99. -_H264_SAMPLE_ASPECT_RATIOS = [ - (1, 1), (12, 11), (10, 11), (16, 11), (40, 33), (24, 11), (20, 11), - (32, 11), (80, 33), (18, 11), (15, 11), (64, 33), (160, 99), (4, 3), - (3, 2), (2, 1), -] - +def test_canvas_extent_uses_the_decoder_for_an_arbitrary_declared_ratio() -> None: + pixel_aspect = Fraction(18333, 20000) + decoded = _display_dimensions(1926, 1080, pixel_aspect, 0) -@pytest.mark.parametrize(("numerator", "denominator"), _H264_SAMPLE_ASPECT_RATIOS) -def test_every_standard_sample_aspect_ratio_survives_the_float_round_trip( - numerator: int, denominator: int -) -> None: - """The presentation layer hands the mapping a float, so the exact ratio has - to be recoverable for every ratio a real container can declare -- not only - the handful used as examples. A denominator bound below 99 loses 160:99. - """ - exact = Fraction(numerator, denominator) - - recovered = Fraction(float(exact)).limit_denominator(_MAX_ASPECT_DENOMINATOR) - - assert recovered == exact - - -@pytest.mark.parametrize(("numerator", "denominator"), _H264_SAMPLE_ASPECT_RATIOS) -@pytest.mark.parametrize("rotation", [0, 90, 180, 270]) -def test_canvas_extent_matches_the_decoder_for_every_standard_ratio( - numerator: int, denominator: int, rotation: int -) -> None: - exact = Fraction(numerator, denominator) - - decoded = _display_dimensions(1926, 1080, exact, rotation) - transform = _orientation_transform(1926, 1080, rotation, float(exact)) + transform = _orientation_transform( + 1926, + 1080, + decoded[0], + decoded[1], + 0, + float(pixel_aspect), + ) assert transform.viewport == SizeF(*decoded)