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

Allow streaming from other yt-dlp supported sources #209

Closed
aquelemiguel opened this issue Oct 25, 2022 · 17 comments · Fixed by #223
Closed

Allow streaming from other yt-dlp supported sources #209

aquelemiguel opened this issue Oct 25, 2022 · 17 comments · Fixed by #223
Assignees
Labels
🔧 improvement General improvements to existing features

Comments

@aquelemiguel
Copy link
Owner

📝 Description

yt-dlp allows for streaming from multiple other sources other than YouTube. A lot more actually.

In the play command, we check the query/url in this fashion:

let query_type = if url.contains("spotify.com") {
  // extract metadata from the spotify link and search it on youtube
}
else if url.contains("youtube.com") {
  // pass the link directly to yt-dlp
}
else {
  // pass the provided query to yt-dlp
}

There are a few issues with this approach:

  • It limits the source to YouTube, meaning a user would not be able to stream from SoundCloud, Twitter or any other media providing website supported by yt-dlp.
  • It does not take into account shortened links that redirect to YouTube (like bit.ly or more flagrant, youtu.be) and searches the link in YouTube, which is pretty dumb.
  • It does not include URL validation. "/play query: ftp://youtube.com" isn't a valid link to pass to yt-dlp, for instance. We should ensure the link is valid before passing it to yt-dlp.

🪜 Reproduction Steps

N/A

ℹ Environment / Computer Info

  • Parrot version: v1.4.2
  • Operating System: Windows 11

📸 Screenshots

No response

@aquelemiguel aquelemiguel added 💬 discussion Further information is requested 👓 triage This issue is being reviewed labels Oct 25, 2022
@aquelemiguel
Copy link
Owner Author

Asking for your input @joao-conde and @afonsojramos as to what would be the best approach for distinguishing a YouTube query from a valid YouTube link.

@aquelemiguel aquelemiguel added the 🎃 hacktoberfest Suitable for Hacktoberfest label Oct 25, 2022
@joao-conde
Copy link
Collaborator

Not just valid youtube link, but all other links from https://github.com/yt-dlp/yt-dlp/blob/master/supportedsites.md right?
Can check for the http:// begining and if not present assume its a query.

Additionally, we could have blacklisted sites, like p**rnhub, or allow that to be set on a per guild basis, using our guild settings

@aquelemiguel
Copy link
Owner Author

aquelemiguel commented Oct 31, 2022

@joao-conde I've got an implementation in the works, it's just missing the blacklist. How would you add this? Would mods have to add every single 18+ website manually? There are a lot of supported websites, would be a chore.

@joao-conde
Copy link
Collaborator

Better to have some control than none. Mods would do something of an includes logic, like "/block porn" would block sites with porn in the name. "/block actuallydomain.com" would block URLs with that.

@StaticRocket
Copy link
Contributor

Easiest solution to implement would be have a switch to toggle default behavior (allow/block all) and then mods provide overrides for each domain. It goes without saying that any command specific solution would depend on #124 , unless we just want to say whoever sets up the bot gets to decide and slap it into an env var.

@joao-conde
Copy link
Collaborator

Nah we need the settings. I am un-assigning myself, can't finish it. Feel free to @StaticRocket or @aquelemiguel

@aquelemiguel
Copy link
Owner Author

@StaticRocket @joao-conde I'm experimenting with the new modals feature for this allow/block domains config.
It's fetching the allowed list from the GuildSettings and (should soon) update it on submit.

Opinions? I think it looks neat and cuts down on the # of commands we'd need otherwise.

image

@StaticRocket
Copy link
Contributor

@aquelemiguel , the only concern I have is if the default behavior should be an allow list or a block list. I see one being useful for keeping things under lock and key but for those running the bot on a trusted server it may be tedious to allow almost all domains one by one.

@joao-conde
Copy link
Collaborator

I think it should be a black list approach, by default allow all. The modal looks good to me.

@aquelemiguel
Copy link
Owner Author

aquelemiguel commented Nov 8, 2022

@StaticRocket @joao-conde Taking both your inputs into account, please feedback on this approach where we'd add two required fields to the modal - an allow list and a block list:

  • I think by default all domains except for youtube.com should be blocked, so this should be the modal's initial state:

    Allow list: youtube.com
    Block list: *

  • If a mod wants to just block a few specific domains, they can add a wildcard to the allow list and declare them in the block list:

    Allow list: *
    Block list: twitter.com

  • For logical reasons, exactly one of the fields must have a wildcard at all times. AFAIK, there's no form validation, so it gets a little tricky. I suggest we prioritize blocking. For example, the user submits:

    Allow list: *
    Block list: *

    But what actually gets stored is the default:

    Allow list: youtube.com
    Block list: *

  • In this scenario where the user did not provide at least one wildcard:

    Allow list: youtube.com;twitter.com
    Block list: youtube.com

    For each conflicting domain, it should IMO still prioritize blocking. Since youtube.com is both being allowed and blocked, we block it:

    Allow list: twitter.com
    Block list: *

  • This would both satisfy trusted guilds where it's alright to stream from anywhere and larger servers where it should be more restrictive.

  • With new supported links being added to yt-dlp, mods may choose to block or allow them by default.

Is this implementation confusing? Am I overlooking any nasty edge cases?

@afonsojramos
Copy link
Collaborator

I'd say that's the fairest implementation of this feature. I do believe that in the case that if neither have a wildcard, you assume the default behaviour.

Question about the modals:

  1. Does it maintain state? If someone opens that modal after someone made changes, do you see those changes?
  2. Is there not a possibility of having two separate fields? One for the AllowList and another for the BlockList?

@joao-conde
Copy link
Collaborator

@aquelemiguel I was suggesting the exact opposite, and so was @StaticRocket I think.

  • have only a black list
  • all domains are allowed except the ones on the black list
  • black list domains are added as "strings" that if "contained" in the domain, it is blocked

@afonsojramos
Copy link
Collaborator

@joao-conde Initially, I was also supporting that approach, however, I do see the value for large servers to work on an AllowList.

@aquelemiguel
Copy link
Owner Author

@afonsojramos

Does it maintain state? If someone opens that modal after someone made changes, do you see those changes?

Yes, it reads from and writes to the GuildSettings, so if you were to block a domain, I would see it upon opening the modal.

Is there not a possibility of having two separate fields? One for the AllowList and another for the BlockList?

Definitely, that's what I suggested!

@aquelemiguel
Copy link
Owner Author

@joao-conde yt-dlp supports too many supported 18+ domains, it'd be hell for some servers to manually block all (and having to periodically check for new undesired domains). However simultaneously, some other servers may want to allow most. I think that was the point @StaticRocket was making and what prompted my approach.

@joao-conde
Copy link
Collaborator

Agreed, go with 2 lists for allowed and unallowed

@StaticRocket
Copy link
Contributor

@joao-conde yt-dlp supports too many supported 18+ domains, it'd be hell for some servers to manually block all (and having to periodically check for new undesired domains). However simultaneously, some other servers may want to allow most. I think that was the point @StaticRocket was making and what prompted my approach.

Right, that and the general extractor they have can apply to domains that aren't necessarily listed on yt-dlp's compatibility page. An allow list approach becomes tedious with that. I run a server with a bunch of impatient goofs that'll pitch a fit if they can't blast a random meme across voice chat from god-knows-where.

@aquelemiguel aquelemiguel removed 💬 discussion Further information is requested 🎃 hacktoberfest Suitable for Hacktoberfest 👓 triage This issue is being reviewed labels Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔧 improvement General improvements to existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants