Fix YouTube Livestreams #7
Conversation
Handle miniget stream error ahead of time.
Allow pass down requestOptions. && Handle miniget stream error ahead of time. Fixes fent#2
Update sinon to the latest version 🚀
|
This is my current prototype of a working solution. Please edit it and make it match whatever you'd like. Example usage: # In line 68 on fent/node-ytdl-core, make these changes.
* if (format.live) {
* var req = m3u8stream(url, {
* chunkReadahead: +info.live_chunk_readahead,
* requestOptions: options.requestOptions,
+ ytdlOptions: {isYtdlCore: true}
* });
* req.on('error', stream.emit.bind(stream, 'error'));
* stream.destroy = req.end.bind(req);Edit: Also if somebody could do the tests for me that'd be great. I'm not exactly sure what it outputs at the moment so I can't write one. Also I'll make the PR on ytdl-core when we decide how we want this to be done. |
Codecov Report
@@ Coverage Diff @@
## master #7 +/- ##
===========================================
- Coverage 100% 80.35% -19.65%
===========================================
Files 3 3
Lines 120 168 +48
Branches 22 31 +9
===========================================
+ Hits 120 135 +15
- Misses 0 33 +33
Continue to review full report at Codecov.
|
lib/m3u8-parser.js
Outdated
| var timeout = setTimeout(function() { | ||
| self.ytdlSavedLines = []; | ||
| return self.emit('ytdl-end'); | ||
| clearTimeout(timeout); // Just in case timeout is still set, we want to make sure it's destroyed. |
There was a problem hiding this comment.
I do realize this line is now redundant, I added it when there wasn't a return for the self.emit. I was having issues in this area, though, with it requesting 4-5 times a second. I don't know if this still happens, but I think it is happening very slowly. The longer you listen, the more you hear stuttering. Try adding a console.log here to see how many times it calls. It should only all every 5 seconds (or however long is).
TL;DR: I believe I ended up creating a memory leak right here.
lib/index.js
Outdated
| @@ -56,12 +57,16 @@ module.exports = function(playlistURL, options) { | |||
| } | |||
|
|
|||
| var tid; | |||
| var rpts; | |||
There was a problem hiding this comment.
rpts means "Repeat Times". It's the amount of times refreshPlaylist() has been called when ytdl-error runs. If it loops more than 5 times from ytdl-errors in a row, then it's probably crashing and time to stop.
lib/index.js
Outdated
| @@ -56,12 +57,16 @@ module.exports = function(playlistURL, options) { | |||
| } | |||
|
|
|||
| var tid; | |||
| var rpts; | |||
| var ytft = true; | |||
There was a problem hiding this comment.
ytft means "YouTube First Time". This is required, as we have to let the m3u8 know if it's the first time. More information on this will be in my comment on the actual part that handles time.
lib/index.js
Outdated
| @@ -74,6 +79,21 @@ module.exports = function(playlistURL, options) { | |||
| totalItems++; | |||
| requestQueue.push(item, onQueuedEnd); | |||
| }); | |||
| parser.on('ytdl-end', function() { | |||
There was a problem hiding this comment.
We do not want to call the normal end, as that sets a 10 minute timeout, when we need to call every 5 seconds. So, this custom function will do just that. It deletes the parser just in case that causes a memory leak (because we create a new one every loop and don't "return"), and then calls refreshPlaylist again. It finally sets ytft to false and rpts to 0 (see above).
There was a problem hiding this comment.
Did you notice the program building up memory? If so, don't think it would be from here, parser is not used or saved outside of this function. The delete operator also cannot delete variables, it only deletes keys from an object. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/delete
lib/index.js
Outdated
| rpts = 0; | ||
| ytft = false; | ||
| }); | ||
| parser.on('ytdl-error', function() { |
There was a problem hiding this comment.
When the parser has an internal error when parsing time or something like that, this will be called. If rpts (see above) is more than or equal to 5, then error out and stop with 'Too Many Requests' signaling an internal error. Optionally, we might want to add a second argument called "err" as when this event is called it does come with reasoning.
lib/m3u8-parser.js
Outdated
| } else { | ||
| // Save the last line in case it has been broken up. | ||
| lastLine = line; | ||
| if (this.isYtdlCore) { |
There was a problem hiding this comment.
Add a check for if you want the old functionality or the YouTube functionality. Might want to rename the variable, as I'm sure this isn't just used in YtdlCore.
lib/m3u8-parser.js
Outdated
| // Save the last line in case it has been broken up. | ||
| lastLine = line; | ||
| if (this.isYtdlCore) { | ||
| for (var i = 0; i < lines.length; i++) { |
There was a problem hiding this comment.
Synchronously loop through all of the lines instead of Asynchronously, that way we are sure that all of the packets got into the array before the 'end' event is called from the ReadableStream.
lib/m3u8-parser.js
Outdated
| for (var i = 0; i < lines.length; i++) { | ||
| var line = lines[i]; | ||
| if (i < lines.length - 1) { | ||
| self.ytdlSavedLines.push(line); |
There was a problem hiding this comment.
Push every single packet into the array individually, instead of pushing sections of packets into the array.
lib/m3u8-parser.js
Outdated
| } | ||
| }); | ||
| } else { |
There was a problem hiding this comment.
Keep the old functionality just in case somebody wants to use this for something other than Ytdl-Core. m3u8 is very complicated.
lib/m3u8-parser.js
Outdated
| callback(); | ||
| }; | ||
|
|
||
| this.on('end', function() { |
There was a problem hiding this comment.
When the end event is called, due to the fact we synchronously pushed the packets into the array we can be certain that every packet is now in the array, and we can now parse it manually using the function (of course, checking if isYtdlCore is true first).
|
I'm pretty sure https://github.com/fent/node-m3u8stream/pull/7/files#diff-0dd6548c03db28e92002c355f52d343bL50 is causing the memory leak. I'm going to wait to continue on this project until you've commented. |
There was a problem hiding this comment.
There's a few things I'd do differently. I don't think making this feature specific to ytdl is necessary as this can be applied to other similar m3u8 playlists.
I'm not sure I understand the ytdl events for the parser. But the parser, I'd change it so that it would associates segment properties, such as time, to that segment. Compared to storing every line separately.
lib/index.js
Outdated
| clearTimeout(tid); | ||
| fetchingPlaylist = true; | ||
| var req = miniget(playlistURL, requestOptions); | ||
| req.on('error', onError); | ||
| var parser = req.pipe(new m3u8()); | ||
| if (ytdlOptions && ytft) ytdlOptions.firstTime = true; |
There was a problem hiding this comment.
Better to not manage state by altering objects that were passed from outside the module.
lib/index.js
Outdated
| @@ -74,6 +79,21 @@ module.exports = function(playlistURL, options) { | |||
| totalItems++; | |||
| requestQueue.push(item, onQueuedEnd); | |||
| }); | |||
| parser.on('ytdl-end', function() { | |||
There was a problem hiding this comment.
Did you notice the program building up memory? If so, don't think it would be from here, parser is not used or saved outside of this function. The delete operator also cannot delete variables, it only deletes keys from an object. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/delete
|
To be honest, this was mostly just to demonstrate what we had to do to get it to actually work, so you could understand what I was talking about (because through text none of that would have made sense lol). I'm not exactly sure how you want to do this specifically. |
|
Yeah, I think I understand now. Thanks for taking the time to put this together and explain. |
|
@fent Yeah no problem! This was definitely a learning process for me haha! :D If you need me to explain anything else about it just ask ;P |
|
waits patiently to see if he forgets about this |
|
So I've decided to take this on again, but this time in a production environment. I have a question, though. I'm not sure what to classify the option as because it's really indescribable. Possibly, "youtubeFormatting" or something similar? I'm just going to use "youtubeFormatting" for my current tests until I get a response :) Edit: I just started testing with the new system and oh my god is it laggy... So I'm currently unable to execute this as of right now. Every time I got the last item in a request, it would take up to 10 seconds to call the next request and each segment is only 5 seconds long. I don't know if there's a way we can do this. Of course, I'm not an expert in m3u8 so there's probably a way but idk. I'm willing to work with you if you'd like, I'm FireController1847#3577 on Discord. |
|
Okay, so I've made some changes and I've come to this. This is actually working very well as of right now, but there is one issue where it will just cut off (stop working) over a few seconds. It's working extremely well, though, and is much better than my previous commit. To enable this mode, all you add are two options (using YTDL as the example): let req = m3u8stream(url, {
chunkReadahead: +info.live_chunk_readahead,
requestOptions: options.requestOptions,
+ refreshInterval: 4000,
+ youtubeFormatting: true
}); |
There was a problem hiding this comment.
Overall, there's just one issue that I'm not sure how to fix and it's the issue where it stops working about 5-10 seconds in. It works fine between each jump initially until that point, in which I'm not sure if the stream is just not working or what. This should be a much easier basis for you to build upon, instead of my (terrible code) of an example I had last time.
lib/index.js
Outdated
| @@ -61,7 +61,7 @@ module.exports = (playlistURL, options) => { | |||
| fetchingPlaylist = true; | |||
| var req = miniget(playlistURL, requestOptions); | |||
| req.on('error', onError); | |||
| var parser = req.pipe(new m3u8()); | |||
| var parser = req.pipe(new m3u8(options)); | |||
There was a problem hiding this comment.
Pass the options through, as they're needed for youtubeFormatting detection
lib/m3u8-parser.js
Outdated
| super({ decodeStrings: false }); | ||
| this.options = options; | ||
| this._firstLines = []; |
There was a problem hiding this comment.
We must save _firstLines so we output the initial tags.
lib/m3u8-parser.js
Outdated
| super({ decodeStrings: false }); | ||
| this.options = options; | ||
| this._firstLines = []; | ||
| this.first = true; |
There was a problem hiding this comment.
This is required so we know which lines are _firstLines
lib/m3u8-parser.js
Outdated
| } else { | ||
| // Save the last line in case it has been broken up. | ||
| this._lastLine = line; | ||
| if (!this.options.youtubeFormatting) { |
There was a problem hiding this comment.
If we do not want to use youtubeFormatting, use the old way
lib/m3u8-parser.js
Outdated
| } | ||
| }); | ||
| } else { | ||
| this._lastLine = lines[lines.length - 2]; |
There was a problem hiding this comment.
Set the last line to before the blank output [' '], which is two back. This is the last audio output.
lib/m3u8-parser.js
Outdated
|
|
||
| _final(callback) { | ||
| if (this.options.youtubeFormatting) { | ||
| const firstIndex = this._firstLines.findIndex(a => a.includes('#EXTINF')); |
There was a problem hiding this comment.
Find the first audio mark in _firstLines
lib/m3u8-parser.js
Outdated
| _final(callback) { | ||
| if (this.options.youtubeFormatting) { | ||
| const firstIndex = this._firstLines.findIndex(a => a.includes('#EXTINF')); | ||
| for (let i = 0; i < firstIndex.length - 1; i++) { |
There was a problem hiding this comment.
Loop through everything before the first audio mark so we get all of those juicy details about the stream
lib/m3u8-parser.js
Outdated
| } | ||
| }); | ||
| this._parseLine(this._lastLine); |
There was a problem hiding this comment.
Parse the final audio line
|
@fent Any particular status on this new commit? |
|
Hi, sorry, I haven't had the time to look at this in detail. |
|
Ah, that's okay, I fully understand. |
|
Um? |
|
Sorry, was changing some things around. But don't worry, I still have this PR/feature in mind |
|
Alright, cool. Just let me know if you need anything more explained! |
|
@fent Sorry I'm pestering you about this, but it's been ~4 months since I made this PR with no significant changes. I'd love to be able to help out, but it's not really clear what is needed to do so (considering this was closed). This is a bug that many Discord.js users are hoping will get fixed, as it causes unstability in their programs. If there's anything you need me to do, please tell me, as I want to get this fixed as fast as possible. (And yes, I know it's only been 14 days since my last message but... yeah, I have no excuse) |
Sorry for the big delay. It's cool to feel the need to alert me. I'll try to prioritize this when I next dive into my open source stuff. |
|
Alright cool. Thanks 👍 |
|
@fent This is your 3-monthly reminder to not forget about this PR! :P |
|
@fent This is your do-it-when-a-reasonable-amount-of-time-has-passed reminder to not forget about this PR |
|
|
|
@fent ily pls ;-; we should throw a party at the anniversary 🤔 |
|
Holy fek, it is almost a year old. |
|
Sorry for the big delay. One of my goals as a dev is to not neglect any of my active repos. Which I've failed to do here :( I'll try to look at this at the beginning of next week. |
|
Awesome, can't wait. Glad to hear you haven't entirely forgotten about it :) |
This was specifically aimed towards youtube live streams, which contain segments for the past 4 hours streamed. An additional option was added, `liveBuffer` to work with `begin`. The `refreshInterval` option was removed, the playlist will now only refresh as the last segment in the playlist is approached. This works better for streams played at a variable rate. closes #7
|
Woohoo! Glad we finally got it done @fent. Good on ya' 🎉 |
YouTube Livestreams currently start very early in the livestream, due to the way that YouTube's m3u8 works. This will cause it to read the stream from the ~4h mark (if the stream is archived).
Fixes fent/node-ytdl-core#157
Fixes fent/node-ytdl-core#263