Kosinkadink / ComfyUI-VideoHelperSuite

Nodes related to video workflows
GNU General Public License v3.0
524 stars 92 forks source link

Add h264_nvenc, hevc_nvenc (NVIDIA) hw encoding #91

Closed iemesowum closed 8 months ago

iemesowum commented 8 months ago

In Video Combine, libx264 may crash with 1000+ input images.

To avoid the crash, add hardware encoding supported by NVIDIA GPUs.

Add bitrate option which precludes crf. Add megabit option, which so the user may specify bitrate in Mb or Kb. Add hvc1 video tag, for better support on with Apple quicktime - to hevc_nvenc and libx265 video formats. This allows previews to show up in Safari too.

Add *.swp to .gitignore.

AustinMroz commented 8 months ago

Given it solves an issue and doesn't change anything if the new format isn't selected, I think it's worth merging.

I had been toying with the idea of hardware accelerating video operations for a while. The ability for users to enable hardware acceleration when available was one of the driving motivations behind creating the system for video_formats, but I had shelved the idea after running into a number of caveats:

There is potential value in making the choice of hardware/software encode an option on the existing h264 format using the new format widgets, but I don't see a need to block this behind the development branch.

iemesowum commented 8 months ago
  • I understood (potentially incorrectly) that hardware encoding would have lower quality compared libx264

You're correct - I'm seeing some pixelation with -crf 20 (which is ignored by h264_nvenc). I usually use -b:v <num> instead for better results.

iemesowum commented 8 months ago

@AustinMroz - I've addressed the pixelation issue, and added some performance improvements. Please take a look.

AustinMroz commented 8 months ago

I greatly appreciate the time you've spent here. The recent changes are quite relevant to my work on the devleopment branch. While that code should be stable, I don't currently have a timeline for when it will be merged.

The ability to have some formats with different config options has been a recent focus of mine and the related work here is largely subsumed by the mentioned format widgets implementation. For instance, using bitrate instead of crf on the development branch could be defined entirely in a format.json with no code changes:

{
    "main_pass":
    [
        "-n", "-c:v", "h264_nvenc",
        "-b:v", ["bit_rate","INT", {"default": 10000, "min": 1000, "max": 50000, "step": 1000, "display": "bitrate (b)"}]
        "-pix_fmt", "yuv420p"
    ],
     "extension": "mp4"
}

(albeit at the cost of being able to use Mb instead of bits)

Hardware decoding is something I'm still very interested in, but I don't think it makes sense utilizing it in Video Combine or tying it to the formats implementation. Since data is passed as a raw format, there isn't actually any decoding that is done and I believe the flag either does nothing or would force an additional copy to and from the gpu when the pix_fmt is changed. Rather, my interest is with the new preview generation code that is in the development branch. When it's enabled, there's some pretty significant performance issues with trying to pull ~80 frames from 20 minutes into a video that I believe hardware accelerated decoding would solve.

The new code saves the frame data to a temporary file before performing the encode. While I don't mind this if it's somehow required, I have a strong personal preference for avoiding files when they aren't required and I'm not seeing a reason for why the change was made in the code.

iemesowum commented 8 months ago

I'm happy to help.

The reason I added the temporary file is that I noticed the image data was being piped into ffmpeg, which is usually so slow as to negate hardware decoding. However, I hadn't realized the input data was raw, so I've removed the temporary file.

Hardware accelerated decoding shouldn't be too tricky if the implementation uses ffmpeg like in VideoCombine, instead of opencv. It seems that opencv requires a special local build to support CUDA.

I do like your changes on develop - I could refactor - I was hoping to get this in relatively soon, so as to help others who've hit the issue I hit.

A boolean switch for Mb vs b could be used to allow the user to choose, perhaps.

AustinMroz commented 8 months ago

Apologies for the delay. The decision was made to release the large number of backlogged development branch changes and, now that the resultant issues seem resolved, I'd like to prioritize getting this merged.

I have some vague ideas for further extending the format system to solve the units issue, allow for variable reuse, and expose ffmpeg's builtin math functionality (1*2M works as expected), but I don't wish to delay this pull request further. I think a simple solution like what you described or taking a string with a default like 10M will work for now.

iemesowum commented 8 months ago

Apologies for the delay. The decision was made to release the large number of backlogged development branch changes and, now that the resultant issues seem resolved, I'd like to prioritize getting this merged.

I have some vague ideas for further extending the format system to solve the units issue, allow for variable reuse, and expose ffmpeg's builtin math functionality (1*2M works as expected), but I don't wish to delay this pull request further. I think a simple solution like what you described or taking a string with a default like 10M will work for now.

@AustinMroz

Thanks for the update. I've rebased and I've completed the refactor with local testing.

I've also added HEVC hardware encoding support and fixed a bug with playback on Apple systems. Please take a look.

iemesowum commented 8 months ago

In my testing yesterday, I noticed that libx264, libx265 would crash ffmpeg, and even freeze my PC wth as little as 50 image frames.

h264_nvenc and hevc_nvenc are 100% reliable for me, however.

iemesowum commented 8 months ago

Hello @Kosinkadink

Austin is on vacation. Would you please review?

Kosinkadink commented 8 months ago

Hey, I'll take a look in the next hour hour. I'll just make sure nothing is broken on my end as a sanity check, and then I'll merge.

Kosinkadink commented 8 months ago

Got busy for a bit and got delayed, but looks good I think. Worse case, we issue a hotfix, but I'll merge the changes in now.

iemesowum commented 8 months ago

Thank you, @Kosinkadink.