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 out of bounds error #570

Merged
merged 2 commits into from
Oct 5, 2017
Merged

Conversation

shawwn
Copy link
Contributor

@shawwn shawwn commented May 25, 2017

Closes #19, #246, #269, #281.

iter_chunks now uses np.linspace to generate indices. In addition
to fixing the out of bounds error, this also ensures each chunk
generates a uniform number of indices.

`iter_chunks` now uses `np.linspace` to generate indices. In addition
to fixing the out of bounds error, this also ensures each chunk
generates an approximately equal number of indices.
@shawwn
Copy link
Contributor Author

shawwn commented May 25, 2017

Hmm, it looks like this PR doesn't solve all the cases. I'll keep investigating.

@coveralls
Copy link

coveralls commented May 25, 2017

Coverage Status

Coverage increased (+0.1%) to 54.185% when pulling 2f569d0 on shawwn:fixes/indices-out-of-range into f63a985 on Zulko:master.


pospos = list(range(0, totalsize, chunksize))+[totalsize]
pospos = np.linspace(0, totalsize, nchunks + 1, endpoint=True, dtype=int)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it intended to add + 1 to nchunks on L70 and add + 1 again in pospos ?

Copy link
Contributor Author

@shawwn shawwn May 25, 2017

Choose a reason for hiding this comment

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

Yes. Since we're generating endpoint=True, we need to pass in one more than the number of chunks. I.e. if nchunks is 1, we tell linspace to return two points (a start and an end).

@mbeacom mbeacom added the bug-fix For PRs and issues solving bugs. label May 25, 2017
@shawwn
Copy link
Contributor Author

shawwn commented May 25, 2017

As far as I can tell, that last commit fixes all remaining errors on my machine. I've been making heavy use of moviepy to generate spectrograms of a large number of videos, and I kept running into the IndexError problem. After these commits, I haven't seen any errors after processing ~30 videos.

@coveralls
Copy link

coveralls commented May 25, 2017

Coverage Status

Coverage increased (+0.1%) to 54.183% when pulling aec97bf on shawwn:fixes/indices-out-of-range into f63a985 on Zulko:master.

@shawwn
Copy link
Contributor Author

shawwn commented May 25, 2017

Ok, I've successfully stress-tested this PR by processing several hour-long videos without encountering any errors at all. I believe this PR is ready to merge.

The test suite seems to pass, but on my machine python3 setup.py test results in six errors like:

E           OSError: [Errno 32] Broken pipe
E           
E           MoviePy error: FFMPEG encountered the following error while writing file /tmp/crop1.webm:
E           
E            crop1TEMP_MPY_wvf_snd.ogg: No such file or directory

moviepy/video/io/ffmpeg_writer.py:178: OSError

I'm not quite sure why that's happening, but I don't think it's related to this PR.

If you have any questions or concerns, let me know.

@tburrows13
Copy link
Collaborator

@shawwn I don't think those errors are relevant. As long as it passes on Travis, I think it should be ok.

@shawwn
Copy link
Contributor Author

shawwn commented May 25, 2017

Ok, cool. In that case, this PR is ready to merge.

@mbeacom mbeacom added this to the Release v0.2.3.3 milestone May 25, 2017
@mbeacom mbeacom added review-needed PRs which need to be reviewed (further) by project maintainers. tests-needed PRs/code submissions which need test cases added to them. labels May 25, 2017
@shawwn
Copy link
Contributor Author

shawwn commented Jun 2, 2017

After heavy usage, I still haven't run into any out of bounds errors (or any other problems).

@stonebig
Copy link
Contributor

stonebig commented Aug 5, 2017

merging soon ?

@kevinmcalear
Copy link

Any idea when this can be merged? This would fix an issue that I'm currently dealing with.

@shawwn
Copy link
Contributor Author

shawwn commented Sep 27, 2017

@mbeacom I still haven't run into any out of bounds errors, and @stonebig @kevinmcalear have requested this be merged. Since this PR is now four months old, can it be accepted?

@shawwn
Copy link
Contributor Author

shawwn commented Sep 27, 2017

(Or @Zulko.)

@shawwn
Copy link
Contributor Author

shawwn commented Sep 29, 2017

@Zulko @mbeacom, could this PR be merged? Several users have asked for this, and after heavy usage I still haven't run into any out of bounds errors or any other problems.

I suspect this PR has languished because it changes the bounds checking. In general, it's good to be suspicious of doing this carelessly. But the math is correct now; the reason users have been running into errors is that the math was off in certain corner cases.

Another reason is probably that this seems like a magical, unexplained change in moviepy/audio/io/readers.py:

             try:
                  result = np.zeros((len(tt),self.nchannels))
                  indices = frames - self.buffer_startframe
 +                if len(self.buffer) < self.buffersize // 2:
 +                    indices = indices - (self.buffersize // 2 - len(self.buffer) + 1)
                  result[in_time] = self.buffer[indices]
                  return result

Again, it's good to be hesitant of changes like that, but FWIW I arrived at this solution by running moviepy on a video that breaks, then sat in pdb.trace() and looked at the actual data. The above snippet fixes the out of bounds errors.

If other users are running into out of bounds errors with moviepy and just need a fix, I suppose you can apply the above changes manually. But it'd be nice to get this merged.

@Zulko Zulko merged commit cd80d62 into Zulko:master Oct 5, 2017
@Zulko
Copy link
Owner

Zulko commented Oct 5, 2017

Sorry for the delay. I believe most maintainers are too busy right now to give this much thought so we'll have to trust you on this (I hope that was the right decision :) ).

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. review-needed PRs which need to be reviewed (further) by project maintainers. tests-needed PRs/code submissions which need test cases added to them.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unclosed processes.
7 participants