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

Joining files together, "safe", and relative filenames #457

Closed
brainwane opened this issue May 7, 2022 · 4 comments · Fixed by #460
Closed

Joining files together, "safe", and relative filenames #457

brainwane opened this issue May 7, 2022 · 4 comments · Fixed by #460

Comments

@brainwane
Copy link

Thank you again for ffmprovisr!

I suggest we change the "join files together" recipe so the filenames don't have the initial ./ because that seems to read as unsafe to ffmpeg.

Today I consulted this recipe and tried:

ffmpeg -f concat -i list.txt -c copy gift.mp4

The contents of list.txt had relative filenames, as suggested in the recipe:

file './1.mp4'
  file './2.mp4'
  file './3.mp4'
  file './4.mp4'
  file './5.mp4'

But I got the error Unsafe file name './1.mp4'. Thank you for the link in that recipe to https://trac.ffmpeg.org/wiki/Concatenate -- that plus this StackOverflow thread led me to add -safe 0 into the command, and it worked.

I was trying to figure out what I had done wrong, and looked at the link you'd provided regarding relative vs. absolute paths plus this Trac ticket. It looks like maybe it prefers NOT to have the ./ at the start of the filename, so I changed the lines in list.txt to

file '1.mp4'
  file '2.mp4'
  file '3.mp4'
  file '4.mp4'
  file '5.mp4'

and ran ffmpeg -f concat -i list.txt -c copy gift2.mp4 and it seemed to work fine.

I'm using ffmpeg 4.3.4 on Debian buster.

@kfrn
Copy link
Member

kfrn commented May 10, 2022

Hi @brainwane, thanks so much for flagging this!

You're totally right - I just checked this too. As you say, it's not working currently with the explicit reference to the current directory - to get that to work you need the safe flag set to 0. (Which is also necessary when you're working with absolute paths).

I know the current recipe worked in the past, I wonder when that changed 🤔 - certainly more recently than the date of that TRAC ticket!

What the recipe should say is: to use just the filenames (can be quoted or not - edit, depending on what they contain), and probably add a note about having to run the command from the directory in which the files and list txt file reside. For completeness, we could cover the safe flag and its options, to give people info on how to run the command with absolute/unsafe filepaths.

Is updating the recipe something that you'd like to take on, @brainwane? Happy to assist if necessary ☺️

@brainwane
Copy link
Author

Thanks for the explanation @kfrn and for the fix in #458 (I had been assuming the indentation was meaningful but it looks like it was not). And thank you for your work on the tool!

I know the current recipe worked in the past, I wonder when that changed 🤔 - certainly more recently than the date of that TRAC ticket!

Raises a question re: which versions of ffmpeg to support, which I've filed as #459.

What the recipe should say is: to use just the filenames (can be quoted or not - edit, depending on what they contain), and probably add a note about having to run the command from the directory in which the files and list txt file reside. For completeness, we could cover the safe flag and its options, to give people info on how to run the command with absolute/unsafe filepaths.

So, it sounds like the recipe should advise people: if possible, run the command from the same directory where the files and the txt file reside. Otherwise you'll have to use safe (and here's how).

Thanks for the context!

Is updating the recipe something that you'd like to take on, @brainwane? Happy to assist if necessary ☺️

I am not going to take this on, but thanks for the offer of help @kfrn.

@kieranjol
Copy link
Collaborator

Thanks for flagging this, as mentioned in #459, it looks like this changed came about last August FFmpeg/FFmpeg@46fb395 . I might attempt a fix now but my ffmprovisr skillz are rusty :)

@kfrn
Copy link
Member

kfrn commented May 10, 2022

Hey good find, @kieranjol! I was getting ready for work so didn't want to start digging 😅

kieranjol added a commit that referenced this issue May 12, 2022
join files recipe - remove './' from example, fixes #457
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 a pull request may close this issue.

3 participants