Closed Kosinkadink closed 5 months ago
Thanks. I'll roll out a fix for this along with Gifski support later this evening
It was a misleading combination of a couple different minor errors
get_audio
function did not wrap it's call to ffmpeg. On error it raises a subprocess.CalledProcessError-audio
output fileget_audio
file just captured stdout instead of stdout and stderr, e.stderr isn't set when an error is thrown by the subprocess in get_audio
Under normal circumstances, I consider the output of e.stderr (not res.stderr) nicer since it passes the actual digested message from ffmpeg.I've applied a fix that adds error catching to the subprocess call in get_audio
and causes Video Combine to skip the audio pass when a connected audio input does not resolve to an actual audio stream. Now, when a gif (or other file without audio) is used for Load Video and has audio wired to a Video Combine, an error is printed to the console indicating that the audio extraction failed (and why), but the workflow still completes as if the audio were not wired.
Examples: if a gif is loaded in Load Video, and the audio output is inputted into Video Combine, an error will be thrown when it attempts to do the ffmpeg call (also, the actual error message parsing should be changed to str(e) instead of res.stderr, as stderr is something that only exists on the result of the subprocess call, not all exceptions.![image](https://github.com/Kosinkadink/ComfyUI-VideoHelperSuite/assets/7365912/b1b38fcc-f155-4731-98fb-6daed19b9af7)
I also had a report that the same audio issue appears to happen if a video file (.mp4) has no audio and is then passed to Video Combine. Perhaps the something is up with the lazy_eval setup? Or something in the Video Combine node to handle such cases without throwing an exception.
Steps to reproduce: 1) Load a .gif with Load Video nodes 2) Connect the loaded images + audio to a Video Combine node, and make it a format that accepts audio (like .mp4) 3) An error will now be thrown, and due to not using str(e), it will throw an error about the nested NoneType error
I personally was not able to recreate the issue with using an .mp4 as source for the audio like another user has, but getting robust enough fix to avoid that happening would be good.