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

Switch to Distube's ytdl-core fork (resolves playback issue) #1042

Merged
merged 7 commits into from
Jul 18, 2024

Conversation

MarcoCoreDuo
Copy link
Contributor

Closes #1037

I don't think ytdl-core will receive any updates anymore and play-dl also has this playback issue. Distube's fork has some extra fixes and also updated decipher code (resolves the playback issue). It works as a drop-in replacement. The only downside is that it needs nodejs >= 18.

  • I updated the changelog

@EugeneLeclerc
Copy link
Contributor

Well I just did the same, you were 30 minutes faster than me: #1044

While we are updating Node, I think we could go directly to Node 20? It is the current LTS and has been for quite a while already.

@EugeneLeclerc EugeneLeclerc mentioned this pull request Jul 12, 2024
3 tasks
Copy link

github-actions bot commented Jul 12, 2024

📦 A new release has been made for this pull request.

To play around with this PR, pull codetheweb/muse:pr-1042 or codetheweb/muse:3c161b77dd2d8148e7821d470ae550626a24b196.

Images are available for x86_64 and ARM64.

Latest commit: 3c161b7

@MarcoCoreDuo
Copy link
Contributor Author

Well I just did the same, you were 30 minutes faster than me: #1044

While we are updating Node, I think we could go directly to Node 20? It is the current LTS and has been for quite a while already.

I'm not really sure if we could use Node 20 without updating dependencies. I am glad that the current Dockerfile now works (Debian-Bookworm had a too-new libc-bin). But I can try building the Dockerfile with Node 20.

@EugeneLeclerc
Copy link
Contributor

EugeneLeclerc commented Jul 12, 2024

I'm not really sure if we could use Node 20 without updating dependencies. I am glad that the current Dockerfile now works (Debian-Bookworm had a too-new libc-bin). But I can try building the Dockerfile with Node 20.

Yeah getting things working in the priority I guess, Node 20 can wait 😄

@MarcoCoreDuo
Copy link
Contributor Author

I'm not really sure if we could use Node 20 without updating dependencies. I am glad that the current Dockerfile now works (Debian-Bookworm had a too-new libc-bin). But I can try building the Dockerfile with Node 20.
Yeah getting things working in the priority I guess, Node 20 can wait 😄

I tried building the image with Node 20, and it does not work because there are no pre-built binaries for @discordjs/opus. So Node 20 would require Python and build tools in the image.

@@ -89,6 +89,7 @@
"@discordjs/opus": "^0.8.0",
"@discordjs/rest": "1.0.1",
"@discordjs/voice": "0.11.0",
"@distube/ytdl-core": "^4.13.5",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could fix current bug in bot

@2KDrop
Copy link

2KDrop commented Jul 16, 2024

Tried running the PR and while it plays songs fine when the queue runs out the bot crashes immediately with the error "No songs in queue to forward to."

Error: No songs in queue to forward to.
at default.manualForward (/usr/app/src/services/player.ts:315:13)
at default.forward (/usr/app/src/services/player.ts:277:10)
at default.onAudioPlayerIdle (/usr/app/src/services/player.ts:586:18)
at AudioPlayer.emit (node:events:517:28)
at AudioPlayer.set state (/usr/app/node_modules/@discordjs/voice/src/audio/AudioPlayer.ts:356:9)
at AudioPlayer.stop (/usr/app/node_modules/@discordjs/voice/src/audio/AudioPlayer.ts:493:9)
at default.forward (/usr/app/src/services/player.ts:283:27)
at default_1.execute (/usr/app/src/commands/skip.ts:37:20)
at Client. (/usr/app/src/bot.ts:79:27)
at Client.emit (node:events:517:28)

Running on Linux Mint through Docker, unsure if that means anything for it

@Stealthii
Copy link

Using the suggested codetheweb/muse:pr-1042 image, I've seen the issue resolved, and not encountered any other warnings or issues during a few hours testing.

@2KDrop
Copy link

2KDrop commented Jul 17, 2024

Did further testing, seems to happen only when skipping the last song in the queue or skipping to the last song in the queue, have tested 3 videos and gotten the same results from all of them.

Old Gods of Asgard's Herald of Darkness audio track - https://www.youtube.com/watch?v=TyC1NzSmFIg
Noah's Face to Face audio track - https://www.youtube.com/watch?v=oLcoljNKh3I
Fart with reverb sound effect - https://www.youtube.com/watch?v=9FLRHejWAo8

All play through fine but upon skipping to the last or the last video itself the bot crashes with the error below.

/usr/app/src/services/player.ts:315
throw new Error('No songs in queue to forward to.');
^

Error: No songs in queue to forward to.
at default.manualForward (/usr/app/src/services/player.ts:315:13)
at default.forward (/usr/app/src/services/player.ts:277:10)
at default.onAudioPlayerIdle (/usr/app/src/services/player.ts:586:18)
at AudioPlayer.emit (node:events:517:28)
at AudioPlayer.set state (/usr/app/node_modules/@discordjs/voice/src/audio/AudioPlayer.ts:356:9)
at AudioPlayer.stop (/usr/app/node_modules/@discordjs/voice/src/audio/AudioPlayer.ts:493:9)
at default.forward (/usr/app/src/services/player.ts:283:27)
at default_1.execute (/usr/app/src/commands/skip.ts:37:20)
at Client. (/usr/app/src/bot.ts:79:27)
at Client.emit (node:events:517:28)

Node.js v18.20.4

I've attached a screenshot to showcase an example of what to do to recreate the bug
image

@MarcoCoreDuo
Copy link
Contributor Author

MarcoCoreDuo commented Jul 17, 2024

Did further testing, seems to happen only when skipping the last song in the queue or skipping to the last song in the queue, have tested 3 videos and gotten the same results from all of them.

Old Gods of Asgard's Herald of Darkness audio track - https://www.youtube.com/watch?v=TyC1NzSmFIg Noah's Face to Face audio track - https://www.youtube.com/watch?v=oLcoljNKh3I Fart with reverb sound effect - https://www.youtube.com/watch?v=9FLRHejWAo8

All play through fine but upon skipping to the last or the last video itself the bot crashes with the error below.

/usr/app/src/services/player.ts:315 throw new Error('No songs in queue to forward to.'); ^

Error: No songs in queue to forward to. at default.manualForward (/usr/app/src/services/player.ts:315:13) at default.forward (/usr/app/src/services/player.ts:277:10) at default.onAudioPlayerIdle (/usr/app/src/services/player.ts:586:18) at AudioPlayer.emit (node:events:517:28) at AudioPlayer.set state (/usr/app/node_modules/@discordjs/voice/src/audio/AudioPlayer.ts:356:9) at AudioPlayer.stop (/usr/app/node_modules/@discordjs/voice/src/audio/AudioPlayer.ts:493:9) at default.forward (/usr/app/src/services/player.ts:283:27) at default_1.execute (/usr/app/src/commands/skip.ts:37:20) at Client. (/usr/app/src/bot.ts:79:27) at Client.emit (node:events:517:28)

Node.js v18.20.4

I've attached a screenshot to showcase an example of what to do to recreate the bug image

Thank you for reporting. This was an error from a different pr by me. This also resulted in the bot always skipping an additional song. Should now be fixed.

@Stealthii
Copy link

Using the suggested codetheweb/muse:pr-1042 image, I've seen the issue resolved, and not encountered any other warnings or issues during a few hours testing.

Spoke too soon, got these exceptions today:

tmp-muse-1  | Error: type lockupViewModel is not known
tmp-muse-1  |     at parseItem (/usr/app/node_modules/ytsr/lib/parseItem.js:83:13)
tmp-muse-1  |     at catchAndLogFunc (/usr/app/node_modules/ytsr/lib/parseItem.js:90:12)
tmp-muse-1  |     at module.exports (/usr/app/node_modules/ytsr/lib/parseItem.js:119:46)
tmp-muse-1  |     at /usr/app/node_modules/ytsr/lib/main.js:53:34
tmp-muse-1  |     at Array.map (<anonymous>)
tmp-muse-1  |     at module.exports (/usr/app/node_modules/ytsr/lib/main.js:53:25)
tmp-muse-1  |     at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
tmp-muse-1  |     at KeyValueCacheProvider.wrap (/usr/app/src/services/key-value-cache.ts:53:20)
tmp-muse-1  |     at async run (file:///usr/app/node_modules/p-queue/dist/index.js:109:36)
tmp-muse-1  |
tmp-muse-1  | /********************************************************************************************************************************************************************************************************
tmp-muse-1  | failed at func parseItem: type lockupViewModel is not known
tmp-muse-1  | pls post the the files in /usr/app/node_modules/ytsr/dumps to https://github.com/TimeForANinja/node-ytsr/issues
tmp-muse-1  | os: linux-x64, node.js: v18.20.4, ytsr: 3.8.4

I've just pulled the updates from 3c161b7 and will test today.

@MarcoCoreDuo
Copy link
Contributor Author

Using the suggested codetheweb/muse:pr-1042 image, I've seen the issue resolved, and not encountered any other warnings or issues during a few hours testing.

Spoke too soon, got these exceptions today:

tmp-muse-1  | Error: type lockupViewModel is not known
tmp-muse-1  |     at parseItem (/usr/app/node_modules/ytsr/lib/parseItem.js:83:13)
tmp-muse-1  |     at catchAndLogFunc (/usr/app/node_modules/ytsr/lib/parseItem.js:90:12)
tmp-muse-1  |     at module.exports (/usr/app/node_modules/ytsr/lib/parseItem.js:119:46)
tmp-muse-1  |     at /usr/app/node_modules/ytsr/lib/main.js:53:34
tmp-muse-1  |     at Array.map (<anonymous>)
tmp-muse-1  |     at module.exports (/usr/app/node_modules/ytsr/lib/main.js:53:25)
tmp-muse-1  |     at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
tmp-muse-1  |     at KeyValueCacheProvider.wrap (/usr/app/src/services/key-value-cache.ts:53:20)
tmp-muse-1  |     at async run (file:///usr/app/node_modules/p-queue/dist/index.js:109:36)
tmp-muse-1  |
tmp-muse-1  | /********************************************************************************************************************************************************************************************************
tmp-muse-1  | failed at func parseItem: type lockupViewModel is not known
tmp-muse-1  | pls post the the files in /usr/app/node_modules/ytsr/dumps to https://github.com/TimeForANinja/node-ytsr/issues
tmp-muse-1  | os: linux-x64, node.js: v18.20.4, ytsr: 3.8.4

I've just pulled the updates from 3c161b7 and will test today.

I think this isn't an issue caused by this PR (see #1000). It's an error with ytsr (deprecated module for doing YT search requests). There is also an open PR for replacing it with Distube's fork #1024.

@EugeneLeclerc
Copy link
Contributor

I think this isn't an issue caused by this PR (see #1000). It's an error with ytsr (deprecated module for doing YT search requests). There is also an open PR for replacing it with Distube's fork #1024.

Yeah this PR cannot and should not fix everything. The 403 errors are fixed, thats already a good enough reason to merge as-is.

Copy link
Collaborator

@codetheweb codetheweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the delay, thank you for the patch!

@codetheweb codetheweb merged commit fcc8d88 into museofficial:master Jul 18, 2024
5 checks passed
Copy link

🚀 Released in Release v2.9.0.

@MarcoCoreDuo MarcoCoreDuo deleted the distube-ytdl-core branch July 18, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to play any songs
6 participants