Faster rfs implementation #60

End-of-Eternity · 2021-07-13T12:19:43Z

I haven't tested this yet, but creating PR for review.

DJATOM · 2021-07-13T12:46:58Z

Do you have some numbers on that? Theoretically it produces less Trim invocations, but without redundant caching it's not an issue (I believe things will be improved with coming doodle1 update). I think cpp realization is the fastest (comparing initialization time), but maybe not so convenient (no 2D arrays allowed for plugins, etc).

End-of-Eternity · 2021-07-13T12:52:09Z

@DJATOM Not any hard numbers yet. I was having issues earlier with the tonnes of splicing that lvf.rfs used originally, which initialises hundreds, if not thousands of Trim's and Splice's. The frame request dependancy chain there seems to be pretty devastating with large numbers of ranges. You're probably right that the plugin version is faster (I didn't know that existed when I came up with this idea), though in testing this function seems pretty damn fast - and given the convinience, I think that this is fine really.

End-of-Eternity · 2021-07-13T12:53:06Z

Note: I do also need to add some checks in for OOB frame and clip accesses within select_frames

Doesn't satisfy mypy - can fix, but adds excess complexity that I disagree with. There's probably a cleverer fix that doesn't do this, but I don't really know mypy, and I'm also too tired for this nonsense

* Add select_frames function borrowed from Jaded-Encoding-Thaumaturgy/lvsfunc#60 * replace_ranges and adjust_clip_frames use it * Add Numpy dependency

End-of-Eternity · 2021-08-13T15:46:03Z

Closing this in favour of the vardefunc commit above

End-of-Eternity · 2021-08-13T16:00:40Z

Unclosing since it turns out people are still interested.

tomato39 · 2021-08-14T05:12:59Z

lvsfunc/util.py

+    if len(clips) > 1 and not all(clip.format == clips[0].format for clip in clips[1:]):
+        raise ValueError("All input clips must be of the same format.")
+
+    def select_frames_func(n: int, clips: List[vs.VideoNode], indicies: List[Tuple[int, int]]) -> vs.VideoNode:


this doesn't need the clips and indices arguments, the nested function has access to the parent function's scope

Of course - I missed this as it was originally written as a seperate function.

tomato39 · 2021-08-14T05:13:23Z

lvsfunc/util.py

@@ -1,7 +1,8 @@
 """
    Helper functions for the main functions in the script.
 """
-from typing import Any, Callable, List, Optional, Sequence, Type, TypeVar, Tuple, Union
+from functools import partial
+from typing import Any, Callable, List, Optional, Sequence, Type, TypeVar, Tuple, Union, cast


cast imported but unused

tomato39 · 2021-08-14T05:13:30Z

lvsfunc/util.py

@@ -1,7 +1,8 @@
 """
    Helper functions for the main functions in the script.
 """
-from typing import Any, Callable, List, Optional, Sequence, Type, TypeVar, Tuple, Union
+from functools import partial


partial can be removed, see below

tomato39 · 2021-08-14T05:14:10Z

lvsfunc/util.py

+    Select frames from one or more clips at specified indices.
+
+    :param clips:   A clip or a list of clips to select the frames from
+    :param indexes: Indices of frames to select.


you want indices, not indicies or indexes (misspelled throughout the function)

Writing code while drunk is a bad idea

tomato39 · 2021-08-14T05:15:35Z

lvsfunc/util.py

+def select_frames(clips: Union[vs.VideoNode, List[vs.VideoNode]],
+                  indicies: Union[List[int], List[Tuple[int, int]]]) -> vs.VideoNode:
+    """
+    Select frames from one or more clips at specified indices.


Might be more understandable to say something like Builds a clip from a list of clips and frame numbers

Yeah I intended for this to be rewritten before merging. I still don't really like the description here, but I'm unsure how to nicely describe what this does in a short sentence.

tomato39 · 2021-08-14T05:17:43Z

lvsfunc/util.py

@@ -102,6 +103,45 @@ def get_prop(frame: vs.VideoFrame, key: str, t: Type[T]) -> T:
    return prop


+def select_frames(clips: Union[vs.VideoNode, List[vs.VideoNode]],
+                  indicies: Union[List[int], List[Tuple[int, int]]]) -> vs.VideoNode:


i'm not really sure what the point of unioning indices with List[int] is, it does make the function a bit more ergonomic but in reality indices should almost always be generated not directly user-entered

I disagree with this point - The function is not intended to be called by the end user, however the flexibility of the function is part of the point of it's design. Why be less flexible if I don't have to? In any case, rfs ect. are intended to be the external functions, but if the user wishes to do their own thing then they can. If you have a better idea of what types these arguments could be feel free to suggest, though I think that writing even dataclasses for this would be over the top for something so simple.

tomato39 · 2021-08-14T05:24:02Z

lvsfunc/util.py

+    :return:        The selected frames in a single clip
+    """
+    clips = [clips] if isinstance(clips, vs.VideoNode) else clips
+    for pos, index in enumerate(indicies):


removing the union removes the issues with this code, otherwise

for index in indices: if isinstance(index, int): clip_idx, frame_idx = (0, index) else: clip_idx, frame_idx = index

then

if isinstance(index, int): clip_idx, frame_idx = (0, indices[n]) else: clip_idx, frame_idx = indices[n] return clips[clip_idx][frame_idx]

in the frameeval

I would rather not have any more logic than is strictly necessary within the frame eval. However I agree that expanding this into multiple lines would help with both readability and mypy.

that makes sense for VS filters, but doing a typecheck and conditional tuple construction is virtually free in the context of video processing

tomato39 · 2021-08-14T05:24:18Z

lvsfunc/util.py

+    clips = [clips] if isinstance(clips, vs.VideoNode) else clips
+    for pos, index in enumerate(indicies):
+        if isinstance(index, int):
+            indicies[pos] = (0, index)


in addition to the mypy issues, this is bad because mutating user input is bad

Whilst mutating the userinput is probably not the best idea, I still don't like mypy telling me what I can and can't do >:(
Creating a copy of the list would probably a solution

honestly i'd just throw this entire logic into the frameeval, it's not going to impact performance at all

tomato39 · 2021-08-14T05:27:36Z

lvsfunc/util.py

@@ -174,19 +214,20 @@ def replace_ranges(clip_a: vs.VideoNode,
    if ranges is None:
        return clip_a

-    out = clip_a
+    out_indicies = list(zip([0] * clip_a.num_frames, range(clip_a.num_frames)))
+    clip_b_indices = list(zip([1] * clip_b.num_frames, range(clip_b.num_frames)))


probably better to just generate the indices we need for clip_b inside the loop instead of slicing a precomputed list

Inside the loop meaning inside the frameeval? Not sure I understand this point.

tomato39 · 2021-08-14T05:32:20Z

lvsfunc/util.py

@@ -174,19 +214,20 @@ def replace_ranges(clip_a: vs.VideoNode,
    if ranges is None:
        return clip_a

-    out = clip_a
+    out_indicies = list(zip([0] * clip_a.num_frames, range(clip_a.num_frames)))
+    clip_b_indices = list(zip([1] * clip_b.num_frames, range(clip_b.num_frames)))

    nranges = normalize_ranges(clip_b, ranges)

    for start, end in nranges:


since we're now using a normal List instead of a VideoNode, we can just do slice assignment

for start, end in nranges: out_indices[start:end+1] = list(zip([1] * (end + 1 - start), range(start, end+1)))

Yup you've lost me now, please post the full block that this is relevant to so I can better understand.

out_indicies = [(0, i) for i in range(clip_a.num_frames)] for start, end in nranges: out_indices[start:end+1] = [(1, i) for i in range(start, end+1)]

can replace the entire for loop

End-of-Eternity · 2021-08-21T21:02:14Z

Apologies for the delay in replying here - I've been busy being dead/on holiday. If I miss any further comments please ping me in IEW or drop me a dm.

With regards to the FrameEval vs ModifyFrame discussion - calling get_frame within a ModifyFrame is not a good idea, however there is an overhead (if minor) to using FrameEval. I have had an idea on how to merge the best of both of these into one plugin, which I intend to have an implementation for finished within a week or so. For this function though, the difference between the two is minimal enough that it can be safely ignored.

End-of-Eternity · 2021-08-21T21:03:08Z

Oh and I will make some changes over the following days. I am on holiday though so may be slow, as I do not have easy access to a proper environment to work in.

LightArrowsEXE · 2022-02-21T23:54:54Z

As this PR hasn't seen any movement in over half a year, I'm closing it. I'll see if I can update rfs at some point to use faster implementations.

End-of-Eternity added 3 commits July 13, 2021 13:08

faster rfs implementation without splicing

6dac178

improve docstring

46bc9ab

consistant use of 'indicies' and a change from ModifyFrame to FrameEval

bb2a8f7

End-of-Eternity added 2 commits July 13, 2021 14:15

fix some typing stuff within select_frames_func

c5edb72

add OOB checks

270b039

Doesn't satisfy mypy - can fix, but adds excess complexity that I disagree with. There's probably a cleverer fix that doesn't do this, but I don't really know mypy, and I'm also too tired for this nonsense

Ichunjo added a commit to Ichunjo/vardefunc that referenced this pull request Aug 9, 2021

rfs overhaul

fd9989d

* Add select_frames function borrowed from Jaded-Encoding-Thaumaturgy/lvsfunc#60 * replace_ranges and adjust_clip_frames use it * Add Numpy dependency

End-of-Eternity closed this Aug 13, 2021

End-of-Eternity deleted the fast_rfs branch August 13, 2021 15:46

End-of-Eternity restored the fast_rfs branch August 13, 2021 16:00

End-of-Eternity reopened this Aug 13, 2021

tomato39 requested changes Aug 14, 2021

View reviewed changes

LightArrowsEXE mentioned this pull request Sep 28, 2021

Checklist for v0.4.0 #75

Closed

14 tasks

LightArrowsEXE linked an issue Oct 12, 2021 that may be closed by this pull request

Checklist for v0.4.0 #75

Closed

14 tasks

LightArrowsEXE closed this Feb 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster rfs implementation #60

Faster rfs implementation #60

End-of-Eternity commented Jul 13, 2021

DJATOM commented Jul 13, 2021

End-of-Eternity commented Jul 13, 2021

End-of-Eternity commented Jul 13, 2021

End-of-Eternity commented Aug 13, 2021

End-of-Eternity commented Aug 13, 2021

tomato39 Aug 14, 2021

End-of-Eternity Aug 20, 2021

tomato39 Aug 14, 2021

End-of-Eternity Aug 20, 2021

tomato39 Aug 14, 2021

tomato39 Aug 14, 2021

End-of-Eternity Aug 20, 2021

tomato39 Aug 14, 2021

End-of-Eternity Aug 20, 2021

tomato39 Aug 14, 2021

End-of-Eternity Aug 20, 2021

tomato39 Aug 14, 2021

End-of-Eternity Aug 21, 2021

tomato39 Aug 21, 2021

tomato39 Aug 14, 2021

End-of-Eternity Aug 21, 2021

tomato39 Aug 21, 2021

tomato39 Aug 14, 2021

End-of-Eternity Aug 21, 2021

tomato39 Aug 14, 2021 •

edited

End-of-Eternity Aug 21, 2021

tomato39 Aug 21, 2021

End-of-Eternity commented Aug 21, 2021

End-of-Eternity commented Aug 21, 2021

LightArrowsEXE commented Feb 21, 2022

Faster rfs implementation #60

Faster rfs implementation #60

Conversation

End-of-Eternity commented Jul 13, 2021

DJATOM commented Jul 13, 2021

End-of-Eternity commented Jul 13, 2021

End-of-Eternity commented Jul 13, 2021

End-of-Eternity commented Aug 13, 2021

End-of-Eternity commented Aug 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomato39 Aug 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

End-of-Eternity commented Aug 21, 2021

End-of-Eternity commented Aug 21, 2021

LightArrowsEXE commented Feb 21, 2022

tomato39 Aug 14, 2021 •

edited