-
Notifications
You must be signed in to change notification settings - Fork 77
Find default hardware encoder in VideoEncoder #1124
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
Find default hardware encoder in VideoEncoder #1124
Conversation
test/test_encoders.py
Outdated
| ), | ||
| ], | ||
| ) | ||
| @pytest.mark.parametrize("format", ("mov", "mp4", "mkv")) |
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.
These formats all default to h264_nvenc, so IN_GITHUB_CI is no longer necessary to avoid av1_nvenc.
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.
By removing codec we have lost important testing coverage:
- manually passing a codec that is not
None - passing a non-default codec for that given container format
- testing on av1
Let's put all these back. To test the new behavior that this PR is enabling, all we have to do is to specify a codec=None case. Something like this (with comments to help understanding the intent):
@pytest.mark.parametrize(
("format", "codec"),
[
("mov", None), # should default to h264_nvenc
("mov", "h264_nvenc"),
("mp4", "hevc_nvenc"), # non-default
("avi", "h264_nvenc"),
pytest.param(
("mkv", "av1_nvenc"),
marks=pytest.mark.skipif(
IN_GITHUB_CI, reason="av1_nvenc is not supported on CI"
),
),
],
)Note also that I separated "format" and "codec" into their own parameter input - that's the more conventional way of passing couples of parameters with pytest. You'll have to update the function's signature to accept both format and codec separately.
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.
Good point, I've added back these tests so we are explicitly testing hevc_nvenc and av1_nvenc.
NicolasHug
left a comment
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.
Thanks @Dan-Flores , made a first pass
src/torchcodec/_core/Encoder.cpp
Outdated
| avCodec = hwCodec.value(); | ||
| } | ||
| } | ||
| TORCH_CHECK(avCodec != nullptr, "Video codec not found"); |
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.
Not from this PR but we might as well address it here:
We have this TORCH_CHECK just above, and we also have a similar one in the first if branch above
TORCH_CHECK(
avCodec != nullptr,
"Video codec ",
codec,
" not found. To see available codecs, run: ffmpeg -encoders");
Let's just have a single TORCH_CHECK after this whole if/else block? The error message mentioning ffmpeg -encoders is good.
| } | ||
|
|
||
| return std::nullopt; | ||
| } |
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.
This is 99% the same code as the existing findCodec(). Let's just extend findCodec and add an optional bool decoder parameter who's default is true.
src/torchcodec/_core/Encoder.cpp
Outdated
| TORCH_CHECK( | ||
| avFormatContext_->oformat != nullptr, | ||
| "Output format is null, unable to find default codec."); | ||
| avCodec = avcodec_find_encoder(avFormatContext_->oformat->video_codec); |
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 we should logically call avcodec_find_encoder after we have tried findHardwareEncoder. The reason is: for whatever reason, avcodec_find_encoder could return an error (like if there's no CPU codec for a given configuration) but findHardwareEncoder might be able to find the right one. So I thikn we should logically do:
if onCUDA:
codec = findHardwareEncoder() # first try NVENC specific stuff
if codec is still not found:
codec = avcodec_find_encoder()
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.
Thanks for the suggestion, I've updated the code to conditionally call avcodec_find_encoder iff there is no hardware encoder for the default codec.
src/torchcodec/_core/Encoder.cpp
Outdated
| avCodec = avcodec_find_encoder(avFormatContext_->oformat->video_codec); | ||
| // If frames are on a CUDA device, try to substitute the default codec | ||
| // with its hardware equivalent | ||
| if (frames_.device().is_cuda() && deviceInterface_) { |
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.
This frames_.device().is_cuda() && deviceInterface_ is a bit imprecise because when frames are on CUDA, we should always have defined a device interface. If it's not defined, then it's a bug. This should be:
if frames on cuda:
assert device_interface exists
# continue here
test/test_encoders.py
Outdated
| ), | ||
| ], | ||
| ) | ||
| @pytest.mark.parametrize("format", ("mov", "mp4", "mkv")) |
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.
By removing codec we have lost important testing coverage:
- manually passing a codec that is not
None - passing a non-default codec for that given container format
- testing on av1
Let's put all these back. To test the new behavior that this PR is enabling, all we have to do is to specify a codec=None case. Something like this (with comments to help understanding the intent):
@pytest.mark.parametrize(
("format", "codec"),
[
("mov", None), # should default to h264_nvenc
("mov", "h264_nvenc"),
("mp4", "hevc_nvenc"), # non-default
("avi", "h264_nvenc"),
pytest.param(
("mkv", "av1_nvenc"),
marks=pytest.mark.skipif(
IN_GITHUB_CI, reason="av1_nvenc is not supported on CI"
),
),
],
)Note also that I separated "format" and "codec" into their own parameter input - that's the more conventional way of passing couples of parameters with pytest. You'll have to update the function's signature to accept both format and codec separately.
NicolasHug
left a comment
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.
Thanks @Dan-Flores . Left some Qs but approving to unblock.
test/test_encoders.py
Outdated
| @needs_ffmpeg_cli | ||
| @pytest.mark.needs_cuda | ||
| # TODO-VideoEncoder: Auto-select codec for GPU encoding | ||
| # @pytest.mark.parametrize("format", ("mov", "mp4", "mkv")) |
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.
can remove
| format, codec = format_codec | ||
| device = "cuda" | ||
| qp = 1 # Lossless (qp=0) is not supported on av1_nvenc, so we use 1 | ||
| qp = 1 # Use near lossless encoding to reduce noise and support av1_nvenc |
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.
QQ would we be able to assert closer expected results (stricter tolerance) if we set qp = 0 for the other codecs? If we can, then it might be worth it
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.
No, at qp = 0 or qp = 1 the highest assertion we can make is 96% of frames match.
| if codec == "av1_nvenc": | ||
| ffmpeg_cmd.extend(["-rc", "constqp"]) # Set rate control mode for AV1 | ||
| ffmpeg_cmd.extend(["-qp", str(qp)]) # Use lossless qp for other codecs | ||
| ffmpeg_cmd.extend(["-qp", str(qp)]) |
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.
There are two if codec == "av1_nvenc": that were removed, above and below. Just checking if that was by design, and why?
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.
yes this is intentional, I realized constqp is the default rate control mode when qp is set for the nvenc codecs, so its not necessary to set it here.
To improve ease of use of the
VideoEncoderon GPU, this PR adds logic to find and use a hardware enabledcodecif one is available.A boolean arg is added to findCodec to flip the function to search for encoders. This means it has the following behaviors:
codechas a hardware equivalent (ex.libx264,h264_nvenc), the function will correctly return the hardware encoder.codecdoes NOT have a hardware equivalent(ex. thempegcodec used foravi), no hardware encoder will be used by default.