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 fx.freeze frame indexing. Fix #1307. #1348

Closed
wants to merge 4 commits into from

Conversation

jkolbly
Copy link
Contributor

@jkolbly jkolbly commented Oct 11, 2020

This fixes issue #1307 by making the fx.freeze use the last timestamp instead of 1 second before the last timestamp.
To prevent out-of-bounds errors, the default make_frame for BitmapClip and DataVideoClip have been updated to take the last frame in case the timestamp is greater than the length of the clip, as it makes the most sense for the timestamp equal to the duration of the clip to be the last frame, instead of 1 frame past the end of the clip.
The test test_freeze has been updated to reflect this change, as it had previously assumed a conversion of timestamps to frames that was inconsistent. Basically, it went from assuming that the second frame was from 1≤t≤2, which is illogical, to 1≤t<2, which can be consistent across the entire 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

@coveralls
Copy link

coveralls commented Oct 11, 2020

Coverage Status

Coverage decreased (-0.02%) to 67.438% when pulling 4dfc349 on ToxicOverload:fix_freeze_indexing into b2218d3 on Zulko:master.

@tburrows13
Copy link
Collaborator

tburrows13 commented Oct 11, 2020

I don't have time to properly review this right now, but it looks like it is a good change. You do need to run black -t py36 . on it to pass the code format check though.

@tburrows13 tburrows13 added the hacktoberfest-accepted For Hacktoberfest PRs. See https://hacktoberfest.digitalocean.com/faq label Oct 11, 2020
@jkolbly
Copy link
Contributor Author

jkolbly commented Oct 11, 2020

My bad! I thought VSCode was doing that for me, but I forgot I had disabled format on save earlier. Should be fixed now

@tburrows13 tburrows13 added the bug-fix For PRs and issues solving bugs. label Oct 17, 2020
@mondeja mondeja added the needs-more-info Waiting for submitter's reply, feedback, updates,... label Jan 16, 2021
@mondeja mondeja removed the needs-more-info Waiting for submitter's reply, feedback, updates,... label Jan 23, 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. hacktoberfest-accepted For Hacktoberfest PRs. See https://hacktoberfest.digitalocean.com/faq
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants