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

Lastrosade sc only #574

Merged
merged 20 commits into from Mar 2, 2022
Merged

Conversation

lastrosade
Copy link
Contributor

This pull request adds the --sc-only boolean argument which forces av1an to stop after scene detection as per #573

Right now it simply deletes the temporary directory after the run

What would be optimal would be to not create any temp dir if both:

  1. The input is a vpy script.
  2. -s is specified.

@redzic
Copy link
Collaborator

redzic commented Feb 16, 2022

It looks like the formatting check failed, run cargo fmt to fix the formatting and then push the commit

@lastrosade
Copy link
Contributor Author

Do I need to write unit tests?

@redzic
Copy link
Collaborator

redzic commented Feb 16, 2022

I'm not exactly sure how you would write a unit test for this, but we can add an integration test instead (which are in .github/workflows/tests.yml).

@redzic
Copy link
Collaborator

redzic commented Feb 16, 2022

Perhaps we should require -s to be specified if --sc-only is set?

@lastrosade
Copy link
Contributor Author

Yes, let me try to figure that out.

@redzic
Copy link
Collaborator

redzic commented Feb 16, 2022

@lastrosade The easiest way to do it would probably be with this method: https://docs.rs/clap/3.1.0/clap/struct.Arg.html#method.requires_if

@lastrosade
Copy link
Contributor Author

This is a train wreck

@lastrosade
Copy link
Contributor Author

lastrosade commented Feb 16, 2022

It seems that requires_if only wants strings, maybe I need requires_all

@lastrosade
Copy link
Contributor Author

lastrosade commented Feb 16, 2022

I figured it out, it was dumb and simple but for some reason I try to over complicate things.

@lastrosade
Copy link
Contributor Author

lastrosade commented Feb 16, 2022

The integration test failed with Error: Failed to parse scenes file "tt_sif.json", this likely means that the scenes file is corrupted I doubt this has anything to do with my patch.

Is the video generating an empty scene file?
I'll test this out.

@lastrosade
Copy link
Contributor Author

running the same test on the same file works fine for me.

@shssoichiro
Copy link
Collaborator

It seems the error is coming from this branch:

let (mut scenes, frames) = if (self.scenes.is_some() && scene_file.exists()) || self.resume {
  used_existing_cuts = true;
  crate::split::read_scenes_from_file(scene_file.as_ref())?
}

Which should only get hit if the scenes file already exists prior to running your test command. It shouldn't be... so this is a bit weird.

Just to prove we're not insane, could you try to rm tt_sif.json prior to calling av1an in your CI test?

@lastrosade
Copy link
Contributor Author

It did not fix it

Copy link
Owner

@master-of-zen master-of-zen left a comment

Choose a reason for hiding this comment

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

Looks good to me

@mergify mergify bot merged commit f5ec489 into master-of-zen:master Mar 2, 2022
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.

None yet

4 participants