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 dropped frames calculation #15

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

beauwright
Copy link

The drop_frame_adjustment function could return a result that was 1-3 frames off. Took a stab at fixing it based on David Heidelberger's pseudocode and it seems to work correctly now.

@beauwright beauwright changed the base branch from main to dev May 26, 2024 20:41
@peake100
Copy link
Contributor

Thank you for the PR!

It looks like there are a lot of unrelated changes due to your work being based on main instead of dev. main has a lot of extra files added by the release process in CI.

Would you mind making your changes to the dev branch of your fork instead, then make a new PR targeting this repo's dev?

Thanks!

@peake100
Copy link
Contributor

peake100 commented May 27, 2024

It also looks like you are getting some clippy warnings in CI.

You can run make lint to run all my linters locally

@peake100
Copy link
Contributor

Please also add some tests to assert that the bad cases have been fixed to prevent regressions!

Ideally we would also assert that the internal time representation equals the expected representation for the equivalent non-drop values.

@peake100 peake100 self-assigned this May 27, 2024
@peake100 peake100 added the bug Something isn't working label May 27, 2024
@peake100 peake100 linked an issue May 27, 2024 that may be closed by this pull request
Copy link
Contributor

@peake100 peake100 left a comment

Choose a reason for hiding this comment

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

.

@beauwright
Copy link
Author

Understood, I'll update the PR with the change on the dev branch, Clippy warning fixed, and tests added in the next day or two 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parsing for drop frame timecodes can be incorrect
3 participants