gacopl / dvmkv2mp4

Convert any Dolby Vision/HDR10+ MKV to DV MP4 that runs on many devices
198 stars 36 forks source link

Add destination and temporary directory options #49

Closed masupilamie closed 3 months ago

masupilamie commented 4 months ago

As discussed via e-mail I've added two optional arguments to the script that allow usage of different directories for converted and temporary files. This change touches a lot of the code, so probably best to review. Tested with DV71 and HDR10+ files. This would close issue #21 and allows for conversion speedup by using different filesystems (hardware). It also enables usage when the source directory is read-only or should be left unchanged.

gacopl commented 4 months ago

on a quick look dest dir is not applied in mp4box, dvconverting cannot be in tmp otherwise the whole thing stops working in multiple files dir. probably more, also default tempdir would be ok to have ./tmp

masupilamie commented 4 months ago

dest dir is not applied in mp4box Do you mean the execution of mp4string variable? If so, at that point the output variable already has the destination directory added when the --dest-dir option is used.

dvconverting cannot be in tmp My reasoning behind moving the dvconverting "lock" file to temp (or dest when the option is used), is to allow for read-only source directories. Using temp/dest for the lock file does not seem to negatively impact multiple file directories (tested with 2 files). The only way this could result in multiple converts at the same time is when the script is executed again with different parameters, which is in my opinion an acceptable compromise.

./tmp as default temp dir I have added a commit to support this,

probably more Let me know when other things need to be fixed or you still have concerns. In my testing the script and new options work as expected, tested with HDR10+ and DV71 files.

gacopl commented 4 months ago

all it takes to have 2 triggers to run this script on same dir, imagine tv show season if dvconverting will be not in place things will break, because of all the tmp files going on, there cannot be 2 conversions running on same source dir

masupilamie commented 4 months ago

I think I understand what you mean. In that case the script without my changes will already run into issues when it is executed multiple times in the same directory with multiple mkv files in it, since the dvconverting file uses the input variable. I suggest to make the .dvconverting file static so it locks the whole directory during script run (ie just .dvconverting without the filename as prefix).

I've made the dvconverting file static and added the option to move the lock file to temp, see last commit.

masupilamie commented 4 months ago

Thanks for reviewing my commits.

As I see it there are multiple ways how to handle lock files and the temporary directory:

The last option is probably the best / safest since there is no risk of temporary files being used for other conversions. This also allows to have a lock per conversion instead of a lock for the whole script run. I've added this logic, it creates a subfolder in the working directory (or --temp-dir/--dest-dir when the option is provided) using file name with '.tmp' as suffix for the subdirectory name. Let me know how you feel about this approach.

I've also removed the --lock-in-temp option since the lock file will now always be in the temporary directory. Lock file will only be removed when debug is not enabled to prevent debug files conflicting with a new run.

gacopl commented 3 months ago

ok looks ok have you tested it properly with without debug etc to make sure files are where they should be and not get deleted :) ?

masupilamie commented 3 months ago

Yes, tested both with and without debug and using only working directory and with --dest-dir/--temp-dir options active. Did not encounter any unexpected behaviour. Note: having debug on with multiple files in a directory can generate a lot of used disk space, but I consider this normal debug behaviour.

gacopl commented 3 months ago

ok i trust you :D thanks for your work

masupilamie commented 3 months ago

Glad I could contribute to the script. Maybe I have some other improvement ideas but I'll share those via an issue / feature request so it can be discussed before coding it.