Add hardware encoding support for ffmpeg #480
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add FFMPEG_PREFER_HARDWARE env var to enable hardware acceleration Use h264_nvenc and hevc_nvenc encoders when hardware preferred Fall back to software encoders (libx264/libx265) when disabled Add comprehensive tests for hardware/software encoding modes Update README with new environment variable documentation This enables GPU-accelerated video encoding for better performance on systems with NVIDIA GPUs.
- Detect video codec using ffprobe instead of file extensions - Auto-enable CUDA hwaccel for supported codecs when FFMPEG_PREFER_HARDWARE=true - Skip probing for image formats to optimize performance - Add comprehensive tests for hardware acceleration logic
…logging - Add `checkNvidiaGpuAvailable()` function using nvidia-smi to detect GPU presence - Add comprehensive logging for hardware acceleration decisions: - GPU detection status - Hardware vs software encoding/decoding choices - CUDA codec support detection - Encoder selection decisions - Cache GPU availability results to avoid repeated checks - Add `resetNvidiaGpuCache()` for testing - Update tests to handle GPU availability mocking This provides visibility into hardware acceleration decisions and ensures CUDA is only attempted when NVIDIA GPU hardware is actually available.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 3 files
Prompt for AI agents (all 4 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/converters/ffmpeg.ts">
<violation number="1" location="src/converters/ffmpeg.ts:870">
P1: Hardware encoder selection doesn't check GPU availability. If `FFMPEG_PREFER_HARDWARE=true` but no GPU is detected, this will use `h264_nvenc` anyway, causing ffmpeg to fail. Should check `preferHardware && gpuAvailable`.</violation>
<violation number="2" location="src/converters/ffmpeg.ts:896">
P1: CUDA hardware acceleration is added without checking GPU availability. This will cause ffmpeg to fail with `-hwaccel cuda` when no NVIDIA GPU is present, even though the codec detection passed. Should check `gpuAvailable` as well.</violation>
</file>
<file name="tests/converters/ffmpeg.test.ts">
<violation number="1" location="tests/converters/ffmpeg.test.ts:256">
P1: Test assertion checks wrong call index. The comment states `calls[0]` is `nvidia-smi` and `calls[1]` is `ffmpeg`, but this assertion checks `calls[0]` which would never contain `-hwaccel cuda`. Should check `calls[1]` (the ffmpeg call) to verify CUDA is not added.</violation>
<violation number="2" location="tests/converters/ffmpeg.test.ts:256">
P1: Test assertion checks wrong call index. When hardware is preferred, `calls[0]` is the `nvidia-smi` args (as documented in other tests), not the `ffmpeg` call. This assertion will always pass trivially since `nvidia-smi` args never contain `-hwaccel cuda`. Should check `calls[2]` for the ffmpeg arguments.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
src/converters/ffmpeg.ts
Outdated
| // This only applies if FFMPEG_ARGS doesn't already specify a hardware accelerator | ||
| const hasHardwareAccel = ffmpegArgs.includes("-hwaccel"); | ||
|
|
||
| if (preferHardware && !hasHardwareAccel) { |
There was a problem hiding this comment.
P1: CUDA hardware acceleration is added without checking GPU availability. This will cause ffmpeg to fail with -hwaccel cuda when no NVIDIA GPU is present, even though the codec detection passed. Should check gpuAvailable as well.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/converters/ffmpeg.ts, line 896:
<comment>CUDA hardware acceleration is added without checking GPU availability. This will cause ffmpeg to fail with `-hwaccel cuda` when no NVIDIA GPU is present, even though the codec detection passed. Should check `gpuAvailable` as well.</comment>
<file context>
@@ -730,6 +888,21 @@ export async function convert(
+ // This only applies if FFMPEG_ARGS doesn't already specify a hardware accelerator
+ const hasHardwareAccel = ffmpegArgs.includes("-hwaccel");
+
+ if (preferHardware && !hasHardwareAccel) {
+ const supportsCuda = await isCudaSupportedCodec(filePath, fileType, execFile);
+ if (supportsCuda) {
</file context>
Suggested change
| if (preferHardware && !hasHardwareAccel) { | |
| if (preferHardware && gpuAvailable && !hasHardwareAccel) { |
✅ Addressed in 0093109
src/converters/ffmpeg.ts
Outdated
| break; | ||
| case "h264": | ||
| extraArgs.push("-c:v", "libx264"); | ||
| if (preferHardware) { |
There was a problem hiding this comment.
P1: Hardware encoder selection doesn't check GPU availability. If FFMPEG_PREFER_HARDWARE=true but no GPU is detected, this will use h264_nvenc anyway, causing ffmpeg to fail. Should check preferHardware && gpuAvailable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/converters/ffmpeg.ts, line 870:
<comment>Hardware encoder selection doesn't check GPU availability. If `FFMPEG_PREFER_HARDWARE=true` but no GPU is detected, this will use `h264_nvenc` anyway, causing ffmpeg to fail. Should check `preferHardware && gpuAvailable`.</comment>
<file context>
@@ -717,10 +867,18 @@ export async function convert(
break;
case "h264":
- extraArgs.push("-c:v", "libx264");
+ if (preferHardware) {
+ extraArgs.push("-c:v", "h264_nvenc");
+ } else {
</file context>
Suggested change
| if (preferHardware) { | |
| if (preferHardware && gpuAvailable) { |
✅ Addressed in 0093109
tests/converters/ffmpeg.test.ts
Outdated
|
|
||
| console.log = originalConsoleLog; | ||
|
|
||
| expect(calls[0]).not.toEqual(expect.arrayContaining(["-hwaccel", "cuda"])); |
There was a problem hiding this comment.
P1: Test assertion checks wrong call index. The comment states calls[0] is nvidia-smi and calls[1] is ffmpeg, but this assertion checks calls[0] which would never contain -hwaccel cuda. Should check calls[1] (the ffmpeg call) to verify CUDA is not added.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/converters/ffmpeg.test.ts, line 256:
<comment>Test assertion checks wrong call index. The comment states `calls[0]` is `nvidia-smi` and `calls[1]` is `ffmpeg`, but this assertion checks `calls[0]` which would never contain `-hwaccel cuda`. Should check `calls[1]` (the ffmpeg call) to verify CUDA is not added.</comment>
<file context>
@@ -121,6 +154,137 @@ test("uses libx266 for h266.mp4", async () => {
+
+ console.log = originalConsoleLog;
+
+ expect(calls[0]).not.toEqual(expect.arrayContaining(["-hwaccel", "cuda"]));
+ expect(loggedMessage).toBe("stdout: Fake stdout");
+
</file context>
✅ Addressed in 0093109
tests/converters/ffmpeg.test.ts
Outdated
|
|
||
| console.log = originalConsoleLog; | ||
|
|
||
| expect(calls[0]).not.toEqual(expect.arrayContaining(["-hwaccel", "cuda"])); |
There was a problem hiding this comment.
P1: Test assertion checks wrong call index. When hardware is preferred, calls[0] is the nvidia-smi args (as documented in other tests), not the ffmpeg call. This assertion will always pass trivially since nvidia-smi args never contain -hwaccel cuda. Should check calls[2] for the ffmpeg arguments.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/converters/ffmpeg.test.ts, line 256:
<comment>Test assertion checks wrong call index. When hardware is preferred, `calls[0]` is the `nvidia-smi` args (as documented in other tests), not the `ffmpeg` call. This assertion will always pass trivially since `nvidia-smi` args never contain `-hwaccel cuda`. Should check `calls[2]` for the ffmpeg arguments.</comment>
<file context>
@@ -121,6 +154,137 @@ test("uses libx266 for h266.mp4", async () => {
+
+ console.log = originalConsoleLog;
+
+ expect(calls[0]).not.toEqual(expect.arrayContaining(["-hwaccel", "cuda"]));
+ expect(loggedMessage).toBe("stdout: Fake stdout");
+
</file context>
Suggested change
| expect(calls[0]).not.toEqual(expect.arrayContaining(["-hwaccel", "cuda"])); | |
| // calls[0] is nvidia-smi, calls[1] is ffprobe, calls[2] is ffmpeg | |
| expect(calls[2]).not.toEqual(expect.arrayContaining(["-hwaccel", "cuda"])); |
✅ Addressed in 0093109
…ation - Check both preferHardware AND gpuAvailable before using NVENC codecs (h264_nvenc, hevc_nvenc) - Check gpuAvailable before adding CUDA hwaccel flag for input decoding - Fix test assertion to check correct call index for CUDA hwaccel verification This prevents hardware acceleration attempts when GPU is unavailable, addressing code review feedback.
- Adjusted formatting of callback responses in the ffmpeg test file for better readability. - Ensured consistent indentation and line breaks for JSON strings in mock responses. This enhances code clarity and maintainability in the test suite.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🚀 Enhanced Hardware Acceleration & Smart Conversion Filtering
This PR adds comprehensive hardware acceleration support and intelligent conversion filtering to ConvertX, significantly improving performance and user experience.
🎯 Key Features Added
🔧 Hardware Acceleration
nvidia-smiffprobeinstead of file extensions🎛️ Environment Variables
FFMPEG_PREFER_HARDWARE=true: Enables hardware acceleration when available🧠 Smart UI Filtering
📊 Comprehensive Logging
🛠️ Technical Implementation
FFmpeg Integration
FFMPEG_ARGSwhile adding intelligenceUI Enhancements
Testing Coverage
🔍 Architecture Decisions
Hardware Detection Strategy
nvidia-smicallsCodec Compatibility
UI/UX Philosophy
📈 Performance Benefits
🧪 Testing Results
🔄 Backward Compatibility
🎨 User Experience
This enhancement transforms ConvertX from a basic file converter into an intelligent, hardware-accelerated media processing platform while maintaining simplicity and reliability.
Summary by cubic
Adds GPU-accelerated FFmpeg encoding and decoding. When enabled, we use NVENC for H.264/H.265 and CUDA for input decoding to speed up conversions and reduce CPU usage.
Written for commit 947be70. Summary will update automatically on new commits.