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

missing check for file:/// on making a new File #1260

Open
SrRapero720 opened this issue May 3, 2024 · 13 comments
Open

missing check for file:/// on making a new File #1260

SrRapero720 opened this issue May 3, 2024 · 13 comments

Comments

@SrRapero720
Copy link

SrRapero720 commented May 3, 2024

When the mrl is converted to ASCII. it first converts the uri into a File to then check if starts with file://

problem starts when you already sends the mrl with the file:// protocol. Making a new File will ends in a nested path such as file:///C:\path\of\the\process\C:\path\of\the\process\config\myvideo.mp4

https://github.com/caprica/vlcj-natives/blob/72e800fd461bf63a7ea4aa0008fdfa7f71e14c0d/src/main/java/uk/co/caprica/vlcj/binding/support/strings/NativeUri.java#L120-L129

the fix for that is before convert to file, check if the uri starts with file:/// (according with the format of new URL()), so it can be safety converted into a file to then convert it into a ASCII string

if (value.startsWith("file:///")) value = value.substring("file:///".size() - 1)
@SrRapero720 SrRapero720 changed the title late check on file:// missing check for file:/// making a new File May 3, 2024
@SrRapero720 SrRapero720 changed the title missing check for file:/// making a new File missing check for file:/// at making a new File May 3, 2024
@SrRapero720 SrRapero720 changed the title missing check for file:/// at making a new File missing check for file:/// on making a new File May 3, 2024
@caprica
Copy link
Owner

caprica commented May 3, 2024

I am not so sure about this to be honest. There is quite a bit of history around this functionality, at the moment I can't recall all of the details unfortunately, and it's been like this a long time.

I'll look into it.

@SrRapero720
Copy link
Author

SrRapero720 commented May 11, 2024

doing some look into it
it only do the file:/// check when it contains unicode charachers, but doesn't do the check when it doesn't
basically if i pass a path like A:\developer-code\modding\watermedia\src\main\resources\videolan it will consider it as a valid uri with no unicode, passing the protocol check and of couse this will not work

adding (another) basic check also fixes it too.
paths on java really confuses me, considering exists like 4 ways to manage these (URL, URI, File and Path). but certainly this can be addressed in a more cleaner way than fill the code with a lot of file:/// checks

for me works as a workarround
image

@caprica
Copy link
Owner

caprica commented May 11, 2024

if you pass "A:\whatever" then it isn't a URL, it's a local file.

@caprica
Copy link
Owner

caprica commented May 11, 2024

There was already an issue I opened in the vlcj project itself to determine if this code was still required at all in fact.

I have a nagging doubt about this code, it has been in vlcj for a long time, for some specific reasons.

Hopefully there's something in the commit log about this file which will prompt me to remember.

@caprica
Copy link
Owner

caprica commented May 11, 2024

#1142

@caprica
Copy link
Owner

caprica commented May 16, 2024

For context:
#415
#645

@SrRapero720
Copy link
Author

if you pass "A:\whatever" then it isn't a URL, it's a local file.

Indeed, but local files in a file:/// are considered URLs

Anyway, seeing MRL doesn't longer need a special encoding. The best option is change the MRL type from String to java.lang.URL. which also help to ensure whatever you're passing is a valid URL (just internally changing it toString())

for file protocol is enough doif (result.startsWidth("file:/")) result = result.replace("file:/", "file:///");

@caprica
Copy link
Owner

caprica commented May 16, 2024

OK, but really if you do have a local file, you're not supposed to pass it as a URL.

LibVLC has two dedicated native functions for loading a local file vs an "MRL". An MRL is like a URL but I'm not sure it's 100% interchangeable.

It's not Java's URL class that's doing the work here, it has to be serialised a string, and passed to the native function as that string.

This is why I'm a bit wary of this problem, as it comes down to how VLC handles MRLs.

@SrRapero720
Copy link
Author

passing the file path as an URL works fine
when I pass the file path directly it simply just don't work
No error is emitted

@SrRapero720
Copy link
Author

SrRapero720 commented Aug 3, 2024

Regardless my suggestion of switch encodeUri and convert from a string to java.io.File and java.lang.URL
after some time experimenting with this, URL doesn't have support for some protocols that VLC supports like srt://. the idea of use URL is to validate the url syntax by the dependent side before send it to VLCJ, so internally only have to convert into a string and use libvlc_media_new_location. and for File and Path directly call libvlc_media_new_path

It's not Java's URL class that's doing the work here, it has to be serialised a string, and passed to the native function as that string.

Yes, neither URI or File are doing nothing except "validate if is a path or a url".

@caprica
Copy link
Owner

caprica commented Aug 3, 2024

Could you please maybe summarise the current difficulty here, i.e. the current state of this issue/request from your point of view?

For wider context, I am struggling to remember the full history of why this code is in vlcj since it has likely been here for over 10 years in some form or another.

I think part of it is to do with handling UNICODE character conversions, which was a particular problem with vlcj on older versions of Java, and maybe VLC too.

@SrRapero720
Copy link
Author

SrRapero720 commented Aug 5, 2024

I was a little disconnected so i lost the track of what was happening inside. i have to re-study all the code

Summary

My issue is based on the process to parse a file URL. URL#toString() converts an URL like file:///c:/path/to/file.mp4 into file:/c:/path/to/file. That url is invalid if you try to place it back on a URL like new URL(new URL("file:///c:/path/to/file").toString()) it throws an exception

Also VLC rejects that Uri syntax/protocol (file:/c:/) so not sure why Java returns that cursed Uri.

What i did is convert the mrl string to always have the file:/// so i can avoid do any odd convertion.
Problem is the method encoder, which converts the file:///c:/path/to/file.mp4 into a "nested path" of itself like this file:///C:\path\of\the\process\C:\path\of\the\process\config\myvideo.mp4 as i mentioned before

This was caused by this code:
https://github.com/caprica/vlcj-natives/blob/72e800fd461bf63a7ea4aa0008fdfa7f71e14c0d/src/main/java/uk/co/caprica/vlcj/binding/support/strings/NativeUri.java#L120-L129

that code basically wraps the URL as a file, and because is not a normalized path string it gets attached into process dir, making the path looks like file:///C:\path\of\the\process\C:\path\of\the\process\config\myvideo.mp4

ASSUMPTIONS (nothing checked)

I honestly removed any ASCII encoding and it still works, accepts few odd urls with unicode characters. Thats because in the old days ASCII was the standard, considering how often was required tell to gradle, java and few text editors to manually use UTF-8. I assume VLC 2 was using ASCII as default but got changed on VLC 3(?

In any case i was working with that on watermedia for a long time and there wasn't reported issues about URL's getting failed to load by VLC due to bad MRL sources

IMPORTANT SPOTS

  • encodeUri breaks File URLs
  • encodeUri to ASCII isn't needed anymore

My suggestions

One of the fixes i did on the silly fork i made was use java.io.File for local files and java.lang.URI for URLs. that doesn't do any encoding to ASCII or UTF-8, it simply serves to idenfity the type of the MRL. if was a file or a "online source". Internally VLCJ doesn't have to adivinate what type of MRL is, instead you're making the dependent dev to serve the proper MRL type.

Extra

URI also forces dev to encode the URL to mitigate "non-valid" chars
image

VLC supports both types of file MRL, as URL or as a File path, URL Encoded or not
image
image

@caprica
Copy link
Owner

caprica commented Jan 23, 2025

This issue is still being considered.

@caprica caprica transferred this issue from caprica/vlcj-natives Jan 23, 2025
@caprica caprica added this to the vlcj 5.0.0 Release milestone Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants