From 54374b46f5e76c327557418cd61069b657d31932 Mon Sep 17 00:00:00 2001
From: Mark Reid <mindmark@gmail.com>
Date: Sat, 17 Feb 2024 23:05:00 -0800
Subject: [PATCH 1/2] Normalize track lengths with gaps before flatten stacks
 fixes #1430

Signed-off-by: Mark Reid <mindmark@gmail.com>
---
 src/opentimelineio/stackAlgorithm.cpp |  63 +++++++-
 tests/CMakeLists.txt                  |   2 +-
 tests/test_stack_algo.cpp             | 218 ++++++++++++++++++++++++++
 3 files changed, 281 insertions(+), 2 deletions(-)
 create mode 100644 tests/test_stack_algo.cpp

diff --git a/src/opentimelineio/stackAlgorithm.cpp b/src/opentimelineio/stackAlgorithm.cpp
index 6800cbc2de..77129c62eb 100644
--- a/src/opentimelineio/stackAlgorithm.cpp
+++ b/src/opentimelineio/stackAlgorithm.cpp
@@ -5,10 +5,12 @@
 #include "opentimelineio/track.h"
 #include "opentimelineio/trackAlgorithm.h"
 #include "opentimelineio/transition.h"
+#include "opentimelineio/gap.h"
 
 namespace opentimelineio { namespace OPENTIMELINEIO_VERSION {
 
 typedef std::map<Track*, std::map<Composable*, TimeRange>> RangeTrackMap;
+typedef std::vector<SerializableObject::Retainer<Track>> TrackRetainerVector;
 
 static void
 _flatten_next_item(
@@ -123,10 +125,49 @@ _flatten_next_item(
     }
 }
 
+// make all tracks the same length by adding a gap to the end if they are shorter
+static void
+_normalize_tracks_lengths(std::vector<Track*>& tracks,
+                          TrackRetainerVector& tracks_retainer,
+                          ErrorStatus*         error_status)
+{
+    RationalTime duration;
+    for (auto track: tracks)
+    {
+        duration = std::max(duration, track->duration(error_status));
+        if (is_error(error_status))
+        {
+            return;
+        }
+    }
+
+    for(int i = 0; i < tracks.size(); i++)
+    {
+        Track *track = tracks[i];
+        RationalTime track_duration = track->duration(error_status);
+        if (track_duration < duration)
+        {
+            track = static_cast<Track*>(track->clone(error_status));
+            if (is_error(error_status))
+            {
+                return;
+            }
+            tracks_retainer.push_back(SerializableObject::Retainer<Track>(track));
+            track->append_child(new Gap(duration - track_duration), error_status);
+            if (is_error(error_status))
+            {
+                return;
+            }
+            tracks[i] = track;
+        }
+    }
+}
+
 Track*
 flatten_stack(Stack* in_stack, ErrorStatus* error_status)
 {
     std::vector<Track*> tracks;
+    TrackRetainerVector tracks_retainer;
     tracks.reserve(in_stack->children().size());
 
     for (auto c: in_stack->children())
@@ -151,6 +192,12 @@ flatten_stack(Stack* in_stack, ErrorStatus* error_status)
         }
     }
 
+    _normalize_tracks_lengths(tracks, tracks_retainer, error_status);
+    if (is_error(error_status))
+    {
+        return nullptr;
+    }
+
     Track* flat_track = new Track;
     flat_track->set_name("Flattened");
 
@@ -168,6 +215,20 @@ flatten_stack(Stack* in_stack, ErrorStatus* error_status)
 Track*
 flatten_stack(std::vector<Track*> const& tracks, ErrorStatus* error_status)
 {
+    std::vector<Track*> flat_tracks;
+    TrackRetainerVector tracks_retainer;
+    flat_tracks.reserve(tracks.size());
+    for (auto track : tracks)
+    {
+        flat_tracks.push_back(track);
+    }
+
+    _normalize_tracks_lengths(flat_tracks, tracks_retainer, error_status);
+    if (is_error(error_status))
+    {
+        return nullptr;
+    }
+
     Track* flat_track = new Track;
     flat_track->set_name("Flattened");
 
@@ -175,7 +236,7 @@ flatten_stack(std::vector<Track*> const& tracks, ErrorStatus* error_status)
     _flatten_next_item(
         range_track_map,
         flat_track,
-        tracks,
+        flat_tracks,
         -1,
         nullopt,
         error_status);
diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index e206fb8cf7..85cb216f86 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -15,7 +15,7 @@ foreach(test ${tests_opentime})
            WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR})
 endforeach()
 
-list(APPEND tests_opentimelineio test_clip test_serialization test_serializableCollection test_timeline test_track test_editAlgorithm)
+list(APPEND tests_opentimelineio test_clip test_serialization test_serializableCollection test_stack_algo test_timeline test_track test_editAlgorithm)
 foreach(test ${tests_opentimelineio})
     add_executable(${test} utils.h utils.cpp ${test}.cpp)
 
diff --git a/tests/test_stack_algo.cpp b/tests/test_stack_algo.cpp
new file mode 100644
index 0000000000..39a953d73c
--- /dev/null
+++ b/tests/test_stack_algo.cpp
@@ -0,0 +1,218 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright Contributors to the OpenTimelineIO project
+
+#include "opentime/timeRange.h"
+#include "utils.h"
+
+#include <opentimelineio/clip.h>
+#include <opentimelineio/stack.h>
+#include <opentimelineio/track.h>
+#include <opentimelineio/stackAlgorithm.h>
+
+#include <iostream>
+
+namespace otime = opentime::OPENTIME_VERSION;
+namespace otio  = opentimelineio::OPENTIMELINEIO_VERSION;
+
+int
+main(int argc, char** argv)
+{
+    Tests tests;
+
+    tests.add_test(
+        "test_flatten_stack_01", [] {
+        using namespace otio;
+
+        otio::RationalTime rt_0_24{0, 24};
+        otio::RationalTime rt_150_24{150, 24};
+        otio::TimeRange tr_0_150_24{rt_0_24, rt_150_24};
+
+        // all three clips are identical, but placed such that A is over B and
+        // has no gap or end over C
+        // 0         150          300
+        // [    A     ]
+        // [    B     |     C     ]
+        //
+        // should flatten to:
+        // [    A     |     C     ]
+
+        otio::SerializableObject::Retainer<otio::Clip> cl_A =
+            new otio::Clip("track1_A", nullptr, tr_0_150_24);
+        otio::SerializableObject::Retainer<otio::Clip> cl_B =
+            new otio::Clip("track1_B", nullptr, tr_0_150_24);
+        otio::SerializableObject::Retainer<otio::Clip> cl_C =
+            new otio::Clip("track1_C", nullptr, tr_0_150_24);
+
+        otio::SerializableObject::Retainer<otio::Track> tr_over =
+            new otio::Track();
+        tr_over->append_child(cl_A);
+
+        otio::SerializableObject::Retainer<otio::Track> tr_under =
+            new otio::Track();
+        tr_under->append_child(cl_B);
+        tr_under->append_child(cl_C);
+
+        otio::SerializableObject::Retainer<otio::Stack> st =
+            new otio::Stack();
+        st->append_child(tr_under);
+        st->append_child(tr_over);
+
+        auto result = flatten_stack(st);
+
+        assertEqual(result->children()[0]->name(), std::string("track1_A"));
+        assertEqual(result->children().size(), 2);
+        assertEqual(result->duration().value(), 300);
+    });
+
+    tests.add_test(
+        "test_flatten_stack_02", [] {
+        using namespace otio;
+
+        otio::RationalTime rt_0_24{0, 24};
+        otio::RationalTime rt_150_24{150, 24};
+        otio::TimeRange tr_0_150_24{rt_0_24, rt_150_24};
+
+        // all four clips are identical, but placed such that A is over B and
+        // has no gap or end over C. The bottom track is also shorter.
+        // 0         150          300
+        // [    A     ]
+        // [    B     |     C     ]
+        // [    D     ]
+        //
+        // should flatten to:
+        // [    A     |     C     ]
+
+        otio::SerializableObject::Retainer<otio::Clip> cl_A =
+            new otio::Clip("track1_A", nullptr, tr_0_150_24);
+        otio::SerializableObject::Retainer<otio::Clip> cl_B =
+            new otio::Clip("track1_B", nullptr, tr_0_150_24);
+        otio::SerializableObject::Retainer<otio::Clip> cl_C =
+            new otio::Clip("track1_C", nullptr, tr_0_150_24);
+        otio::SerializableObject::Retainer<otio::Clip> cl_D =
+            new otio::Clip("track1_D", nullptr, tr_0_150_24);
+
+        otio::SerializableObject::Retainer<otio::Track> tr_top =
+            new otio::Track();
+        tr_top->append_child(cl_A);
+
+        otio::SerializableObject::Retainer<otio::Track> tr_middle =
+            new otio::Track();
+        tr_middle->append_child(cl_B);
+        tr_middle->append_child(cl_C);
+
+        otio::SerializableObject::Retainer<otio::Track> tr_bottom =
+            new otio::Track();
+
+        tr_bottom->append_child(cl_D);
+
+        otio::SerializableObject::Retainer<otio::Stack> st =
+            new otio::Stack();
+        st->append_child(tr_bottom);
+        st->append_child(tr_middle);
+        st->append_child(tr_top);
+
+        auto result = flatten_stack(st);
+
+        assertEqual(result->children()[0]->name(), std::string("track1_A"));
+        assertEqual(result->children().size(), 2);
+        assertEqual(result->duration().value(), 300);
+    });
+
+    tests.add_test(
+        "test_flatten_stack_03", [] {
+        using namespace otio;
+
+        otio::RationalTime rt_0_24{0, 24};
+        otio::RationalTime rt_150_24{150, 24};
+        otio::TimeRange tr_0_150_24{rt_0_24, rt_150_24};
+
+        // all three clips are identical but the middle track is empty
+        // 0         150          300
+        // [    A     ]
+        // []
+        // [    B     |     C     ]
+        //
+        // should flatten to:
+        // [    A     |     C     ]
+
+        otio::SerializableObject::Retainer<otio::Clip> cl_A =
+            new otio::Clip("track1_A", nullptr, tr_0_150_24);
+        otio::SerializableObject::Retainer<otio::Clip> cl_B =
+            new otio::Clip("track1_B", nullptr, tr_0_150_24);
+        otio::SerializableObject::Retainer<otio::Clip> cl_C =
+            new otio::Clip("track1_C", nullptr, tr_0_150_24);
+
+        otio::SerializableObject::Retainer<otio::Track> tr_top =
+            new otio::Track();
+        tr_top->append_child(cl_A);
+
+        otio::SerializableObject::Retainer<otio::Track> tr_middle =
+            new otio::Track();
+
+        otio::SerializableObject::Retainer<otio::Track> tr_bottom =
+            new otio::Track();
+
+        tr_bottom->append_child(cl_B);
+        tr_bottom->append_child(cl_C);
+
+
+        otio::SerializableObject::Retainer<otio::Stack> st =
+            new otio::Stack();
+        st->append_child(tr_bottom);
+        st->append_child(tr_middle);
+        st->append_child(tr_top);
+
+        auto result = flatten_stack(st);
+
+        assertEqual(result->children()[0]->name(), std::string("track1_A"));
+        assertEqual(result->children().size(), 2);
+        assertEqual(result->duration().value(), 300);
+    });
+
+    tests.add_test(
+        "test_flatten_vector_01", [] {
+        using namespace otio;
+
+        otio::RationalTime rt_0_24{0, 24};
+        otio::RationalTime rt_150_24{150, 24};
+        otio::TimeRange tr_0_150_24{rt_0_24, rt_150_24};
+
+        // all three clips are identical, but placed such that A is over B and
+        // has no gap or end over C, tests vector version
+        // 0         150          300
+        // [    A     ]
+        // [    B     |     C     ]
+        //
+        // should flatten to:
+        // [    A     |     C     ]
+
+        otio::SerializableObject::Retainer<otio::Clip> cl_A =
+            new otio::Clip("track1_A", nullptr, tr_0_150_24);
+        otio::SerializableObject::Retainer<otio::Clip> cl_B =
+            new otio::Clip("track1_B", nullptr, tr_0_150_24);
+        otio::SerializableObject::Retainer<otio::Clip> cl_C =
+            new otio::Clip("track1_C", nullptr, tr_0_150_24);
+
+        otio::SerializableObject::Retainer<otio::Track> tr_over =
+            new otio::Track();
+        tr_over->append_child(cl_A);
+
+        otio::SerializableObject::Retainer<otio::Track> tr_under =
+            new otio::Track();
+        tr_under->append_child(cl_B);
+        tr_under->append_child(cl_C);
+
+        std::vector<Track *> st;
+        st.push_back(tr_under);
+        st.push_back(tr_over);
+
+        auto result = flatten_stack(st);
+
+        assertEqual(result->children()[0]->name(), std::string("track1_A"));
+        assertEqual(result->children().size(), 2);
+        assertEqual(result->duration().value(), 300);
+    });
+
+    tests.run(argc, argv);
+    return 0;
+}
\ No newline at end of file

From d5430bd1929c7ae787f8f49e33eea74eb4192182 Mon Sep 17 00:00:00 2001
From: Mark Reid <mindmark@gmail.com>
Date: Thu, 14 Mar 2024 22:15:35 -0700
Subject: [PATCH 2/2] Added more comments about the purpose the track retainer
 Renamed track variables in normalized_tracks_lengths to be more clear.

Signed-off-by: Mark Reid <mindmark@gmail.com>
---
 src/opentimelineio/stackAlgorithm.cpp | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/src/opentimelineio/stackAlgorithm.cpp b/src/opentimelineio/stackAlgorithm.cpp
index 77129c62eb..56f65bbda1 100644
--- a/src/opentimelineio/stackAlgorithm.cpp
+++ b/src/opentimelineio/stackAlgorithm.cpp
@@ -125,7 +125,9 @@ _flatten_next_item(
     }
 }
 
-// make all tracks the same length by adding a gap to the end if they are shorter
+// add a gap to end of a track if it is shorter then the longest track.
+// shorter tracks are clones and get added to the tracks_retainer.
+// a new track will replace the original pointer in the track vector.
 static void
 _normalize_tracks_lengths(std::vector<Track*>& tracks,
                           TrackRetainerVector& tracks_retainer,
@@ -143,22 +145,23 @@ _normalize_tracks_lengths(std::vector<Track*>& tracks,
 
     for(int i = 0; i < tracks.size(); i++)
     {
-        Track *track = tracks[i];
-        RationalTime track_duration = track->duration(error_status);
+        Track *old_track = tracks[i];
+        RationalTime track_duration = old_track->duration(error_status);
         if (track_duration < duration)
         {
-            track = static_cast<Track*>(track->clone(error_status));
+            Track *new_track = static_cast<Track*>(old_track->clone(error_status));
             if (is_error(error_status))
             {
                 return;
             }
-            tracks_retainer.push_back(SerializableObject::Retainer<Track>(track));
-            track->append_child(new Gap(duration - track_duration), error_status);
+            // add track to retainer so it can be freed later
+            tracks_retainer.push_back(SerializableObject::Retainer<Track>(new_track));
+            new_track->append_child(new Gap(duration - track_duration), error_status);
             if (is_error(error_status))
             {
                 return;
             }
-            tracks[i] = track;
+            tracks[i] = new_track;
         }
     }
 }
@@ -167,6 +170,9 @@ Track*
 flatten_stack(Stack* in_stack, ErrorStatus* error_status)
 {
     std::vector<Track*> tracks;
+    // tracks are cloned if they need to be normalized
+    // they get added to this retainer so they can be
+    // freed when the algorithm is complete
     TrackRetainerVector tracks_retainer;
     tracks.reserve(in_stack->children().size());
 
@@ -216,6 +222,9 @@ Track*
 flatten_stack(std::vector<Track*> const& tracks, ErrorStatus* error_status)
 {
     std::vector<Track*> flat_tracks;
+    // tracks are cloned if they need to be normalized
+    // they get added to this retainer so they can be
+    // freed when the algorithm is complete
     TrackRetainerVector tracks_retainer;
     flat_tracks.reserve(tracks.size());
     for (auto track : tracks)