Skip to content
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

Fix for a rare FFMPEG stream closing error #451

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

linkjay
Copy link

@linkjay linkjay commented Jan 14, 2019

There is an issue on some systems where an FFMPEG stream will close and stop the song at some duration throughout.

Upon further investigation, it appears that the stdout buffer that FFMPEG reads from the network stream is reading more than is available and stops entirely. (something like that)

To fix this, bumping up the highWaterMark variable to the highest number possible will remove this issue.

It's hard to test this issue. Basically, it appears as if nothing is wrong for some people on any system. Anybody can just play what they want. Then, by pure luck, everything from there on out will stop halfway through. It's got to be some system-level thing that is going wrong, but this fixes it.

We've tested this on multiple different bots with different libraries and they work just fine with this fix.

set high watermark to max 32bit int to fix a rare stream closing issue
@ghost
Copy link

ghost commented Jan 14, 2019

Can confirm, also happens with other Discord libs like discord.js. I was debugging this for a few nights with Linkjay, and oddly enough it was only happening to me at first -- even with both of us testing on multiple machines. We even tried sending eachother our entire node_modules folders. Not sure what changed that it's happening for him now too. I could repro this on multiple machines, on multiple distros, multiple versions of node, while using old library versions, with or without ytdl-core (even while using a manual ffmpeg child_process). This only started happening about a month ago. Would be nice to figure out why this is suddenly happening, but for now this fixes it.

My understanding is that the child process should pause writing to the stdout stream once it has enough buffered to meet highWaterMark (in bytes), to allow the reading end of the stream to catch up, but it isn't for some reason. I couldn't figure out any reason why. Bumping up highWaterMark to the max allowed number (max 32bit int) seems to resolve the issue as the buffer simply won't get full anymore, and the full output of ffmpeg can be stored.

Simple code we used to test: eristest.tar.gz
Song should cut out consistently at around 2:38, right after Eminem says to "stop the beat".
I was using node v11.5.0 at the time, for reference.

@abalabahaha abalabahaha changed the base branch from master to dev January 14, 2019 06:26
@abalabahaha
Copy link
Owner

Thanks for doing the investigative work here! Possibly one cause of #213, along with a couple other reported WTFs.

Code-wise, comments should follow capitalization and spelling like the rest of the lib. 2^32-1 should not be hard-coded, maybe use the Infinity global or something.

More importantly though, nullifying highWaterMark has serious implications on memory management, especially for music bots with high/long-running stream concurrency. So this proposed fix cannot be merged as-is. Let's try to find a cause rather than treat a symptom.

@linkjay
Copy link
Author

linkjay commented Jan 19, 2019

I'm open to being wrong here, but I don't think that is true when it comes to the memory implications. I'm running my bot on a 2 GB RAM VPS and tested out a lot of different 10 - 30 minute videos and a 3 hour video with no memory implications.

I and Doug have been running our bots (his on discord.js with special ffmpeg; mine with eris and hard coded highwatermark in the library) just fine with this fix. I'll continue to test and see if I can run into these memory issues that you say, but I believe this is a fix.

I will add another commit making my code consistent with the rest of the library.

Also, the integer we used in this commit is actually 2^31-1. That number was from Doug and I assume it's just so it doesn't completely hit the limit, and it still does the job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants