-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
build: dynamically generate mpv.desktop file protocols #14145
Conversation
dfe21d1
to
ca50758
Compare
Download the artifacts for this pull request: |
6d719be
to
c5cbf50
Compare
I also removed the |
48c8bb8
to
6aae461
Compare
6aae461
to
71ff64e
Compare
71ff64e
to
7513cd9
Compare
f9f50dd
to
4ca5deb
Compare
4ca5deb
to
579b410
Compare
@@ -1826,6 +1826,21 @@ if get_option('cplayer') | |||
command: [osxbundle, mpv.full_path(), '@SOURCE_ROOT@'], | |||
) | |||
endif | |||
|
|||
if not win32 and not darwin | |||
if meson.can_run_host_binaries() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unclear to me what should be the value of X-KDE-Protocols
if this fails. But I guess it can be left as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current logic is that X-KDE-Protocols
has everything and stuff will be selectively removed if possible. Another possible alternative would be to keep a list somewhere of some subset of X-KDE-Protocols
that we always add by default and possibly add other ones like smb
if possible.
579b410
to
58f7694
Compare
We don't have version compare macros, but if we had we could deprecate |
Is all that effort for |
I can't answer this question, but we cannot have this habit of breaking things, just because it is probably not used. |
But users don't need to discover the ffmpeg bluray protocol. mpv has its own. I get the naming conflict but I don't see an actual concrete advantage of switching one of the so far supported ways of playing BD disks to a somewhat incompatible different demuxer. |
Unless this bluray thing is really useless I'd suggest following this protocol. |
I guess I misunderstood what 2 means. I agree it has to be deprecated for at least one full release cycle. So it is deprecated in stable and removed in next stable. So I guess that makes it 2. Anyway, I would just add an exception for |
b596738
to
74cd034
Compare
I changed the last commit to a deprecation instead. |
Please list a single reason on why we want to map |
So the problem is that we lookup |
Is it possible to put these dynamic protocols to the end of probing order? Every time ffmpeg adds a protocol there is a potential for name conflict with a mpv built-in one. This also means that whether the built-in implementation is selected depends on whether ffmpeg is built with that protocol or not, which is certainly undesirable. |
The only usage of this function is freed in mpv's generic property code, so no other changes are needed.
Previously, all stream protocols were a static list in mpv. This is okay for own builtin stuff, but for protocols that depend on ffmpeg it's not so great. Support for certain protocols may or may not be enabled in a user's ffmpeg and the protocol list that mpv generates should ideally match this. Fix this by implementing a get_protocols method for stream_lavf that will have different results depending on the ffmpeg mpv is built against. We keep the safe and unsafe protocols separation. The former is essentially a whitelist. Any protocol that is found in ffmpeg but is not in the safe whitelist is considered unsafe. In the stream list, ffmpeg is moved to the bottom so any possible protocols that are added in the future don't automatically take precedence over any builtin mpv ones.
If we can run the built mpv binary, then it is possible to use a custom target that reads the protocols we have available in mpv and write the mpv.desktop file based on the output. For cases where this is not possible (e.g. cross compiling), then just install the unmodified mpv.desktop file like before. Fixes mpv-player#8731 and fixes mpv-player#14124.
74cd034
to
2606891
Compare
Because stream_bluray is not really maintained in mpv at all? It's not like this is some wonderful feature that is thoroughly tested and constantly used. In fairness, the same is probably true for ffmpeg but why go out of our way to hide it?
Yes, I forgot that this was recently added. I don't know what the best way to deal with this is unfortunately. It's probable that ffmpeg's dvd is better than mpv's already (another thing that nobody maintains) or will be soon.
Sure I moved it to the bottom of the stream list. |
The fact it is not maintained, doesn't mean it doesn't work. We don't get any complaints about that, so for the small amount of people who use it it works ok. Also ffmpeg playlist selection is very simple, maybe mpv is simple too, but protocol like this is not a ffmpeg job. I know we don't support menus and stuff, but ffmpeg will always just select playlist and try to play it, while media player like mpv may do more with it. mpv is not ffplay. And more complex things like bluray support should be implemented in the player, maybe some day someone wants to improve it, maybe not, but the API boundary between mpv and ffmpeg would make it impossible to do proper work.
Completely disagree, same reasons as above. I know dvd demuxer was heavily crippled by wm4 and now it is not doing more "advanced" stuff like menus. But ffmpeg demuxer is in it's infancy and doesn't even support seeking currently with certain discs. ffmpeg is not some golden standard, look at their ML sometime... And similar thing is with our Matroska demuxer, our is just better, and while I recently added some missing feature, it still supports more things than lavf. |
AFAIK, ffmpeg alleged supports bluray angles and mpv doesn't (did not personally test it). Also #7111 exists and to my knowledge it is still completely valid. So I would consider it to be pretty broken. I know from personal experience that seeking on CDs is totally unreliable. I doubt that DVDs or BDs are better. And again ffmpeg support is probably subpar as well but I don't see a strong reason why we should believe ours is automatically better and should be encouraged.
No I don't think this is similar at all. I was going to bring it up as a counterargument actually. Our mkv demuxer is undisputedly better and works well. You can not say the same about disc playback in mpv. It's crap here. |
Fair. But even though our code may be subpar, we shouldn't try to hide it. If it is broken it has to be deprecated and removed. As long as we support certain protocol, first-party mapping should prefer our code for better or for worse. You can use |
I'm only suggesting that we show both in |
If the ffmpeg demuxers are better than what we have then it would be reasonable to plan to drop ours entirely. It just shouldn't be done half way and drive-by in this PR. So for this PR I propose
mpv never supported DVD menues. What wm4 did was remove some disc-specific hacks that were needed to have seeking work correctly on DVD/BD because the extra code was bothering him I guess, and it's been broken since. |
No one suggested that and this PR doesn't do that. I don't understand why "just list both" is apparently so controversial but whatever I will stop wasting my time on this and forcibly drop the ffmpeg listings. It's not like anyone even uses discs with mpv anyway. Edit: and done. |
The naming of these conflict with existing mpv protocols, so skip if we get them. Users can still use them via lavf://bluray: or lavf://dvd: if they wish.
2606891
to
00506d5
Compare
It did. But either way, got removed, because it interfered with internals. And that's fine, I guess if someone not want to maintain some parts of code. But it is still something that is doable in mpv codebase, but would be impossible to do in ffmpeg. ffmpeg is not really designed to support interactivity. |
Another attempt at this.