From 8680cd26757c156d46b5865e6a74326574a784c4 Mon Sep 17 00:00:00 2001 From: Troy Hernandez Date: Sat, 6 Jun 2026 21:18:40 -0500 Subject: [PATCH 1/2] Fix edit algorithms deleting the wrong child via remove_child() Composition only declares `remove_child(int index, ErrorStatus*)`, but `overwrite()` and `insert()` call it with a `Composable*` / `Retainer`: composition->remove_child(transition); // overwrite() and insert() composition->remove_child(items.back()); // overwrite() The pointer argument decays to `bool` (always true) -> `int(1)`, so these calls remove the child at index 1 (or, via adjusted_vector_index, the last child when size == 1), NOT the item that was passed. As a result: - overwrite() over a multi-item span deletes the wrong clips: the partially-overwritten clip is removed while a fully-overwritten clip survives. - transition removal in overwrite()/insert() removes index 1 instead of the transition. Pass the already-computed (and bounds-checked) `index` for transitions, and the index of `items.back()` in the removal loop. Repro (overwrite A|M|B with X over a span that ends at the track end): before: A[0+5] B[20+5] X[2+10] (M removed, B kept -- wrong) after: A[0+5] M[10+3] X[2+10] (M trimmed, B removed -- correct) Adds a regression test (fails before, passes after); the existing test_editAlgorithm suite continues to pass. Signed-off-by: Troy Hernandez --- src/opentimelineio/algo/editAlgorithm.cpp | 7 +-- tests/test_editAlgorithm.cpp | 52 +++++++++++++++++++++++ 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/src/opentimelineio/algo/editAlgorithm.cpp b/src/opentimelineio/algo/editAlgorithm.cpp index 55e41f226..0e9a32b34 100644 --- a/src/opentimelineio/algo/editAlgorithm.cpp +++ b/src/opentimelineio/algo/editAlgorithm.cpp @@ -119,7 +119,7 @@ overwrite( || static_cast(index) >= composition->children().size()) continue; - composition->remove_child(transition); + composition->remove_child(index); } } } @@ -254,7 +254,8 @@ overwrite( // Remove the completely overwritten items. while (!items.empty()) { - composition->remove_child(items.back()); + composition->remove_child( + composition->index_of_child(items.back())); items.pop_back(); } @@ -291,7 +292,7 @@ insert( || static_cast(index) >= composition->children().size()) continue; - composition->remove_child(transition); + composition->remove_child(index); } } } diff --git a/tests/test_editAlgorithm.cpp b/tests/test_editAlgorithm.cpp index 30b82858d..42181d927 100644 --- a/tests/test_editAlgorithm.cpp +++ b/tests/test_editAlgorithm.cpp @@ -734,6 +734,58 @@ main(int argc, char** argv) TimeRange(RationalTime(36.0, 24.0), RationalTime(12.0, 24.0)) }); }); + tests.add_test("test_edit_overwrite_partial_first_full_last", [] { + // Overwrite that partially covers one clip and fully covers the next, + // ending exactly at the composition end. Regression test: the loop that + // removes the fully-overwritten items called remove_child() with a + // pointer, but Composition only declares remove_child(int), so the + // pointer decayed to bool->int(1) and the WRONG child was deleted (the + // partially-overwritten clip survived in place of the fully-covered one). + SerializableObject::Retainer clip_0 = new Clip( + "clip_0", + nullptr, + TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0))); + SerializableObject::Retainer clip_1 = new Clip( + "clip_1", + nullptr, + TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0))); + SerializableObject::Retainer clip_2 = new Clip( + "clip_2", + nullptr, + TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0))); + SerializableObject::Retainer track = new Track(); + track->append_child(clip_0); + track->append_child(clip_1); + track->append_child(clip_2); + + SerializableObject::Retainer over = new Clip( + "over", + nullptr, + TimeRange(RationalTime(0.0, 24.0), RationalTime(100.0, 24.0))); + OTIO_NS::ErrorStatus error_status; + algo::overwrite( + over, + track, + TimeRange(RationalTime(36.0, 24.0), RationalTime(36.0, 24.0)), + true, + nullptr, + &error_status); + + // clip_0 unchanged, clip_1 trimmed to its first half, clip_2 fully + // overwritten (removed), over inserted last. + assert(!is_error(error_status)); + assert(track->children().size() == 3); + assert(dynamic_cast(track->children()[1].value)->name() + == "clip_1"); + assert(dynamic_cast(track->children()[2].value)->name() + == "over"); + assert_track_ranges( + track, + { TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0)), + TimeRange(RationalTime(24.0, 24.0), RationalTime(12.0, 24.0)), + TimeRange(RationalTime(36.0, 24.0), RationalTime(36.0, 24.0)) }); + }); + tests.add_test("test_edit_overwrite_4", [] { // Create a track with one long clip. SerializableObject::Retainer clip_0 = new Clip( From 6670f49bc44f313b01bc080b4c14fcb1a505f97d Mon Sep 17 00:00:00 2001 From: Troy Hernandez Date: Thu, 11 Jun 2026 08:19:01 -0500 Subject: [PATCH 2/2] Make Retainer::operator bool() explicit Requested in PR review: the implicit Retainer -> bool -> int conversion chain is what allowed remove_child() to compile and silently remove the child at index 1. Marking the conversion explicit turns that mistake into a compile error, while contextual conversion keeps if (retainer), &&, and friends working unchanged. The library and all C++ tests build cleanly with no call-site changes needed, and the full test suite passes. Signed-off-by: Troy Hernandez --- src/opentimelineio/serializableObject.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/opentimelineio/serializableObject.h b/src/opentimelineio/serializableObject.h index 8a97110ae..4f2e6c6b6 100644 --- a/src/opentimelineio/serializableObject.h +++ b/src/opentimelineio/serializableObject.h @@ -626,7 +626,7 @@ class OTIO_API_TYPE SerializableObject T* operator->() const noexcept { return value; } - operator bool() const noexcept { return value != nullptr; } + explicit operator bool() const noexcept { return value != nullptr; } Retainer(T const* so = nullptr) : value((T*) so)