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

feat(lib): add audio to partial movie files and section videos #3763

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

jeertmans
Copy link
Contributor

@jeertmans jeertmans commented May 16, 2024

Overview: What does this pull request change?

With this PR, audio is added when writing partial movies. Before, it was only added at the end, so partial movie files and section videos did not contain audio.

Motivation and Explanation: Why and how do your changes improve the library?

My main motivation is jeertmans/manim-slides#375, in line with what I mentioned in #3501 (@behackl).

Further Information and Comments

The current PR is still in draft for the following reasons:

  • it only works is audio (self.add_sound) is added before the first animation, I need to understand why and how to fix this (zero-padding does not seem to work as the concat output does not contain any audio stream in this case);
  • must add tests;
  • [ ] check if we can avoid padding audio every time we write some partial movie; I think zero-padding is necessary, having possibly empty audio streams seems to be a problem;
  • check if this works for various output formats, especially webm;
  • and check if audio is correctly split between two or more video files (i.e., does the concatenated version sounds like the original audio?).

Idea: it might be interesting to rethink the add_sound function to, e.g., let the user pass an AudioSegment instead of a file. This would be very useful for truncating audio, for example. That could also be a separate PR.

Reviewer Checklist

  • The PR title is descriptive enough for the changelog, and the PR is labeled correctly

@jeertmans
Copy link
Contributor Author

Note: the commit history looks very weird, even though I rebased onto your main branch...

if ptype == "video":
packet.stream = output_video_stream
elif ptype == "audio":
packet.stream = output_audio_stream

Check failure

Code scanning / CodeQL

Potentially uninitialized local variable Error

Local variable 'output_audio_stream' may be used before it is initialized.
@chopan050 chopan050 changed the title feat(lib): feat(lib): add audio to partial movie files and section videos May 16, 2024
@chopan050
Copy link
Contributor

Hello, and thanks for your contribution!

The current PR is still in draft for the following reasons:

Is it still a draft? (asking because of the failing tests) If so, please mark it as a draft, thanks!

@jeertmans
Copy link
Contributor Author

Hello, and thanks for your contribution!

The current PR is still in draft for the following reasons:

Is it still a draft? (asking because of the failing tests) If so, please mark it as a draft, thanks!

Hello! Yes it is, I forgot to tick the « draft » box :)

@jeertmans jeertmans marked this pull request as draft May 17, 2024 08:44
@jeertmans jeertmans marked this pull request as ready for review May 17, 2024 11:57
@jeertmans
Copy link
Contributor Author

jeertmans commented May 17, 2024

@chopan050 This is now ready for reviewing! I haven't added any test yet, but not sure where / how to correctly test this. I have looked at the current test suite, and we don't have many tests for sound nor partial movie files.

Edit: it seems that adding audio alters the duration of the media (could be logic) but also the frame rate?? So some tests (based on video metadata) are failing...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

2 participants