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

[cli] Move output command state out of CliContext #426

Merged
merged 3 commits into from
Oct 6, 2024

Conversation

Breakthrough
Copy link
Owner

@Breakthrough Breakthrough commented Sep 28, 2024

This PR makes it a lot easier to integrate new output commands into the CLI. The goal is to have the controller treat all output commands the same by operating on the result of the processing pipeline. For dependent jobs, they can still use the shared context to communicate results. This has a few benefits:

  • much easier to add new commands that operate on the result of scene detection and far less code
  • can now specify output commands multiple times, e.g. can run split-video twice to generate output videos with different parameters
  • logging parameters is more consistent now with --verbosity debug

This should also help reduce the friction to adding new output formats, supporting #156, #323, and #388. As part of this change, some log statements will now be emitted later when a command is actually running. Previously, some commands would emit things like the location of saved files when processing the command-line arguments, rather than when the sub-command was about to run.

@Breakthrough Breakthrough added this to the 0.6.5 milestone Sep 28, 2024
To simplify things, all output commands can be treated similarily
as they all act on the result of the processing pipeline. This
allows new commands to be added without needing to explicitly define
their values in CliContext, and makes the values that do remain there
much more meaningful.

It also allows commands to be specified multiple times gracefully
and with different options, so for example `save-images` can now be
run twice with different encoding parameters.
@wjs018
Copy link
Collaborator

wjs018 commented Oct 5, 2024

Alright, I have had a good look through this and I think I have wrapped my head around it. I don't use the cli too often, so I always need to refresh my memory on how it all works. If I understand what this is doing correctly, then it works like this under this paradigm:

  1. scenedetect/__main__.py calls run_scenedetect from scenedetect/_cli/controller.py
  2. The cli is parsed by scenedetect/_cli/__init__.py and any commands that are output commands are simply added to context.commands with named arguments for later use
  3. The run_scenedetect function then proceeds to do the detection (or loads the scenes)
  4. Within run_scenedetect the _detect function returns the detected scenes and cut list
  5. The list of context.commands are then looped through with the scenes and cut list passed to each command

I think this makes sense and does make it easier to add additional output formats in the future. To add a new output format, I think the changes that would need to be made would be:

  • The cli command would need to be added to scenedetect/_cli/__init__.py, adding the command to context.commands with the proper args
  • The logic of the output processing/formatting would need to be defined in scenedetect/_cli/commands.py. This could be pretty a pretty barebones reference to a separate file that does the heavy lifing (like what is done for export-html or save-images)

@Breakthrough
Copy link
Owner Author

Though still WIP, feel free to check out #427 for a concrete example of how this simplifies adding new CLI commands.

@Breakthrough Breakthrough merged commit 81e6a56 into main Oct 6, 2024
36 checks passed
@Breakthrough Breakthrough deleted the output-pipeline branch October 20, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants