-
Notifications
You must be signed in to change notification settings - Fork 3k
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
base: master
Are you sure you want to change the base?
Conversation
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>
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 |
@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:
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
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 |
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)); |
There was a problem hiding this comment.
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)
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