Remove old TODOs (meta-pytorch#1481)

Author: NicolasHug

src/torchcodec/_core/SingleStreamDecoder.cpp

@@ -1329,8 +1329,6 @@ bool SingleStreamDecoder::canWeAvoidSeeking() const {
   if (lastDecodedAvFramePts_ == cursor_) {
     // We are seeking to the exact same frame as we are currently at. Without
     // caching we have to rewind back and decode the frame again.
-    // TODO: https://github.com/pytorch/torchcodec/issues/84 we could
-    // implement caching.
     return false;
   }
   // We are seeking forwards. We can skip a seek if both the last decoded frame

src/torchcodec/encoders/_multi_stream_encoder.py

@@ -7,22 +7,18 @@
 from torchcodec import _core
 
 
-# TODO MultiStreamEncoder: the stream_index values here are per media-type,
-# while everywhere else in the code base (and particularly in the public decoder
-# APIs) they are absolute across all media types. That'll quickly becomes
-# confusing, and we should definitely not expose this one as-is. We should either:
-# - keep it private but rename it to something that's not stream_index
-# - make it absolute per container, if we ever want to expose it.
 class VideoStream:
     """A video stream within an :class:`Encoder`.
 
     Returned by :meth:`Encoder.add_video`. Use :meth:`add_frames` to feed
     video frames into this stream.
     """
 
-    def __init__(self, encoder_tensor: Tensor, stream_index: int):
+    def __init__(self, encoder_tensor: Tensor, stream_identifier: int):
         self._encoder_tensor = encoder_tensor
-        self._stream_index = stream_index
+        # This is a per-media-type index, not absolute across all media types,
+        # hence why we don't call this stream_index
+        self._stream_identifier = stream_identifier
 
     def add_frames(self, frames: Tensor) -> None:
         """Add video frames to this stream.
@@ -36,7 +32,7 @@ def add_frames(self, frames: Tensor) -> None:
                 :meth:`Encoder.add_video`.
         """
         _core.streaming_encoder_add_frames(
-            self._encoder_tensor, frames, self._stream_index
+            self._encoder_tensor, frames, self._stream_identifier
         )
 
 
@@ -47,9 +43,11 @@ class AudioStream:
     audio samples into this stream.
     """
 
-    def __init__(self, encoder_tensor: Tensor, stream_index: int):
+    def __init__(self, encoder_tensor: Tensor, stream_identifier: int):
         self._encoder_tensor = encoder_tensor
-        self._stream_index = stream_index
+        # This is a per-media-type index, not absolute across all media types,
+        # hence why we don't call this stream_index
+        self._stream_identifier = stream_identifier
 
     def add_samples(self, samples: Tensor) -> None:
         """Add audio samples to this stream.
@@ -61,7 +59,7 @@ def add_samples(self, samples: Tensor) -> None:
                 match the ``num_channels`` passed to :meth:`Encoder.add_audio`.
         """
         _core.streaming_encoder_add_samples(
-            self._encoder_tensor, samples, self._stream_index
+            self._encoder_tensor, samples, self._stream_identifier
         )
 
 
@@ -166,7 +164,7 @@ def add_video(
             device = torch.get_default_device()
         device = str(device)
         preset = str(preset) if isinstance(preset, int) else preset
-        stream_index = _core.streaming_encoder_add_video_stream(
+        stream_identifier = _core.streaming_encoder_add_video_stream(
             self._encoder_tensor,
             height=height,
             width=width,
@@ -180,7 +178,7 @@ def add_video(
                 str(x) for k, v in (extra_options or {}).items() for x in (k, v)
             ],
         )
-        return VideoStream(self._encoder_tensor, stream_index)
+        return VideoStream(self._encoder_tensor, stream_identifier)
 
     def add_audio(
         self,
@@ -211,15 +209,15 @@ def add_audio(
             An audio stream object. Use its :meth:`~AudioStream.add_samples`
             method to feed samples into the stream.
         """
-        stream_index = _core.streaming_encoder_add_audio_stream(
+        stream_identifier = _core.streaming_encoder_add_audio_stream(
             self._encoder_tensor,
             sample_rate=sample_rate,
             num_channels=num_channels,
             bit_rate=bit_rate,
             output_num_channels=out_num_channels,
             output_sample_rate=out_sample_rate,
         )
-        return AudioStream(self._encoder_tensor, stream_index)
+        return AudioStream(self._encoder_tensor, stream_identifier)
 
     def open_file(self, dest: str | Path) -> "Encoder":
         """Open a file for writing the encoded output.

test/test_decoders.py

@@ -39,7 +39,6 @@
     get_ffmpeg_minor_version,
     get_python_version,
     H264_10BITS,
-    H265_10BITS,
     H265_VIDEO,
     in_fbcode,
     make_video_decoder,
@@ -1643,8 +1642,6 @@ def test_10bit_gpu_fallsback_to_cpu(self):
         # do the color conversion on the CPU.
         # Here we just assert that the GPU results are the same as the CPU
         # results.
-        # TODO see other TODO below in test_10bit_videos_cpu: we should validate
-        # the frames against a reference.
         #
         # This test exercises the FFmpeg CUDA interface specifically: its CPU
         # fallback delegates directly to CpuDeviceInterface, so the output
@@ -1675,15 +1672,6 @@ def test_10bit_gpu_fallsback_to_cpu(self):
         frames_cpu = decoder_cpu.get_frames_at(frame_indices).data
         assert_frames_equal(frames_gpu.cpu(), frames_cpu)
 
-    @pytest.mark.parametrize("device", all_supported_devices())
-    @pytest.mark.parametrize("asset", (H264_10BITS, H265_10BITS))
-    def test_10bit_videos(self, device, asset):
-        # This just validates that we can decode 10-bit videos.
-        # TODO validate against the ref that the decoded frames are correct
-
-        decoder, _ = make_video_decoder(asset.path, device=device)
-        decoder.get_frame_at(10)
-
     def setup_frame_mappings(tmp_path, file, stream_index):
         json_path = tmp_path / "custom_frame_mappings.json"
         custom_frame_mappings = NASA_VIDEO.generate_custom_frame_mappings(stream_index)
@@ -1811,20 +1799,21 @@ def test_get_frames_at_tensor_indices(self):
         decoder.get_frames_played_at(torch.tensor([0, 1], dtype=torch.int))
         decoder.get_frames_played_at(torch.tensor([0, 1], dtype=torch.float))
 
-    # TODONVDEC P1:
-    # - unskip equality assertion checks on FFMpeg4. The comparison
-    #   checks are failing on very few pixels, e.g.:
+    # Note [NVDEC vs FFmpeg CUDA pixel mismatches]:
+    # These tests compare the NVDEC (beta) CUDA backend against the FFmpeg
+    # CUDA backend. There are two known sources of pixel mismatches:
     #
-    #   E   Mismatched elements: 648586 / 82944000 (0.8%)
-    #   E   Greatest absolute difference: 164 at index (20, 2, 27, 96)
-    #   E   Greatest relative difference: inf at index (5, 1, 112, 186)
+    # 1. FFmpeg 4: small pixel differences on a few pixels (< 1%), cause
+    #    unknown. We don't investigate further since FFmpeg 4 is not a
+    #    priority.
     #
-    #   So we're skipping them to unblock for now, but we should call
-    #   assert_tensor_close_on_at_least or something like that.
-    # - unskip equality assertion checks for MPEG4 asset. The frames are decoded
-    #   fine, it's the color conversion that's different. The frame from the
-    #   NVDEC interface is mapped to 709 by the matrix coefficient
-    #   using NVCUVID while the one from the FFmpeg CUDA interface is 601.
+    # 2. MPEG4 asset: NVCUVID's parser reports matrix_coefficients=1
+    #    (BT.709) for the MPEG4 asset, even though the bitstream has no
+    #    color metadata. This is an NVIDIA-internal heuristic. FFmpeg's
+    #    parser leaves colorspace as UNSPECIFIED, which both swscale (CPU)
+    #    and our color conversion code treat as BT.601. So the NVDEC
+    #    backend uses BT.709 while the FFmpeg CUDA backend (and CPU) use
+    #    BT.601 for this asset, leading to different RGB output.
 
     @needs_cuda
     @pytest.mark.parametrize(
@@ -1864,7 +1853,7 @@ def test_nvdec_cuda_interface_get_frame_at(
         for frame_index in indices:
             ref_frame = ref_decoder.get_frame_at(frame_index)
             nvdec_frame = nvdec_decoder.get_frame_at(frame_index)
-            # TODONVDEC P1 see above
+            # See Note [NVDEC vs FFmpeg CUDA pixel mismatches]
             if ffmpeg_major_version > 5 and asset is not TEST_SRC_2_720P_MPEG4:
                 torch.testing.assert_close(
                     nvdec_frame.data, ref_frame.data, rtol=0, atol=0
@@ -1911,7 +1900,7 @@ def test_nvdec_cuda_interface_get_frames_at(
 
         ref_frames = ref_decoder.get_frames_at(indices)
         nvdec_frames = nvdec_decoder.get_frames_at(indices)
-        # TODONVDEC P1 see above
+        # See Note [NVDEC vs FFmpeg CUDA pixel mismatches]
         if ffmpeg_major_version > 5 and asset is not TEST_SRC_2_720P_MPEG4:
             torch.testing.assert_close(
                 nvdec_frames.data, ref_frames.data, rtol=0, atol=0
@@ -1954,7 +1943,7 @@ def test_nvdec_cuda_interface_get_frame_played_at(self, asset, seek_mode):
         for pts in timestamps:
             ref_frame = ref_decoder.get_frame_played_at(pts)
             nvdec_frame = nvdec_decoder.get_frame_played_at(pts)
-            # TODONVDEC P1 see above
+            # See Note [NVDEC vs FFmpeg CUDA pixel mismatches]
             if ffmpeg_major_version > 5 and asset is not TEST_SRC_2_720P_MPEG4:
                 torch.testing.assert_close(
                     nvdec_frame.data, ref_frame.data, rtol=0, atol=0
@@ -1996,7 +1985,7 @@ def test_nvdec_cuda_interface_get_frames_played_at(self, asset, seek_mode):
 
         ref_frames = ref_decoder.get_frames_played_at(timestamps)
         nvdec_frames = nvdec_decoder.get_frames_played_at(timestamps)
-        # TODONVDEC P1 see above
+        # See Note [NVDEC vs FFmpeg CUDA pixel mismatches]
         if ffmpeg_major_version > 5 and asset is not TEST_SRC_2_720P_MPEG4:
             torch.testing.assert_close(
                 nvdec_frames.data, ref_frames.data, rtol=0, atol=0
@@ -2043,7 +2032,7 @@ def test_nvdec_cuda_interface_backwards(self, asset, seek_mode):
 
             ref_frame = ref_decoder.get_frame_at(frame_index)
             nvdec_frame = nvdec_decoder.get_frame_at(frame_index)
-            # TODONVDEC P1 see above
+            # See Note [NVDEC vs FFmpeg CUDA pixel mismatches]
             if ffmpeg_major_version > 5 and asset is not TEST_SRC_2_720P_MPEG4:
                 torch.testing.assert_close(
                     nvdec_frame.data, ref_frame.data, rtol=0, atol=0
@@ -2071,8 +2060,9 @@ def test_cuda_mpeg4_mp4_first_frame(self, seek_mode):
         assert frame0.pts_seconds == expected_frame0.pts_seconds
         assert frame0.duration_seconds == expected_frame0.duration_seconds
         assert frame0.data.shape == expected_frame0.data.shape
-        # Strict pixel equality is skipped — TODONVDEC P1 above (BT.601 vs
-        # BT.709 color matrix mismatch between the ffmpeg and default cuda).
+        # Strict pixel equality is skipped — see Note [NVDEC vs FFmpeg CUDA
+        # pixel mismatches] (BT.601 vs BT.709 color matrix mismatch between the
+        # ffmpeg and default cuda backend for this MPEG4 asset).
 
     @needs_cuda
     def test_nvdec_cuda_interface_cpu_fallback(self):