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.

Sign up for GitHub

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

Use with x.get_frame instead of x.get_frame globally #171

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

Use with x.get_frame instead of x.get_frame globally #171

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2d6227f
Use `with x.get_frame` instead of `x.get_frame` globally
LightArrowsEXE Feb 3, 2025
586b642
Copy props
LightArrowsEXE Feb 3, 2025
e6daf9b
Get key directly like previously
LightArrowsEXE Feb 3, 2025
b698ef0
get_var_infos: Call function again with frame
LightArrowsEXE Feb 3, 2025
598b737
Immediately close() get_frame(n) where applicable
LightArrowsEXE Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion vstools/enums/other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,8 @@ def from_clip(cls: type[SarSelf], clip: HoldsPropValueT) -> SarSelf:
if isinstance(clip, vs.RawFrame):
props = clip.props
elif isinstance(clip, vs.RawNode):
props = clip.get_frame(0).props
with clip.get_frame(0) as frame:
props = frame.props.copy()
else:
props = clip

Expand Down
3 changes: 2 additions & 1 deletion vstools/exceptions/color.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ def check(func: FuncExceptT, to_check: vs.VideoNode) -> None:
"""

try:
to_check.get_frame(0)
with to_check.get_frame(0):
pass
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, whoops. yeah, that's true

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

except vs.Error as e:
if 'no path between colorspaces' in str(e):
raise InvalidColorspacePathError(func, e)
Expand Down
3 changes: 2 additions & 1 deletion vstools/functions/heuristics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ def video_heuristics(
heuristics = dict[str, PropEnum]()

if props is True:
props_dict = clip.get_frame(0).props
with clip.get_frame(0) as frame:
props_dict = frame.props.copy()
else:
props_dict = props or None

Expand Down
3 changes: 2 additions & 1 deletion vstools/utils/info.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ def get_var_infos(frame: vs.VideoNode | vs.VideoFrame) -> tuple[vs.VideoFormat,
if isinstance(frame, vs.VideoNode) and not (
frame.width and frame.height and frame.format
):
frame = frame.get_frame(0)
with frame.get_frame(0) as frame:
pass
LightArrowsEXE marked this conversation as resolved.
Show resolved Hide resolved

assert frame.format

Expand Down
8 changes: 4 additions & 4 deletions vstools/utils/misc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,10 @@ def match_clip(
clip = clip.resize.Bicubic(format=ref.format.id, matrix=Matrix.from_video(ref))

if matrices:
ref_frame = ref.get_frame(0)
clip = clip.std.SetFrameProps(
_Matrix=Matrix(ref_frame), _Transfer=Transfer(ref_frame), _Primaries=Primaries(ref_frame)
)
with ref.get_frame(0) as ref_frame:
clip = clip.std.SetFrameProps(
_Matrix=Matrix(ref_frame), _Transfer=Transfer(ref_frame), _Primaries=Primaries(ref_frame)
)

return clip.std.AssumeFPS(fpsnum=ref.fps.numerator, fpsden=ref.fps.denominator)

Expand Down
5 changes: 3 additions & 2 deletions vstools/utils/props.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,16 @@ def get_prop(
if isinstance(obj, vs.RawFrame):
props = obj.props
elif isinstance(obj, vs.RawNode):
props = obj.get_frame(0).props
with obj.get_frame(0) as frame:
props = frame.props.copy()
else:
props = obj

prop: Any = MISSING

try:
try:
prop = props[key]
prop = props.get(key, MISSING)
except Exception:
if isinstance(key, type) and issubclass(key, PropEnum):
key = key.prop_key
Expand Down
6 changes: 4 additions & 2 deletions vstools/utils/vs_proxy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,10 +169,12 @@ def clear_cache() -> None:
try:
for output in get_outputs().values():
if isinstance(output, VideoOutputTuple):
output.clip.get_frame(0)
with output.clip.get_frame(0):
pass
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same try/finally idea?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing, though I found out this is apparantly valid

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

Copy link

@petzku petzku Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

break
except Exception:
core.std.BlankClip().get_frame(0)
with core.std.BlankClip().get_frame(0):
pass
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

core.max_cache_size = cache_size
except Exception:
...
Expand Down
Loading