RFC Discussion: Rfc/core video nodes

[github-actions[bot]]

This is the discussion thread for RFC PR #28.

Please provide feedback and discuss the RFC here rather than in the PR comments.

PR Link: #28

[Immac]

The bot messed up the link to the proposal [Full Rendered Proposal](https://github.com/Comfy-Org/rfcs/blob/rfc/core-video-nodes/rfcs/####-Core Video Nodes.md), should I rename the file and push an update?

[christian-byrne]

I fixed it 👍

[AustinMroz]

As the primary developer on VHS, I desire greater ease of video I/O for end users. Naturally, I support the addition of Core Video Nodes. However, this level of granularity does not seem well aligned with making things easier for users. While exposing options is good,

• The average user, even if well versed in the functions of codecs, will be frustrated with needing to drag in and organize 8 different nodes to round trip a video.
  • At a minimum, I would suggest having "Load Video" be a simple node like SaveWebm, and making a separate node like "Load Container". There is precedence for having advanced and simple versions of nodes with KSampler.
• Dividing nodes like this makes it exceedingly difficult to produce previews accurate to load options later down the line. If a container has multiple video streams, will it ever be possible to preview the one a user selects?

Also, I strongly encourage discussion on whether the start of a video should be determined with frames or seconds. I greatly regret choosing to use frame counts in VHS and strongly wish for such a decision with core nodes to be well thought out. Using timestamps is both more intuitive to end users and offers substantially improved performance when working with long videos.

[Immac]

As the primary developer on VHS, I desire greater ease of video I/O for end users. Naturally, I support the addition of Core Video Nodes. However, this level of granularity does not seem well aligned with making things easier for users. While exposing options is good,

Thank you for your response, I'd like to address that the granularity here is precisely to enable users, workflow designers and custom node developers achieve more by abstracting into nodes some of the more gritty parts of video creation. If needed an "Easy Mode" version of the nodes is easier to create down the road already having the more granular nodes accounted for.

• The average user, even if well versed in the functions of codecs, will be frustrated with needing to drag in and organize 8 different nodes to round trip a video.

This is already the case with any ComfyUI workflow already, also, things like Codec selection can be simplified into "combo widgets" with default parameters while still giving the flexibility of having dedicated nodes for each Codec and its options.

• At a minimum, I would suggest having "Load Video" be a simple node like SaveWebm, and making a separate node like "Load Container". There is precedence for having advanced and simple versions of nodes with KSampler.

Advanced and Simple versions of these nodes would certainly be great to have, as an addendum to the KSampler example I'd like to point out Sampler Custom and Sampler Custom Advanced, as examples of granularity in ComfyUI.

• Dividing nodes like this makes it exceedingly difficult to produce previews accurate to load options later down the line. If a container has multiple video streams, will it ever be possible to preview the one a user selects?

While I understand the concern, this would probably best be handled with a Preview Stream/Video node that has to be run, rather than automatically getting the preview, just like with most image operations, you don't get to preview what happens unless you run it.

Also, I strongly encourage discussion on whether the start of a video should be determined with frames or seconds. I greatly regret choosing to use frame counts in VHS and strongly wish for such a decision with core nodes to be well thought out. Using timestamps is both more intuitive to end users and offers substantially improved performance when working with long videos.

Can you elaborate more on this point? If we are to do processes on the frames themselves, we would usually decode the video and act upon the frames, if you need to change a Stream's length in time instead you do it directly to the stream? On this I'd like more explanation as I just started to learn the ins and outs of video manipulation at a low level this weekend, I still have a lot to learn.

[AustinMroz]

Can you elaborate more on this point? If we are to do processes on the frames themselves, we would usually decode the video and act upon the frames, if you need to change a Stream's length in time instead you do it directly to the stream? On this I'd like more explanation as I just started to learn the ins and outs of video manipulation at a low level this weekend, I still have a lot to learn.

Decoding frames is very slow. As an example of pulling a single frame 7 minutes (12600 frames) into a 1920x1080@30fps video on my system:

• PyAV naive: 51.64 seconds
• PyAV packet count: 0.74 seconds
• PyAV seek: 0.17 seconds
• VHS Load Video: 15.86 seconds
• VHS Load Video FFmpeg: 1.05 seconds

From doing actual profiling, packet counting in PyAV is faster than I expected. I expected a time far closer to the opencv equivalent used in the VHS Load Video node.

Example PyAV load methods for profiling

import av
import torch

def convert_frame(frame):
    tensor = torch.from_numpy(frame.to_rgb().to_ndarray()) \
            .to(torch.float32)
    tensor.div_(255)
    return tensor,

class LoadAV:
    @classmethod
    def INPUT_TYPES(s):
        return {"required": { "video": ("STRING", {"default": "input/bigbuckbunny.mp4"}),
                             "skip_first_frames": ("INT", {"min": 0, "max": 2**53-1,
                                                           "default": 12600})}}
    FUNCTION = "decode"
    #OUTPUT_NODE = True #For tighter profiling
    RETURN_TYPES = ("IMAGE",)
    def decode(self, video, skip_first_frames):
        cont = av.open(video)
        i = 0
        for frame in cont.decode(video=0):
            i += 1
            if i > skip_first_frames:
                return convert_frame(frame),
        raise RuntimeError("not enough frames")
class LoadAVPacket(LoadAV):
    def decode(self, video, skip_first_frames):
        cont = av.open(video)
        i = 0
        for packet in cont.demux(video=0):
            i += 1
            if packet.is_keyframe:
                last_keyframe = packet.pts, i
            if i > skip_first_frames:
                break
        else:
            raise RuntimeError("not enough frames")
        cont.seek(last_keyframe[0], stream=packet.stream)
        fit = cont.decode(video=0)
        for i in range(last_keyframe[1]-1, skip_first_frames):
            next(fit)
        return convert_frame(next(fit)),
class LoadAVSeek(LoadAV):
    def decode(self, video, skip_first_frames):
        cont = av.open(video)
        stream = cont.streams.video[0]
        rate = float(stream.average_rate)
        pts = int((skip_first_frames+1) / (rate * stream.time_base))
        cont.seek(pts, stream=stream)
        for frame in cont.decode(video=0):
            if frame.pts >= pts:
                return convert_frame(frame),
        raise RuntimeError("not enough frames")

NODE_CLASS_MAPPINGS = {
        "LoadAV": LoadAV,
        "LoadAVPacket": LoadAVPacket,
        "LoadAVSeek": LoadAVSeek,
}

It also bears repeating that decoded video has a very heavy memory footprint. A single blue noodle to pass 10 seconds of 1920x1080@30fps video is ~7.5 GB of memory. Memory optimizations are crucial when working with video and trying to do things the Comfy way,