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

Fix audio normalizing for mute clips #1401

Merged
merged 6 commits into from
Jan 13, 2021

Conversation

jcbrockschmidt
Copy link
Contributor

@jcbrockschmidt jcbrockschmidt commented Jan 5, 2021

Resolves issue #1388. Code that only runs when the fix is applied can be found in tests/test_fx.py within test_normalize_muted, and here for convenience:

make_frame = lambda t: np.array([0.0])
clip = AudioClip(make_frame, duration=1, fps=44100)
clip = audio_normalize(clip)
  • I have provided code that clearly demonstrates the bug and that only works correctly when applying this fix
  • I have added suitable tests demonstrating a fixed bug or new/changed feature to the test suite in tests/
  • I have properly documented new or changed features in the documentation or in the docstrings
  • I have properly explained unusual or unexpected code in the comments around it
  • I have formatted my code using black -t py36

@jcbrockschmidt
Copy link
Contributor Author

jcbrockschmidt commented Jan 5, 2021

Does this change warrant a change to the docstring for audio_normalize (check box 3)?

Also, does this change warrant a comment explaining what is happening (check box 4), or is it simple enough to be left as is?

@coveralls
Copy link

coveralls commented Jan 5, 2021

Coverage Status

Coverage increased (+0.02%) to 67.546% when pulling abde0d7 on jcbrockschmidt:fix-normalize-muted into 6ab3efb on Zulko:master.

Adds clarifying comments to the code and sentence the docstring for the
audio_normalize effect.
@tburrows13 tburrows13 added the bug-fix For PRs and issues solving bugs. label Jan 12, 2021
@tburrows13
Copy link
Collaborator

Hey, sorry for the late response. This looks great, and I like your docstring change and comment.
I'll merge it once I've added in a changelog entry.

Copy link
Collaborator

@mondeja mondeja left a comment

Choose a reason for hiding this comment

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

Thank you for the fix. I have some suggestions that could improve your pull request. I'm not sure if @tburrows13 agrees with them, could confirm it?

moviepy/audio/fx/audio_normalize.py Show resolved Hide resolved
moviepy/audio/fx/audio_normalize.py Outdated Show resolved Hide resolved
tests/test_fx.py Outdated Show resolved Hide resolved
@mondeja mondeja linked an issue Jan 13, 2021 that may be closed by this pull request
jcbrockschmidt and others added 2 commits January 13, 2021 14:04
Copy link
Collaborator

@mondeja mondeja left a comment

Choose a reason for hiding this comment

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

For me is perfect. Feel free to merge it @tburrows13
Thank you for this fix @jcbrockschmidt!

@tburrows13 tburrows13 merged commit f5944d4 into Zulko:master Jan 13, 2021
tburrows13 pushed a commit to tburrows13/moviepy that referenced this pull request Jan 19, 2021
@mondeja mondeja mentioned this pull request Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix For PRs and issues solving bugs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

audio_normalize() of mute video leads to division by zero
4 participants