Adds SwarmVideoResampleFPS; resamples controlnet preview videos#1387
Conversation
|
|
||
| class SwarmVideoResampleFPS(io.ComfyNode): | ||
| MIN_FPS: float = 1.0 | ||
| MAX_FPS: float = 120.0 |
There was a problem hiding this comment.
general rule for comfy inputs range limits is go way wider than seems sane. Never know when somebody's gonna complain about the lat of support for their 640 fps input video.
There was a problem hiding this comment.
Copied from the CreateVideo node. I'll bump it to 1000? Doesn't seem like it would be valid down the chain though.
edit: ah, for input. For output the CreateVideo node would max out at 120fps.
Do you want a 1000 max input, and 120 max output?
There was a problem hiding this comment.
I want this node to not be the node in the chain that breaks things and need to be edited to accommodate.
| description="Resample a video from fps_in to fps_out while preserving total duration.", | ||
| inputs=[ | ||
| io.Image.Input( | ||
| "images", |
There was a problem hiding this comment.
very unnecessary linesplits
There was a problem hiding this comment.
Changed, but imho split lines for more than 2 parameters is vastly more readable than having single lines except for something that should go in a single line:
io.Image.Input("images", tooltip="The images to resample."),
io.Float.Input("fps_in", min=cls.MIN_FPS, max=cls.MAX_FPS, step=cls.STEP_FPS, tooltip="Source frame rate."),
io.Float.Input("fps_out", default=cls.DEFAULT_FPS_OUT, min=cls.MIN_FPS, max=cls.MAX_FPS, step=cls.STEP_FPS, tooltip="Target frame rate."),
io.Combo.Input("method", options=[cls.METHOD_LINEAR, cls.METHOD_NEAREST], default=cls.METHOD_LINEAR,
tooltip=(
"linear: each output frame is a linear blend of the two source frames bracketing its timestamp. "
"Equivalent to ffmpeg's framerate filter. Slightly more expensive; avoids the duplicated-frame artifact. "
"See https://ffmpeg.org/ffmpeg-filters.html#framerate\n"
"nearest: each output frame is the source frame closest in time. "
"Equivalent to ffmpeg's fps filter. Cheap; can produce visible judder on pans. "
"See https://ffmpeg.org/ffmpeg-filters.html#fps-1"
),
),There was a problem hiding this comment.
writing(
like,
this,
is
awful
)
and I can't understand why you'd do it. It's significantly less readable to my eyes.
io.Image.Input("images", tooltip="The images to resample."), is "Here's an input, it's images type with this tooltip text", perfect and simple
whereas the multiline split goes into the mode of "here's a wall of unstructured data, skip it unless/until you're willing to spend 5 minutes parsing a wall of data"
| return io.NodeOutput(resampled, float(fps_out)) | ||
|
|
||
| @classmethod | ||
| def _source_positions(cls, frame_count_out: int, fps_in: float, fps_out: float, device: torch.device) -> torch.Tensor: |
There was a problem hiding this comment.
there's more function body here than code, for a function used only once. This is "looks proper" code that's actually quite messy.
There was a problem hiding this comment.
I'm going to ask you to reconsider this specific comment, but changing it pending.
_source_positions() could be boiled down to a single line:
return torch.arange(frame_count_out, dtype=torch.float64, device=device) / fps_out * fps_inbut then anyone not familiar with this functionality will read this as a foreign language. Yes it makes sense to you and I, but I'm going to take a minute or two in a year to understand what's going on. Anyone new to this whole world of 1girl will not understand what this line is actually doing.
There was a problem hiding this comment.
Nobody who is lacking the tools to decipher what source_positions means or what a torch.arange is going to understand either version of this code, before or after.
You can leave a # comment explaining in one line what it is, maybe an example of inputs -> outputs, that much can help clarify.
| resampled = cls._sample_linear(images, source_positions) | ||
|
|
||
| logger.info( | ||
| "SwarmVideoResampleFPS: %d frames @ %s fps -> %d frames @ %s fps (%s)", |
There was a problem hiding this comment.
why would you do this in a world where f strings exist
There was a problem hiding this comment.
Force of habit from enterprise logging libs 👍
| class SwarmVideoResampleFPS(io.ComfyNode): | ||
| MIN_FPS: float = 1.0 | ||
| MAX_FPS_IN: float = 1000.0 | ||
| MAX_FPS_OUT: float = 120.0 |
There was a problem hiding this comment.
this split is silly, just allow [0.01, 99999] there's no reason to have strict range limits anywhere here except creating future edge case bugs to fix
| "Equivalent to ffmpeg's framerate filter. Slightly more expensive; avoids the duplicated-frame artifact. " | ||
| "See https://ffmpeg.org/ffmpeg-filters.html#framerate\n" | ||
| "nearest: each output frame is the source frame closest in time. " | ||
| "Equivalent to ffmpeg's fps filter. Cheap; can produce visible judder on pans. " |
| ], | ||
| outputs=[ | ||
| io.Image.Output("images"), | ||
| io.Float.Output("fps"), |
| { | ||
| ["video"] = NodePath(result, 0) | ||
| }); | ||
| NodeHelpers["video_components_split"] = splitNode; |
There was a problem hiding this comment.
This method of forwarding data is liable to get mixed up from a different load call in the same workflowgen
There was a problem hiding this comment.
Should I add a new property to WGNodeData? SourceFPSNode or similar?
There was a problem hiding this comment.
Probably change public int? FPS = null; to a JToken that can either be a raw value or a node path ref
| ), | ||
| ), | ||
| ], | ||
| outputs=[io.Image.Output("images"), io.Float.Output("fps")], |
There was a problem hiding this comment.
second out is redundant, just returns an input
| ["method"] = "linear" | ||
| }); | ||
| imageNodeActual = imageNodeActual.WithPath([resampleNode, 0]); | ||
| imageNodeActual.FPS = 24; |
| ["scale_method"] = "lanczos" | ||
| }); | ||
| imageNodeActual = imageNodeActual.WithPath([multipleOf8, 0]); | ||
| if (imageNodeActual.DataType == WGNodeData.DT_VIDEO && imageNodeActual.FPS is not null) |
There was a problem hiding this comment.
wrong order of operations, this should be before preproc
| g.CurrentMedia = g.CurrentMedia.WithPath(interp); | ||
| videoFps *= mult; | ||
| g.CurrentMedia.FPS = videoFps; | ||
| g.CurrentMedia.FPS = videoFps.HasValue ? videoFps.Value : null; |
Adds new Comfy node
SwarmVideoResampleFPS, which allows resampling videos FPS to a target FPS.