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

Add start frame and frames option to video comparison #45

Merged
merged 8 commits into from
Dec 3, 2024

Conversation

superyu1337
Copy link
Contributor

@superyu1337 superyu1337 commented Dec 1, 2024

Adds two new arguments:

  • -s (--start) sets the frame to start the comparison process at. 0 is the first frame.
    • Renamed: --skip-frames sets the amount of frames to skip.
  • --frames The amount of frames (in total) to compare.

Also adjusts the progress bar to only count the amount of frames that get computed.

Todo

  • Fix compatibility with large --increment arg values
  • Clippy errors

Closes #19

@FreezyLemon
Copy link
Contributor

Maybe rename -s to --skip or --skip-frames? I think it's more intuitive and avoids the zero- vs one-based indexing thing.

@superyu1337
Copy link
Contributor Author

Should --skip-frames be an Option<usize> internally or should I leave it at a usize with 0 as default?

@FreezyLemon
Copy link
Contributor

Simple usize seems fine. skip_frames = 0 should be easy to understand

@superyu1337 superyu1337 marked this pull request as ready for review December 2, 2024 08:48
@superyu1337
Copy link
Contributor Author

Well, it's ready to review then.
The changes heavily adjust the logic used to skip frames with --intermediate. I can't count btw, so someone gotta sanity check this. But from what I could tell it works fine.

Copy link
Contributor

@FreezyLemon FreezyLemon left a comment

Choose a reason for hiding this comment

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

I think there's a logic error hiding there w.r.t. end_frame. And forgive me for being strict, but I don't like calc_score becoming so complex if we can avoid it.

src/video.rs Outdated Show resolved Hide resolved
src/video.rs Outdated Show resolved Hide resolved
src/video.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@FreezyLemon FreezyLemon left a comment

Choose a reason for hiding this comment

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

Tested it a bit and everything works fine. One nitpick though

src/video.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@FreezyLemon FreezyLemon 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!

@FreezyLemon FreezyLemon merged commit 45b492e into rust-av:main Dec 3, 2024
2 checks passed
@superyu1337 superyu1337 deleted the frame-range branch December 4, 2024 09:09
@FreezyLemon
Copy link
Contributor

@shssoichiro I forgot to squash this when merging. Not sure if it bothers you, if it does we can squash it manually on main

@shssoichiro
Copy link
Contributor

No biggie. I'd rather keep it unsquashed than deal with rewriting history in main. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Request] Frame range option
3 participants