-
Notifications
You must be signed in to change notification settings - Fork 74
Expose cpu_fallback in VideoDecoder API #1093
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |||||||||||||||||
| import io | ||||||||||||||||||
| import json | ||||||||||||||||||
| import numbers | ||||||||||||||||||
| from dataclasses import dataclass | ||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||
| from typing import List, Literal, Optional, Sequence, Tuple, Union | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -22,6 +23,47 @@ | |||||||||||||||||
| from torchcodec.transforms import DecoderTransform, Resize | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| @dataclass | ||||||||||||||||||
| class FallbackInfo: | ||||||||||||||||||
| """Information about decoder fallback status. | ||||||||||||||||||
|
|
||||||||||||||||||
| This class tracks whether the decoder fell back to CPU decoding. | ||||||||||||||||||
|
|
||||||||||||||||||
| Usage: | ||||||||||||||||||
| - Use ``str(fallback_info)`` or ``print(fallback_info)`` to see the cpu fallback status | ||||||||||||||||||
| - Use ``bool(fallback_info)`` to check if any fallback occurred | ||||||||||||||||||
|
|
||||||||||||||||||
| Attributes: | ||||||||||||||||||
| status_known (bool): Whether the fallback status has been determined. | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's OK to expose publicly. Let's just document that:
We can link to this concept of CUDA backend by linking to https://meta-pytorch.org/torchcodec/stable/generated/torchcodec.decoders.set_cuda_backend.html#torchcodec.decoders.set_cuda_backend
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, we'll probably want to document this class publicly in |
||||||||||||||||||
| """ | ||||||||||||||||||
|
|
||||||||||||||||||
| def __init__(self): | ||||||||||||||||||
| self.status_known = False | ||||||||||||||||||
| self.__nvcuvid_unavailable = False | ||||||||||||||||||
| self.__video_not_supported = False | ||||||||||||||||||
|
||||||||||||||||||
|
|
||||||||||||||||||
| def __bool__(self): | ||||||||||||||||||
| """Returns True if fallback occurred.""" | ||||||||||||||||||
| return self.status_known and ( | ||||||||||||||||||
| self.__nvcuvid_unavailable or self.__video_not_supported | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| def __str__(self): | ||||||||||||||||||
| """Returns a human-readable string representation of the cpu fallback status.""" | ||||||||||||||||||
| if not self.status_known: | ||||||||||||||||||
| return "Fallback status: Unknown" | ||||||||||||||||||
|
|
||||||||||||||||||
| reasons = [] | ||||||||||||||||||
| if self.__nvcuvid_unavailable: | ||||||||||||||||||
| reasons.append("NVcuvid unavailable") | ||||||||||||||||||
| if self.__video_not_supported: | ||||||||||||||||||
| reasons.append("Video not supported") | ||||||||||||||||||
|
|
||||||||||||||||||
| if reasons: | ||||||||||||||||||
| return "Fallback status: Falling back due to: " + ", ".join(reasons) | ||||||||||||||||||
| return "Fallback status: No fallback required" | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| class VideoDecoder: | ||||||||||||||||||
mollyxu marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
| """A single-stream video decoder. | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -180,13 +222,42 @@ def __init__( | |||||||||||||||||
| custom_frame_mappings=custom_frame_mappings_data, | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| self._fallback_info = FallbackInfo() | ||||||||||||||||||
| self._has_decoded_frame = False | ||||||||||||||||||
|
||||||||||||||||||
| std::string CudaDeviceInterface::getDetails() { | |
| // Note: for this interface specifically the fallback is only known after a | |
| // frame has been decoded, not before: that's when FFmpeg decides to fallback, | |
| // so we can't know earlier. | |
| return std::string("FFmpeg CUDA Device Interface. Using ") + | |
| (usingCPUFallback_ ? "CPU fallback." : "NVDEC."); | |
| } |
To know whether a frame has been decoded yet, I think we can simply set a boolean field to true when
| void CudaDeviceInterface::convertAVFrameToFrameOutput( |
is called. This would be a new private boolean attribute on the CudaDeviceInterface class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment is slightly misleading because it's only really true for the ffmpeg interface. Can I suggest the following - and also please check me on my understanding here:
We only query the CPU fallback info if status is unknown. That happens either when:
- this @Property has never been called before
- no frame has been decoded yet on the FFmpeg interface.
Note that for the beta interface, we're able to know the fallback status right when the VideoDecoder
is instantiated, but thestatus_knownattribute is initialized to False.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 nits:
- No need to define a separate
_update_cpu_fallback()function, it can just be inlined here - The convention (I think it's a convention??) is to use the same name for the
@propertyand for the underlying cached object. That is, I thinkself._fallback_infoshould just beself._cpu_fallback. It makes it more obvious that it relates to the@cpu_fallbackproperty.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so this _FallbackInfo__nvcuvid_unavailable is the name-mangling consequence of using double leading underscore. Let's definitely use single underscores :)
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1737,6 +1737,92 @@ def test_set_cuda_backend(self): | |
| with set_cuda_backend(backend): | ||
| VideoDecoder(H265_VIDEO.path, device=f"cuda:{bad_device_number}") | ||
|
|
||
| def test_cpu_fallback_before_after_decoding(self): | ||
| decoder = VideoDecoder(NASA_VIDEO.path) | ||
|
|
||
| # Before accessing any frames, status should be unknown | ||
| assert not decoder.cpu_fallback.status_known | ||
| assert str(decoder.cpu_fallback) == "Fallback status: Unknown" | ||
| assert not bool(decoder.cpu_fallback) | ||
|
|
||
| # After accessing frames, status should be known | ||
| _ = decoder[0] | ||
| assert decoder.cpu_fallback.status_known | ||
| assert str(decoder.cpu_fallback) != "Fallback status: Unknown" | ||
|
|
||
| def test_cpu_fallback_no_fallback_on_cpu_device(self): | ||
| """Test that CPU device doesn't trigger fallback (it's not a fallback scenario).""" | ||
| decoder = VideoDecoder(NASA_VIDEO.path, device="cpu") | ||
|
|
||
| _ = decoder[0] | ||
|
|
||
| assert decoder.cpu_fallback.status_known | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this scenario, we should be able to assert that the status is known before we decode any frame? |
||
| assert not bool(decoder.cpu_fallback) | ||
| assert "No fallback required" in str(decoder.cpu_fallback) | ||
|
|
||
| @needs_cuda | ||
| def test_cpu_fallback_h265_video_ffmpeg_cuda(self): | ||
| """Test that H265 video triggers CPU fallback on FFmpeg CUDA interface.""" | ||
| # H265_VIDEO is known to trigger CPU fallback on FFmpeg CUDA | ||
| # because its dimensions are too small | ||
| decoder = VideoDecoder(H265_VIDEO.path, device="cuda") | ||
|
|
||
| _ = decoder.get_frame_at(0) | ||
|
|
||
| assert decoder.cpu_fallback.status_known | ||
| assert bool(decoder.cpu_fallback) | ||
| assert "Fallback status: Falling back due to:" in str(decoder.cpu_fallback) | ||
|
|
||
| @needs_cuda | ||
| def test_cpu_fallback_h265_video_beta_cuda(self): | ||
| """Test that H265 video triggers CPU fallback on Beta CUDA interface.""" | ||
| with set_cuda_backend("beta"): | ||
| decoder = VideoDecoder(H265_VIDEO.path, device="cuda") | ||
|
|
||
| _ = decoder.get_frame_at(0) | ||
|
|
||
| assert decoder.cpu_fallback.status_known | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the beta interface, we should be able to assert that
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ping on this, we seem to not have tests for the beta interface now? |
||
| assert bool(decoder.cpu_fallback) | ||
| assert "Fallback status: Falling back due to:" in str(decoder.cpu_fallback) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's assert that it's due to the video being not supported |
||
|
|
||
| @needs_cuda | ||
| def test_cpu_fallback_no_fallback_on_supported_video(self): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's parametrize this over both interfaces - the beta and ffpmeg ones |
||
| """Test that supported videos don't trigger fallback on CUDA.""" | ||
| decoder = VideoDecoder(NASA_VIDEO.path, device="cuda") | ||
|
|
||
| _ = decoder[0] | ||
|
|
||
| assert not bool(decoder.cpu_fallback) | ||
| assert "No fallback required" in str(decoder.cpu_fallback) | ||
|
|
||
| def test_cpu_fallback_status_cached(self): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIUC this test mostly tests that the output value doesn't change, not that it's cached. I think testing the cache behavior is potentially really difficult, and perhaps not needed after all. I'd suggest to remove it? |
||
| """Test that cpu_fallback status is determined once and then cached.""" | ||
| decoder = VideoDecoder(NASA_VIDEO.path) | ||
|
|
||
| _ = decoder[0] | ||
| first_status = str(decoder.cpu_fallback) | ||
| assert decoder.cpu_fallback.status_known | ||
|
|
||
| _ = decoder[1] | ||
| second_status = str(decoder.cpu_fallback) | ||
| assert decoder.cpu_fallback.status_known | ||
|
|
||
| assert first_status == second_status | ||
|
|
||
| def test_cpu_fallback_multiple_access_methods(self): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this test is technically a subset of the previous one: if we 'cache' the Maybe want you wanted to check is that the status becomes "known" and that it works with multiple decoding method? If that's the case I think we'd need to re-create the VideoDecoder object in-between method calls. But TBH, I'm not sure it's a critical test to have, so I might suggest to remove it too. |
||
| """Test that cpu_fallback works with different frame access methods.""" | ||
| decoder = VideoDecoder(NASA_VIDEO.path) | ||
|
|
||
| _ = decoder.get_frame_at(0) | ||
| assert decoder.cpu_fallback.status_known | ||
| status_after_get_frame = str(decoder.cpu_fallback) | ||
|
|
||
| _ = decoder.get_frames_in_range(1, 3) | ||
| assert str(decoder.cpu_fallback) == status_after_get_frame | ||
|
|
||
| _ = decoder.get_frame_played_at(0.5) | ||
| assert str(decoder.cpu_fallback) == status_after_get_frame | ||
|
|
||
NicolasHug marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| class TestAudioDecoder: | ||
| @pytest.mark.parametrize("asset", (NASA_AUDIO, NASA_AUDIO_MP3, SINE_MONO_S32)) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could keep this class private for now, but I was thinking, maybe it could be public and named
CpuFallbackStatus? I'm hoping that it might resolve both @scotts preference and mine:The
CpuFallbackStatusclass will be public and visible from the docs, and it will be clear from the docstring that thedec.cpu_fallbackattribute is an instance of this class. The attribute namecpu_fallbackis fairly generic and allows us to extend the functionality in the future, while the class name itself communicates that it it's not just a simplebool, or a simple string.