celluloid-player / celluloid

A simple GTK+ frontend for mpv
https://celluloid-player.github.io
GNU General Public License v3.0
1.16k stars 93 forks source link

Unable to use option suffix #635

Closed SebiderSushi closed 3 years ago

SebiderSushi commented 3 years ago

Overview Description: Unable to use a suffix when passing a list option on to mpv.

I am trying to play a locally cloned HLS stream. Since FFmpeg applies different restrictions for "allowed media extensions" when playing a stream via HTTP versus the local filesystem, it is required to manually whitelist the affected filetypes. It is possible to specify the value ALL and whitelist all filetypes, but to me it seems more sane to just whitelist the filetypes that i really need. Enumerating all of those requires separating them with commata like so allowed_extensions=ts,key. However, the mpv option --demuxer-lavf-o= parses commata in order to separate multiple key-value pairs and i haven't found a way to escape that. As a workaround it is possible to use the --mpv-demuxer-lavf-o-append= option which only takes one key-value pair and hence does not parse for commata. Unfortunately, celluloid appears unable to pass these modifiers properly on to mpv

Steps to Reproduce:

  1. Get a local HLS manifest with local files.
  2. celluloid --mpv-demuxer-lavf-o-append=allowed_extensions='ts,key' m3u8

Actual Results: Celluloid fails to apply the option

Expected Results: Celluloid passes it on to mpv as is the case when calling with the plain --mpv-demuxer-lavf-o= option like so:
celluloid --mpv-demuxer-lavf-o=allowed_extensions='ts,key' m3u8,
or manually with mpv demuxer-lavf-o-append=allowed_extensions='ts,key' m3u8

Version: 0.20 mpv version 0.33.0

gnome-mpv commented 3 years ago

There's a bug related to parsing mpv options with quoted values in the latest release. It's now fixed in git master, but your example option still doesn't work. I did a little bit of testing, and it looks like action suffixes cannot be used through libmpv at all, so this is something that has to be fixed in mpv.

SebiderSushi commented 3 years ago

Okay thanks a lot!

avih commented 3 years ago

t looks like action suffixes cannot be used through libmpv at all, so this is something that has to be fixed in mpv

I think this depends on how exactly you try to use it. It should be supported at the config file, same as at the command line, like doing foo-add=bar,baz at the config file, or --foo-add=bar,baz at the CLI invocation.

But at runtime there's no property foo-add so you can't use a command set foo-add bar,baz.

The way to change list options (with same actions and semantics) is via the change-list command.

gnome-mpv commented 3 years ago

But at runtime there's no property foo-add so you can't use a command set foo-add bar,baz.

We're using mpv_set_option_string() to pass CLI options to mpv, so I guess that explains why it doesn't work.

The way to change list options (with same actions and semantics) is via the change-list command.

Thanks. It's good to know there's a workaround, although ideally there should be a way to get the same behavior as mpv's CLI options without additional parsing/processing. It'd be more robust to changes in mpv that way.

For example, suppose mpv renames some of the action suffixes or introduces new ones. If we were to parse the options and use change-list, we'll need to adapt our code to the new changes. However, if action suffixes worked in mpv_set_option_string(), everything will continue to work fine with no changes (at least for the case where we're just passing through options from the user).

avih commented 3 years ago

However, if action suffixes worked in mpv_set_option_string()

First of all, this actually works for some or maybe even all of the list properties, as long as you use the property foo and not foo-add (especially when you wanted to avoid the suffix).

However, this is generally a bad idea to use compound values programmatically. The documentation is for CLI options, and that's why the docs title is "List options" rather than "list properties".

The CLI options parser has to compromise between being reasonably useful and succinct most of the time, and allow completely arbitrary sets of values, some of them possibly nested.

When you access mpv programmatically, then you should prefer much less ambiguous interfaces, such as those provided by the change-list command.

Also, the suffixes will probably not change anytime soon, and mpv does try to keep the interface backward compatible whenever possible. But if they do change, then it's part of the price that you have to pay when the library which you use makes progress.

Did you ever try to use ffmpeg, for instance? every two years or so a set of interfaces is deprecated and replaced with new ones. That's how it works. Everyone does best efforts to avoid hurting users, but sometimes the cost and hindrance for progress is simply too much.

SebiderSushi commented 3 years ago

Someone from the mpv team redirected me to the section about escaping special characters in the mpv documentation: https://github.com/mpv-player/mpv/issues/8489#issuecomment-829500931

This solves my use case both directly with mpv and when using mpv through celluloid. I can achieve what i want by running celluloid --mpv-demuxer-lavf-o=allowed_extensions=[ts,key] m3u8.

Feel free to reopen the issue if you think there is still something to do here.