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

m3u: introduce EXT-MPV to allow specifying per-file parameters #12806

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vvuk
Copy link

@vvuk vvuk commented Nov 5, 2023

m3u files are handy, but it would be nice to be able to specify per-file parameters. This PR introduces a #EXT-MPV:param=value directive that applies to the next file in the playlist. The existing per-file params mechanism is used.

Sample usage:

#EXT-MPV:image-display-duration=5
image.jpg
#EXT-MPV:start=120
#EXT-MPV:length=20
foo.mp4
#EXT-MPV:image-display-duration=60
image2.jpg

Much of this can already be done via .edl files, but edl files (seem to?) require edl to parse all of the contents, which can be prohibitive in case of lots of entries.

@kasper93
Copy link
Contributor

kasper93 commented Nov 5, 2023

If you introduce custom #EXT-MPV it has to be documented

@vvuk
Copy link
Author

vvuk commented Nov 5, 2023

If you introduce custom #EXT-MPV it has to be documented

Yep, happy to! I should have mentioned -- would like to get some feedback first and see if this approach/format is fine, and then I'll add some docs (+ changelog etc, whatever's required)

}
} else if (bstr_startswith0(line_dup, "#EXT-X-")) {
p->format = "hls";
} else if (bstr_eatstart0(&line_dup, "#EXT-MPV:")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be #EXT-X-MPV, X denotes that extension is not standard

@sfan5
Copy link
Member

sfan5 commented Nov 5, 2023

There is a problem:
Arbitrary mpv options cannot be safely set from untrusted data (try ctrl+f on the manual for "untrusted", "insecure" or "unsafe")
Do I have a better suggestion? No, but whatever use case you have is probably better left up to a script than added to mpv itself.

@kasper93
Copy link
Contributor

kasper93 commented Nov 5, 2023

would like to get some feedback first and see if this approach/format is fine

There are two issues from this approach

  1. hooking into m3u will possibly pollute playlist in the wild with pointless mpv extension
  2. executing any option from the playlist is security issue

So either we define some subset of safe options to be able to specify like #EXT-MPV-LENGTH:20 and possibly few others. It is question to you what would be useful. But arbitrary option parsing is imho no-go in main player.

EDIT:

I see sfan5 had similar comment. The idea of external script is probably better, this way it would be opt-in by users that are aware of this functionality.

@vvuk
Copy link
Author

vvuk commented Nov 5, 2023

Ah, hmm. I wasn't thinking of security issues since I was just thinking of local usage. Perhaps adding a command line option to allow m3u options, so it's opt-in? For m3u extensions though.. VLC and others already do it, so I think that ship has sailed. But requiring a command line option to enable it should resolve this too, since there just won't be any point to putting the extension flags in the wild.

Doing things via a script is interesting though; I keep forgetting that mpv is scriptable. I'll look into that if this doesn't seem workable.

@kasper93
Copy link
Contributor

kasper93 commented Nov 5, 2023

I don't oppose that such future may be useful, but it needs to be thought through. That's why I asked for docs initially, because it is easier to discuss if we have full specification of a idea.

As for the option to enable parsing of this extension, it could be a list of allowed properties/options to be set from inside m3u. Which by default, could be empty or have few safe options. Extending to more would be opt-in on user discretion.

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

Successfully merging this pull request may close these issues.

3 participants