From cfa5dfa3cd97d328aff8f4b44c62405030d493e8 Mon Sep 17 00:00:00 2001 From: Jonathan Thomas Date: Sat, 19 Sep 2026 18:23:52 -0500 Subject: [PATCH 1/8] Bumping version to 1.0.1, SO 32, requiring OpenShotAudio 1.0.1 --- CMakeLists.txt | 4 ++-- src/CMakeLists.txt | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 9a812bb7b..279fce9bd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -24,8 +24,8 @@ For more information, please visit . set(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake/Modules") ################ PROJECT VERSION #################### -set(PROJECT_VERSION_FULL "1.0.0") -set(PROJECT_SO_VERSION 31) +set(PROJECT_VERSION_FULL "1.0.1") +set(PROJECT_SO_VERSION 32) # Remove the dash and anything following, to get the #.#.# version for project() STRING(REGEX REPLACE "\-.*$" "" VERSION_NUM "${PROJECT_VERSION_FULL}") diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 7452807ac..ce95a60d2 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -237,7 +237,7 @@ add_feature_info("Wayland screen capture" OPENSHOT_WAYLAND_CAPTURE "Use the opti # Find JUCE-based openshot Audio libraries if(NOT TARGET OpenShot::Audio) # Only load if necessary (not for integrated builds) - find_package(OpenShotAudio 1.0.0 REQUIRED) + find_package(OpenShotAudio 1.0.1 REQUIRED) endif() target_link_libraries(openshot PUBLIC OpenShot::Audio) From b590640fbaa0938a1624497a2729e364cef5aef8 Mon Sep 17 00:00:00 2001 From: Jonathan Thomas Date: Sun, 20 Sep 2026 00:15:23 -0500 Subject: [PATCH 2/8] Speed up JSON keyframe updates and add focused transform benchmarks Interactive transform edits repeatedly replace growing X/Y keyframe curves. Applying each update copied the same JSON data through several layers and rebuilt keyframe storage from scratch, making edits increasingly expensive as an animation accumulated points. Move JSON values that are already owned by the loader through the existing by-value interfaces instead of copying them again. Clip and effect loading now share a base-field helper that reads common fields without copying all of the derived animation data. Parsed SetJson input and nested color/point values use the same ownership approach. Keep allocated keyframe capacity across replacements and append sorted points directly. Unordered input and duplicate frames still use AddPoint, preserving ordering and replacement behavior. Existing public signatures, virtual method layout, JSON format, and timeline locking remain unchanged. Add two optional benchmark cases for a 720p/24fps image clip with 0 or 1,000 existing keyframes per axis. Each performs 100 successive X/Y transform updates through Timeline::ApplyJsonDiff and reports the median of three trials. Input preparation stays outside the measurement, and final point counts and values are checked so a skipped update cannot look faster. These cases belong to the separate benchmark executable, not normal CTest runs. Report Ops/s because an operation can now be a JSON edit or a frame. Document the reproduction commands and local before/after measurements. The measured cost per update fell from 0.82 to 0.33 ms for the initially empty curves and from 17.99 to 6.73 ms for curves starting at 1,000 points. These isolate native JSON application; they do not measure GUI rendering or guarantee the same performance on other machines. Add regression coverage for caller-owned JSON, sorted and unordered input, duplicate and fractional frames, interpolation handles, large curve replacement, multiple actions in one diff, unchanged properties, and cache invalidation. Validation: rebuilt the library, benchmark, and affected test executables; all 197 KeyFrame, Timeline, Clip, Point, Color, ColorGrade, Mask, and EffectMask tests passed in 6.2 seconds. Both remaining transform benchmark cases also completed successfully. The slower 5,000-point benchmark case is intentionally omitted. --- doc/JSON-KEYFRAME-BENCHMARK.md | 45 ++++++++++++++++++++++++ src/Clip.cpp | 63 +++++++++++++++++----------------- src/ClipBase.cpp | 4 +++ src/ClipBase.h | 3 ++ src/Color.cpp | 15 ++++---- src/EffectBase.cpp | 11 +++--- src/KeyFrame.cpp | 20 +++++++---- src/Point.cpp | 13 +++---- src/Timeline.cpp | 34 +++++++++--------- tests/Benchmark.cpp | 62 +++++++++++++++++++++++++++++++-- tests/KeyFrame.cpp | 43 +++++++++++++++++++++++ tests/Timeline.cpp | 50 +++++++++++++++++++++++++++ 12 files changed, 289 insertions(+), 74 deletions(-) create mode 100644 doc/JSON-KEYFRAME-BENCHMARK.md diff --git a/doc/JSON-KEYFRAME-BENCHMARK.md b/doc/JSON-KEYFRAME-BENCHMARK.md new file mode 100644 index 000000000..047148947 --- /dev/null +++ b/doc/JSON-KEYFRAME-BENCHMARK.md @@ -0,0 +1,45 @@ +# JSON keyframe edit benchmark + +Build `openshot-benchmark`, then run from the repository root: + +```sh +cmake --build build --target openshot-benchmark -j2 +QT_QPA_PLATFORM=offscreen build/tests/openshot-benchmark --test 'Timeline JSON transforms (0 existing keys)' +QT_QPA_PLATFORM=offscreen build/tests/openshot-benchmark --test 'Timeline JSON transforms (1000 existing keys)' +``` + +Each case uses an image clip on layer 5 of a 1280×720, 24 fps timeline. It +starts with the specified number of keys **per axis**, then adds 100 successive +`location_x` / `location_y` samples with Bezier interpolation and handles. Each +JSON diff contains the complete growing curves, matching the editor's transform +update format. Both axes are sent in one diff. The result verifies the final +point counts and values so a discarded update cannot appear faster. + +Payload generation, serialization, initial population, and clip/timeline setup +are outside the timer. The measured operation is `Timeline::ApplyJsonDiff`, +including JSON parsing, native keyframe replacement, and cache invalidation. +There is no concurrent player or frame rendering. This isolates application cost; +it is not a measurement of end-to-end GUI responsiveness or lock contention. + +The benchmark reports the median operations/second of three trials. One operation +is a JSON update here; for the existing rendering trials it is a frame. +Milliseconds per update = `1000 / operations_per_second`. + +## Local comparison + +Baseline: release-20260919 at cfa5dfa3, before native JSON/keyframe optimizations. +Optimized build: local changes described below. Both use the same benchmark, +build configuration (`-O3 -DNDEBUG`), system JsonCpp, and machine. These timings +are local measurements, not cross-machine performance guarantees. + +| Existing keys per axis | Baseline ms/update | Optimized ms/update | +| --- | ---: | ---: | +| 0 (grows to 100) | 0.82 | 0.33 | +| 1,000 (grows to 1,100) | 17.99 | 6.73 | + +The changes transfer already-owned JSON values through the existing by-value +interfaces, avoid copying derived animation data just to load base clip fields, +and retain keyframe storage with a fast append path for sorted input. Unsorted +and duplicate frame coordinates still use the existing `AddPoint` behavior. +Existing public signatures, virtual method layout, and the JSON format are unchanged. +The complete incoming animation still has to be parsed on every update. diff --git a/src/Clip.cpp b/src/Clip.cpp index a03df145e..723fff0e4 100644 --- a/src/Clip.cpp +++ b/src/Clip.cpp @@ -10,6 +10,7 @@ // // SPDX-License-Identifier: LGPL-3.0-or-later +#include #include "Clip.h" #include "AudioResampler.h" @@ -1035,9 +1036,9 @@ void Clip::SetJson(const std::string value) { // Parse JSON string into JSON objects try { - const Json::Value root = openshot::stringToJson(value); + Json::Value root = openshot::stringToJson(value); // Set all values that match - SetJsonValue(root); + SetJsonValue(std::move(root)); } catch (const std::exception& e) { @@ -1047,7 +1048,7 @@ void Clip::SetJson(const std::string value) { } // Load Json::Value into this object -void Clip::SetJsonValue(const Json::Value root) { +void Clip::SetJsonValue(Json::Value root) { auto ensure_default_keyframe = [](Keyframe& kf, double default_value) { if (kf.GetCount() == 0) { kf = Keyframe(default_value); @@ -1055,7 +1056,7 @@ void Clip::SetJsonValue(const Json::Value root) { }; // Set parent data - ClipBase::SetJsonValue(root); + SetBaseJsonValue(root); // Older project files predate reader-applied orientation metadata and stored // phone/camera rotation as ordinary clip rotation/scale keyframes. @@ -1097,59 +1098,59 @@ void Clip::SetJsonValue(const Json::Value root) { if (!root["waveform_mode"].isNull()) waveform_mode = root["waveform_mode"].asInt(); if (!root["scale_x"].isNull()) - scale_x.SetJsonValue(root["scale_x"]); + scale_x.SetJsonValue(std::move(root["scale_x"])); if (!root["scale_y"].isNull()) - scale_y.SetJsonValue(root["scale_y"]); + scale_y.SetJsonValue(std::move(root["scale_y"])); if (!root["location_x"].isNull()) - location_x.SetJsonValue(root["location_x"]); + location_x.SetJsonValue(std::move(root["location_x"])); if (!root["location_y"].isNull()) - location_y.SetJsonValue(root["location_y"]); + location_y.SetJsonValue(std::move(root["location_y"])); if (!root["alpha"].isNull()) - alpha.SetJsonValue(root["alpha"]); + alpha.SetJsonValue(std::move(root["alpha"])); if (!root["corner_radius"].isNull()) - corner_radius.SetJsonValue(root["corner_radius"]); + corner_radius.SetJsonValue(std::move(root["corner_radius"])); if (!root["margin"].isNull()) - margin.SetJsonValue(root["margin"]); + margin.SetJsonValue(std::move(root["margin"])); if (!root["rotation"].isNull()) - rotation.SetJsonValue(root["rotation"]); + rotation.SetJsonValue(std::move(root["rotation"])); if (!root["time"].isNull()) - time.SetJsonValue(root["time"]); + time.SetJsonValue(std::move(root["time"])); if (!root["volume"].isNull()) - volume.SetJsonValue(root["volume"]); + volume.SetJsonValue(std::move(root["volume"])); if (!root["wave_color"].isNull()) - wave_color.SetJsonValue(root["wave_color"]); + wave_color.SetJsonValue(std::move(root["wave_color"])); if (!root["shear_x"].isNull()) - shear_x.SetJsonValue(root["shear_x"]); + shear_x.SetJsonValue(std::move(root["shear_x"])); if (!root["shear_y"].isNull()) - shear_y.SetJsonValue(root["shear_y"]); + shear_y.SetJsonValue(std::move(root["shear_y"])); if (!root["origin_x"].isNull()) - origin_x.SetJsonValue(root["origin_x"]); + origin_x.SetJsonValue(std::move(root["origin_x"])); if (!root["origin_y"].isNull()) - origin_y.SetJsonValue(root["origin_y"]); + origin_y.SetJsonValue(std::move(root["origin_y"])); if (!root["channel_filter"].isNull()) - channel_filter.SetJsonValue(root["channel_filter"]); + channel_filter.SetJsonValue(std::move(root["channel_filter"])); if (!root["channel_mapping"].isNull()) - channel_mapping.SetJsonValue(root["channel_mapping"]); + channel_mapping.SetJsonValue(std::move(root["channel_mapping"])); if (!root["has_audio"].isNull()) - has_audio.SetJsonValue(root["has_audio"]); + has_audio.SetJsonValue(std::move(root["has_audio"])); if (!root["has_video"].isNull()) - has_video.SetJsonValue(root["has_video"]); + has_video.SetJsonValue(std::move(root["has_video"])); if (!root["perspective_c1_x"].isNull()) - perspective_c1_x.SetJsonValue(root["perspective_c1_x"]); + perspective_c1_x.SetJsonValue(std::move(root["perspective_c1_x"])); if (!root["perspective_c1_y"].isNull()) - perspective_c1_y.SetJsonValue(root["perspective_c1_y"]); + perspective_c1_y.SetJsonValue(std::move(root["perspective_c1_y"])); if (!root["perspective_c2_x"].isNull()) - perspective_c2_x.SetJsonValue(root["perspective_c2_x"]); + perspective_c2_x.SetJsonValue(std::move(root["perspective_c2_x"])); if (!root["perspective_c2_y"].isNull()) - perspective_c2_y.SetJsonValue(root["perspective_c2_y"]); + perspective_c2_y.SetJsonValue(std::move(root["perspective_c2_y"])); if (!root["perspective_c3_x"].isNull()) - perspective_c3_x.SetJsonValue(root["perspective_c3_x"]); + perspective_c3_x.SetJsonValue(std::move(root["perspective_c3_x"])); if (!root["perspective_c3_y"].isNull()) - perspective_c3_y.SetJsonValue(root["perspective_c3_y"]); + perspective_c3_y.SetJsonValue(std::move(root["perspective_c3_y"])); if (!root["perspective_c4_x"].isNull()) - perspective_c4_x.SetJsonValue(root["perspective_c4_x"]); + perspective_c4_x.SetJsonValue(std::move(root["perspective_c4_x"])); if (!root["perspective_c4_y"].isNull()) - perspective_c4_y.SetJsonValue(root["perspective_c4_y"]); + perspective_c4_y.SetJsonValue(std::move(root["perspective_c4_y"])); // Core clip transforms should never remain empty after load. Empty JSON // point arrays can be produced by editing flows that remove every keyframe. diff --git a/src/ClipBase.cpp b/src/ClipBase.cpp index 4d1418edf..39eb8a4a3 100644 --- a/src/ClipBase.cpp +++ b/src/ClipBase.cpp @@ -78,6 +78,10 @@ Json::Value ClipBase::JsonValue() const { // Load Json::Value into this object void ClipBase::SetJsonValue(const Json::Value root) { + SetBaseJsonValue(root); +} + +void ClipBase::SetBaseJsonValue(const Json::Value& root) { // Set data from Json (if key is found) if (!root["id"].isNull()) diff --git a/src/ClipBase.h b/src/ClipBase.h index 732160c16..ff1ab755c 100644 --- a/src/ClipBase.h +++ b/src/ClipBase.h @@ -39,6 +39,9 @@ namespace openshot { std::string previous_properties; ///< This string contains the previous JSON properties openshot::TimelineBase* timeline; ///< Pointer to the parent timeline instance (if any) + /// Load common clip fields without copying derived animation data. + void SetBaseJsonValue(const Json::Value& root); + /// Generate JSON for a property Json::Value add_property_json(std::string name, float value, std::string type, std::string memo, const Keyframe* keyframe, float min_value, float max_value, bool readonly, int64_t requested_frame) const; diff --git a/src/Color.cpp b/src/Color.cpp index 7cc15f384..a65e7e1fd 100644 --- a/src/Color.cpp +++ b/src/Color.cpp @@ -10,6 +10,7 @@ // // SPDX-License-Identifier: LGPL-3.0-or-later +#include #include #include "Color.h" @@ -102,9 +103,9 @@ void Color::SetJson(const std::string value) { // Parse JSON string into JSON objects try { - const Json::Value root = openshot::stringToJson(value); + Json::Value root = openshot::stringToJson(value); // Set all values that match - SetJsonValue(root); + SetJsonValue(std::move(root)); } catch (const std::exception& e) { @@ -114,15 +115,15 @@ void Color::SetJson(const std::string value) { } // Load Json::Value into this object -void Color::SetJsonValue(const Json::Value root) { +void Color::SetJsonValue(Json::Value root) { // Set data from Json (if key is found) if (!root["red"].isNull()) - red.SetJsonValue(root["red"]); + red.SetJsonValue(std::move(root["red"])); if (!root["green"].isNull()) - green.SetJsonValue(root["green"]); + green.SetJsonValue(std::move(root["green"])); if (!root["blue"].isNull()) - blue.SetJsonValue(root["blue"]); + blue.SetJsonValue(std::move(root["blue"])); if (!root["alpha"].isNull()) - alpha.SetJsonValue(root["alpha"]); + alpha.SetJsonValue(std::move(root["alpha"])); } diff --git a/src/EffectBase.cpp b/src/EffectBase.cpp index d3fd1c80d..415e1c9af 100644 --- a/src/EffectBase.cpp +++ b/src/EffectBase.cpp @@ -10,6 +10,7 @@ // // SPDX-License-Identifier: LGPL-3.0-or-later +#include #include #include #include @@ -136,7 +137,7 @@ void EffectBase::SetJson(const std::string value) { { Json::Value root = openshot::stringToJson(value); // Set all values that match - SetJsonValue(root); + SetJsonValue(std::move(root)); } catch (const std::exception& e) { @@ -146,7 +147,7 @@ void EffectBase::SetJson(const std::string value) { } // Load Json::Value into this object -void EffectBase::SetJsonValue(const Json::Value root) { +void EffectBase::SetJsonValue(Json::Value root) { const std::string original_id = this->Id(); const std::string original_parent_effect_id = this->info.parent_effect_id; @@ -159,7 +160,7 @@ void EffectBase::SetJsonValue(const Json::Value root) { root["id"].asString() == original_parent_effect_id && root["id"].asString() != original_id; if (applying_parent_payload) { - my_root = root; + my_root = std::move(root); my_root["id"] = original_id; my_root["parent_effect_id"] = original_parent_effect_id; } else if (parentEffect){ @@ -167,7 +168,7 @@ void EffectBase::SetJsonValue(const Json::Value root) { my_root["id"] = this->Id(); my_root["parent_effect_id"] = this->info.parent_effect_id; } else { - my_root = root; + my_root = std::move(root); } // Legacy compatibility: older shared-mask JSON stored source trim @@ -178,7 +179,7 @@ void EffectBase::SetJsonValue(const Json::Value root) { my_root["end"] = my_root["mask_end"]; // Set parent data - ClipBase::SetJsonValue(my_root); + SetBaseJsonValue(my_root); // Set data from Json (if key is found) if (!my_root["order"].isNull()) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index 8c7615816..ebf95cb56 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -357,9 +357,9 @@ void Keyframe::SetJson(const std::string value) { // Parse JSON string into JSON objects try { - const Json::Value root = openshot::stringToJson(value); + Json::Value root = openshot::stringToJson(value); // Set all values that match - SetJsonValue(root); + SetJsonValue(std::move(root)); } catch (const std::exception& e) { @@ -369,22 +369,28 @@ void Keyframe::SetJson(const std::string value) { } // Load Json::Value into this object -void Keyframe::SetJsonValue(const Json::Value root) { +void Keyframe::SetJsonValue(Json::Value root) { // Clear existing points Points.clear(); - Points.shrink_to_fit(); if (root.isObject() && !root["Points"].isNull()) { + // Reuse allocation across edits, and append the usual sorted input directly. + const size_t count = root["Points"].size(); + if (count > Points.capacity()) + Points.reserve(std::max(count, Points.capacity() * 2)); // loop through points in JSON Object - for (const auto existing_point : root["Points"]) { + for (auto& existing_point : root["Points"]) { // Create Point Point p; // Load Json into Point - p.SetJsonValue(existing_point); + p.SetJsonValue(std::move(existing_point)); // Add Point to Keyframe - AddPoint(p); + if (Points.empty() || Points.back().co.X < p.co.X) + Points.push_back(p); + else + AddPoint(p); // Preserve ordering and last-wins duplicate handling. } } else if (root.isNumeric()) { // Create Point from Numeric value diff --git a/src/Point.cpp b/src/Point.cpp index 8e7e54729..d7ef2c2b8 100644 --- a/src/Point.cpp +++ b/src/Point.cpp @@ -10,6 +10,7 @@ // // SPDX-License-Identifier: LGPL-3.0-or-later +#include #include "Point.h" #include "Exceptions.h" @@ -89,9 +90,9 @@ void Point::SetJson(const std::string value) { // Parse JSON string into JSON objects try { - const Json::Value root = openshot::stringToJson(value); + Json::Value root = openshot::stringToJson(value); // Set all values that match - SetJsonValue(root); + SetJsonValue(std::move(root)); } catch (const std::exception& e) { @@ -101,14 +102,14 @@ void Point::SetJson(const std::string value) { } // Load Json::Value into this object -void Point::SetJsonValue(const Json::Value root) { +void Point::SetJsonValue(Json::Value root) { if (!root["co"].isNull()) - co.SetJsonValue(root["co"]); // update coordinate + co.SetJsonValue(std::move(root["co"])); // update coordinate if (!root["handle_left"].isNull()) - handle_left.SetJsonValue(root["handle_left"]); // update coordinate + handle_left.SetJsonValue(std::move(root["handle_left"])); // update coordinate if (!root["handle_right"].isNull()) - handle_right.SetJsonValue(root["handle_right"]); // update coordinate + handle_right.SetJsonValue(std::move(root["handle_right"])); // update coordinate if (!root["interpolation"].isNull()) interpolation = (InterpolationType) root["interpolation"].asInt(); if (!root["handle_type"].isNull()) diff --git a/src/Timeline.cpp b/src/Timeline.cpp index 544c910ad..48c9b2608 100644 --- a/src/Timeline.cpp +++ b/src/Timeline.cpp @@ -21,6 +21,7 @@ #include "effects/Mask.h" #include +#include #include #include #include @@ -1400,24 +1401,25 @@ void Timeline::ApplyJsonDiff(std::string value) { // Parse JSON string into JSON objects try { - const Json::Value root = openshot::stringToJson(value); + Json::Value root = openshot::stringToJson(value); const uint64_t initial_cache_epoch = CacheEpoch(); - // Process the JSON change array, loop through each item - for (const Json::Value change : root) { + // Each change is owned here and consumed once. Move it through the + // existing by-value API instead of copying entire animation curves. + for (auto& change : root) { std::string change_key = change["key"][(uint)0].asString(); // Process each type of change if (change_key == "clips") // Apply to CLIPS - apply_json_to_clips(change); + apply_json_to_clips(std::move(change)); else if (change_key == "effects") // Apply to EFFECTS - apply_json_to_effects(change); + apply_json_to_effects(std::move(change)); else // Apply to TIMELINE - apply_json_to_timeline(change); + apply_json_to_timeline(std::move(change)); } @@ -1499,7 +1501,7 @@ void Timeline::apply_json_to_clips(Json::Value change) { { if (e->Id() == effect_id) { // Apply the change to the effect directly - apply_json_to_effects(change, e); + apply_json_to_effects(std::move(change), e); // Effect-only diffs must clear the owning clip cache. if (existing_clip->GetCache()) { @@ -1532,7 +1534,7 @@ void Timeline::apply_json_to_clips(Json::Value change) { clip->ParentTimeline(this); // Set properties of clip from JSON - clip->SetJsonValue(change["value"]); + clip->SetJsonValue(std::move(change["value"])); // Add clip to timeline AddClip(clip); @@ -1546,7 +1548,7 @@ void Timeline::apply_json_to_clips(Json::Value change) { int64_t old_ending_frame = ((existing_clip->Position() + existing_clip->Duration()) * info.fps.ToDouble()) + 1; // Update clip properties from JSON - existing_clip->SetJsonValue(change["value"]); + existing_clip->SetJsonValue(std::move(change["value"])); // Calculate new start and end frames after the update int64_t new_starting_frame = (existing_clip->Position() * info.fps.ToDouble()) + 1; @@ -1614,7 +1616,7 @@ void Timeline::apply_json_to_effects(Json::Value change) { // Now that we found the effect, apply the change to it if (existing_effect || change_type == "insert") { // Apply change to effect - apply_json_to_effects(change, existing_effect); + apply_json_to_effects(std::move(change), existing_effect); } } @@ -1647,7 +1649,7 @@ void Timeline::apply_json_to_effects(Json::Value change, EffectBase* existing_ef allocated_effects.insert(e); // Load Json into Effect - e->SetJsonValue(change["value"]); + e->SetJsonValue(std::move(change["value"])); // Add Effect to Timeline AddEffect(e); @@ -1664,7 +1666,7 @@ void Timeline::apply_json_to_effects(Json::Value change, EffectBase* existing_ef final_cache->Remove(old_starting_frame - 8, old_ending_frame + 8); // Update effect properties from JSON - existing_effect->SetJsonValue(change["value"]); + existing_effect->SetJsonValue(std::move(change["value"])); } } else if (change_type == "delete") { @@ -1705,16 +1707,16 @@ void Timeline::apply_json_to_timeline(Json::Value change) { // Check for valid property if (root_key == "color") // Set color - color.SetJsonValue(change["value"]); + color.SetJsonValue(std::move(change["value"])); else if (root_key == "viewport_scale") // Set viewport scale - viewport_scale.SetJsonValue(change["value"]); + viewport_scale.SetJsonValue(std::move(change["value"])); else if (root_key == "viewport_x") // Set viewport x offset - viewport_x.SetJsonValue(change["value"]); + viewport_x.SetJsonValue(std::move(change["value"])); else if (root_key == "viewport_y") // Set viewport y offset - viewport_y.SetJsonValue(change["value"]); + viewport_y.SetJsonValue(std::move(change["value"])); else if (root_key == "duration") { // Update duration of timeline info.duration = change["value"].asDouble(); diff --git a/tests/Benchmark.cpp b/tests/Benchmark.cpp index f15593a10..9fdd9f05e 100644 --- a/tests/Benchmark.cpp +++ b/tests/Benchmark.cpp @@ -136,7 +136,7 @@ using namespace openshot; using namespace std; using Clock = chrono::steady_clock; -using TrialResult = pair; // (frames, elapsed_seconds) +using TrialResult = pair; // (frames or JSON updates, elapsed_seconds) using TrialFunc = function; using Trial = pair; @@ -198,7 +198,7 @@ static void print_results(const vector& records) { }; cout << "| " << left << setw(static_cast(max_name)) << "Trial" - << " | " << right << setw(8) << "FPS" + << " | " << right << setw(8) << "Ops/s" << " | Chart |\n"; cout << "|:" << string(max_name, '-') << "-|" << string(8, '-') << ":|:" @@ -219,6 +219,58 @@ static TrialResult timed_read(ReaderBase& r) { return {BENCH_FRAMES, chrono::duration(Clock::now() - t0).count()}; } +// Model dragging a clip's X/Y transform: each edit sends the complete, growing +// curves, as openshot-qt does. Prepare strings outside the measured interval so +// this measures ApplyJsonDiff (including parsing), not JSON generation or render. +static TrialResult timed_transform_json(const string& image, int existing_keys) { + constexpr int edits = 100; + Clip clip(image); + clip.Id("json-benchmark-clip"); + clip.Layer(5); + clip.End((existing_keys + edits + 24) / 24.0); + Timeline timeline(1280, 720, Fraction(24, 1), 44100, 2, LAYOUT_STEREO); + timeline.AddClip(&clip); + timeline.Open(); + + Json::Value diff(Json::arrayValue); + Json::Value change(Json::objectValue); + change["type"] = "update"; + change["key"].append("clips"); + Json::Value id(Json::objectValue); + id["id"] = clip.Id(); + change["key"].append(id); + diff.append(change); + auto& value = diff[0]["value"]; + value["location_x"]["Points"] = Json::Value(Json::arrayValue); + value["location_y"]["Points"] = Json::Value(Json::arrayValue); + Json::StreamWriterBuilder writer; + writer["indentation"] = ""; + vector payloads; + payloads.reserve(edits); + for (int frame = 1; frame <= existing_keys + edits; ++frame) { + // Include Bezier handles, matching real project keyframes. + value["location_x"]["Points"].append(Point(frame, sin(frame * 0.05) * 0.4, BEZIER).JsonValue()); + value["location_y"]["Points"].append(Point(frame, cos(frame * 0.05) * 0.4, BEZIER).JsonValue()); + if (frame == existing_keys) + timeline.ApplyJsonDiff(Json::writeString(writer, diff)); + if (frame > existing_keys) + payloads.push_back(Json::writeString(writer, diff)); + } + auto start = Clock::now(); + for (const auto& payload : payloads) + timeline.ApplyJsonDiff(payload); + double elapsed = chrono::duration(Clock::now() - start).count(); + // A broken/no-op diff must not masquerade as an optimization. + const int final_frame = existing_keys + edits; + if (clip.location_x.GetCount() != final_frame || clip.location_y.GetCount() != final_frame || + abs(clip.location_x.GetValue(final_frame) - sin(final_frame * 0.05) * 0.4) > 1e-6 || + abs(clip.location_y.GetValue(final_frame) - cos(final_frame * 0.05) * 0.4) > 1e-6) + throw runtime_error("Transform JSON benchmark did not apply the expected keyframes"); + timeline.Close(); + timeline.RemoveClip(&clip); + return {edits, elapsed}; +} + #if defined(OPENSHOT_HAS_AUDIOVISUALIZATION) || defined(OPENSHOT_HAS_BEATSYNC) static std::shared_ptr make_audio_visualization_frame(int64_t frame_number) { const int width = 1280; @@ -317,6 +369,12 @@ int main(int argc, char* argv[]) { vector trials; trials.reserve(40); + for (int keys : {0, 1000}) { + trials.emplace_back("Timeline JSON transforms (" + to_string(keys) + " existing keys)", + [&, keys]() { return timed_transform_json(overlay, keys); }); + } + + trials.emplace_back("FFmpegReader", [&]() -> TrialResult { FFmpegReader r(video); r.Open(); diff --git a/tests/KeyFrame.cpp b/tests/KeyFrame.cpp index 00c1050c0..b188b04a8 100644 --- a/tests/KeyFrame.cpp +++ b/tests/KeyFrame.cpp @@ -832,3 +832,46 @@ TEST_CASE( "Tracker stroke width compensates for preview raster scaling", "[libo CHECK(blue_run_at_center() >= 3); } #endif + +TEST_CASE("JSON keyframe loading preserves AddPoint semantics", "[libopenshot][keyframe][json]") +{ + std::vector frames; + SECTION("sorted") { frames = {1, 4, 8, 20}; } + SECTION("unordered and duplicate frames") { frames = {20, 4, 8, 4, 1, 20}; } + SECTION("fractional frames") { frames = {1, 1.25, 1.75, 1.25, 2}; } + SECTION("large sorted curve") { + for (int i = 1; i <= 5000; ++i) frames.push_back(i); + } + Json::Value data; + data["Points"] = Json::Value(Json::arrayValue); + Keyframe expected; + for (size_t i = 0; i < frames.size(); ++i) { + Point point(frames[i], i * 0.125, static_cast(i % 3)); + // Only Bezier points serialize custom handles. + if (point.interpolation == BEZIER) { + point.handle_left = Coordinate(0.2, 0.8); + point.handle_right = Coordinate(0.7, 0.1); + } + data["Points"].append(point.JsonValue()); + expected.AddPoint(point); + } + const Json::Value original = data; + Keyframe actual(99); + actual.SetJsonValue(data); + CHECK(data == original); + CHECK(actual.JsonValue() == expected.JsonValue()); + for (int frame = 1; frame <= 20; ++frame) + CHECK(actual.GetValue(frame) == Approx(expected.GetValue(frame))); + + // Replacing a large animation must discard old points, even when capacity + // is retained internally for the next drag sample. + actual.SetJsonValue(Json::Value(0.75)); + CHECK(actual.GetCount() == 1); + CHECK(actual.GetValue(1) == Approx(0.75)); + Json::Value empty; + empty["Points"] = Json::Value(Json::arrayValue); + actual.SetJsonValue(empty); + CHECK(actual.GetCount() == 0); + actual.SetJsonValue(data); + CHECK(actual.JsonValue() == expected.JsonValue()); +} diff --git a/tests/Timeline.cpp b/tests/Timeline.cpp index 9b87293ce..77019db99 100644 --- a/tests/Timeline.cpp +++ b/tests/Timeline.cpp @@ -2183,3 +2183,53 @@ TEST_CASE("GetMaxFrame ignores tiny float overshoot at clip end", "[libopenshot] REQUIRE(t.GetMaxTime() * t.info.fps.ToDouble() < 505.0001); CHECK(t.GetMaxFrame() == 505); } + +TEST_CASE("JSON transform edits preserve other properties and multiple actions", "[libopenshot][timeline][json]") +{ + DummyReader reader; + Clip clip(&reader); + clip.Id("transform-json"); + clip.End(300); + clip.Layer(5); + clip.alpha = Keyframe(0.75); + Timeline timeline(1280, 720, Fraction(24, 1), 44100, 2, LAYOUT_STEREO); + timeline.AddClip(&clip); + Json::Value change; + change["type"] = "update"; + change["key"].append("clips"); + Json::Value id; + id["id"] = clip.Id(); + change["key"].append(id); + Keyframe x, y; + for (int frame = 1; frame <= 1000; ++frame) { + x.AddPoint(frame, frame * 0.001, LINEAR); + y.AddPoint(frame, -frame * 0.001, BEZIER); + } + change["value"]["location_x"] = x.JsonValue(); + change["value"]["location_y"] = y.JsonValue(); + Json::Value changes(Json::arrayValue); + changes.append(change); + Json::Value alpha_change = change; + alpha_change["value"] = Json::Value(Json::objectValue); + alpha_change["value"]["alpha"] = Keyframe(0.5).JsonValue(); + changes.append(alpha_change); + const auto epoch = timeline.CacheEpoch(); + timeline.ApplyJsonDiff(changes.toStyledString()); + CHECK(clip.location_x.JsonValue() == x.JsonValue()); + CHECK(clip.location_y.JsonValue() == y.JsonValue()); + CHECK(clip.alpha.GetValue(1) == Approx(0.5)); + CHECK(clip.Layer() == 5); + CHECK(clip.End() == Approx(300)); + CHECK(timeline.CacheEpoch() > epoch); + // A subsequent edit replaces the curve and leaves the other axis intact. + x.AddPoint(500, 0.9, CONSTANT); + change["value"].removeMember("location_y"); + change["value"]["location_x"] = x.JsonValue(); + changes.clear(); + changes.append(change); + timeline.ApplyJsonDiff(changes.toStyledString()); + CHECK(clip.location_x.JsonValue() == x.JsonValue()); + CHECK(clip.location_y.JsonValue() == y.JsonValue()); + CHECK(clip.alpha.GetValue(1) == Approx(0.5)); + timeline.RemoveClip(&clip); +} From e9bc7831b3265f8eb2a0c67ec123aba44cfcf6d4 Mon Sep 17 00:00:00 2001 From: Jonathan Thomas Date: Thu, 24 Sep 2026 19:23:50 -0500 Subject: [PATCH 3/8] Fix Qt player frame handoff and seek races --- src/Qt/PlayerPrivate.cpp | 110 +++++++++++++++++++-------------- src/Qt/PlayerPrivate.h | 12 ++-- src/Qt/VideoPlaybackThread.cpp | 24 ++++--- src/Qt/VideoPlaybackThread.h | 4 ++ tests/QtPlayer.cpp | 77 +++++++++++++++++++++++ 5 files changed, 171 insertions(+), 56 deletions(-) diff --git a/src/Qt/PlayerPrivate.cpp b/src/Qt/PlayerPrivate.cpp index bacaef73a..3cc3d722d 100644 --- a/src/Qt/PlayerPrivate.cpp +++ b/src/Qt/PlayerPrivate.cpp @@ -23,8 +23,9 @@ namespace openshot int close_to_sync = 5; // Constructor PlayerPrivate::PlayerPrivate(openshot::RendererBase *rb) - : renderer(rb), Thread("player"), video_position(1), audio_position(0), - speed(1), reader(NULL), last_video_position(1), max_sleep_ms(125000), playback_frames(0), is_dirty(true) + : Thread("player"), playback_frames(0), video_position(1), audio_position(0), + reader(nullptr), speed(1), last_speed(1), renderer(rb), + last_video_position(1), max_sleep_ms(125000), is_dirty(true) { videoCache = new openshot::VideoCacheThread(); audioPlayback = new openshot::AudioPlaybackThread(videoCache); @@ -97,10 +98,16 @@ namespace openshot } // Get the current video frame - frame = getFrame(); + const uint64_t frame_seek_generation = seek_generation.load(); + auto frame_to_render = getFrame(); + // A seek during decoding invalidates the old frame. Do not hand + // that frame to the renderer or overwrite the new playhead. + if (seek_generation.load() != frame_seek_generation) + continue; + const int64_t rendered_position = video_position.load(); // Set the video frame on the video thread and render frame - videoPlayback->frame = frame; + videoPlayback->SetFrame(std::move(frame_to_render)); videoPlayback->rendered.reset(); videoPlayback->render.signal(); // Keep decode/position advancement aligned with actual preview updates. @@ -113,7 +120,11 @@ namespace openshot videoPlayback->rendered.wait(render_wait_ms); // Keep track of the last displayed frame - last_video_position = video_position; + { + std::lock_guard lock(frame_mutex); + if (seek_generation.load() == frame_seek_generation) + last_video_position = rendered_position; + } last_speed = speed; // Calculate the diff between 'now' and the predicted frame end time @@ -142,51 +153,60 @@ namespace openshot // Get the next displayed frame (based on speed and direction) std::shared_ptr PlayerPrivate::getFrame() { - try { - // Getting new frame, so clear this flag - is_dirty = false; - - // Get the next frame (based on speed) - if (video_position + speed >= 1 && video_position + speed <= reader->info.video_length) { - video_position = video_position + speed; - - } else if (video_position + speed < 1) { - // Start of reader (prevent negative frame number and pause playback) - video_position = 1; - speed = 0; - } else if (video_position + speed > reader->info.video_length) { - // End of reader (prevent negative frame number and pause playback) - video_position = reader->info.video_length; - speed = 0; - } - - if (frame && frame->number == video_position && video_position == last_video_position) { - // return cached frame - return frame; - } - else - { - // Increment playback frames (always in the positive direction) - playback_frames += std::abs(speed); - - // Update playhead hint for cache window tracking without triggering seek behavior. - videoCache->NotifyPlaybackPosition(video_position); - - // return frame from reader - return reader->GetFrame(video_position); - } - - } catch (const ReaderClosed & e) { - // ... - } catch (const OutOfBoundsFrame & e) { - // ... - } - return std::shared_ptr(); + int64_t position; + uint64_t generation; + ReaderBase* current_reader; + { + std::lock_guard lock(frame_mutex); + // Getting new frame, so clear this flag + is_dirty = false; + + // Get the next frame (based on speed) + const int current_speed = speed.load(); + const int64_t next_position = video_position.load() + current_speed; + if (next_position >= 1 && next_position <= reader->info.video_length) { + video_position = next_position; + } else if (next_position < 1) { + video_position = 1; + speed = 0; + } else { + video_position = reader->info.video_length; + speed = 0; + } + + position = video_position.load(); + if (frame && frame->number == position && position == last_video_position) + return frame; + // Increment playback frames (always in the positive direction) + playback_frames += std::abs(speed.load()); + // Update cache position before releasing the seek lock. + videoCache->NotifyPlaybackPosition(position); + current_reader = reader; + generation = seek_generation.load(); + } + + // Decoding can take much longer than a frame. Keep Seek responsive while + // it runs, then publish only if no newer seek invalidated the result. + std::shared_ptr decoded; + try { + decoded = current_reader->GetFrame(position); + } catch (const ReaderClosed &) { + } catch (const OutOfBoundsFrame &) { + } + { + std::lock_guard lock(frame_mutex); + if (seek_generation.load() != generation) + return {}; + frame = std::move(decoded); + return frame; + } } // Seek to a new position void PlayerPrivate::Seek(int64_t new_position) { + std::lock_guard lock(frame_mutex); + ++seek_generation; video_position = new_position; last_video_position = 0; // Drop local frame reference so same-frame refreshes cannot reuse stale diff --git a/src/Qt/PlayerPrivate.h b/src/Qt/PlayerPrivate.h index 303ba2c2e..e015bbe20 100644 --- a/src/Qt/PlayerPrivate.h +++ b/src/Qt/PlayerPrivate.h @@ -20,6 +20,8 @@ #include "../Qt/AudioPlaybackThread.h" #include "../Qt/VideoPlaybackThread.h" #include "../Qt/VideoCacheThread.h" +#include +#include namespace openshot { @@ -30,19 +32,21 @@ namespace openshot class PlayerPrivate : juce::Thread { std::shared_ptr frame; /// The current frame + std::mutex frame_mutex; /// Protects frame publication and seek state transitions + std::atomic seek_generation{0}; int64_t playback_frames; /// The # of frames since playback started - int64_t video_position; /// The current frame position. + std::atomic video_position; /// The current frame position. int64_t audio_position; /// The current frame position. openshot::ReaderBase *reader; /// The reader which powers this player openshot::AudioPlaybackThread *audioPlayback; /// The audio thread openshot::VideoPlaybackThread *videoPlayback; /// The video thread openshot::VideoCacheThread *videoCache; /// The cache thread - int speed; /// The speed and direction to playback a reader (1=normal, 2=fast, 3=faster, -1=rewind, etc...) + std::atomic speed; /// The speed and direction to playback a reader (1=normal, 2=fast, 3=faster, -1=rewind, etc...) int last_speed; /// The previous speed and direction (used to detect a change) openshot::RendererBase *renderer; - int64_t last_video_position; /// The last frame actually displayed + std::atomic last_video_position; /// The last frame actually displayed int max_sleep_ms; /// The max milliseconds to sleep (when syncing audio and video) - bool is_dirty; /// Detect if a frame needs to be refreshed (calls to Seek() set this to true) + std::atomic is_dirty; /// Detect if a frame needs to be refreshed (calls to Seek() set this to true) /// Constructor PlayerPrivate(openshot::RendererBase *rb); diff --git a/src/Qt/VideoPlaybackThread.cpp b/src/Qt/VideoPlaybackThread.cpp index e716244c9..5a50a4a4b 100644 --- a/src/Qt/VideoPlaybackThread.cpp +++ b/src/Qt/VideoPlaybackThread.cpp @@ -17,6 +17,7 @@ #include "Frame.h" #include "RendererBase.h" #include "Logger.h" +#include namespace openshot { @@ -35,10 +36,14 @@ namespace openshot // Get the currently playing frame number (if any) int64_t VideoPlaybackThread::getCurrentFramePosition() { - if (frame) - return frame->number; - else - return 0; + std::lock_guard lock(frame_mutex); + return frame ? frame->number : 0; + } + + void VideoPlaybackThread::SetFrame(std::shared_ptr next_frame) + { + std::lock_guard lock(frame_mutex); + frame = std::move(next_frame); } // Start the thread @@ -47,17 +52,22 @@ namespace openshot while (!threadShouldExit()) { // Make other threads wait on the render event bool need_render = render.wait(500); + std::shared_ptr frame_to_render; + if (need_render) { + std::lock_guard lock(frame_mutex); + frame_to_render = frame; + } - if (need_render && frame) + if (frame_to_render) { // Debug Logger::Instance()->AppendDebugMethod( "VideoPlaybackThread::run (before render)", - "frame->number", frame->number, + "frame->number", frame_to_render->number, "need_render", need_render); // Render the frame to the screen - renderer->paint(frame); + renderer->paint(frame_to_render); } // Signal to other threads that the rendered event has completed diff --git a/src/Qt/VideoPlaybackThread.h b/src/Qt/VideoPlaybackThread.h index 67bd55c73..dbd9eda7d 100644 --- a/src/Qt/VideoPlaybackThread.h +++ b/src/Qt/VideoPlaybackThread.h @@ -16,6 +16,8 @@ #include #include +#include +#include namespace openshot { @@ -31,6 +33,7 @@ namespace openshot { RendererBase *renderer; std::shared_ptr frame; + std::mutex frame_mutex; WaitableEvent render; WaitableEvent rendered; bool reset; @@ -42,6 +45,7 @@ namespace openshot /// Get the currently playing frame number (if any) int64_t getCurrentFramePosition(); + void SetFrame(std::shared_ptr next_frame); /// Start the thread void run(); diff --git a/tests/QtPlayer.cpp b/tests/QtPlayer.cpp index 1bc8fd5f6..87d87575d 100644 --- a/tests/QtPlayer.cpp +++ b/tests/QtPlayer.cpp @@ -13,7 +13,11 @@ #include "openshot_catch.h" #include +#include +#include +#include +#include "DummyReader.h" #include "QtPlayer.h" #include "Qt/VideoRenderer.h" @@ -36,6 +40,39 @@ class TestRenderer : public openshot::RendererBase (void) image; } }; + +class SlowRenderer : public openshot::RendererBase +{ +public: + std::atomic renders{0}; + void OverrideWidget(uintptr_t) override {} + +protected: + void render(std::shared_ptr image) override + { + if (image) + ++renders; + // Longer than the player's render timeout: the next frame may be + // submitted while this renderer still owns the previous one. + std::this_thread::sleep_for(std::chrono::milliseconds(120)); + } +}; + +class SlowReader : public openshot::DummyReader +{ +public: + std::atomic decoding{false}; + SlowReader() : DummyReader(openshot::Fraction(30, 1), 16, 16, 44100, 2, 30) {} + + std::shared_ptr GetFrame(int64_t frame_number) override + { + decoding = true; + std::this_thread::sleep_for(std::chrono::milliseconds(250)); + auto frame = DummyReader::GetFrame(frame_number); + decoding = false; + return frame; + } +}; } // namespace TEST_CASE("QtPlayer_GetRendererQObject_ReturnsVideoRendererAddress", "[libopenshot][qtplayer]") @@ -58,3 +95,43 @@ TEST_CASE("QtPlayer_SetQWidget_Overload_ForwardsPointer", "[libopenshot][qtplaye CHECK(renderer.last_widget == reinterpret_cast(widget)); } + +TEST_CASE("QtPlayer keeps frames alive during slow rendering and seeks", "[libopenshot][qtplayer][threading]") +{ + SlowRenderer renderer; + openshot::DummyReader reader(openshot::Fraction(30, 1), 16, 16, 44100, 2, 30); + reader.Open(); + openshot::QtPlayer player(&renderer); + player.Reader(&reader); + player.Play(); + + std::thread seeker([&player] { + for (int i = 0; i < 80; ++i) { + player.Seek(1 + (i % 60), false); + std::this_thread::sleep_for(std::chrono::milliseconds(2)); + } + }); + seeker.join(); + std::this_thread::sleep_for(std::chrono::milliseconds(350)); + player.Stop(); + CHECK(renderer.renders.load() > 0); +} + +TEST_CASE("QtPlayer seek does not wait for a slow frame decode", "[libopenshot][qtplayer][threading]") +{ + TestRenderer renderer; + SlowReader reader; + reader.Open(); + openshot::QtPlayer player(&renderer); + player.Reader(&reader); + player.Play(); + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(2); + while (!reader.decoding && std::chrono::steady_clock::now() < deadline) + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + REQUIRE(reader.decoding.load()); + const auto start = std::chrono::steady_clock::now(); + player.Seek(20, false); + const auto elapsed = std::chrono::steady_clock::now() - start; + CHECK(elapsed < std::chrono::milliseconds(100)); + player.Stop(); +} From 03b66ebe921dabd8932e8776010de14247953f9c Mon Sep 17 00:00:00 2001 From: Jonathan Thomas Date: Fri, 25 Sep 2026 12:10:59 -0500 Subject: [PATCH 4/8] Fix transition routing and stacking for clips with tied start times Preserve clip order for equal layer and position values, and route video and audio transitions according to compositing order. Cover fade directions, rounded start frames, audio gains, and JSON order preservation. --- src/Timeline.cpp | 17 ++---- src/Timeline.h | 5 +- tests/Timeline.cpp | 140 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 149 insertions(+), 13 deletions(-) diff --git a/src/Timeline.cpp b/src/Timeline.cpp index 48c9b2608..72811719f 100644 --- a/src/Timeline.cpp +++ b/src/Timeline.cpp @@ -1084,8 +1084,9 @@ std::shared_ptr Timeline::GetFrame(int64_t requested_frame) if (!ci.intersects) continue; const int layer = ci.clip->Layer(); auto it = top_start_for_layer.find(layer); - if (it == top_start_for_layer.end() || ci.start_pos > it->second) { - top_start_for_layer[layer] = ci.start_pos; // strictly greater to match prior logic + // The last composited clip wins ties at the same start frame. + if (it == top_start_for_layer.end() || ci.start_pos >= it->second) { + top_start_for_layer[layer] = ci.start_pos; top_clip_for_layer[layer] = ci.clip; } } @@ -1956,15 +1957,9 @@ std::pair Timeline::ResolveTransitionAudioGains(Clip* source_clip, // Keep the current top/non-top clip routing intact when two clips overlap. if (audible_clips.size() == 2) { - auto top_it = std::max_element( - audible_clips.begin(), - audible_clips.end(), - [](const AudibleClipInfo& lhs, const AudibleClipInfo& rhs) { - if (lhs.start_pos != rhs.start_pos) - return lhs.start_pos < rhs.start_pos; - return std::less()(lhs.clip, rhs.clip); - }); - if ((is_top_clip && source_clip != top_it->clip) || (!is_top_clip && source_clip == top_it->clip)) + // Collected in compositing order, including equal-position ties. + Clip* top_clip = audible_clips.back().clip; + if ((is_top_clip && source_clip != top_clip) || (!is_top_clip && source_clip == top_clip)) return {1.0f, 1.0f}; } diff --git a/src/Timeline.h b/src/Timeline.h index d797f0744..e2deaee8c 100644 --- a/src/Timeline.h +++ b/src/Timeline.h @@ -53,8 +53,9 @@ namespace openshot { return lhs->Layer() < rhs->Layer(); if (lhs->Position() != rhs->Position()) return lhs->Position() < rhs->Position(); - // Stable tie-breaker on address to avoid equivalence when layer/position match - return std::less()(lhs, rhs); + // list::sort is stable: preserve compositing order for tied clips, + // including their project order when loading a saved timeline. + return false; } }; diff --git a/tests/Timeline.cpp b/tests/Timeline.cpp index 77019db99..0249df6d3 100644 --- a/tests/Timeline.cpp +++ b/tests/Timeline.cpp @@ -187,6 +187,99 @@ class TimelineSolidColorReader : public ReaderBase { void SetJsonValue(const Json::Value root) override { ReaderBase::SetJsonValue(root); } }; +TEST_CASE("Transition follows compositing order for clips at the same position", "[libopenshot][timeline][mask][same-position]") +{ + TimelineSolidColorReader red(32, 32, 30, 1, 1800, QColor(255, 0, 0)); + TimelineSolidColorReader blue(32, 32, 30, 1, 600, QColor(0, 0, 255)); + Clip first, second; + // Exercise both address orders: insertion order must determine visibility. + Clip* bottom = std::less()(&first, &second) ? &first : &second; + Clip* top = bottom == &first ? &second : &first; + if (GENERATE(false, true)) + std::swap(bottom, top); + bottom->Reader(&red); + top->Reader(&blue); + for (auto clip : {bottom, top}) { + clip->Layer(5); + clip->Position(197.0 / 30.0); + clip->End(10.0); + } + bottom->Start(9.2); + bottom->End(52.2); + // Different subframe positions can also resolve to the same start frame. + top->Position(top->Position() + GENERATE(0.0, 0.001)); + + Mask transition; + transition.MaskReader(new TimelineSolidColorReader(32, 32, 30, 1, 600, QColor(128, 128, 128))); + transition.Layer(5); + transition.Position(top->Position()); + transition.End(10.0); + transition.contrast = Keyframe(3.0); + bool fade_in = true; + SECTION("Fade in") {} + SECTION("Fade out") { fade_in = false; } + transition.brightness = Keyframe(fade_in ? 1.0 : -1.0); + transition.brightness.AddPoint(301, fade_in ? -1.0 : 1.0, LINEAR); + + Timeline timeline(32, 32, Fraction(30, 1), 44100, 2, LAYOUT_STEREO); + timeline.AddClip(bottom); + timeline.AddClip(top); + timeline.AddEffect(&transition); + timeline.Open(); + const QColor begin = timeline.GetFrame(198)->GetImage()->pixelColor(16, 16); + const QColor middle = timeline.GetFrame(348)->GetImage()->pixelColor(16, 16); + const QColor end = timeline.GetFrame(497)->GetImage()->pixelColor(16, 16); + CHECK(begin == (fade_in ? QColor(255, 0, 0) : QColor(0, 0, 255))); + CHECK(middle.red() == Approx(128).margin(3)); + CHECK(middle.blue() == Approx(128).margin(3)); + CHECK(end == (fade_in ? QColor(0, 0, 255) : QColor(255, 0, 0))); + CHECK(timeline.GetFrame(498)->GetImage()->pixelColor(16, 16) == QColor(255, 0, 0)); + timeline.Close(); +} + +TEST_CASE("Timeline preserves tied clip order through sorting and JSON reload", "[libopenshot][timeline][same-position][json]") +{ + DummyReader reader; + Clip first(&reader), second(&reader), earlier(&reader), higher(&reader); + Clip* bottom = std::less()(&first, &second) ? &first : &second; + Clip* top = bottom == &first ? &second : &first; + if (GENERATE(false, true)) + std::swap(bottom, top); + bottom->Id("bottom"); + top->Id("top"); + earlier.Id("earlier"); + higher.Id("higher"); + for (auto clip : {bottom, top, &earlier, &higher}) { + clip->Layer(5); + clip->Position(1.0); + clip->End(3.0); + } + earlier.Position(0.0); + higher.Layer(6); + higher.Position(0.0); + Timeline timeline(32, 32, Fraction(30, 1), 44100, 2, LAYOUT_STEREO); + // Persist source readers, as project JSON does, rather than FrameMapper wrappers. + timeline.AutoMapClips(false); + // Add out of layer/position order to verify those still take precedence. + for (auto clip : {&higher, bottom, top, &earlier}) + timeline.AddClip(clip); + timeline.SortTimeline(); + timeline.SortTimeline(); + const std::vector expected{"earlier", "bottom", "top", "higher"}; + auto clip_ids = [](Timeline& value) { + std::vector ids; + for (auto clip : value.Clips()) + ids.push_back(clip->Id()); + return ids; + }; + CHECK(clip_ids(timeline) == expected); + Timeline restored(32, 32, Fraction(30, 1), 44100, 2, LAYOUT_STEREO); + restored.SetJson(timeline.Json()); + CHECK(clip_ids(restored) == expected); + restored.SortTimeline(); + CHECK(clip_ids(restored) == expected); +} + class TimelineConstantAudioReader : public ReaderBase { private: bool is_open = false; @@ -420,6 +513,53 @@ TEST_CASE("Timeline honors Mask fade_audio_hint with equal-power overlapping aud t.Close(); } +TEST_CASE("Transition audio follows compositing order for tied start frames", "[libopenshot][timeline][audio][transition][same-position]") +{ + TimelineConstantAudioReader bottom_reader(32, 32, 30, 1, 48000, 2, 90, 1.0f); + TimelineConstantAudioReader top_reader(32, 32, 30, 1, 48000, 2, 90, 1.0f); + Clip first, second; + Clip* bottom = std::less()(&first, &second) ? &first : &second; + Clip* top = bottom == &first ? &second : &first; + if (GENERATE(false, true)) + std::swap(bottom, top); + bottom->Reader(&bottom_reader); + top->Reader(&top_reader); + for (auto clip : {bottom, top}) { + clip->Layer(0); + clip->Position(1.0); + clip->End(2.0); + } + top->Position(top->Position() + GENERATE(0.0, 0.001)); + bottom->channel_filter = Keyframe(0.0); + top->channel_filter = Keyframe(1.0); + Mask transition; + transition.Layer(0); + transition.Position(1.0); + transition.End(1.0); + transition.fade_audio_hint = GENERATE(false, true); + Timeline timeline(32, 32, Fraction(30, 1), 48000, 2, LAYOUT_STEREO); + timeline.AddClip(bottom); + timeline.AddClip(top); + timeline.AddEffect(&transition); + timeline.Open(); + for (int number : {31, 45, 60}) { + auto frame = timeline.GetFrame(number); + REQUIRE(frame->GetAudioSamplesCount() > 0); + // Both clips begin at the left transition edge, so both fade in. + // Channel isolation ensures neither clip silently bypasses its gain. + const double previous = transition.fade_audio_hint + ? expected_equal_power_gain(number - 1, 31, 60, true) : 1.0; + const double current = transition.fade_audio_hint + ? expected_equal_power_gain(number, 31, 60, true) : 1.0; + for (int channel : {0, 1}) { + CHECK(frame->GetAudioSamples(channel)[0] == Approx(previous).margin(0.0002)); + CHECK(frame->GetAudioSamples(channel)[frame->GetAudioSamplesCount() - 1] + == Approx(current).margin(0.002)); + } + } + timeline.Close(); +} + TEST_CASE("Timeline uses transition edge proximity for single-clip fade audio", "[libopenshot][timeline][audio][transition][single]") { const Fraction fps(30, 1); const int sample_rate = 48000; From a2e1297aa40f8165344d3939845fd10f1d5d545b Mon Sep 17 00:00:00 2001 From: Jonathan Thomas Date: Fri, 25 Sep 2026 19:43:06 -0500 Subject: [PATCH 5/8] Fix preview alignment, export timestamps, and capture color range Align reduced preview dimensions after aspect fitting while preserving native output sizes. Pad export scaler rows and copy planes by stride, including compatibility handling for legacy GIF palettes. Preserve delayed audio encoder timestamps, skip unavailable packets, and use actual resampler output counts. Share JPEG-range normalization with live capture and add sizing, export, audio, GIF, and color regression tests. --- src/FFmpegColorRange.h | 30 ++++++++++++ src/FFmpegReader.cpp | 36 ++++---------- src/FFmpegWriter.cpp | 69 ++++++++++++++++++--------- src/PreviewSize.h | 19 ++++++++ src/ReaderBase.h | 1 + src/ScreenCaptureReader.cpp | 9 +++- src/Timeline.cpp | 12 ++++- src/Timeline.h | 4 +- tests/FFmpegReader.cpp | 62 ++++++++++++++++++++++++ tests/FFmpegWriter.cpp | 90 +++++++++++++++++++++++++++++++++-- tests/ScreenCaptureReader.cpp | 37 ++++++++++++++ tests/Timeline.cpp | 49 +++++++++++++++++++ 12 files changed, 359 insertions(+), 59 deletions(-) create mode 100644 src/FFmpegColorRange.h create mode 100644 src/PreviewSize.h diff --git a/src/FFmpegColorRange.h b/src/FFmpegColorRange.h new file mode 100644 index 000000000..88be5cb2e --- /dev/null +++ b/src/FFmpegColorRange.h @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: LGPL-3.0-or-later +#pragma once + +#include "FFmpegUtilities.h" + +// Normalize deprecated JPEG-range YUVJ formats before creating swscale contexts. +// swscale expects non-YUVJ formats plus explicit color-range metadata. +inline AVPixelFormat NormalizeDeprecatedPixFmt(AVPixelFormat pix_fmt, bool& is_full_range) { + switch (pix_fmt) { + case AV_PIX_FMT_YUVJ420P: + is_full_range = true; + return AV_PIX_FMT_YUV420P; + case AV_PIX_FMT_YUVJ422P: + is_full_range = true; + return AV_PIX_FMT_YUV422P; + case AV_PIX_FMT_YUVJ444P: + is_full_range = true; + return AV_PIX_FMT_YUV444P; + case AV_PIX_FMT_YUVJ440P: + is_full_range = true; + return AV_PIX_FMT_YUV440P; +#ifdef AV_PIX_FMT_YUVJ411P + case AV_PIX_FMT_YUVJ411P: + is_full_range = true; + return AV_PIX_FMT_YUV411P; +#endif + default: + return pix_fmt; + } +} diff --git a/src/FFmpegReader.cpp b/src/FFmpegReader.cpp index f6564a55e..ca32da3fd 100644 --- a/src/FFmpegReader.cpp +++ b/src/FFmpegReader.cpp @@ -23,6 +23,8 @@ #include #include "FFmpegUtilities.h" +#include "FFmpegColorRange.h" +#include "PreviewSize.h" #include "effects/CropHelpers.h" #include "FFmpegReader.h" @@ -75,31 +77,6 @@ int hw_de_on = 0; AVHWDeviceType hw_de_av_device_type_global = AV_HWDEVICE_TYPE_NONE; #endif -// Normalize deprecated JPEG-range YUVJ formats before creating swscale contexts. -// swscale expects non-YUVJ formats plus explicit color-range metadata. -static AVPixelFormat NormalizeDeprecatedPixFmt(AVPixelFormat pix_fmt, bool& is_full_range) { - switch (pix_fmt) { - case AV_PIX_FMT_YUVJ420P: - is_full_range = true; - return AV_PIX_FMT_YUV420P; - case AV_PIX_FMT_YUVJ422P: - is_full_range = true; - return AV_PIX_FMT_YUV422P; - case AV_PIX_FMT_YUVJ444P: - is_full_range = true; - return AV_PIX_FMT_YUV444P; - case AV_PIX_FMT_YUVJ440P: - is_full_range = true; - return AV_PIX_FMT_YUV440P; -#ifdef AV_PIX_FMT_YUVJ411P - case AV_PIX_FMT_YUVJ411P: - is_full_range = true; - return AV_PIX_FMT_YUV411P; -#endif - default: - return pix_fmt; - } -} FFmpegReader::FFmpegReader(const std::string &path, bool inspect_reader) : FFmpegReader(path, DurationStrategy::VideoPreferred, inspect_reader) {} @@ -2016,7 +1993,7 @@ void FFmpegReader::ProcessVideoPacket(int64_t requested_frame) { // Determine if image needs to be scaled (for performance reasons) int original_height = src_height; - if (max_width != 0 && max_height != 0 && max_width < width && max_height < height) { + if (max_width > 0 && max_height > 0 && (max_width < width || max_height < height)) { // Override width and height (but maintain aspect ratio) float ratio = float(width) / float(height); int possible_width = round(max_height * ratio); @@ -2031,6 +2008,13 @@ void FFmpegReader::ProcessVideoPacket(int64_t requested_frame) { width = max_width; height = possible_height; } + // Clip aspect ratios, animated scale/crop, and decode-size limits can + // undo timeline alignment. Align the final reduced RGB dimensions here. + if (src_width >= 4 && src_height >= 4) { + const QSize aligned = AlignPreviewSize(QSize(width, height)); + width = aligned.width(); + height = aligned.height(); + } } // Determine required buffer size and allocate buffer diff --git a/src/FFmpegWriter.cpp b/src/FFmpegWriter.cpp index a9adb9e04..012c2d3f2 100644 --- a/src/FFmpegWriter.cpp +++ b/src/FFmpegWriter.cpp @@ -945,7 +945,6 @@ void FFmpegWriter::flush_encoders() { if (pkt->duration <= 0) { pkt->duration = audio_codec_ctx->frame_size > 0 ? audio_codec_ctx->frame_size : audio_input_frame_size; } - const int64_t packet_duration = pkt->duration; av_packet_rescale_ts(pkt, audio_codec_ctx->time_base, audio_st->time_base); pkt->stream_index = audio_st->index; pkt->flags |= AV_PKT_FLAG_KEY; @@ -957,7 +956,7 @@ void FFmpegWriter::flush_encoders() { + av_err2string(error_code) + "]", "error_code", error_code); } - audio_timestamp += packet_duration; + // Flushing emits already-submitted samples; do not advance the input clock. AV_FREE_PACKET(pkt); } av_packet_free(&pkt); @@ -974,8 +973,7 @@ void FFmpegWriter::flush_encoders() { break; } - // Since the PTS can change during encoding, set the value again. This seems like a huge hack, - // but it fixes lots of PTS related issues when I do this. + // Legacy encoding API timestamp fallback. pkt->pts = pkt->dts = audio_timestamp; if (pkt->duration <= 0) { pkt->duration = audio_codec_ctx->frame_size > 0 ? audio_codec_ctx->frame_size : audio_input_frame_size; @@ -1801,7 +1799,7 @@ void FFmpegWriter::write_audio_packets(bool is_final, std::shared_ptrnb_samples = total_frame_samples / channels_in_frame; + audio_converted->nb_samples = total_frame_samples / info.channels; av_samples_alloc(audio_converted->data, audio_converted->linesize, info.channels, audio_converted->nb_samples, output_sample_fmt, 0); Logger::Instance()->AppendDebugMethod( @@ -1850,8 +1848,11 @@ void FFmpegWriter::write_audio_packets(bool is_final, std::shared_ptrnb_samples // number of input samples to convert ); - // Set remaining samples - remaining_frame_samples = total_frame_samples; + // Resampling can buffer samples internally. Only copy samples actually + // returned, not the estimated capacity (which can read past the buffer). + if (nb_samples < 0) + throw ErrorEncodingAudio("Could not resample audio", nb_samples); + remaining_frame_samples = nb_samples * info.channels; // Create a new array (to hold all resampled S16 audio samples) all_resampled_samples = (int16_t *) av_malloc( @@ -2109,11 +2110,14 @@ void FFmpegWriter::write_audio_packets(bool is_final, std::shared_ptrpts = pkt->dts = audio_timestamp; + if (error_code == 0 && got_packet_ptr > 0) { + + // Delayed encoders (AAC) return timestamps for earlier input frames. + // Preserve those timestamps, just as the flush path does. + if (pkt->pts == AV_NOPTS_VALUE) + pkt->pts = audio_timestamp; + if (pkt->dts == AV_NOPTS_VALUE) + pkt->dts = pkt->pts; if (pkt->duration <= 0) { pkt->duration = frame_nb_samples; } @@ -2136,8 +2140,8 @@ void FFmpegWriter::write_audio_packets(bool is_final, std::shared_ptrdata[0])); @@ -2177,8 +2181,20 @@ AVFrame *FFmpegWriter::allocate_avframe(PixelFormat pix_fmt, int width, int heig // Create buffer (if not provided) if (!new_buffer) { + int palette_padding = 0; +#ifdef AV_PIX_FMT_FLAG_PSEUDOPAL + // Old av_frame_ref/av_image_copy still read a synthetic RGB8 palette, + // even though av_image_get_buffer_size excludes it. Keep it readable + // when the encoder takes a reference to this externally owned buffer. + if (av_pix_fmt_desc_get(pix_fmt)->flags & AV_PIX_FMT_FLAG_PSEUDOPAL) + palette_padding = 1024; +#endif // New Buffer - new_buffer = (uint8_t *) av_malloc(*buffer_size * sizeof(uint8_t)); + new_buffer = (uint8_t *) av_malloc(*buffer_size + palette_padding); + if (!new_buffer) + throw OutOfMemory("Could not allocate video frame buffer", path); + if (palette_padding) + memset(new_buffer + *buffer_size, 0, palette_padding); // Attach buffer to AVFrame AV_COPY_PICTURE_DATA(new_av_frame, new_buffer, pix_fmt, width, height); new_av_frame->width = width; @@ -2233,7 +2249,7 @@ void FFmpegWriter::process_video_packet(std::shared_ptr frame) { persistent_dst_frame->height = info.height; persistent_dst_size = av_image_get_buffer_size( - dst_fmt, info.width, info.height, 1 + dst_fmt, info.width, info.height, 32 ); if (persistent_dst_size < 0) throw ErrorEncodingVideo("Invalid destination image size", -1); @@ -2251,7 +2267,7 @@ void FFmpegWriter::process_video_packet(std::shared_ptr frame) { dst_fmt, info.width, info.height, - 1 + 32 ); } @@ -2305,12 +2321,19 @@ void FFmpegWriter::process_video_packet(std::shared_ptr frame) { if (!new_frame) throw OutOfMemory("Could not allocate new_frame via allocate_avframe", path); - // Copy persistent_dst_buffer → new_frame buffer - memcpy( - new_frame->data[0], - persistent_dst_buffer, - static_cast(bytes_final) - ); + // Copy visible pixels from padded scaler planes into the packed encoder frame. +#ifdef AV_PIX_FMT_FLAG_PSEUDOPAL + // Older FFmpeg's av_image_copy also copies a synthetic palette for RGB8 + // (GIF), although av_image_get_buffer_size no longer allocates that palette. + if (av_pix_fmt_desc_get(dst_fmt)->flags & AV_PIX_FMT_FLAG_PSEUDOPAL) { + av_image_copy_plane(new_frame->data[0], new_frame->linesize[0], + persistent_dst_frame->data[0], persistent_dst_frame->linesize[0], + av_image_get_linesize(dst_fmt, info.width, 0), info.height); + } else +#endif + av_image_copy(new_frame->data, new_frame->linesize, + const_cast(persistent_dst_frame->data), + persistent_dst_frame->linesize, dst_fmt, info.width, info.height); // Queue the deep‐copied frame for encoding add_avframe(frame, new_frame); diff --git a/src/PreviewSize.h b/src/PreviewSize.h new file mode 100644 index 000000000..9c0431ba8 --- /dev/null +++ b/src/PreviewSize.h @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: LGPL-3.0-or-later + +#ifndef OPENSHOT_PREVIEW_SIZE_H +#define OPENSHOT_PREVIEW_SIZE_H + +#include +#include + +namespace openshot { +// Apply only to reduced previews, after fitting the aspect ratio. Four RGBA +// pixels occupy 16 bytes. Align both axes so quarter-turn orientation also +// produces aligned rows. Tiny previews have a minimum size of 4x4. +inline QSize AlignPreviewSize(const QSize& size) { + return QSize(std::max(4, size.width() / 4 * 4), + std::max(4, size.height() / 4 * 4)); +} +} + +#endif diff --git a/src/ReaderBase.h b/src/ReaderBase.h index 304ae4cc9..1815ed623 100644 --- a/src/ReaderBase.h +++ b/src/ReaderBase.h @@ -97,6 +97,7 @@ namespace openshot void ParentClip(openshot::ClipBase* new_clip); /// Set an optional maximum decoded frame size. Use 0,0 to disable the limit. + /// Reduced FFmpeg previews align both dimensions to four pixels (minimum 4x4). void SetMaxDecodeSize(int width, int height); /// Return the current maximum decoded frame width (0 when unlimited). diff --git a/src/ScreenCaptureReader.cpp b/src/ScreenCaptureReader.cpp index 2ef556b5d..8c6ee32a0 100644 --- a/src/ScreenCaptureReader.cpp +++ b/src/ScreenCaptureReader.cpp @@ -11,6 +11,7 @@ // SPDX-License-Identifier: LGPL-3.0-or-later #include "ScreenCaptureReader.h" +#include "FFmpegColorRange.h" #include "CaptureAudioBuffer.h" #include @@ -1118,7 +1119,9 @@ std::shared_ptr ScreenCaptureReader::DecodeNextFrame(int64_t number) const int width = source_frame->width > 0 ? source_frame->width : info.width; const int height = source_frame->height > 0 ? source_frame->height : info.height; - const PixelFormat src_fmt = static_cast(source_frame->format); + bool src_full_range = source_frame->color_range == AVCOL_RANGE_JPEG; + const PixelFormat src_fmt = NormalizeDeprecatedPixFmt( + static_cast(source_frame->format), src_full_range); sws_context = sws_getCachedContext( sws_context, @@ -1136,6 +1139,10 @@ std::shared_ptr ScreenCaptureReader::DecodeNextFrame(int64_t number) throw InvalidFile("Unable to create capture pixel conversion context.", InputName()); } + const int* coefficients = sws_getCoefficients(SWS_CS_DEFAULT); + sws_setColorspaceDetails(sws_context, coefficients, src_full_range ? 1 : 0, + coefficients, 1, 0, 1 << 16, 1 << 16); + const int bytes_per_pixel = 4; const size_t buffer_size = static_cast(width) * height * bytes_per_pixel; unsigned char* buffer = static_cast(aligned_malloc(buffer_size)); diff --git a/src/Timeline.cpp b/src/Timeline.cpp index 72811719f..b06bc7df9 100644 --- a/src/Timeline.cpp +++ b/src/Timeline.cpp @@ -11,6 +11,7 @@ // SPDX-License-Identifier: LGPL-3.0-or-later #include "Timeline.h" +#include "PreviewSize.h" #include "CacheBase.h" #include "CacheDisk.h" @@ -1858,15 +1859,22 @@ void Timeline::ClearAllCache(bool deep) { BumpCacheEpoch(); } -// Set Max Image Size (used for performance optimization). Convenience function for setting -// Settings::Instance()->MAX_WIDTH and Settings::Instance()->MAX_HEIGHT. +// Set the preview size without changing the project's native dimensions. void Timeline::SetMaxSize(int width, int height) { + // Ignore transient invalid widget sizes (for example while hidden). + if (width <= 0 || height <= 0 || info.width <= 0 || info.height <= 0) + return; // Maintain aspect ratio regardless of what size is passed in QSize display_ratio_size = QSize(info.width, info.height); QSize proposed_size = QSize(std::min(width, info.width), std::min(height, info.height)); // Scale QSize up to proposed size display_ratio_size.scale(proposed_size, Qt::KeepAspectRatio); + // Preserve exact full-resolution output, including non-aligned profiles and + // tiny test images. Only reduced previews use the aligned sampling grid. + if (display_ratio_size != QSize(info.width, info.height) && + info.width >= 4 && info.height >= 4) + display_ratio_size = AlignPreviewSize(display_ratio_size); // Update preview settings preview_width = display_ratio_size.width(); diff --git a/src/Timeline.h b/src/Timeline.h index e2deaee8c..e70304125 100644 --- a/src/Timeline.h +++ b/src/Timeline.h @@ -352,8 +352,8 @@ namespace openshot { Json::Value JsonValue() const override; ///< Generate Json::Value for this object void SetJsonValue(const Json::Value root) override; ///< Load Json::Value into this object - /// Set Max Image Size (used for performance optimization). Convenience function for setting - /// Settings::Instance()->MAX_WIDTH and Settings::Instance()->MAX_HEIGHT. + /// Fit the preview within these bounds, aligning reduced dimensions to four + /// pixels (minimum 4x4). Ignore nonpositive bounds; preserve native output size. void SetMaxSize(int width, int height); /// @brief Apply a special formatted JSON object, which represents a change to the timeline (add, update, delete) diff --git a/tests/FFmpegReader.cpp b/tests/FFmpegReader.cpp index 4efd55236..16c5e12a7 100644 --- a/tests/FFmpegReader.cpp +++ b/tests/FFmpegReader.cpp @@ -314,6 +314,68 @@ TEST_CASE( "Max_Decode_Size_FFmpegReader", "[libopenshot][ffmpegreader]" ) r.Close(); } +TEST_CASE("Reduced decoded frames have aligned rows", "[libopenshot][ffmpegreader][preview-size]") +{ + for (const QSize bounds : {QSize(638, 359), QSize(640, 352), QSize(1280, 359), + QSize(638, 720), QSize(1, 1)}) { + CAPTURE(bounds.width(), bounds.height()); + FFmpegReader reader(std::string(TEST_MEDIA_PATH) + "sintel_trailer-720p.mp4"); + reader.SetMaxDecodeSize(bounds.width(), bounds.height()); + reader.Open(); + auto frame = reader.GetFrame(1); + REQUIRE(frame != nullptr); + CHECK(frame->GetWidth() % 4 == 0); + CHECK(frame->GetHeight() % 4 == 0); + CHECK(frame->GetWidth() >= 4); + CHECK(frame->GetHeight() >= 4); + CHECK(frame->GetWidth() <= std::max(4, bounds.width())); + CHECK(frame->GetHeight() <= std::max(4, bounds.height())); + CHECK(frame->GetImage()->bytesPerLine() % 16 == 0); + CHECK(reinterpret_cast(frame->GetPixels()) % 16 == 0); + reader.Close(); + } +} + +TEST_CASE("Clip fitting cannot undo preview alignment", "[libopenshot][ffmpegreader][preview-size]") +{ + FFmpegReader reader(std::string(TEST_MEDIA_PATH) + "sintel_trailer-720p.mp4"); + reader.Open(); + Clip clip(&reader); + // A 16:9 source in a portrait preview is fitted again by the reader. + Timeline timeline(1080, 1920, Fraction(30, 1), 44100, 2, LAYOUT_STEREO); + timeline.SetMaxSize(333, 591); + timeline.AddClip(&clip); + clip.scale_x = Keyframe(1.13); + clip.scale_y = Keyframe(1.07); + clip.Open(); + auto frame = reader.GetFrame(1); + CHECK(frame->GetWidth() < reader.info.width); + CHECK(frame->GetWidth() % 4 == 0); + CHECK(frame->GetHeight() % 4 == 0); + CHECK(frame->GetImage()->bytesPerLine() % 16 == 0); + clip.Close(); + timeline.Close(); +} + +TEST_CASE("Quarter-turn previews retain aligned rows", "[libopenshot][ffmpegreader][preview-size]") +{ + FFmpegReader reader(std::string(TEST_MEDIA_PATH) + "sintel_trailer-720p.mp4"); + reader.SetMaxDecodeSize(359, 638); + reader.ApplyOrientationMetadata(true); + reader.Open(); + // Model the display orientation discovered from container metadata. + reader.source_rotation = 90; + std::swap(reader.info.width, reader.info.height); + auto frame = reader.GetFrame(1); + CHECK(frame->GetWidth() <= 359); + CHECK(frame->GetHeight() <= 638); + CHECK(frame->GetHeight() > frame->GetWidth()); + CHECK(frame->GetWidth() % 4 == 0); + CHECK(frame->GetHeight() % 4 == 0); + CHECK(frame->GetImage()->bytesPerLine() % 16 == 0); + reader.Close(); +} + TEST_CASE( "Seek", "[libopenshot][ffmpegreader]" ) { // Create a reader diff --git a/tests/FFmpegWriter.cpp b/tests/FFmpegWriter.cpp index 87c318d06..7109b7b25 100644 --- a/tests/FFmpegWriter.cpp +++ b/tests/FFmpegWriter.cpp @@ -47,6 +47,7 @@ AVStream* first_video_stream(AVFormatContext* format_context) TEST_CASE("Raw video export preserves all color planes and frame ownership", "[libopenshot][ffmpegwriter][rawvideo]") { + const int width = GENERATE(64, 1080); QTemporaryDir directory; REQUIRE(directory.isValid()); // NUT uses a different stream time base from the codec's frame rate. @@ -55,11 +56,11 @@ TEST_CASE("Raw video export preserves all color planes and frame ownership", // Multiple frames and repeated exports exercise packet/frame cleanup. for (int pass = 0; pass < 2; ++pass) { FFmpegWriter writer(path); - writer.SetVideoOptions(true, "rawvideo", Fraction(30, 1), 64, 64, + writer.SetVideoOptions(true, "rawvideo", Fraction(30, 1), width, 64, Fraction(1, 1), false, false, 1000000); writer.Open(); for (int number = 1; number <= 3; ++number) { - auto frame = std::make_shared(number, 64, 64, number == 2 ? "blue" : "red"); + auto frame = std::make_shared(number, width, 64, number == 2 ? "blue" : "red"); writer.WriteFrame(frame); } writer.Close(); @@ -75,7 +76,7 @@ TEST_CASE("Raw video export preserves all color planes and frame ownership", int packets = 0; while (av_read_frame(input, &packet) >= 0) { if (packet.stream_index == stream->index) { - CHECK(packet.size == 64 * 64 * 3 / 2); // Complete YUV420P image + CHECK(packet.size == width * 64 * 3 / 2); // Complete YUV420P image CHECK(packet.pts * av_q2d(stream->time_base) == Approx(packets / 30.0).margin(0.00001)); ++packets; } @@ -92,9 +93,9 @@ TEST_CASE("Raw video export preserves all color planes and frame ownership", CHECK(reader.info.video_length == 3); for (int number = 1; number <= 3; ++number) { auto frame = reader.GetFrame(number); - REQUIRE(frame->GetWidth() == 64); + REQUIRE(frame->GetWidth() == width); REQUIRE(frame->GetHeight() == 64); - const QColor color = frame->GetImage()->pixelColor(32, 32); + const QColor color = frame->GetImage()->pixelColor(width - 1, 63); CHECK(color.green() < 10); CHECK(color.red() == Approx(number == 2 ? 0 : 255).margin(10)); CHECK(color.blue() == Approx(number == 2 ? 255 : 0).margin(10)); @@ -103,6 +104,31 @@ TEST_CASE("Raw video export preserves all color planes and frame ownership", } } +TEST_CASE("GIF export copies padded RGB8 rows without a synthetic palette", + "[libopenshot][ffmpegwriter][padded-gif]") +{ + QTemporaryDir directory; + REQUIRE(directory.isValid()); + const std::string path = directory.filePath("padded.gif").toStdString(); + FFmpegWriter writer(path); + writer.SetVideoOptions(true, "gif", Fraction(25, 1), 1080, 32, + Fraction(1, 1), false, false, 1000000); + writer.Open(); + for (int number = 1; number <= 3; ++number) + writer.WriteFrame(std::make_shared(number, 1080, 32, "red")); + writer.Close(); + FFmpegReader reader(path); + reader.Open(); + auto frame = reader.GetFrame(1); + REQUIRE(frame->GetWidth() == 1080); + REQUIRE(frame->GetHeight() == 32); + const QColor color = frame->GetImage()->pixelColor(1079, 31); + CHECK(color.red() > 230); + CHECK(color.green() < 20); + CHECK(color.blue() < 20); + reader.Close(); +} + TEST_CASE( "Webm", "[libopenshot][ffmpegwriter]" ) { // Reader @@ -437,3 +463,57 @@ TEST_CASE( "SizeOrdering_vp9_CRF", "[libopenshot][ffmpegwriter][filesize]" ) reader.Close(); } + +TEST_CASE("AAC export preserves delayed packet timestamps through flush", + "[libopenshot][ffmpegwriter][audio-timestamps]") +{ + QTemporaryDir directory; + REQUIRE(directory.isValid()); + const std::string path = directory.filePath("aac.mp4").toStdString(); + const Fraction fps(30000, 1001); + const int source_rate = GENERATE(48000, 44100); + FFmpegWriter writer(path); + writer.SetVideoOptions(true, "libx264", fps, 16, 16, Fraction(1, 1), false, false, 100000); + writer.SetAudioOptions(true, "aac", 48000, 2, LAYOUT_STEREO, 160000); + writer.Open(); + int total_samples = 0; + for (int number = 1; number <= 230; ++number) { + const int samples = Frame::GetSamplesPerFrame(number, fps, source_rate, 2); + auto frame = std::make_shared(number, 16, 16, "red", samples, 2); + frame->SampleRate(source_rate); + frame->ChannelsLayout(LAYOUT_STEREO); + frame->AddAudioSilence(samples); + writer.WriteFrame(frame); + total_samples += samples; + } + writer.Close(); + AVFormatContext* input = nullptr; + REQUIRE(avformat_open_input(&input, path.c_str(), nullptr, nullptr) == 0); + std::unique_ptr guard( + input, [](AVFormatContext* context) { avformat_close_input(&context); }); + REQUIRE(avformat_find_stream_info(input, nullptr) >= 0); + const int index = av_find_best_stream(input, AVMEDIA_TYPE_AUDIO, -1, -1, nullptr, 0); + REQUIRE(index >= 0); + AVStream* stream = input->streams[index]; + AVPacket packet = {}; + int count = 0; + int64_t previous = AV_NOPTS_VALUE; + while (av_read_frame(input, &packet) >= 0) { + if (packet.stream_index == index) { + REQUIRE(packet.dts != AV_NOPTS_VALUE); + CHECK(packet.pts == packet.dts); + const int64_t sample_dts = av_rescale_q(packet.dts, stream->time_base, AVRational{1, 48000}); + if (count) CHECK(sample_dts - previous == 1024); + else CHECK(sample_dts == -1024); // AAC encoder priming + previous = sample_dts; + ++count; + } + av_packet_unref(&packet); + } + const int64_t expected_samples = av_rescale(total_samples, 48000, source_rate); + CHECK(count == (expected_samples + 1023) / 1024 + 1); + // MP4 edit-list durations round to milliseconds (48 samples). Rate + // conversion can also retain a short filter tail in the resampler. + CHECK(av_rescale_q(stream->duration, stream->time_base, AVRational{1, 48000}) + == Approx(expected_samples).margin(source_rate == 48000 ? 48 : 96)); +} diff --git a/tests/ScreenCaptureReader.cpp b/tests/ScreenCaptureReader.cpp index 13455e14e..e8cd29f76 100644 --- a/tests/ScreenCaptureReader.cpp +++ b/tests/ScreenCaptureReader.cpp @@ -16,6 +16,7 @@ #include "CaptureAudioBuffer.h" #include "ScreenCaptureReader.h" #include "WaylandBufferUtilities.h" +#include "FFmpegColorRange.h" #include #include @@ -24,6 +25,42 @@ using namespace openshot; +TEST_CASE("JPEG capture conversion preserves full-range black and white", + "[libopenshot][screencapturereader][color-range]") +{ + for (auto format : {AV_PIX_FMT_YUVJ420P, AV_PIX_FMT_YUVJ422P, + AV_PIX_FMT_YUVJ444P, AV_PIX_FMT_YUVJ440P}) { + bool full_range = false; + const auto normalized = NormalizeDeprecatedPixFmt(format, full_range); + CHECK(full_range); + CHECK(normalized != format); + uint8_t* source[4] = {}; + int strides[4] = {}; + const int size = av_image_alloc(source, strides, 16, 16, normalized, 32); + REQUIRE(size > 0); + memset(source[0], 128, size); + uint8_t* dest[4] = {}; + int dest_strides[4] = {}; + REQUIRE(av_image_alloc(dest, dest_strides, 16, 16, AV_PIX_FMT_RGBA, 32) > 0); + SwsContext* context = sws_getContext(16, 16, normalized, 16, 16, + AV_PIX_FMT_RGBA, SWS_BILINEAR, nullptr, nullptr, nullptr); + REQUIRE(context != nullptr); + const int* coefficients = sws_getCoefficients(SWS_CS_DEFAULT); + REQUIRE(sws_setColorspaceDetails(context, coefficients, full_range, coefficients, + 1, 0, 1 << 16, 1 << 16) >= 0); + for (int level : {0, 16, 235, 255}) { + for (int row = 0; row < 16; ++row) + memset(source[0] + row * strides[0], level, 16); + REQUIRE(sws_scale(context, source, strides, 0, 16, dest, dest_strides) == 16); + for (int channel = 0; channel < 3; ++channel) + CHECK(int(dest[0][channel]) == Approx(level).margin(2)); + } + sws_freeContext(context); + av_freep(&source[0]); + av_freep(&dest[0]); + } +} + TEST_CASE("PulseAudio and WASAPI clocks align to the same recording samples", "[libopenshot][screencapturereader][audio][sync]") { diff --git a/tests/Timeline.cpp b/tests/Timeline.cpp index 0249df6d3..e0b4a9aa2 100644 --- a/tests/Timeline.cpp +++ b/tests/Timeline.cpp @@ -37,6 +37,55 @@ using namespace openshot; +TEST_CASE("Preview sizes stay aligned after aspect fitting", "[libopenshot][timeline][preview-size]") +{ + for (const QSize native : {QSize(1920, 1080), QSize(1080, 1920), + QSize(1440, 1080), QSize(854, 480), QSize(427, 240)}) { + Timeline timeline(native.width(), native.height(), Fraction(30, 1), 44100, 2, LAYOUT_STEREO); + for (const QSize bounds : {QSize(640, 352), QSize(638, 359), QSize(333, 591), + QSize(799, 451), QSize(1, 1), QSize(0, 360), QSize(-1, 360)}) { + CAPTURE(native.width(), native.height(), bounds.width(), bounds.height()); + const QSize previous(timeline.preview_width, timeline.preview_height); + timeline.SetMaxSize(bounds.width(), bounds.height()); + if (bounds.width() <= 0 || bounds.height() <= 0) { + CHECK(QSize(timeline.preview_width, timeline.preview_height) == previous); + continue; + } + if (bounds.width() >= native.width() && bounds.height() >= native.height()) { + CHECK(QSize(timeline.preview_width, timeline.preview_height) == native); + continue; + } + CHECK(timeline.preview_width % 4 == 0); + CHECK(timeline.preview_height % 4 == 0); + CHECK(timeline.preview_width >= 4); + CHECK(timeline.preview_height >= 4); + CHECK(timeline.preview_width <= std::max(4, std::min(bounds.width(), native.width()))); + CHECK(timeline.preview_height <= std::max(4, std::min(bounds.height(), native.height()))); + QSize fitted = native.scaled(QSize(std::min(bounds.width(), native.width()), + std::min(bounds.height(), native.height())), Qt::KeepAspectRatio); + CHECK(std::abs(timeline.preview_width - fitted.width()) <= 4); + CHECK(std::abs(timeline.preview_height - fitted.height()) <= 4); + const QSize result(timeline.preview_width, timeline.preview_height); + timeline.SetMaxSize(bounds.width(), bounds.height()); + CHECK(QSize(timeline.preview_width, timeline.preview_height) == result); + } + // Restoring native size must not resize exports or saved full-size frames. + timeline.SetMaxSize(native.width(), native.height()); + CHECK(timeline.preview_width == native.width()); + CHECK(timeline.preview_height == native.height()); + CHECK(timeline.info.width == native.width()); + CHECK(timeline.info.height == native.height()); + } +} + +TEST_CASE("Native tiny timelines remain exact", "[libopenshot][timeline][preview-size]") +{ + Timeline timeline(2, 2, Fraction(30, 1), 44100, 2, LAYOUT_STEREO); + timeline.SetMaxSize(100, 100); + CHECK(timeline.preview_width == 2); + CHECK(timeline.preview_height == 2); +} + TEST_CASE("Deleting a JSON-owned clip invalidates its former range", "[libopenshot][timeline][sentry-delete]") { Timeline timeline(2, 2, Fraction(30, 1), 44100, 2, LAYOUT_STEREO); From 54ada2010d2f1e361af44b9c51c035917a6db7ef Mon Sep 17 00:00:00 2001 From: Jonathan Thomas Date: Sat, 26 Sep 2026 21:23:40 -0500 Subject: [PATCH 6/8] Fix Codacy findings in Timeline tests --- tests/Timeline.cpp | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/Timeline.cpp b/tests/Timeline.cpp index e0b4a9aa2..cc11be55d 100644 --- a/tests/Timeline.cpp +++ b/tests/Timeline.cpp @@ -10,6 +10,8 @@ // // SPDX-License-Identifier: LGPL-3.0-or-later +#include +#include #include #include #include @@ -264,9 +266,8 @@ TEST_CASE("Transition follows compositing order for clips at the same position", transition.Position(top->Position()); transition.End(10.0); transition.contrast = Keyframe(3.0); - bool fade_in = true; - SECTION("Fade in") {} - SECTION("Fade out") { fade_in = false; } + const bool fade_in = GENERATE(true, false); + CAPTURE(fade_in); transition.brightness = Keyframe(fade_in ? 1.0 : -1.0); transition.brightness.AddPoint(301, fade_in ? -1.0 : 1.0, LINEAR); @@ -316,9 +317,10 @@ TEST_CASE("Timeline preserves tied clip order through sorting and JSON reload", timeline.SortTimeline(); const std::vector expected{"earlier", "bottom", "top", "higher"}; auto clip_ids = [](Timeline& value) { + const auto clips = value.Clips(); std::vector ids; - for (auto clip : value.Clips()) - ids.push_back(clip->Id()); + std::transform(clips.begin(), clips.end(), std::back_inserter(ids), + [](const auto clip) { return clip->Id(); }); return ids; }; CHECK(clip_ids(timeline) == expected); From f92622179ff41e69af74064f69cc8ef1f43dc80d Mon Sep 17 00:00:00 2001 From: Jonathan Thomas Date: Sat, 26 Sep 2026 21:29:47 -0500 Subject: [PATCH 7/8] Add missing REUSE copyright and license metadata --- doc/JSON-KEYFRAME-BENCHMARK.md | 5 +++++ src/FFmpegColorRange.h | 1 + src/PreviewSize.h | 1 + 3 files changed, 7 insertions(+) diff --git a/doc/JSON-KEYFRAME-BENCHMARK.md b/doc/JSON-KEYFRAME-BENCHMARK.md index 047148947..90756c21b 100644 --- a/doc/JSON-KEYFRAME-BENCHMARK.md +++ b/doc/JSON-KEYFRAME-BENCHMARK.md @@ -1,3 +1,8 @@ + + # JSON keyframe edit benchmark Build `openshot-benchmark`, then run from the repository root: diff --git a/src/FFmpegColorRange.h b/src/FFmpegColorRange.h index 88be5cb2e..383f133a0 100644 --- a/src/FFmpegColorRange.h +++ b/src/FFmpegColorRange.h @@ -1,3 +1,4 @@ +// SPDX-FileCopyrightText: 2026 OpenShot Studios, LLC // SPDX-License-Identifier: LGPL-3.0-or-later #pragma once diff --git a/src/PreviewSize.h b/src/PreviewSize.h index 9c0431ba8..3c5832e9e 100644 --- a/src/PreviewSize.h +++ b/src/PreviewSize.h @@ -1,3 +1,4 @@ +// SPDX-FileCopyrightText: 2026 OpenShot Studios, LLC // SPDX-License-Identifier: LGPL-3.0-or-later #ifndef OPENSHOT_PREVIEW_SIZE_H From 1f54aa12791ee4422c7e6869ddb331740804aa23 Mon Sep 17 00:00:00 2001 From: Jonathan Thomas Date: Sun, 27 Sep 2026 01:45:30 -0500 Subject: [PATCH 8/8] Preserve imported clip location coordinate conventions Persist canvas or geometry location units per clip so legacy animation is evaluated without rewriting keyframes. Keep auto behavior for new clips and add historical render and JSON regression coverage. --- src/Clip.cpp | 13 +++++++- src/Clip.h | 3 ++ tests/Clip.cpp | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 1 deletion(-) diff --git a/src/Clip.cpp b/src/Clip.cpp index 723fff0e4..90e8eee81 100644 --- a/src/Clip.cpp +++ b/src/Clip.cpp @@ -78,6 +78,7 @@ void Clip::init_settings() ClipBase::End(0.0); gravity = GRAVITY_CENTER; scale = SCALE_FIT; + location_coordinate_system = "auto"; anchor = ANCHOR_CANVAS; display = FRAME_DISPLAY_NONE; mixing = VOLUME_MIX_NONE; @@ -969,6 +970,7 @@ Json::Value Clip::JsonValue() const { root["parentObjectId"] = parentObjectId; root["gravity"] = gravity; root["scale"] = scale; + root["location_coordinate_system"] = location_coordinate_system; root["anchor"] = anchor; root["display"] = display; root["mixing"] = mixing; @@ -1085,6 +1087,12 @@ void Clip::SetJsonValue(Json::Value root) { gravity = (GravityType) root["gravity"].asInt(); if (!root["scale"].isNull()) scale = (ScaleType) root["scale"].asInt(); + if (!root["location_coordinate_system"].isNull()) { + const auto& coordinates = root["location_coordinate_system"]; + location_coordinate_system = coordinates.isString() ? coordinates.asString() : "auto"; + if (location_coordinate_system != "canvas" && location_coordinate_system != "geometry") + location_coordinate_system = "auto"; + } if (!root["anchor"].isNull()) anchor = (AnchorType) root["anchor"].asInt(); if (!root["display"].isNull()) @@ -1693,7 +1701,10 @@ QTransform Clip::get_transform(std::shared_ptr frame, int width, int heig } return location * (canvas_size - anchored_position); }; - if (scale == SCALE_CROP) { + // Preserve imported location curves in their original units. Converting only + // their keyframes cannot preserve animated scale/margin or reader resizing. + if (location_coordinate_system == "geometry" || + (location_coordinate_system != "canvas" && scale == SCALE_CROP)) { x += location_offset(location_x_value, x - layout_x, layout_width, scaled_source_width); y += location_offset(location_y_value, y - layout_y, layout_height, scaled_source_height); } else { diff --git a/src/Clip.h b/src/Clip.h index 8901141e9..072cb0c4e 100644 --- a/src/Clip.h +++ b/src/Clip.h @@ -184,6 +184,9 @@ namespace openshot { public: openshot::GravityType gravity; ///< The gravity of a clip determines where it snaps to its parent openshot::ScaleType scale; ///< The scale determines how a clip should be resized to fit its parent + /// Location units retained by imported projects: "auto", "canvas", or "geometry". + /// Auto uses geometry-relative units for Crop and canvas-relative units otherwise. + std::string location_coordinate_system; openshot::AnchorType anchor; ///< The anchor determines what parent a clip should snap to openshot::FrameDisplayType display; ///< The format to display the frame number (if any) openshot::VolumeMixType mixing; ///< What strategy should be followed when mixing audio with other clips diff --git a/tests/Clip.cpp b/tests/Clip.cpp index cf4c10391..413cd2fbb 100644 --- a/tests/Clip.cpp +++ b/tests/Clip.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -1973,3 +1974,93 @@ TEST_CASE("Reverse time curve (sample-exact, no resampling)", "[libopenshot][cli r.Close(); cache.Clear(); } + +TEST_CASE("Location coordinate convention survives JSON and partial updates", "[libopenshot][clip][json][location-compat]") +{ + Clip clip; + CHECK(clip.location_coordinate_system == "auto"); + for (const std::string coordinates : {"canvas", "geometry"}) { + clip.SetJson("{\"location_coordinate_system\":\"" + coordinates + "\"}"); + clip.location_x.AddPoint(51, -0.25, BEZIER); + Clip restored; + restored.SetJson(clip.Json()); + CHECK(restored.location_coordinate_system == coordinates); + CHECK(restored.location_x.Json() == clip.location_x.Json()); + restored.SetJson("{\"scale\":0}"); + CHECK(restored.location_coordinate_system == coordinates); + } + clip.SetJson("{\"location_coordinate_system\":\"unknown\"}"); + CHECK(clip.location_coordinate_system == "auto"); + clip.SetJson("{\"location_coordinate_system\":123}"); + CHECK(clip.location_coordinate_system == "auto"); +} + +TEST_CASE("Imported locations match historical rendered image bounds", "[libopenshot][clip][transform][location-compat]") +{ + // Bounds measured from unmodified v0.7.0 (canvas) and v1.0.0 (geometry) + // engines, using QtImageReader. In particular SCALE_NONE must use the + // decoded image dimensions, not dimensions guessed from reader metadata. + struct HistoricalCase { + ScaleType scale; + bool animated; + const char* coordinates; + int bounds[3][4]; // left, top, right, bottom at frames 1, 51, 101 + }; + const HistoricalCase cases[] = { + {SCALE_FIT, false, "canvas", {{160,63,319,152},{160,63,319,152},{160,63,319,152}}}, + {SCALE_FIT, true, "canvas", {{160,63,319,152},{120,41,319,175},{80,18,319,179}}}, + {SCALE_STRETCH, false, "canvas", {{160,63,319,152},{160,63,319,152},{160,63,319,152}}}, + {SCALE_STRETCH, true, "canvas", {{160,63,319,152},{120,41,319,175},{80,18,319,179}}}, + {SCALE_CROP, false, "canvas", {{160,63,319,152},{160,63,319,152},{160,63,319,152}}}, + {SCALE_CROP, true, "canvas", {{160,63,319,152},{120,41,319,175},{80,18,319,179}}}, + {SCALE_NONE, false, "canvas", {{220,97,259,118},{220,97,259,118},{220,97,259,118}}}, + {SCALE_NONE, true, "canvas", {{200,86,279,130},{180,74,299,141},{160,63,319,152}}}, + {SCALE_FIT, false, "geometry", {{140,59,299,148},{140,59,299,148},{140,59,299,148}}}, + {SCALE_FIT, true, "geometry", {{140,59,299,148},{110,38,319,172},{80,18,319,179}}}, + {SCALE_STRETCH, false, "geometry", {{140,59,299,148},{140,59,299,148},{140,59,299,148}}}, + {SCALE_STRETCH, true, "geometry", {{140,59,299,148},{110,38,319,172},{80,18,319,179}}}, + {SCALE_CROP, false, "geometry", {{140,59,299,148},{140,59,299,148},{140,59,299,148}}}, + {SCALE_CROP, true, "geometry", {{140,59,299,148},{110,38,319,172},{80,18,319,179}}}, + {SCALE_NONE, false, "geometry", {{185,89,224,110},{185,89,224,110},{185,89,224,110}}}, + {SCALE_NONE, true, "geometry", {{170,79,249,123},{155,69,274,135},{140,59,299,148}}}, + }; + QTemporaryDir directory; + REQUIRE(directory.isValid()); + const QString path = directory.filePath("source.png"); + QImage source(160, 90, QImage::Format_RGBA8888); + source.fill(Qt::red); + REQUIRE(source.save(path)); + for (const auto& test : cases) { + INFO("scale=" << test.scale << " animated=" << test.animated << " units=" << test.coordinates); + QtImageReader reader(path.toStdString()); + reader.Open(); + Clip clip(&reader); + clip.End(4.0); + clip.Layer(1); + clip.scale = test.scale; + clip.location_coordinate_system = test.coordinates; + clip.location_x = Keyframe(0.25); + clip.location_y = Keyframe(0.1); + clip.scale_x = Keyframe(0.5); + clip.scale_y = Keyframe(0.5); + if (test.animated) { + clip.scale_x.AddPoint(101, 1.0, LINEAR); + clip.scale_y.AddPoint(101, 1.0, LINEAR); + } + Timeline timeline(320, 180, Fraction(30, 1), 44100, 2, LAYOUT_STEREO); + timeline.AddClip(&clip); + timeline.SetMaxSize(320, 180); + timeline.Open(); + for (int index = 0; index < 3; ++index) { + const int frame = 1 + 50 * index; + INFO("frame=" << frame); + const QRect bounds = red_bounds(*timeline.GetFrame(frame)->GetImage()); + REQUIRE_FALSE(bounds.isNull()); + CHECK(bounds.left() == Approx(test.bounds[index][0]).margin(1)); + CHECK(bounds.top() == Approx(test.bounds[index][1]).margin(1)); + CHECK(bounds.right() == Approx(test.bounds[index][2]).margin(1)); + CHECK(bounds.bottom() == Approx(test.bounds[index][3]).margin(1)); + } + timeline.Close(); + } +}