-
Notifications
You must be signed in to change notification settings - Fork 390
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
Implemented additional template variables for ffmpeg video splitter #291
Conversation
* Correct RFC number RFC 4180 is the CSV specification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Here's a quick checklist I think we should use going forwards:
- Target for next release branch (v0.6.1 in this case, please rebase the PR)
- Ensure code has unit tests (I think this is clear enough to not need them or can be added in future)
- Update the documentation if required
I was hoping anyone could retarget it but I think the author has to. I'll see if there's any kind of Git hooks that can automate this. I'm hoping to keep main/master always at the latest stable release is why, but in the long term maybe we should instead use a branch named "stable" for that purpose.
I also noticed a few of the workflows aren't running on PRs (Windows builds + code formatting checks) so I have some TODOs there as well.
Edit: I also want to see if you're able to merge this once it's approved now that you should have the correct permissions.
Forgot to mention, re:
Nice find, that's very interesting indeed. I wonder if that could be addressed in the future with #239 if we use a style that includes a suffix as well as a prefix. |
This pull request introduces 1 alert when merging e48fc24 into de30d9b - view on LGTM.com new alerts:
|
Merged v0.6.1 branch from upstream and rebased the PR to compare against the v0.6.1 branch. Still remaining is to update docs. Happy to take care of it in the next couple days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved once lint issues fixed.
Would it be possible to add this to the custom ffmpeg settings |
@camjac251 absolutely, that's a very good idea. Can you file a feature request for that so we can track the work? Thanks. |
Implemented the suggested template variables per #231. These new template variables are only possible for ffmpeg as mkvmerge does not support changing file output names in the same way and it is not planned for the future either (per this issue).
One quirk I found while testing this out is that a template variable cannot have an underscore after it. For example, using
split-video -f '$VIDEO_NAME_$SCENE_NUMBER'
will only replace the$SCENE_NUMBER
variable and will not substitute the$VIDEO_NAME
variable. However, if you runsplit-video -f '$VIDEO_NAME-$SCENE_NUMBER'
then they both substitute just fine. I am not sure if this is a bug with python'ssafe_substitute
function, but thought it was worth mentioning.One final issue I ran into is that if the output filename has a
:
(colon) character in it, then ffmpeg crashes and will not proceed (relevant stack exchange). So, for the timecodes in filenames I replace the colons with semicolons.I have not touched documentation for this yet, but can do so if this is to be merged.
For completeness sake, here are the new variables that are implemented:
$START_TIME
$END_TIME
$START_FRAME
$END_FRAME
The discussion in #231 also mentioned
$FRAME_NUMBER
for thesave-images
command, but it seems that one has been done already (back in February)