mpv icon indicating copy to clipboard operation
mpv copied to clipboard

vobsub: fix loading with url and non-ascii path

Open sarnex opened this issue 2 years ago • 3 comments

When dragging a vobsub idx file onto the mpv window, the file will be a URL, such as file:///path/to/subs.idx

mpv then tries to guess the corressponding vobsub sub file for that idx file, and then passes that guess to ffmpeg.

ffmpeg then internally removes the file:// prefix and does a open() call.

The problem occurs if the URL contains non-ASCII characters where we see percent encoding. The URL is not decoded and passed to ffmpeg directly, so ffmpeg tries to open /path/to/%E5%A4%A9/subs.sub which obviously doesn't exist.

The idx file works because we decode that somewhere else before passing to ffmpeg.

If we know the vobsub idx file path is a URL, update the filename we use for the vobsub sub file guess to use the decoded path to the vobsub idx file

Fixes https://github.com/mpv-player/mpv/issues/10860

Signed-off-by: Nick Sarnie [email protected]

sarnex avatar Nov 13 '22 21:11 sarnex

ffmpeg then internally removes the file:// prefix and does a open() call.

Shouldn't this be fixed on ffmpeg's side? If it's accepting URIs then it should do it properly and do the percent decoding too - just removing the file:// prefix seems half-assed.

N-R-K avatar Nov 23 '22 17:11 N-R-K

@N-R-K So yes, I will try to change ffmpeg and see if the maintainers accept that, for anyone that cares it happens here in libavformat/file.c:

static int file_open(URLContext *h, const char *filename, int flags)
{
    FileContext *c = h->priv_data;
    int access;
    int fd;
    struct stat st;

    av_strstart(filename, "file:", &filename);

But right now there is an inconsistentcy between the format of the filename passed to ffmpeg for the idx vs the sub file.

The idx file is percent decoded by mpv before passing to ffmpeg here in stream_file.c

if (!strict_fs) {
        char *fn = mp_file_url_to_filename(stream, bstr0(stream->url));
        if (fn)
            filename = stream->path = fn;
        url = stream->url;
    }

But it's never decoded by mpv for the idx file, which is what this PR adds.

Like I said I will do a PR for ffmpeg as well, but I still hope this patch is accepted to make things consistent for the idx/sub file on the mpv/ffmpeg layer

sarnex avatar Nov 24 '22 00:11 sarnex

Note: ffmpeg refused the patch on their end and instead want to introduce fs:// outside of file://, so we need this patch to fix mpv

http://ffmpeg.org/pipermail/ffmpeg-devel/2022-November/304234.html

sarnex avatar Nov 24 '22 18:11 sarnex