master-of-zen / Av1an

Cross-platform command-line AV1 / VP9 / HEVC / H264 encoding framework with per scene quality encoding
GNU General Public License v3.0
1.51k stars 156 forks source link

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

Closed shssoichiro closed 3 months ago

shssoichiro commented 3 months ago

Closes #860

Uranite commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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