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

Use absolute instead of canonicalize for better Windows path handling #861

Merged
merged 2 commits into from
Aug 10, 2024

Conversation

shssoichiro
Copy link
Collaborator

Closes #860

@shssoichiro shssoichiro force-pushed the issue-860 branch 2 times, most recently from b15a2fe to bb2184c Compare July 24, 2024 15:19
@Uranite
Copy link
Contributor

Uranite commented Jul 28, 2024

It seems that the cachepath is still canonicalized
notepad++_960_2024-07-28-1722163870
as a result, when using bestsource, the path for the cache is wrong (see how the path repeats, and cache.json is a folder)
C:\Users\redacted\Videos\osu!\osu! 2024.05.25 - 12.33.34.01\split\cache.json\Users\redacted\Videos\osu!\osu! 2024.05.25 - 12.33.34.01.mp4.0.bsindex

Edit: Seems like I was incorrect, even if the path is an absolute path, the behaviour will still be the same because according to bestsource's README it seems to be an intended behaviour

cachepath: The path where cache files are written. Note that the actual index files are written into subdirectories using based on the source location. Defaults to %LOCALAPPDATA% on Windows and ~/bsindex elsewhere.

@Uranite
Copy link
Contributor

Uranite commented Jul 28, 2024

Anyway, here are my proposed change for this PR
By using to_absolute_path for cache_file we can safely remove use path_abs::PathAbs;
Also, bestsource has stopped generating json indexes since a long time now, so I changed it to bsindex (path still all weird)

diff --git a/av1an-core/src/vapoursynth.rs b/av1an-core/src/vapoursynth.rs
index 7c8484e..4a5f1f5 100644
--- a/av1an-core/src/vapoursynth.rs
+++ b/av1an-core/src/vapoursynth.rs
@@ -5,7 +5,6 @@ use std::path::{Path, PathBuf};
 
 use anyhow::{anyhow, bail};
 use once_cell::sync::Lazy;
-use path_abs::PathAbs;
 use std::process::Command;
 use vapoursynth::prelude::*;
 use vapoursynth::video_info::VideoInfo;
@@ -201,13 +200,13 @@ pub fn create_vs_file(
 
   let mut load_script = File::create(&load_script_path)?;
 
-  let cache_file = PathAbs::new(temp.join("split").join(format!(
+  let cache_file = to_absolute_path(&temp.join("split").join(format!(
     "cache.{}",
     match chunk_method {
       ChunkMethod::FFMS2 => "ffindex",
       ChunkMethod::LSMASH => "lwi",
       ChunkMethod::DGDECNV => "dgi",
-      ChunkMethod::BESTSOURCE => "json",
+      ChunkMethod::BESTSOURCE => "bsindex",
       _ => return Err(anyhow!("invalid chunk method")),
     }
   )))?;

@Uranite
Copy link
Contributor

Uranite commented Jul 28, 2024

I can confirm that this PR fixes bestsource performance, my proposed changes is not necessary, but it's up to you if you want

@shssoichiro
Copy link
Collaborator Author

shssoichiro commented Jul 30, 2024

Seems like a good idea to include anyway. Unfortunately what is blocking this PR is that something seems broken with the select chunk method, unrelated to this change, and I haven't been able to track down what is causing it yet. The fact that it broke without any code changes, makes me think it may be related to an ffmpeg or other library update...

@shssoichiro
Copy link
Collaborator Author

@master-of-zen This one should be ready as well now.

@master-of-zen master-of-zen merged commit 5e9228a into master-of-zen:master Aug 10, 2024
5 checks passed
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

Successfully merging this pull request may close these issues.

Change path handling in vapoursynth.rs for compatibility
3 participants