Support negative indices in get_frames_in_range #746
Conversation
facebook-github-bot
added
the
CLA Signed
| IndexError, | ||
| match="Stop index \\(-\\d+\\) must not be less than the start index", | ||
| ): | ||
| decoder.get_frames_in_range(start=0, stop=-1000) |
There was a problem hiding this comment.
I considered possible edge cases for stop, I believe these tests should cover them.
We assert that start is within 0 <= start < self._num_frames, and that stop >= start.
As a result, we do not need to explicitly check that stop is within the valid range, since any higher value is reduced to self._num_frames, and any lower value cannot be below the lowest start value of 0.
There was a problem hiding this comment.
Out of precaution and to be pedantic, maybe also test when both start and stop are positive:
decoder.get_frames_in_range(start=10, stop=0)
so that stop is fully independent of our negative index conversion logic
| with pytest.raises( | ||
| RuntimeError, match=f"Invalid frame index={expected_converted_index}" | ||
| ): | ||
| with pytest.raises(IndexError, match="Index -\\d+ is out of bounds"): |
There was a problem hiding this comment.
Since pytest.raises accepts regex patterns, it is not necessary to calculate the index that will appear in the error.
There was a problem hiding this comment.
Thanks for the PR @Dan-Flores . Left a few comments below, this mostly looks good. There's a discussion point I'd like to dig into with you and @scotts , but this isn't blocking for this PR.
| @@ -772,6 +769,66 @@ def test_get_frames_in_range(self, stream_index, device, seek_mode): | |||
| empty_frames.duration_seconds, NASA_VIDEO.empty_duration_seconds | |||
| ) | |||
|
|
|||
| @pytest.mark.parametrize("device", cpu_and_cuda()) | |||
| @pytest.mark.parametrize("stream_index", [3, None]) | |||
There was a problem hiding this comment.
Nit: I think we can remove the stream_index parametrization, because what we're testing here is (or should be) completely orthogonal to the stream concept.
| f"Index {index} is out of bounds; must be in the range [0, {self._num_frames})." | ||
| ) | ||
| else: | ||
| indices[i] = index |
There was a problem hiding this comment.
In general, we should avoid modifying the user-provided input inplace, as this can be surprising and have unintended effects. Let's keep the list comprehension logic?
| @@ -247,6 +252,8 @@ def get_frames_in_range(self, start: int, stop: int, step: int = 1) -> FrameBatc | |||
| Returns: | |||
| FrameBatch: The frames within the specified range. | |||
| """ | |||
| start = start if start >= 0 else start + self._num_frames | |||
| stop = min(stop if stop >= 0 else stop + self._num_frames, self._num_frames) | |||
There was a problem hiding this comment.
Just to note that eventually, we'll want to support stop=None and make it the default. This is what we have in the audio decoder and that's useful to decode all the frames at once. We should also add it to the time-based API get_frames_played_in_range().
But we should do that separately.
| start=387, stop=390, stream_index=stream_index | ||
| ).to(device) | ||
| frames387_389 = decoder.get_frames_in_range(start=387, stop=1000) | ||
| print(f"{frames387_389.data.shape=}") |
There was a problem hiding this comment.
Seems like a debugging left-over:
| print(f"{frames387_389.data.shape=}") |
| ref_frames386_389 = NASA_VIDEO.get_frame_data_by_range( | ||
| start=386, stop=390, stream_index=stream_index | ||
| ).to(device) | ||
| frames386_389 = decoder.get_frames_in_range(start=-4, stop=1000) |
There was a problem hiding this comment.
Let's check for a negative stop value as well. This should exclude the last frame so you'll have to exclude it from the ref above.
| frames386_389 = decoder.get_frames_in_range(start=-4, stop=1000) | |
| frames386_389 = decoder.get_frames_in_range(start=-4, stop=-1) |
| IndexError, | ||
| match="Stop index \\(-\\d+\\) must not be less than the start index", | ||
| ): | ||
| decoder.get_frames_in_range(start=0, stop=-1000) |
There was a problem hiding this comment.
Out of precaution and to be pedantic, maybe also test when both start and stop are positive:
decoder.get_frames_in_range(start=10, stop=0)
so that stop is fully independent of our negative index conversion logic
| indices = [ | ||
| index if index >= 0 else index + self._num_frames for index in indices | ||
| ] | ||
| for i, index in enumerate(indices): |
There was a problem hiding this comment.
I realize I'm sort of going back on the discussion from https://github.com/pytorch/torchcodec/pull/743/files#r2167398761, so I apologize for not bringing that up earlier. But I wonder if we actually want to do the validation and the conversion in Python. For performance reasons, it might be better to keep everything in C++, as the indices list can potentially be quite long, especially when sampling a large number of frames for long videos. I suspect we can easily have len(indices) > Nx1000, typically when using our samplers on long videos:
As a somewhat related concern I think we'll eventually want to support indices not just as a list, but also as a tensor, e.g. for users who create their own sampling strategy using torch utilities. And it'd be great if we could avoid copying that input tensor before passing it to the underlying decoding ops, which we can only do if we run the validation/conversion in C++.
In any case, we can and should merge this PR as-is regardless of our conclusion, because we are already doing such validation in main, and this PR doesn't add much on top of the existing one.
There was a problem hiding this comment.
Agreed on both points - when I wrote the comment on #743, I was also wondering about performance, and figured let's go for consistency first, then deal with performance later. We can make the checking basically zero-cost on the C++ side if we do it as we iterate through the indices - but we'll want to not do a TORCH_CHECK(), but throw a std::out_of_range so it becomes the right thing on the Python side. We can do all of that on a follow-up PR.
This PR adds the follow ups from #743:
get_frames_at, now that we are walking the list to handle negative indices.get_frames_in_range, including negative indexing and limiting the upper bound provided to the length of the list.The relevant tests are updated as well:
test_get_frames_at_failsto match the new Python error, rather than the C++ error.test_get_frames_in_range_tensor_index_semanticsto test negative and upper bound indices.test_get_frames_in_range_failsto ensure failures occur where expected.