Use with x.get_frame instead of x.get_frame globally

[LightArrowsEXE]

This should help prevent some minor memory leaks. Since it only happens upon init, the memory leaks ultimately shouldn't get worse over time (unless called in i.e. a frame eval). However, since a lot of functions call get_prop, this might negatively impact memory for scripts that depend on a lot of these functions.

[petzku]

The with blocks without any processing (just a pass) look weird, consider using explicit try/finally instead? I'm not fully sure what their purpose is in the first place, though.

[petzku]

Considering this is already inside a try block, it might be cleaner to use an explicit finally instead?

        try:
            to_check.get_frame(0)
        except vs.Error as e:
        # -- snip --
        finally:
            to_check.close()

[LightArrowsEXE]

This doesn't work as suggested. Works if I do something like this instead, though:

        try:
            frame = to_check.get_frame(0)
        except vs.Error as e:
            if 'no path between colorspaces' in str(e):
                raise InvalidColorspacePathError(func, e)
            raise
        finally:
            frame.close()

[petzku]

oh, whoops. yeah, that's true

[astiob]

Yes, close needs to be on the returned frame, not the clip.

But: the adjusted snippet fails if get_frame actually throws, because then the assignment never happens.

[petzku]

same try/finally idea?

[LightArrowsEXE]

Same thing, though I found out this is apparantly valid

                if isinstance(output, VideoOutputTuple):
                    output.clip.get_frame(0).close()
                    break

[petzku]

The (typical) problem with not using with or finally is that close() might not get called if an exception is raised. I'm not sure if this can happen with get_frame() alone, though.

[astiob]

There’s nothing between the get_frame call and the close call, so no opportunities for other exceptions. If get_frame throws, there’s nothing to be closed. If it succeeds, you immediately close it anyway.

[petzku]

Sounds good, get_frame().close() should be perfectly fine then

[astiob]

Ironically, I believe this is actually safer than a with or a try–finally with an assignment if the code is running in the main thread of command-line python, because those constructs have extra bytecode ops (for the __exit__ call, the frame= assignment, etc.), each of which is an additional opportunity for a KeyboardInterrupt to occur.

[petzku]

Not sure if applicable here. What even is the purpose of getting (and immediately discarding) a blank clip's first frame?

[LightArrowsEXE]

Not sure. Figuring out why and whether there's a better method is out of the scope of this PR though imo

[astiob]

This function modifies VapourSynth’s cache limit, so perhaps this is just an arbitrary VapourSynth call to trigger an actual cache update.

[astiob]

The with blocks without any processing (just a pass) look weird, consider using explicit try/finally instead?

No try needed btw; just clip.get_frame(0).close()

[jsaowji]

I don't think there is a memory leak here because a frame going out of scope is freed/closed automatically in its __dealloc__.
Copying the props is a good idea overall but even that at a glance looks like its not being kept around anywhere where it wouldn't be freed automatically.

[astiob]

I do see the code you mention in __dealloc__, but without the close calls, people are in fact seeing warnings like these upon exit:

Warning: Core freed but 63 filter instance(s) still exist
Warning: Core freed but 259850240 bytes still allocated in framebuffers
Warning: Core freed but 5 function instance(s) still exist

And personally, I feel I got an improvement in RAM usage when I replaced frames() calls with frames(close=True) in some batch-processing scripts I have that load and iterate over many clips in sequence, although admittedly, I didn’t rigorously measure this.

So I suspect the __dealloc__ doesn’t actually trigger or work!

[astiob]

…although apparently those warnings are present even with the close calls. Probably a red herring, then. Sorry.

Still, explicitly closing resources seems like good hygiene.