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

Automatically rename existing mp4 files under register_output.mp4.path location to [path].old.[index] #684

Merged

Conversation

WojciechBarczynski
Copy link
Member

@WojciechBarczynski WojciechBarczynski commented Aug 22, 2024

When working on the release demo I found it extremely annoying, that I couldn't automatically overwrite the output file, so I decided to add this option.

Update:
Decided to go with renaming the file instead

@WojciechBarczynski WojciechBarczynski marked this pull request as ready for review August 22, 2024 14:53
@WojciechBarczynski WojciechBarczynski self-assigned this Aug 22, 2024
Copy link
Member

@wkozyra95 wkozyra95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overwrite is not really clear in the context of the output. Yes if you read the docs you will know what it does, but I have a feeling that it does not fit here.

Option to select whether a file should be overiden or not does not seem to be necessary, and if possible I prefer to avoid adding unnecessary stuff. To solve problem from PR description I would check if file already exists and rename it e.g.
test.mp4 -> test.mp4.bak.0/test.mp4.bak.1/test.mp4.bak.2

@WojciechBarczynski
Copy link
Member Author

@wkozyra95
The name comes from ffmpeg -y option docs: https://ffmpeg.org/ffmpeg.html#Main-options

It is a simple quality of usage improvement. Sure, users can do it outside the compositor, but if it's a commonly occurring problem, I don't see the reason not to implement it.

Your solution with versioned files is less ergonomic, as users have to check the name of the generated file to play it. My "workflow" while working with offline processing examples was:

  1. Write script generating output MP4
  2. Play it
  3. Fix stuff
  4. Play it
  5. Fix stuff
  6. Play it ...

Without this change, I needed to delete the file before generating it (I often forgot about it / tried deleting the file even though it wasn't generated). Your solution is also far from perfect for this since I will need to check the name of the last generated file to play it.

In the end, I think that if ffmpeg supports this as one of the main options, it's worth adding it.

@WojciechBarczynski WojciechBarczynski changed the title Add mp4 output overwrite option Automatically rename existing mp4 files under register_output.mp4.path location to [path].old.[index] Aug 23, 2024
@WojciechBarczynski
Copy link
Member Author

I'm not putting it in the changelog, as it seems too specific/minor to be included there.

@wkozyra95
Copy link
Member

I'm not putting it in the changelog, as it seems too specific/minor to be included there.

I don't know, this is sth that might not be worth documenting, but I think CHANGELOG is a place where it might be worth mentioning it. No current section really fits that, we could add Other section maybe or in expo we had Chores (https://github.com/expo/eas-cli/blob/main/CHANGELOG.md)

Up to you, I don't have strong opinion on that.

@WojciechBarczynski WojciechBarczynski merged commit 207f111 into master Aug 23, 2024
2 checks passed
@WojciechBarczynski WojciechBarczynski deleted the @WojciechBarczynski/add_mp4_output_overwrite_option branch August 23, 2024 18:11
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.

2 participants