Moare refactor pleasze

Author: scotts

src/torchcodec/decoders/_video_decoder.py

@@ -518,31 +518,26 @@ def _make_transform_specs(
     ] = []
     curr_input_dims = input_dims
     for transform in transforms:
-        if isinstance(transform, DecoderTransform):
-            output_dims = transform._get_output_dims()
-            converted_transforms.append((transform, curr_input_dims))
-        else:
+        if not isinstance(transform, DecoderTransform):
             if not tv_available:
                 raise ValueError(
                     f"The supplied transform, {transform}, is not a TorchCodec "
                     " DecoderTransform. TorchCodec also accepts TorchVision "
                     "v2 transforms, but TorchVision is not installed."
                 )
             elif isinstance(transform, v2.Resize):
-                tc_transform = Resize._from_torchvision(transform)
-                output_dims = tc_transform._get_output_dims()
-                converted_transforms.append((tc_transform, curr_input_dims))
+                transform = Resize._from_torchvision(transform)
             elif isinstance(transform, v2.RandomCrop):
-                tc_transform = RandomCrop._from_torchvision(transform)
-                output_dims = tc_transform._get_output_dims()
-                converted_transforms.append((tc_transform, curr_input_dims))
+                transform = RandomCrop._from_torchvision(transform)
             else:
                 raise ValueError(
                     f"Unsupported transform: {transform}. Transforms must be "
                     "either a TorchCodec DecoderTransform or a TorchVision "
                     "v2 transform."
                 )
 
+        converted_transforms.append((transform, curr_input_dims))
+        output_dims = transform._get_output_dims()
         curr_input_dims = output_dims if output_dims is not None else curr_input_dims
 
     return ";".join([t._make_transform_spec(dims) for t, dims in converted_transforms])

src/torchcodec/transforms/_decoder_transforms.py

@@ -129,11 +129,6 @@ class RandomCrop(DecoderTransform):
 
     size: Sequence[int]
 
-    # Note that these values are never read by this object or the decoder. We
-    # record them for testing purposes only.
-    _top: Optional[int] = None
-    _left: Optional[int] = None
-
     def _make_transform_spec(
         self, input_dims: Tuple[Optional[int], Optional[int]]
     ) -> str:
@@ -165,10 +160,7 @@ def _make_transform_spec(
             )
 
         top = int(torch.randint(0, height - self.size[0] + 1, size=()).item())
-        self._top = top
-
         left = int(torch.randint(0, width - self.size[1] + 1, size=()).item())
-        self._left = left
 
         return f"crop, {self.size[0]}, {self.size[1]}, {left}, {top}"
 

test/test_transform_ops.py

@@ -163,11 +163,15 @@ def test_random_crop_torchvision(
 
         # We want both kinds of RandomCrop objects to get arrive at the same
         # locations to crop, so we need to make sure they get the same random
-        # seed.
+        # seed. It's used in RandomCrop's _make_transform_spec() method, called
+        # by the VideoDecoder.
         torch.manual_seed(seed)
         tc_random_crop = torchcodec.transforms.RandomCrop(size=(height, width))
         decoder_random_crop = VideoDecoder(video.path, transforms=[tc_random_crop])
 
+        # Resetting manual seed for when TorchCodec's RandomCrop, created from
+        # the TorchVision RandomCrop, is used inside of the VideoDecoder. It
+        # needs to match the call above.
         torch.manual_seed(seed)
         decoder_random_crop_tv = VideoDecoder(
             video.path,
@@ -193,14 +197,11 @@ def test_random_crop_torchvision(
             expected_shape = (video.get_num_color_channels(), height, width)
             assert frame_random_crop_tv.shape == expected_shape
 
+            # Resetting manual seed to make sure the invocation of the
+            # TorchVision RandomCrop matches the two calls above.
+            torch.manual_seed(seed)
             frame_full = decoder_full[frame_index]
-            frame_tv = v2.functional.crop(
-                frame_full,
-                top=tc_random_crop._top,
-                left=tc_random_crop._left,
-                height=tc_random_crop.size[0],
-                width=tc_random_crop.size[1],
-            )
+            frame_tv = v2.RandomCrop(size=(height, width))(frame_full)
             assert_frames_equal(frame_random_crop, frame_tv)
 
     @pytest.mark.parametrize(
@@ -260,18 +261,23 @@ def test_crop_fails(self, error_message, params):
     @pytest.mark.parametrize("seed", [0, 314])
     def test_random_crop_reusable_objects(self, seed):
         torch.manual_seed(seed)
-        random_crop = torchcodec.transforms.RandomCrop(size=(100, 100))
+        random_crop = torchcodec.transforms.RandomCrop(size=(99, 99))
 
         # Create a spec which causes us to calculate the random crop location.
-        _ = random_crop._make_transform_spec((1000, 1000))
-        first_top = random_crop._top
-        first_left = random_crop._left
+        first_spec = random_crop._make_transform_spec((888, 888))
 
         # Create a spec again, which should calculate a different random crop
-        # location.
-        _ = random_crop._make_transform_spec((1000, 1000))
-        assert first_top != random_crop._top
-        assert first_left != random_crop._left
+        # location. Despite having the same image size, the specs should be
+        # different because the crop should be at a different location
+        second_spec = random_crop._make_transform_spec((888, 888))
+        assert first_spec != second_spec
+
+        # Create a spec again, but with a different image size. The specs should
+        # obviously be different, but the original image size should not be in
+        # the spec at all.
+        third_spec = random_crop._make_transform_spec((777, 777))
+        assert third_spec != first_spec
+        assert "888" not in third_spec
 
     @pytest.mark.parametrize(
         "resize, random_crop",