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

Audio error handling made failsafe #377

Merged
7 commits merged into from
Apr 4, 2017
Merged

Audio error handling made failsafe #377

7 commits merged into from
Apr 4, 2017

Conversation

gyglim
Copy link
Contributor

@gyglim gyglim commented Jan 3, 2017

This PR is to improve audio reading robustness.

In case there are audio frames missing, this repeats the last frame, as is done in the case for video frames:
https://github.com/Zulko/moviepy/blob/master/moviepy/video/io/ffmpeg_reader.py#L109

Thus it makes the code more consistent and robust.
Fixing #246

Cheers,
Michael

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 46.896% when pulling 68df86c on gyglim:master into caf955c on Zulko:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 46.896% when pulling 68df86c on gyglim:master into caf955c on Zulko:master.

@coveralls
Copy link

coveralls commented Mar 13, 2017

Coverage Status

Coverage increased (+0.04%) to 46.896% when pulling 68df86c on gyglim:master into caf955c on Zulko:master.

@gyglim
Copy link
Contributor Author

gyglim commented Mar 13, 2017

The PR now only shows a warning when not enough frames can be read (happens when there are less frames than should be, according to fps*duration). Because of that, when iterating over a larger range than the actual clip (as in https://github.com/gyglim/moviepy/blob/68df86c880ec1df0f9f3e0d4b36efa02f8c2b8eb/tests/test_issues.py#L139), it was possible to request frames completely out of the estimated valid range, while before, an error would occur earlier.
Thus, I added an explicit check that the required times are valid according to the estimated range, as otherwise the list of frames to be read is empty in https://github.com/gyglim/moviepy/blob/68df86c880ec1df0f9f3e0d4b36efa02f8c2b8eb/moviepy/audio/io/readers.py#L173

@coveralls
Copy link

coveralls commented Mar 16, 2017

Coverage Status

Coverage increased (+0.04%) to 48.695% when pulling d3b9428 on gyglim:master into d633510 on Zulko:master.

@coveralls
Copy link

coveralls commented Mar 16, 2017

Coverage Status

Coverage increased (+0.07%) to 48.732% when pulling d3b9428 on gyglim:master into d633510 on Zulko:master.

@gyglim
Copy link
Contributor Author

gyglim commented Mar 16, 2017

Hey. Just added a test for this. Is there anything else needed to merge it? @Earney

@ghost
Copy link

ghost commented Mar 16, 2017

@gyglim. Thanks for the pr
Lets see what some of the other project contributors thinks. @Zulko. Is it ok to replace the error message with this code?

@coveralls
Copy link

coveralls commented Mar 17, 2017

Coverage Status

Coverage increased (+0.1%) to 48.79% when pulling befe989 on gyglim:master into d633510 on Zulko:master.

@gyglim
Copy link
Contributor Author

gyglim commented Apr 3, 2017

New issues on the same:
#265
#521

@Zulko Any opposition against merging this PR? Anything I can do to improve it?

@Zulko
Copy link
Owner

Zulko commented Apr 3, 2017

Sorry i thought this was merged. Id say yes to this pr. It's a bit of a hack, but it wont be the first in moviepy, and if it solves that audio problem it's good for me.

@gyglim
Copy link
Contributor Author

gyglim commented Apr 3, 2017

Ok, great :). I agree, but it makes sense and is consistent with what happens for video

@coveralls
Copy link

coveralls commented Apr 3, 2017

Coverage Status

Coverage increased (+0.07%) to 48.954% when pulling 5671b17 on gyglim:master into c611107 on Zulko:master.

@ghost
Copy link

ghost commented Apr 3, 2017

@gyglim lets merge this in.. unless someone objects?

@gyglim
Copy link
Contributor Author

gyglim commented Apr 4, 2017

@Earney Yes, let's do it

@ghost ghost merged commit 43cd9db into Zulko:master Apr 4, 2017
@keikoro keikoro added the audio Related to AudioClip, or handling of audio in general. label Oct 6, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audio Related to AudioClip, or handling of audio in general.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants