Timeline: Add clip/effect lookup api, GetMaxFrame/GetMaxTime method (w/ unit tests)

[ferdnyc]

This PR adds two things to Timeline, primarily intended for use by OpenShot:

1. A lookup API to retrieve Clips, timeline Effects (transitions), and Clip Effects singly by Id string lookup
2. A GetMaxTime() that computes the time the last clip/effect ends and returns it as a double whole-and-partial-seconds value, and a GetMaxFrame() that returns the same thing accounting for frame rate as an int64_t frame number.

Full unit tests are included for Timeline::GetMaxFrame()/Timeline::GetMaxTime(), Timeline::GetClip(), Timeline::GetTimelineEffect(), and Timeline::GetClipEffect(). The Clip::GetEffect() implementation isn't unit-tested directly, but it is used by Timeline::GetClipEffect() so it's tested indirectly.

Along the way, I also deprecated ReaderBase::GetClip and ReaderBase::SetClip, because I wanted to reuse one of the names and anyway those names are awful and confusing. They're now called GetParentClip and SetParentClip. The few calls in the libopenshot source are updated. The old names are still available as deprecated aliases, for now.

Motivation

I noticed that there are a lot of places OpenShot does stuff like this, from video_widget.py:

        # Get new clip for transform
        if clip_id:
            self.transforming_clip = Clip.get(id=clip_id)

            if self.transforming_clip:
                self.transforming_clip_object = None
                clips = get_app().window.timeline_sync.timeline.Clips()
                for clip in clips:
                    if clip.Id() == self.transforming_clip.id:
                        self.transforming_clip_object = clip
                        need_refresh = True
                        break

Which just seemed inefficient. Sometimes OpenShot needs all of the clips, but when it only needs one, why should it search through them all when the library can do it much faster and just return what's needed? So, now, that can be:

        # Get new clip for transform
        if clip_id:
            self.transforming_clip = Clip.get(id=clip_id)
            self.transforming_clip_object =
                get_app().window.timeline_sync.timeline.GetClip(clip_id)
            if self.transforming_clip and self.transforming_clip_object:
                need_refresh = True

(GetClip(id) returns nullptr if it can't find a match, which in Python automatically translates to None. So there's no need to pre-set it to None or treat the result as fragile.)

And then, of course, there are the max-frame computations that get performed every time the preview is going to start playing, or needs to seek to the end of the timeline, etc. From actionJumpEnd_trigger in main_window.py:

        # Determine max frame (based on clips)
        timeline_length = 0.0
        fps = get_app().window.timeline_sync.timeline.info.fps.ToFloat()
        clips = get_app().window.timeline_sync.timeline.Clips()
        for clip in clips:
            clip_last_frame = clip.Position() + clip.Duration()
            if clip_last_frame > timeline_length:
                # Set max length of timeline
                timeline_length = clip_last_frame
        # Convert to int and round
        timeline_length_int = round(timeline_length * fps) + 1

Now, all of that's just:

timeline_length_int = get_app().window.timeline_sync.timeline.GetMaxFrame()

If the max time is needed (I'm sure somewhere we do that, too), it's just

timeline_end = get_app().window.timeline_sync.timeline.GetMaxTime()

[codecov]

Codecov Report

Merging #563 into develop will increase coverage by 0.66%.
The diff coverage is 94.15%.

@@             Coverage Diff             @@
##           develop     #563      +/-   ##
===========================================
+ Coverage    48.69%   49.36%   +0.66%     
===========================================
  Files          129      129              
  Lines        10036    10192     +156     
===========================================
+ Hits          4887     5031     +144     
- Misses        5149     5161      +12     

Impacted Files	Coverage Δ
include/Clip.h	88.88% <ø> (ø)
src/ReaderBase.cpp	14.75% <ø> (-1.83%)	⬇️
include/ReaderBase.h	66.66% <50.00%> (-33.34%)	⬇️
include/Timeline.h	63.63% <63.63%> (-5.12%)	⬇️
src/Clip.cpp	40.30% <77.77%> (+0.57%)	⬆️
src/Timeline.cpp	41.08% <93.33%> (+2.37%)	⬆️
src/FFmpegReader.cpp	68.05% <100.00%> (ø)
src/QtImageReader.cpp	61.32% <100.00%> (ø)
tests/Timeline_Tests.cpp	100.00% <100.00%> (ø)
src/FFmpegWriter.cpp	62.33% <0.00%> (-0.13%)	⬇️
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f71051e...f33d5cb. Read the comment docs.

[ferdnyc]

Results

The savings from GetMaxTime() and GetMaxFrame() are fairly minor. They're meant more as a convenience than anything else. The difference in time elapsed vs. the Python version appears actually even more significant than the lookup methods, but that's misleading because it takes so little time in either version that the differences are meaningless. (When I show the results of my benchmarks, keep in mind that the GetMaxFrame test is running 50,000 iterations, vs. only 2,000 for the GetClip() tests.)

The impact of GetClip() and friends, though, has the potential to be meaningful. I benchmarked it a bit, and here's the test setup I used. (Assume the requisite imports and etc.):

t = openshot.Timeline(
    1280, 720, openshot.Fraction(30, 1), 48000, 2, openshot.LAYOUT_STEREO)
num_clips = 20

def lookup_locally(t, id):
    clips = t.Clips()
    for c in clips:
        if c.Id() == id:
            return c
def request_by_id(t, id):
    c = t.GetClip(id)
    return c
def maxframe_locally(t):
    clips = t.Clips()
    timeline_length = 0.0
    for clip in clips:
        clip_last_frame = clip.Position() + clip.Duration()
        if clip_last_frame > timeline_length:
            # Set max length of timeline
            timeline_length = clip_last_frame
    # Convert to int and round
    timeline_length_int = round(timeline_length * 30) + 1
    return timeline_length_int
def request_maxframe(t):
    n = t.GetMaxFrame()
    return n
def updatePositions(t):
    upd_clips = t.Clips()
    [t.RemoveClip(c) for c in upd_clips]
    for i, c in enumerate(upd_clips):
        p = c.Position()
        c.Position((p * 13333 * i) % 1599)
    shuffle(upd_clips)
    [t.AddClip(c) for c in upd_clips]
def main():
    test_clips = [openshot.Clip(TEST_MEDIA_FILE) for i in range(num_clips)]
    layers = [l for l in range(50, 50+num_clips)]
    starts = [s for s in range(num_clips)]
    positions = [p for p in range(20, 20+num_clips*2, 2)]
    shuffle(layers)
    shuffle(starts)
    shuffle(positions)

    for i, c in enumerate(test_clips):
        id = f"CLIP_NUMBER_{i}"
        c.Id(id)
        c.Layer(layers[i])
        c.Position(positions[i])
        c.Start(starts[i])
    shuffle(test_clips)
    [t.AddClip(c) for c in test_clips]
    t.Open()

So, it creates a somewhat-randomized timeline populated with 20 clips that all have shuffled Layer, Position, and Start values. updatePositions() will re-shuffle the clips and move their Positions, when called.

I ran best-of-5 trials with the timeit module on each of the four test functions (I'll attach the full source file, for anyone curious), and these were the results:

Running best-of-5 test for local Clip search...
  Fastest time: 0.9091/2000 runs (0.00045 per iteration)

Running best-of-5 test for GetClip() lookup by ID...
  Fastest time: 0.0978/2000 runs (0.00005 per iteration)

Running best-of-5 test for local max-frame search...
  Fastest time: 2.7240/50000 runs (0.000054 per iteration)

Running best-of-5 test for GetMaxFrame() request...
  Fastest time: 0.1839/50000 runs (0.000004 per iteration)

Like I said, the GetMaxFrame() results are almost too tiny to measure. The test also isn't entirely valid, as both versions run updatePositions() at intervals so a lot of the time logged is actually spent doing that. But the GetClip() tests only run updatePositions() once at the start, and the rest of the test is them doing this:

# Locally (the Clips()-and-search version)
x = [lookup_locally(t, f"CLIP_NUMBER_{i}") for i in range(20)];
[x_c.Id() for x_c in x]
# Remotely (using t.GetClip(id))
y = [request_by_id(t, f"CLIP_NUMBER_{i}") for i in range(20)];
[y_c.Id() for y_c in y]

Now, obviously that's a bit contrived since it's unfair to the local version. If it actually needed all 20 Clip.Id()s, it would make sense to use Clips() to get the whole list, not request them one-at-a-time via t.GetClip(id).

But in all the places we are grabbing just a single Id, this helps a fair bit.

Benchmark code: Bench_GetClip.py.txt

[ferdnyc]

Heh. You can see the exact point in the unit tests where I just got tired of writing code comments, apparently.

[jonoomph]

@ferdnyc Nice work! I love it. I feel like these methods should have been written a long time ago, lol. This might create some merge issues for me (I'm refactoring the Timeline and Clip classes), but no big deal. Let's get this merged! Thanks!

[ferdnyc]

@jonoomph

I feel like these methods should have been written a long time ago

I was a bit surprised that there were no API calls to do these things. Especially since it causes OpenShot itself to jump through a lot of hoops doing them for itself.

Let's get this merged!

Great, thanks! I just applied all of my self-review suggestions, as soon as Travis finishes I'll send it through. (Then I have to start on the Python-side changes to take advantage of these new calls.)

[ferdnyc]

Oh, fun, seems Travis is a little backed up right now. Took almost 10 minutes for the first job to even start. (Hmm... although, maybe it was still finishing up our previous run, since I put through two quick edits in separate commits. Gotta remember to use that batch button.)

Eh. Regardless, it'll get there eventually I'm sure. Just a reminder that I shouldn't be hanging around waiting on Travis anyway.

[jonoomph]

Also, I temp disabled Traivs to work on some of the older PRs... (i.e. made it not required)