Allow streaming from other yt-dlp supported sources

aquelemiguel commented 1 year ago

📝 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 commented 1 year ago

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.

joao-conde commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

aquelemiguel commented 1 year ago

@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.

StaticRocket commented 1 year ago

@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 commented 1 year ago

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

aquelemiguel commented 1 year ago

@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 commented 1 year ago

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:

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

joao-conde commented 1 year ago

@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 commented 1 year ago

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

aquelemiguel commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

Agreed, go with 2 lists for allowed and unallowed

StaticRocket commented 1 year ago

@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 / parrot