Kosinkadink / ComfyUI-VideoHelperSuite

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

After installing the last version of your nodes Video Combine again enter to endless loop when connecting audio input #213

Closed ShmuelRonen closed 5 months ago

ShmuelRonen commented 6 months ago

After installing the last version of your nodes Video Combine again enter to endless loop when connecting audio input

vootox commented 6 months ago

Same here

ShmuelRonen commented 6 months ago

Same here

I made a cure for that until the developer will do it:

https://github.com/ShmuelRonen/ComfyUI_wav2lip/blob/main/README.md

Kosinkadink commented 6 months ago

If you have a fix, please feel free to submit a PR with the fix.

vootox commented 5 months ago

I made a cure for that until the developer will do it:

https://github.com/ShmuelRonen/ComfyUI_wav2lip/blob/main/README.md

I wasn't keen on copying the folder with 'junk' metadata leftovers (or whatever else!) into a temp ComfyUI, but yes this works.

ShmuelRonen commented 5 months ago

I made a cure for that until the developer will do it: https://github.com/ShmuelRonen/ComfyUI_wav2lip/blob/main/README.md

I wasn't keen on copying the folder with 'junk' metadata leftovers (or whatever else!) into a temp ComfyUI, but yes this works.

Thanks. I clean it up...

Kosinkadink commented 5 months ago

@ShmuelRonen you can submit a PR (fork the repo, commit your changes to the fork, then you can submit a PR), which can then be merged into here. Without it, I don't know what exact changes you made to your code in your fix, so it would be very convenient if you could make the PR, and then you'll also be in the commit history.

ShmuelRonen commented 5 months ago

@ShmuelRonen you can submit a PR (fork the repo, commit your changes to the fork, then you can submit a PR), which can then be merged into here. Without it, I don't know what exact changes you made to your code in your fix, so it would be very convenient if you could make the PR, and then you'll also be in the commit history.

I think that will better if you will update ComfyUI-VideoHelperSuite code for this issue.

Kosinkadink commented 5 months ago

That's what I'm saying, if you already have a fix for it, submit a pull request so AustinMroz and I can review the code and merge it in.

Kosinkadink commented 5 months ago

Or if you're unsure how to do pull requests, you can also copy and paste the changes you made to the code here, and we can go off of that.

ShmuelRonen commented 5 months ago

Or if you're unsure how to do pull requests, you can also copy and paste the changes you made to the code here, and we can go off of that.

nodes.txt Replace .txt to .py

vootox commented 5 months ago

nodes.txt Replace .txt to .py I'm kicking myself for not trusting my intuition, that's the file I was most interested in when comparing between the two after you posted it, I should have tried it on its own. Is this from an earlier version?

Kosinkadink commented 5 months ago

After looking through that code, this isn't really a fix, or even any code change to make current VHS work, I think that's just the code in VHS from several commits ago? While we are not gonna merge that in for the obvious reason that it rolls back several features that have been added since then, it does give a clue as to when things were working for you guys.

For future reference, you don't need to do any manual code adjustments to roll back to a previous version, you can use git commands for that. Since we have to work from square one for a fix, what is the simplest workflow you can create that replicates the issue with ComfyUI getting stuck? What does "endless loop" mean in the context of the bug you are experiencing? I think AustinMroz was unable to replicate the issue in the other issue report a while back, so getting as much info as we can will help find a solution.

ShmuelRonen commented 5 months ago

nodes.txt Replace .txt to .py I'm kicking myself for not trusting my intuition, that's the file I was most interested in when comparing between the two after you posted it, I should have tried it on its own. Is this from an earlier version?

The real cure must cam out of you. I just made it for the users of my wav2lip node.

Kosinkadink commented 5 months ago

@vootox @ShmuelRonen what is the simplest workflow that experiences the bug? Please post one here so we can take a look, as we couldn't replicate it last time this was reported. https://github.com/Kosinkadink/ComfyUI-VideoHelperSuite/issues/209

ShmuelRonen commented 5 months ago

@vootox @ShmuelRonen what is the simplest workflow that experiences the bug? Please post one here so we can take a look, as we couldn't replicate it last time this was reported. #209 wav2lip.json

Kosinkadink commented 5 months ago

Running that workflow and installing wav2lip does not cause any 'endless looping' in the Video Combine node on my end, while running latest VHS. What is the endless looping that you guys are experiencing like? Have you tried pressing Ctrl+F5 after opening up the ComfyUI webpage to make sure javascript is refreshed? What browser and operating system do you experience the issue on?

ShmuelRonen commented 5 months ago

Running that workflow and installing wav2lip does not cause any 'endless looping' in the Video Combine node on my end, while running latest VHS. What is the endless looping that you guys are experiencing like? Have you tried pressing Ctrl+F5 after opening up the ComfyUI webpage to make sure javascript is refreshed? What browser and operating system do you experience the issue on?

As you probably know that different phenomena happen to different users. You can't please everyone. The fact that latest VHS video combine works for you and at the same time puts me in an infinite loop without any error message, is a legitimate situation but on the other hand does not allow me to use your wonderful solution. That's why I wrote in wav2lip README.MD that only those who encounter such a problem I have a solution for them.

vootox commented 5 months ago

Running that workflow and installing wav2lip does not cause any 'endless looping' in the Video Combine node on my end, while running latest VHS. What is the endless looping that you guys are experiencing like? Have you tried pressing Ctrl+F5 after opening up the ComfyUI webpage to make sure javascript is refreshed? What browser and operating system do you experience the issue on?

Audio2Video.json I've tried Edge and Chrome browsers with Portable ComfyUI (and also with Stability Matrix). When I change the latest node.py to the above node.txt (as .py) it works. The endless loop is I see the node stays active (usual green border around edge of an active node) and doesn't complete writing the Video_Audio output file. I've noticed when I try to delete the (incomplete output) file that ffmpeg is also preventing deletion until I close ComfyUI instance. Windows 11.

Kosinkadink commented 5 months ago

@vootox sweet, thanks for the info. That paints a pretty good picture of where the issue could be in code. Is the CPU/GPU in noteworthy usage/does the size of the incomplete file increase at all while it's stuck? And in case it could be related to a specific audio file format, would you be willing to attach an audio file here that experiences the bug?

vootox commented 5 months ago

I let it continue for several hours yesterday and noticed very minimal CPU and GPU usage ~5% each with nothing notable. The RAM was maybe a little high around 30% of 64GB total but my Windows has been high lately sitting around 15% without much going on. I can verify the file was not being continuously written to or growing in size, I'm certain the file is written instantly. Whatever was written appears to be unchanged even after the shutdown. The output file 'test_00001.mp4' = 283KB in size and plays fine, while the 'test_00001-audio.mp4' = 257KB but is incomplete and cannot be played. I did try again just now to be sure but yeah, the same conclusion.

The audio file is an mp3 but I also tried its wav version with same result. percussion-crop-1.zip

AustinMroz commented 5 months ago

Thank you immensely for your assistance, Kosinkadink. The lack of actionable leads has been rough.

The recent flurry of discussion corroborates the rabbit holes I've been chasing, but I'm still unable to properly reproduce. The provided workflows and audio files function for me.

VHS_AUDIO is an immutable wrapper around an immutable bytes object. I'm glad to have confirmation from vootox that the issue can occur with a minimal workflow since it significantly reduces the scope of my search.

The subprocess call to embed audio already uses the safer subprocess.run which should eliminate any possibility of deadlock. There is a timeout argument that can be supplied, but it wouldn't actually solve the issue here and introduces the halting problem to otherwise functioning workflows. I've performed several tests with feeding intentionally invalid audio data to the subprocess call, but such edge cases are correctly handled. The single case I've been able to reach deadlock with is when the wrapped audio data is forcibly set to None. This is given special interpretation by subprocess.run and results in the input stream to ffmpeg never being closed. This would be a separate program state from when the Load Audio subprocess call raises an error (which results in audio embedding being skipped), or even from ffmpeg succeeding and returning no output (which results in an empty binary string and an error on embed, but not deadlock).

It is trivial to add a check for the wrapped audio being None, but it would not actually bring us to the desired solution of functional output and my inability to see how a state could be reach makes me doubt the correctness of it.

hndrr commented 5 months ago

I have been experiencing the same problem for several weeks now, where the error logs are not outputting and the system idles when audio is included.

However, in my environment, downgrading from ffmpeg 7.0 to 6.1.1 solved the problem.

winget uninstall ffmpeg
winget install ffmpeg --version 6.1.1
AustinMroz commented 5 months ago

That gives me a reproduction! It appears that ffmpeg 7.0 is bugged when apad and shortest are combined. A simple standalone execution of ffmpeg -i video.mp4 -i audio.wav -c:v copy -c:a libopus -af apad -shortest output.mp4 also never completes. A quick perusing of the ffmpeg tracker didn't show any posted issues, so I'll look into opening one once things are cleaned up here.

I have a workaround of setting a maximum pad length for audio that I'll roll out in about an hour once I can resolve a minor edge case and verify that it doesn't break on older ffmpeg versions.

vootox commented 5 months ago

I have been experiencing the same problem for several weeks now, where the error logs are not outputting and the system idles when audio is included.

However, in my environment, downgrading from ffmpeg 7.0 to 6.1.1 solved the problem.

winget uninstall ffmpeg
winget install ffmpeg --version 6.1.1

I went the other way and upgraded ffmpeg to the latest but when it still didn't work I was discouraged. This is good news.

vootox commented 5 months ago

That gives me a reproduction! It appears that ffmpeg 7.0 is bugged when apad and shortest are combined. A simple standalone execution of ffmpeg -i video.mp4 -i audio.wav -c:v copy -c:a libopus -af apad -shortest output.mp4 also never completes. A quick perusing of the ffmpeg tracker didn't show any posted issues, so I'll look into opening one once things are cleaned up here.

I have a workaround of setting a maximum pad length for audio that I'll roll out in about an hour once I can resolve a minor edge case and verify that it doesn't break on older ffmpeg versions.

It's so good that you can experience the problem, its in good hands.

AustinMroz commented 5 months ago

The fix here has already been applied, but I would appreciate confirmation that it works for others before I close the issue.

vootox commented 5 months ago

The fix here has already been applied, but I would appreciate confirmation that it works for others before I close the issue.

I hadn't realised you had applied the fix until I updated Comfy moments ago. For me (latest ffmpeg), it's working with and without audio. Without audio, the error below was a (nice) surprise because it reminded me I had forgotten the audio in my video source (no locking occurred) Only then did I realise the fix (most likely) had been applied.

[out#0/wav @ 0000020b9fd88c40] Output file does not contain any stream Error opening output file -. Error opening output files: Invalid argument [VideoHelperSuite] - WARNING - Failed to extract audio