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

added initial scaffolding for SaveImageSequence node #7

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Kosinkadink
Copy link
Owner

Hey, some of the remaining big features I think we need to support are:

  1. Saving image sequences in a subdirectory, with the option to use a formatted timestamp as the folder name, and allowing some customization for the saved image sequence (starting number, zero padding amount, etc).
  2. Allow saving videos in Combine Video node to also be put in a subdirectory.
  3. A node to copy a directory from output into input folder, so that workflows could seamlessly save and then load image sequences.

I want to spend tomorrow looking into some fixes/improvements to Advanced-ControlNet and AnimateDiff, so if you'd have the time to look into this, that would be amazing!

Fizz worked on an initial version of the node for AnimateDiff repo, so that might be a good place to look for the formatted timestamp impl: Kosinkadink/ComfyUI-AnimateDiff-Evolved#11

Kosinkadink and others added 5 commits September 29, 2023 01:04
Timestamp will be implemented in a later commit after being merged with
the fix for timestamp formatting.
A search_folder option has been added to Load Video to allow for video
files to be loaded directly from temp, or output directories

The date formatting support available on the Image save node has been
added to the Video Combine node

A typo in extension name, and several debugging lines have been cleaned
This also adds the client side logic for timestamp_directory

I had hoped to find a solution that would grey out, or hide
directory_name when timestamp_directory is checked, but this should be
sufficiently unambiguous
@AustinMroz
Copy link
Collaborator

I've implemented the SaveImageSequence node as true to your specs as possible.

I've got date formatting hooked up and able to output dates in year first format (yyyy-MM-ddThh:mm:ss / 2023‐09‐29T12:50:27). Currently, this extra parsing functionality is only to the Video Combine node and Save Image Sequence

I'm still pondering viability of a copy node. As a potential alternative, I've added a 'search_folder' option to Load Video to let it instead load videos from the output or temp directory.

As best I can tell folders written in the filename_prefix for the Combine Video node work with no changes needed. So the issue of output management seems to come down to a question of how to expose this information to the user. I've considered implementing a filename prefix node that can be connected to nodes like Video Combine or Save Image for guided configuration, but this is somewhat dependent on me finding a way to dynamically change the number of options on a node.

I've got a short list of things I that need work/testing:

  • Some inconsistencies in how nodes are marked as stale resulting in the graph reprocessing unnecessarily
  • Subdirectories not being not being included in input file lists
  • More js consolidations and cleanup
    but the bulk of the implementation is done and I'm probably going to prioritize looking into the existing issues

@Kosinkadink
Copy link
Owner Author

Awesome, I'll test later today/tomorrow and I can pick up from here

@Kosinkadink
Copy link
Owner Author

Hey, sorry I have not had a chance to check things out in VideoHelperSuite, I took a few days break from coding last weekend and then had my hands full with implementing HotshotXL into AnimateDiff and investigating some Mac and lowvram issues.

At some point this weekend I'll merge the PRs in and do some edits. I'll kinda use this to set some goals I think want for this repo:

  1. Avoid introducing complexity to improve maintainability and consistency - in the case of something like Load Videos node, since we are changing OpenCV to be the main code path, we should make it the only code path - no need to fallback on ffmpeg at all there. That way, if we introduce a new feature (e.g. I want to support "select_every_nth" input), we don't get into the problem of having to implement it twice in two completely different ways.
  2. Keep things in python code unless needed (like UI needing to be changed via javascript for something that 100% needs it). This is also on the same vibe as 1), but .js implementation should be Plan B instead of Plan A when implementing something, just to avoid falling into a hammer and nail situation.

@Kosinkadink
Copy link
Owner Author

What's the intended way to use the date format here?

@AustinMroz
Copy link
Collaborator

I agree with the mindset of both points. I shall keep them in mind in future contributions.

Regarding dates, the vanilla 'Save Image' node has a builtin plugin (web/extensions/core/saveImageExtraOutput.js) that will take a string like %date:yyyy-MM-ddThh:mm:ss% where everything between the colon and closing percent describes the desired format for the date. My best guess for the reasoning of this being done in javascript is so multiple nodes in a single workflow will have the same timestamp of when the workflow is queued instead of individual timestamps of when the node was executed. The existing plugin was hard coded to only use the saveImage node, so just the interesting part (date formatting) was reimplemented here.

This format is more complex than it should have to be for the end user (I was completely unaware that the vanilla functionality existed), so the save image sequence node also has a toggle that will lock directory_name to an appropriate format string.

My intent is to have parity with the builtin date formatting for those who are familiar (like fizzledorf) without requiring the average user understands esoteric format strings.

zhhezhhe pushed a commit to SeedV/ComfyUI-VideoHelperSuite that referenced this pull request Nov 30, 2023
Updated AnimateDiff Loader with proper handling of axes_factor (no longer exposed)
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.

2 participants