build: dynamically generate mpv.desktop file protocols #14145

Dudemanguy · 2024-05-14T18:46:28Z

Another attempt at this.

github-actions · 2024-05-14T19:26:06Z

Download the artifacts for this pull request:

Windows

macOS

stream/stream.c

meson.build

TOOLS/gen-mpv-desktop.py

stream/stream.c

Dudemanguy · 2024-05-16T02:29:44Z

I also removed the bluray:// protocol alias in stream_bluray because that name is already used by an ffmpeg protocol which is now supported with this change.

stream/stream_lavf.c

DOCS/interface-changes/bluray-protocol.txt

TOOLS/gen-mpv-desktop.py

stream/stream_lavf.c

stream/stream.c

DOCS/man/mpv.rst

TOOLS/gen-mpv-desktop.py

kasper93 · 2024-05-20T21:24:39Z

meson.build

@@ -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()


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.

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.

stream/stream.c

stream/stream_lavf.c

TOOLS/gen-mpv-desktop.py

stream/stream_lavf.c

kasper93 · 2024-05-21T20:24:06Z

We don't have version compare macros, but if we had we could deprecate bluray:// and #if its exclusion behind VERSION < 0.40, after that it would be removed, even if we forget.

Dudemanguy · 2024-05-21T20:27:20Z

Is all that effort for bluray:// even worth it? Who even watches discs with mpv?

kasper93 · 2024-05-21T20:36:37Z

I can't answer this question, but we cannot have this habit of breaking things, just because it is probably not used.

sfan5 · 2024-05-21T20:43:13Z

Adding logic to specficially exclude bluray feels arbitrary and again hurts discoverability.

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.

na-na-hi · 2024-05-21T20:46:27Z

Why 2?

DOCS/compatibility.rst:

Important/often used parts must be deprecated for at least 2 releases before they can be broken. There must be at least 1 release where the deprecated functionality still works, and a replacement is available (if technically reasonable).

Unless this bluray thing is really useless I'd suggest following this protocol.

kasper93 · 2024-05-21T20:59:28Z

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 bluray:// for now and it can be reevaluated in another PR if we want to remove it.

Dudemanguy · 2024-05-22T01:28:44Z

I changed the last commit to a deprecation instead.

sfan5 · 2024-05-22T08:50:48Z

Please list a single reason on why we want to map bluray:// to ffmpeg. Everyone seems to be beating around the bush.

kasper93 · 2024-05-22T12:00:05Z

So the problem is that we lookup stream_info_ffmpeg and stream_info_ffmpeg_unsafe first. As I understand in this case dvd:// have exactly same problem as bluray://, no?

na-na-hi · 2024-05-22T12:11:30Z

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.

Dudemanguy · 2024-05-22T14:06:07Z

Please list a single reason on why we want to map bluray:// to ffmpeg.

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?

As I understand in this case dvd:// have exactly same problem as bluray://, no?

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.

Is it possible to put these dynamic protocols to the end of probing order?

Sure I moved it to the bottom of the stream list.

kasper93 · 2024-05-22T14:16:10Z

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?

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.

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.

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.

Dudemanguy · 2024-05-22T14:30:58Z

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.

And similar thing is with our Matroska demuxer, our is just better

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.

kasper93 · 2024-05-22T14:40:36Z

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 ffmpeg://bluray:// or lavf://bluray:// if you want to bypass it.

Dudemanguy · 2024-05-22T14:51:00Z

I'm only suggesting that we show both in --list-protocols. Maybe document the difference if people feel strongly enough about it. The problem is the naming.

sfan5 · 2024-05-22T15:18:25Z

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.
We should also think about compatibility for --bluray-device and --dvd-device or the protocol path/args.

So for this PR I propose

moving the ffmpeg to probe last to prevent possible future name conflicts
hiding the dvd and bluray ffmpeg protocols

I know dvd demuxer was heavily crippled by wm4 and now it is not doing more "advanced" stuff like menus.

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.

Dudemanguy · 2024-05-22T15:25:46Z

It just shouldn't be done half way and drive-by in this PR.

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.

na-na-hi · 2024-05-22T15:51:06Z

mpv never supported DVD menues

It was supported in mplayer and was even rewritten once for mpv: 41fbcee, 0530447

kasper93 · 2024-05-22T16:09:27Z

mpv never supported DVD menues.

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.

Dudemanguy changed the title ~~build: dynamically generate destop file~~ build: dynamically generate mpv.desktop file protocols May 14, 2024

Dudemanguy force-pushed the protocol-generate branch 4 times, most recently from dfe21d1 to ca50758 Compare May 14, 2024 19:09

na-na-hi reviewed May 14, 2024

View reviewed changes

stream/stream.c Outdated Show resolved Hide resolved

sfan5 reviewed May 15, 2024

View reviewed changes

meson.build Outdated Show resolved Hide resolved

TOOLS/gen-mpv-desktop.py Outdated Show resolved Hide resolved

stream/stream.c Outdated Show resolved Hide resolved

stream/stream.c Outdated Show resolved Hide resolved

Dudemanguy force-pushed the protocol-generate branch 6 times, most recently from 6d719be to c5cbf50 Compare May 16, 2024 02:18

Dudemanguy force-pushed the protocol-generate branch from 48c8bb8 to 6aae461 Compare May 16, 2024 02:45

sfan5 reviewed May 16, 2024

View reviewed changes

stream/stream_lavf.c Show resolved Hide resolved

Dudemanguy force-pushed the protocol-generate branch from 6aae461 to 71ff64e Compare May 16, 2024 15:03