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

vobsub: fix loading with url and non-ascii path #10862

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

Conversation

sarnex
Copy link

@sarnex sarnex commented Nov 13, 2022

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 #10860

Signed-off-by: Nick Sarnie sarnex@gentoo.org

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 becuase 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

Signed-off-by: Nick Sarnie <sarnex@gentoo.org>
@N-R-K
Copy link
Contributor

N-R-K commented Nov 23, 2022

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.

@sarnex
Copy link
Author

sarnex commented Nov 24, 2022

@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
Copy link
Author

sarnex commented Nov 24, 2022

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

@@ -565,7 +565,9 @@ static void guess_and_set_vobsub_name(struct demuxer *demuxer, AVDictionary **d)
subname = replace_idx_ext(tmp, start);
if (subname)
subname = talloc_asprintf(tmp, "%s?%.*s", subname, BSTR_P(end));
}
} else {
bfilename = bstr0(mp_file_get_path(tmp, bfilename));
Copy link
Contributor

Choose a reason for hiding this comment

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

This will fail if file is not local.

(also use spaces)

@kasper93 kasper93 added the priority:on-ice may be revisited later label Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:on-ice may be revisited later
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vobsub sub file loading doesn't work for URI with non-ASCII path
3 participants