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

ytdl_hook.lua: search for yt-dlp by default #9209

Merged
merged 1 commit into from
Sep 25, 2021

Conversation

guidocella
Copy link
Contributor

Because youtube-dl is inactive and the yt-dlp fork is becoming more
popular, make mpv use yt-dlp without any extra configuration.

yt-dlp is ordered before youtube-dl because it's more obscure, so users
who have yt-dlp installed are more likely to want to use it rather than
youtube-dl.

io.open and os.getenv('PATH') are not used because they don't handle
UTF-8 on Windows 7 and 8.

exists_in_path is based on
https://github.com/TheAMM/mpv_script_libs/blob/master/helpers.lua

Fixes #9208.

@garoto
Copy link
Contributor

garoto commented Sep 13, 2021

youtube-dl was inactive for quite some time before any forks were made into relevancy, but still everything was just "working fine (tm)", like it always did.

Just ask on IRC about it.

@Dudemanguy
Copy link
Member

youtube-dl does have a real issue where youtube will randomly throttle you for no reason. This lead me to switch to yt-dlp instead which has a partial workaround for this and is more usable. Since youtube is a constantly moving target (thanks google) and youtube-dl breakage will probably only get worse, switching the default seems reasonable.

@garoto
Copy link
Contributor

garoto commented Sep 13, 2021

Ugh, how does the youtube throttling is fixed by what extractor is being used? The ffmpeg network code will fetch video+audio data from whatever media URL is returned by youtube-dl's --dump-single-json?

And I tested yt-dlp's convoluted workarounds, and it made no difference whatsoever while playing youtube hosted media with mpv as far as the newly implemented throthling goes.

@haasn
Copy link
Member

haasn commented Sep 13, 2021

I'm in favor of this for the sole reason being that yt-dlp allows watching age-restricted content (youtube-dl does not). That alone is enough of an advantage for me to consider it an improvement in functionality.

@garoto This patch does not prevent you from using youtube-dl.

@garoto
Copy link
Contributor

garoto commented Sep 13, 2021

Not fond with messing with ytdl_hook.lua with mundane changes like this. "What wiaaboo has to say about it?" is mostly what I care about right now.

@Dudemanguy
Copy link
Member

Ugh, how does the youtube throttling is fixed by what extractor is being used?

It's not a fix. It's a stupid workaround that simply retrys the download if you are being throttled under a certain rate. This is better than what youtube-dl currently does (i.e. nothing). See: yt-dlp/yt-dlp@51d9739

@garoto
Copy link
Contributor

garoto commented Sep 13, 2021

t's a stupid workaround that simply retrys the download if you are being throttled under a certain rate [...]

Dude... that doesn't happen with mpv+yt_hook.lua.

@Dudemanguy
Copy link
Member

If that doesn't happen for you, that's great but it's an open issue at youtube-dl: ytdl-org/youtube-dl#29326

@garoto
Copy link
Contributor

garoto commented Sep 13, 2021

What I mean is that yt-dlps workarounds againt the youtube throttling doesn't apply to mpv+ffmpeg network code as far as I was able to test it.

@CH41UP4B47M4N
Copy link

This discussion seems to have devolved into an argument about whose personal opinion of each project is better than the other.

The argument being made for the patch is that mpv should search for more than just youtube-dl by default. Currently if you do not have youtube-dl installed but you have one of the many forks such as the most popular yt-dlp, mpv will fail trying to open urls that these helpers would handle.

If someone has yt-dlp installed they probably want to use it, especially if it is not installed alongside youtube-dl. If someone doesn't have yt-dlp installed this isn't going to impact them at all. And yt-dlp isn't yet included in most linux distros; one has to very deliberately install it vs youtube-dl, so it would make sense to favor yt-dlp if both are installed side-by-side.

Now the argument could be made that one can simply add the option to their config to use yt-dlp(or another tool), but conversely one could simply do the same thing for youtube-dl if they happen to fall into the single corner case where they have both installed and would prefer to use youtube-dl over yt-dlp. Your personal opinion of which project you would rather use or loyalties to one or another is otherwise irrelevant to this discussion.

The only real concerning questions that don't involve your personal feelings that should be raised in regards to mpv are how this decision would affect this project and it's users long-term. Do we have faith yt-dlp will be around for a while or even outlast youtube-dl at all, or will it be 'dead' just like youtube-dlc in a few months? And is yt-dlp heading in a direction that could be beneficial if youtube-dl remains stagnant?
Or, what legitimate reasons do we have to prefer youtube-dl and if the case is strong enough, can we at least compromise to fallback on yt-dlp if youtube-dl isn't available?

@guidocella
Copy link
Contributor Author

The current directory is now searched for executables on Windows.

@hooke007
Copy link
Contributor

Too early. Many things need to be watched. I share the same opinion with CH41UP4B47M4N.

@guidocella
Copy link
Contributor Author

Because Windows's search in PATH is hard to replicate, this now checks if subprocess's error is "init" to determine if a youtube-dl executable is unavailable.

I think it would simpler if exec just returned result and then use its fields though.

@haasn
Copy link
Member

haasn commented Sep 14, 2021

Ugh, how does the youtube throttling is fixed by what extractor is being used?

yt-dlp uses a different extractor client (android vs web) which reportedly has a lower chance of youtube throttling you.

Anecdotally, I haven't run into the throttling issue since switching to yt-dlp.

Because youtube-dl is inactive and the yt-dlp fork is becoming more
popular, make mpv use yt-dlp without any extra configuration.

yt-dlp is ordered before youtube-dl because it's more obscure, so users
who have yt-dlp installed are more likely to want to use it rather than
youtube-dl.

Fixes mpv-player#9208.
@guidocella
Copy link
Contributor Author

guidocella commented Sep 17, 2021

This now uses the correct path when playing multiple URLs from the second URL onwards, while before the wrong variable was saved in ytdl.path which is what gets called in youtube-dl invokations after the first; you can see it in the Github diff url for force pushes: https://github.com/mpv-player/mpv/compare/f5443a812a4113ecd33f10f5c6f215a94590d457..d1c92bfd79ef81ac804fcc20aee2ed24e8d587aa.

@haasn
Copy link
Member

haasn commented Sep 17, 2021

Seems to work for me.

@N-R-K
Copy link
Contributor

N-R-K commented Sep 24, 2021

Let's say this gets merged, now lets say a couple months later yt-dlp suffers the same fate as countless other youtube-dl forks. Now what?

Do we just keep adding the new shiny fork as default and have the older defaults as fallback and repeat the cycle? Or do we drop yt-dlp all together and break backwards compatibility for users who were utilising the implicit yt-dlp default rule?

As long as youtube-dl works for most cases, I do not see much reason for this change especially given the fact that users can setup yt-dlp very easily if they want that.

@Hrxn
Copy link
Contributor

Hrxn commented Sep 24, 2021

Let's say this gets merged, now lets say a couple months later yt-dlp suffers the same fate as countless other youtube-dl forks. Now what?

Now what? Let's speculate a bit more about things that may or may not happen in a couple months, maybe?

@N-R-K
Copy link
Contributor

N-R-K commented Sep 24, 2021

Now what? Let's speculate a bit more about things that may or may not happen in a couple months, maybe?

Apologies for having some foresight, but I think my last line answers your question about what happens now as well.

especially given the fact that users can setup yt-dlp very easily if they want that.

@haasn
Copy link
Member

haasn commented Sep 25, 2021

Let's say this gets merged, now lets say a couple months later yt-dlp suffers the same fate as countless other youtube-dl forks. Now what?

Thinking about hypotheticals like this wastes more of your time and brain cells than updating entries in a list or reverting a commit.

@N-R-K
Copy link
Contributor

N-R-K commented Sep 25, 2021

If we're talking about wasting time, then this entire PR can be considered just that.

ytdl_path=/path/to/yt-dlp

Copy-pasting this into script-opts/ytdl_hook.conf takes 2 seconds.

@haasn
Copy link
Member

haasn commented Sep 25, 2021

Tested multiple configurations, and it works fine for me. Code looks reasonable now (could be better but meets the par for the rest of the file).

@haasn haasn merged commit cc4ada6 into mpv-player:master Sep 25, 2021
@haasn
Copy link
Member

haasn commented Sep 25, 2021

Copy-pasting this into script-opts/ytdl_hook.conf takes 2 seconds.

But has to be done by everybody, rather than once. Of course, mpv being a popular project with billions of users, you have to multiply those 2 seconds by a lot.

@guidocella guidocella deleted the yt-dlp branch September 25, 2021 12:07
@Pentaphon
Copy link

Pentaphon commented Oct 1, 2021

Let's say this gets merged, now lets say a couple months later yt-dlp suffers the same fate as countless other youtube-dl forks. Now what?

You change it to whatever software people are moving to. Is software set in stone or something? Is this something that has to be done daily like some kind of brain-numbing task? No, it's just updating software to work with current standards when the situation calls for it, while deprecating old, unused ones.

@N-R-K
Copy link
Contributor

N-R-K commented Oct 1, 2021

it's just updating software to work with current standards

What standard? Neither youtube-dl nor yt-dlp are defined in any standard that I know of. And if you're talking about popularity, feel free to re-read the commit msg.

yt-dlp is ordered before youtube-dl because it's more obscure


Besides this is already merged, nothing else to discuss here.

@Pentaphon
Copy link

What standard?

The standard of having more functionality to the end user. See the discussion on age restricted videos and the new extractor.

You're the one still fighting the notion of yt-dlp being searched at all.

@N-R-K
Copy link
Contributor

N-R-K commented Oct 1, 2021

You're the one still fighting the notion of yt-dlp being searched at all.

Where? I said it's merged. Move on.

It's common that in open source development users will raise their opinion. If you do not like that then too bad, I don't think mpv has any plans to taking development private.

And if you're just looking to argue for no reason, then I think I've entertained you enough. You'll have to find a new target. 😄

@Pentaphon
Copy link

It's common that in open source development users will raise their opinion.

And the community is glad yours was not taken seriously. The project needs to adapt.

@CounterPillow
Copy link
Contributor

wow please just kiss already

@arrowgent
Copy link

throttling does appear to be a large concern now, imagine trying to start a 4GB video at 45KB/s or seeking in that video
youtube-dl is still handling the video transfer input to MPV

yt-dlp atleast has a working throttle detection and workaround

until upstream (linux) releases ive just added
--script-opts=ytdl_hook-ytdl_path=yt-dlp
to handle testing and checking my videos...

@G2G2G2G
Copy link

G2G2G2G commented Oct 17, 2021

Maintainer of youtube-dl responded to someone's email and said they were too busy to work on it.. he so far hasn't even offered to give anyone contributor status. youtube-dl is basically unusable for youtube for a few weeks now.

Let's say this gets merged, now lets say a couple months later yt-dlp suffers the same fate as countless other youtube-dl forks. Now what?

add both and prefer one that currently works (yt-dlp) if that isn't on the system fall to youtube-dl
add a flag to choose
...

@CounterPillow
Copy link
Contributor

@G2G2G2G why are you trying to start an argument back up in an already merged PR?

@G2G2G2G
Copy link

G2G2G2G commented Oct 17, 2021

Confirming the maintainer said he's gonna be MIA isn't arguing anything, it's posting a fact.

@mpv-player mpv-player locked as resolved and limited conversation to collaborators Oct 17, 2021
@Akemi
Copy link
Member

Akemi commented Oct 17, 2021

locking the comments for obvious reasons.

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

Successfully merging this pull request may close these issues.

youtube-dl is dead, add yt-dlp as default