From 5ddc6a3b3a16a312571001989f3483c511c453b2 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Tue, 19 Nov 2019 20:37:22 +0100 Subject: [PATCH 01/36] Keyframe::FlipPoints() without temporary vector The Y coordinate of each point is adjusted inplace; reduces memory overhead and iteration count for the loop. --- src/KeyFrame.cpp | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index d86ebc41..60114e6a 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -29,6 +29,7 @@ */ #include "../include/KeyFrame.h" +#include using namespace std; using namespace openshot; @@ -904,17 +905,12 @@ void Keyframe::ScalePoints(double scale) void Keyframe::FlipPoints() { // Loop through each point - std::vector FlippedPoints; - for (int64_t point_index = 0, reverse_index = Points.size() - 1; point_index < Points.size(); point_index++, reverse_index--) { + for (int64_t point_index = 0, reverse_index = Points.size() - 1; point_index < reverse_index; point_index++, reverse_index--) { // Flip the points - Point p = Points[point_index]; - p.co.Y = Points[reverse_index].co.Y; - FlippedPoints.push_back(p); + using std::swap; + swap(Points[point_index].co.Y, Points[reverse_index].co.Y); } - // Swap vectors - Points.swap(FlippedPoints); - // Mark for re-processing needs_update = true; } From 2b18ad099b3336a0f231fe5f86662eaea95251d1 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Tue, 19 Nov 2019 20:50:56 +0100 Subject: [PATCH 02/36] Keyframe::ScalePoints() skip first point without branch in loop --- src/KeyFrame.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index 60114e6a..c7aa9091 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -888,11 +888,7 @@ double Keyframe::Bernstein(int64_t n, int64_t i, double t) { void Keyframe::ScalePoints(double scale) { // Loop through each point (skipping the 1st point) - for (int64_t point_index = 0; point_index < Points.size(); point_index++) { - // Skip the 1st point - if (point_index == 0) - continue; - + for (int64_t point_index = 1; point_index < Points.size(); point_index++) { // Scale X value Points[point_index].co.X = round(Points[point_index].co.X * scale); From 6226e9d8e953bacc80d3f6c46446b53e1f1df793 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Tue, 19 Nov 2019 20:57:19 +0100 Subject: [PATCH 03/36] Keyframe::UpdatePoint() removed redundant code needs_update = true is set both by RemovePoint() and AddPoint(), ReorderPoints() is called by AddPoint(). --- src/KeyFrame.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index c7aa9091..f8fb19c2 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -587,17 +587,11 @@ void Keyframe::RemovePoint(int64_t index) { } void Keyframe::UpdatePoint(int64_t index, Point p) { - // mark as dirty - needs_update = true; - // Remove matching point RemovePoint(index); // Add new point AddPoint(p); - - // Reorder points - ReorderPoints(); } void Keyframe::PrintPoints() { From 5f7766e49e19e8472424f71db7e8162b154304cf Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Tue, 19 Nov 2019 20:59:47 +0100 Subject: [PATCH 04/36] Keyframe::RemovePoint() only set needs_update if a point was removed --- src/KeyFrame.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index f8fb19c2..3035a670 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -550,9 +550,6 @@ int64_t Keyframe::GetCount() { // Remove a point by matching a coordinate void Keyframe::RemovePoint(Point p) { - // mark as dirty - needs_update = true; - // loop through points, and find a matching coordinate for (int64_t x = 0; x < Points.size(); x++) { // Get each point @@ -562,6 +559,8 @@ void Keyframe::RemovePoint(Point p) { if (p.co.X == existing_point.co.X && p.co.Y == existing_point.co.Y) { // Remove the matching point, and break out of loop Points.erase(Points.begin() + x); + // mark as dirty + needs_update = true; return; } } @@ -572,12 +571,11 @@ void Keyframe::RemovePoint(Point p) { // Remove a point by index void Keyframe::RemovePoint(int64_t index) { - // mark as dirty - needs_update = true; - // Is index a valid point? if (index >= 0 && index < Points.size()) { + // mark as dirty + needs_update = true; // Remove a specific point by index Points.erase(Points.begin() + index); } From d47c40dc141e970165b0b8e11ba41a2fa76fec0d Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Tue, 19 Nov 2019 21:31:38 +0100 Subject: [PATCH 05/36] Keyframe::GetDelta() removed unused loop and variables --- src/KeyFrame.cpp | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index 3035a670..6c17d6af 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -479,9 +479,7 @@ double Keyframe::GetDelta(int64_t index) if (index >= 1 && (index + 1) < Values.size()) { int64_t current_value = GetLong(index); int64_t previous_value = 0; - int64_t next_value = 0; int64_t previous_repeats = 0; - int64_t next_repeats = 0; // Loop backwards and look for the next unique value for (std::vector::iterator backwards_it = Values.begin() + index; backwards_it != Values.begin(); backwards_it--) { @@ -495,18 +493,6 @@ double Keyframe::GetDelta(int64_t index) } } - // Loop forwards and look for the next unique value - for (std::vector::iterator forwards_it = Values.begin() + (index + 1); forwards_it != Values.end(); forwards_it++) { - next_value = long(round((*forwards_it).Y)); - if (next_value == current_value) { - // Found same value - next_repeats++; - } else { - // Found non repeating value, no more repeats found - break; - } - } - // Check for matching previous value (special case for 1st element) if (current_value == previous_value) previous_value = 0; From 280504f0075950da17b80dd6475602e19c8913b1 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Tue, 19 Nov 2019 21:36:26 +0100 Subject: [PATCH 06/36] Keyframe::IsIncreasing() remove loop to previous values and counter The previous values do not influence the return value of the function nor does looping over them have any side effect. The next_repeats value is not used in any way, thus it doesn't need to be calculated. --- src/KeyFrame.cpp | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index 6c17d6af..aa544c9c 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -302,31 +302,12 @@ bool Keyframe::IsIncreasing(int index) // Is index a valid point? if (index >= 1 && (index + 1) < Values.size()) { int64_t current_value = GetLong(index); - int64_t previous_value = 0; int64_t next_value = 0; - int64_t previous_repeats = 0; - int64_t next_repeats = 0; - - // Loop backwards and look for the next unique value - for (std::vector::iterator backwards_it = Values.begin() + index; backwards_it != Values.begin(); backwards_it--) { - previous_value = long(round((*backwards_it).Y)); - if (previous_value == current_value) { - // Found same value - previous_repeats++; - } else { - // Found non repeating value, no more repeats found - break; - } - } // Loop forwards and look for the next unique value for (std::vector::iterator forwards_it = Values.begin() + (index + 1); forwards_it != Values.end(); forwards_it++) { next_value = long(round((*forwards_it).Y)); - if (next_value == current_value) { - // Found same value - next_repeats++; - } else { - // Found non repeating value, no more repeats found + if (next_value != current_value) { break; } } From 5ba0ecfb2b65fe84f1ff102863161e14aa5a5708 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Tue, 19 Nov 2019 21:41:08 +0100 Subject: [PATCH 07/36] Keyframe::GetInt() and Keyframe::GetLong() use GetValue GetInt(), GetLong() are exactly the same as GetValue() except that their result is rounded and converted to the repsective data type. Thus avoid code duplication and use GetValue(). --- src/KeyFrame.cpp | 36 ++---------------------------------- 1 file changed, 2 insertions(+), 34 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index aa544c9c..100c108d 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -251,45 +251,13 @@ double Keyframe::GetValue(int64_t index) // Get the rounded INT value at a specific index int Keyframe::GetInt(int64_t index) { - // Check if it needs to be processed - if (needs_update) - Process(); - - // Is index a valid point? - if (index >= 0 && index < Values.size()) - // Return value - return int(round(Values[index].Y)); - else if (index < 0 && Values.size() > 0) - // Return the minimum value - return int(round(Values[0].Y)); - else if (index >= Values.size() && Values.size() > 0) - // return the maximum value - return int(round(Values[Values.size() - 1].Y)); - else - // return a blank coordinate (0,0) - return 0; + return int(round(GetValue(index))); } // Get the rounded INT value at a specific index int64_t Keyframe::GetLong(int64_t index) { - // Check if it needs to be processed - if (needs_update) - Process(); - - // Is index a valid point? - if (index >= 0 && index < Values.size()) - // Return value - return long(round(Values[index].Y)); - else if (index < 0 && Values.size() > 0) - // Return the minimum value - return long(round(Values[0].Y)); - else if (index >= Values.size() && Values.size() > 0) - // return the maximum value - return long(round(Values[Values.size() - 1].Y)); - else - // return a blank coordinate (0,0) - return 0; + return long(round(GetValue(index))); } // Get the direction of the curve at a specific index (increasing or decreasing) From d9322c100c1f95bcf5990653c8e891948e1a91d9 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Tue, 19 Nov 2019 22:01:47 +0100 Subject: [PATCH 08/36] Keyframe::ReorderPoints() use std::sort instead of selection sort With few points the performance doesn't differ that much; using std::sort removes the possibility of introducing bugs into the handmade sorting algorithm, though. --- src/KeyFrame.cpp | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index 100c108d..61b87146 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -29,6 +29,7 @@ */ #include "../include/KeyFrame.h" +#include #include using namespace std; @@ -38,22 +39,11 @@ using namespace openshot; // in ascending order based on the point.co.X value. This simplifies // processing the curve, due to all the points going from left to right. void Keyframe::ReorderPoints() { - // Loop through all coordinates, and sort them by the X attribute - for (int64_t x = 0; x < Points.size(); x++) { - int64_t compare_index = x; - int64_t smallest_index = x; - - for (int64_t compare_index = x + 1; compare_index < Points.size(); compare_index++) { - if (Points[compare_index].co.X < Points[smallest_index].co.X) { - smallest_index = compare_index; - } - } - - // swap items - if (smallest_index != compare_index) { - swap(Points[compare_index], Points[smallest_index]); - } - } + std::sort( + begin(Points), end(Points), + [](Point const & l, Point const & r) { + return l.co.X < r.co.X; + }); } // Constructor which sets the default point & coordinate at X=1 From cb5574180ea187109957311fc8b96a9d44cb9052 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Tue, 19 Nov 2019 22:27:29 +0100 Subject: [PATCH 09/36] Keyframe::AddPoint() add at correct index, keeping Points ordered AddPoint() now searches (binary search) for the best place to insert a new point, thus always maintaining the order of Points. Therefore, ReorderPoints() is no longer needed. CAVEAT: This breaks if some outside code changes (the public member) Points! --- include/KeyFrame.h | 7 ------- src/KeyFrame.cpp | 43 ++++++++++++++++++++++--------------------- 2 files changed, 22 insertions(+), 28 deletions(-) diff --git a/include/KeyFrame.h b/include/KeyFrame.h index a60958a6..9724cc31 100644 --- a/include/KeyFrame.h +++ b/include/KeyFrame.h @@ -66,13 +66,6 @@ namespace openshot { bool needs_update; double FactorialLookup[4]; - /* - * Because points can be added in any order, we need to reorder them - * in ascending order based on the point.co.X value. This simplifies - * processing the curve, due to all the points going from left to right. - */ - void ReorderPoints(); - // Process an individual segment void ProcessSegment(int Segment, Point p1, Point p2); diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index 61b87146..2d4760bc 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -35,16 +35,6 @@ using namespace std; using namespace openshot; -// Because points can be added in any order, we need to reorder them -// in ascending order based on the point.co.X value. This simplifies -// processing the curve, due to all the points going from left to right. -void Keyframe::ReorderPoints() { - std::sort( - begin(Points), end(Points), - [](Point const & l, Point const & r) { - return l.co.X < r.co.X; - }); -} // Constructor which sets the default point & coordinate at X=1 Keyframe::Keyframe(double value) : needs_update(true) { @@ -67,17 +57,28 @@ void Keyframe::AddPoint(Point p) { // mark as dirty needs_update = true; - // Check for duplicate point (and remove it) - Point closest = GetClosestPoint(p); - if (closest.co.X == p.co.X) - // Remove existing point - RemovePoint(closest); - - // Add point at correct spot - Points.push_back(p); - - // Sort / Re-order points based on X coordinate - ReorderPoints(); + // candidate is not less (greater or equal) than the new point in + // the X coordinate. + std::vector::iterator candidate = + std::lower_bound(begin(Points), end(Points), p, [](Point const & l, Point const & r) { + return l.co.X < r.co.X; + }); + if (candidate == end(Points)) { + // New point X is greater than all other points' X, add to + // back. + Points.push_back(p); + } else if ((*candidate).co.X == p.co.X) { + // New point is at same X coordinate as some point, overwrite + // point. + *candidate = p; + } else { + // New point needs to be inserted before candidate; thus move + // candidate and all following one to the right and insert new + // point then where candidate was. + Points.push_back(p); // Make space; could also be a dummy point. + std::move_backward(candidate, end(Points) - 1, end(Points)); + *candidate = p; + } } // Add a new point on the key-frame, with some defaults set (BEZIER) From 504ea0c1ff85f538c7fd8d88d8b01d29999076c6 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Tue, 19 Nov 2019 23:43:28 +0100 Subject: [PATCH 10/36] Make Keyframe::Values and Keyframe::Points vectors private The Values vector should only be accessed from the outside through the GetValue() function. The Points vector should only be accessed using the AddPoint(), RemovePoint(), .. functions. This helps maintain internal invariants (e.g. keeping Points sorted) and allows for future removal / lazy evaluation of Values. The size() of the vectors had been accessed from various parts of the code; the GetLength() (for Values) and GetCount() (for Points) member functions provide access to this information and are already part of the public API. --- include/KeyFrame.h | 4 ++-- src/Clip.cpp | 12 ++++++------ src/Timeline.cpp | 2 +- tests/KeyFrame_Tests.cpp | 12 ++++++------ 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/include/KeyFrame.h b/include/KeyFrame.h index 9724cc31..670f0f5e 100644 --- a/include/KeyFrame.h +++ b/include/KeyFrame.h @@ -65,6 +65,8 @@ namespace openshot { private: bool needs_update; double FactorialLookup[4]; + std::vector Points; ///< Vector of all Points + std::vector Values; ///< Vector of all Values (i.e. the processed coordinates from the curve) // Process an individual segment void ProcessSegment(int Segment, Point p1, Point p2); @@ -82,8 +84,6 @@ namespace openshot { double Bernstein(int64_t n, int64_t i, double t); public: - std::vector Points; ///< Vector of all Points - std::vector Values; ///< Vector of all Values (i.e. the processed coordinates from the curve) /// Default constructor for the Keyframe class Keyframe(); diff --git a/src/Clip.cpp b/src/Clip.cpp index e3f33a18..3ea21a70 100644 --- a/src/Clip.cpp +++ b/src/Clip.cpp @@ -109,10 +109,10 @@ void Clip::init_settings() // Init reader's rotation (if any) void Clip::init_reader_rotation() { // Only init rotation from reader when needed - if (rotation.Points.size() > 1) + if (rotation.GetCount() > 1) // Do nothing if more than 1 rotation Point return; - else if (rotation.Points.size() == 1 && rotation.GetValue(1) != 0.0) + else if (rotation.GetCount() == 1 && rotation.GetValue(1) != 0.0) // Do nothing if 1 Point, and it's not the default value return; @@ -273,7 +273,7 @@ void Clip::Close() float Clip::End() { // if a time curve is present, use its length - if (time.Points.size() > 1) + if (time.GetCount() > 1) { // Determine the FPS fo this clip float fps = 24.0; @@ -314,8 +314,8 @@ std::shared_ptr Clip::GetFrame(int64_t requested_frame) // Is a time map detected int64_t new_frame_number = requested_frame; int64_t time_mapped_number = adjust_frame_number_minimum(time.GetLong(requested_frame)); - if (time.Values.size() > 1) - new_frame_number = time_mapped_number; + if (time.GetLength() > 1) + new_frame_number = time_mapped_number; // Now that we have re-mapped what frame number is needed, go and get the frame pointer std::shared_ptr original_frame; @@ -397,7 +397,7 @@ void Clip::get_time_mapped_frame(std::shared_ptr frame, int64_t frame_num throw ReaderClosed("No Reader has been initialized for this Clip. Call Reader(*reader) before calling this method."); // Check for a valid time map curve - if (time.Values.size() > 1) + if (time.GetLength() > 1) { const GenericScopedLock lock(getFrameCriticalSection); diff --git a/src/Timeline.cpp b/src/Timeline.cpp index 51868157..5adec387 100644 --- a/src/Timeline.cpp +++ b/src/Timeline.cpp @@ -824,7 +824,7 @@ std::shared_ptr Timeline::GetFrame(int64_t requested_frame) ZmqLogger::Instance()->AppendDebugMethod("Timeline::GetFrame (Adding solid color)", "frame_number", frame_number, "info.width", info.width, "info.height", info.height); // Add Background Color to 1st layer (if animated or not black) - if ((color.red.Points.size() > 1 || color.green.Points.size() > 1 || color.blue.Points.size() > 1) || + if ((color.red.GetCount() > 1 || color.green.GetCount() > 1 || color.blue.GetCount() > 1) || (color.red.GetValue(frame_number) != 0.0 || color.green.GetValue(frame_number) != 0.0 || color.blue.GetValue(frame_number) != 0.0)) new_frame->AddColor(Settings::Instance()->MAX_WIDTH, Settings::Instance()->MAX_HEIGHT, color.GetColorHex(frame_number)); diff --git a/tests/KeyFrame_Tests.cpp b/tests/KeyFrame_Tests.cpp index 3c94fa22..519eba86 100644 --- a/tests/KeyFrame_Tests.cpp +++ b/tests/KeyFrame_Tests.cpp @@ -51,7 +51,7 @@ TEST(Keyframe_GetPoint_With_1_Points) k1.AddPoint(openshot::Point(2,3)); CHECK_THROW(k1.GetPoint(-1), OutOfBoundsPoint); - CHECK_EQUAL(1, k1.Points.size()); + CHECK_EQUAL(1, k1.GetCount()); CHECK_CLOSE(2.0f, k1.GetPoint(0).co.X, 0.00001); CHECK_CLOSE(3.0f, k1.GetPoint(0).co.Y, 0.00001); CHECK_THROW(k1.GetPoint(1), OutOfBoundsPoint); @@ -98,7 +98,7 @@ TEST(Keyframe_GetValue_For_Bezier_Curve_2_Points) CHECK_CLOSE(3.81602f, kf.GetValue(40), 0.0001); CHECK_CLOSE(4.0f, kf.GetValue(50), 0.0001); // Check the expected number of values - CHECK_EQUAL(kf.Values.size(), 51); + CHECK_EQUAL(kf.GetLength(), 51); } TEST(Keyframe_GetValue_For_Bezier_Curve_5_Points_40_Percent_Handle) @@ -121,7 +121,7 @@ TEST(Keyframe_GetValue_For_Bezier_Curve_5_Points_40_Percent_Handle) CHECK_CLOSE(1.77569f, kf.GetValue(177), 0.0001); CHECK_CLOSE(3.0f, kf.GetValue(200), 0.0001); // Check the expected number of values - CHECK_EQUAL(kf.Values.size(), 201); + CHECK_EQUAL(kf.GetLength(), 201); } TEST(Keyframe_GetValue_For_Bezier_Curve_5_Points_25_Percent_Handle) @@ -144,7 +144,7 @@ TEST(Keyframe_GetValue_For_Bezier_Curve_5_Points_25_Percent_Handle) CHECK_CLOSE(1.77569f, kf.GetValue(177), 0.0001); CHECK_CLOSE(3.0f, kf.GetValue(200), 0.0001); // Check the expected number of values - CHECK_EQUAL(kf.Values.size(), 201); + CHECK_EQUAL(kf.GetLength(), 201); } TEST(Keyframe_GetValue_For_Linear_Curve_3_Points) @@ -164,7 +164,7 @@ TEST(Keyframe_GetValue_For_Linear_Curve_3_Points) CHECK_CLOSE(4.4f, kf.GetValue(40), 0.0001); CHECK_CLOSE(2.0f, kf.GetValue(50), 0.0001); // Check the expected number of values - CHECK_EQUAL(kf.Values.size(), 51); + CHECK_EQUAL(kf.GetLength(), 51); } TEST(Keyframe_GetValue_For_Constant_Curve_3_Points) @@ -185,7 +185,7 @@ TEST(Keyframe_GetValue_For_Constant_Curve_3_Points) CHECK_CLOSE(8.0f, kf.GetValue(49), 0.0001); CHECK_CLOSE(2.0f, kf.GetValue(50), 0.0001); // Check the expected number of values - CHECK_EQUAL(kf.Values.size(), 51); + CHECK_EQUAL(kf.GetLength(), 51); } TEST(Keyframe_Check_Direction_and_Repeat_Fractions) From 6bc3428307f26b6737555713d692fb4f839d732f Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Thu, 21 Nov 2019 11:35:23 +0100 Subject: [PATCH 11/36] Keyframe::AddPoint() fix: reallocation invalidates iterator Points.push_back can (and will) cause reallocation, which invalidates the candidate iterator. Thus better use an (integer) index. --- src/KeyFrame.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index 2d4760bc..41c38ed8 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -75,9 +75,10 @@ void Keyframe::AddPoint(Point p) { // New point needs to be inserted before candidate; thus move // candidate and all following one to the right and insert new // point then where candidate was. - Points.push_back(p); // Make space; could also be a dummy point. - std::move_backward(candidate, end(Points) - 1, end(Points)); - *candidate = p; + size_t const candidate_index = candidate - begin(Points); + Points.push_back(p); // Make space; could also be a dummy point. INVALIDATES candidate! + std::move_backward(begin(Points) + candidate_index, end(Points) - 1, end(Points)); + Points[candidate_index] = p; } } From 6d81033bff0ec80a854b5395b1f6a12845a0a845 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Fri, 22 Nov 2019 00:36:52 +0100 Subject: [PATCH 12/36] Keyframe::GetPoint() returns a constant reference now Returning a non constant reference is not possible; this allows outside code to invalidate internal invariants (sorted order of points) by modifying the returned point. To make updates the current best approach is to remove the point by index, and then add it again. --- include/KeyFrame.h | 2 +- src/KeyFrame.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/KeyFrame.h b/include/KeyFrame.h index 670f0f5e..4ea076d7 100644 --- a/include/KeyFrame.h +++ b/include/KeyFrame.h @@ -125,7 +125,7 @@ namespace openshot { double GetDelta(int64_t index); /// Get a point at a specific index - Point& GetPoint(int64_t index); + Point const & GetPoint(int64_t index); /// Get current point (or closest point to the right) from the X coordinate (i.e. the frame number) Point GetClosestPoint(Point p); diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index 41c38ed8..79e3ca45 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -449,7 +449,7 @@ double Keyframe::GetDelta(int64_t index) } // Get a point at a specific index -Point& Keyframe::GetPoint(int64_t index) { +Point const & Keyframe::GetPoint(int64_t index) { // Is index a valid point? if (index >= 0 && index < Points.size()) return Points[index]; From bd8240396953a56b4e7b39cc8e16984586a999ac Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Fri, 22 Nov 2019 22:34:22 +0100 Subject: [PATCH 13/36] KeyFrame_Tests: Additional tests to correctly capture old behaviour --- tests/KeyFrame_Tests.cpp | 83 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/tests/KeyFrame_Tests.cpp b/tests/KeyFrame_Tests.cpp index 519eba86..511283e8 100644 --- a/tests/KeyFrame_Tests.cpp +++ b/tests/KeyFrame_Tests.cpp @@ -399,3 +399,86 @@ TEST(Keyframe_Large_Number_Values) CHECK_CLOSE(kf.GetPoint(0).co.Y, 1.0, 0.01); CHECK_CLOSE(kf.GetPoint(1).co.Y, 100.0, 0.01); } + +TEST(Keyframe_Remove_Point) +{ + Keyframe kf; + kf.AddPoint(openshot::Point(Coordinate(1, 1), CONSTANT)); + kf.AddPoint(openshot::Point(Coordinate(3, 100), CONSTANT)); + CHECK_EQUAL(1, kf.GetInt(2)); + kf.AddPoint(openshot::Point(Coordinate(2, 50), CONSTANT)); + CHECK_EQUAL(50, kf.GetInt(2)); + kf.RemovePoint(1); // This is the index of point with X == 2 + CHECK_EQUAL(1, kf.GetInt(2)); + CHECK_THROW(kf.RemovePoint(100), OutOfBoundsPoint); +} + +TEST(Keyframe_Constant_Interpolation_First_Segment) +{ + Keyframe kf; + kf.AddPoint(Point(Coordinate(1, 1), CONSTANT)); + kf.AddPoint(Point(Coordinate(2, 50), CONSTANT)); + kf.AddPoint(Point(Coordinate(3, 100), CONSTANT)); + CHECK_EQUAL(1, kf.GetInt(0)); + CHECK_EQUAL(1, kf.GetInt(1)); + CHECK_EQUAL(50, kf.GetInt(2)); + CHECK_EQUAL(100, kf.GetInt(3)); + CHECK_EQUAL(100, kf.GetInt(4)); +} + +TEST(Keyframe_isIncreasing) +{ + // Which cases need to be tested to keep same behaviour as + // previously? + // + // - "invalid point" => true + // - point where all next values are equal => false + // - point where first non-eq next value is smaller => false + // - point where first non-eq next value is larger => true + Keyframe kf; + kf.AddPoint(1, 1, LINEAR); // testing with linear + kf.AddPoint(3, 5, BEZIER); // testing with bezier + kf.AddPoint(6, 10, CONSTANT); // first non-eq is smaller + kf.AddPoint(8, 8, CONSTANT); // first non-eq is larger + kf.AddPoint(10, 10, CONSTANT); // all next values are equal + kf.AddPoint(15, 10, CONSTANT); + + // "invalid points" + CHECK_EQUAL(true, kf.IsIncreasing(0)); + CHECK_EQUAL(true, kf.IsIncreasing(15)); + // all next equal + CHECK_EQUAL(false, kf.IsIncreasing(12)); + // first non-eq is larger + CHECK_EQUAL(true, kf.IsIncreasing(8)); + // first non-eq is smaller + CHECK_EQUAL(false, kf.IsIncreasing(6)); + // bezier and linear + CHECK_EQUAL(true, kf.IsIncreasing(4)); + CHECK_EQUAL(true, kf.IsIncreasing(2)); +} + +TEST(Keyframe_GetLength) +{ + Keyframe f; + CHECK_EQUAL(0, f.GetLength()); + f.AddPoint(1, 1); + CHECK_EQUAL(1, f.GetLength()); + f.AddPoint(2, 1); + CHECK_EQUAL(3, f.GetLength()); + f.AddPoint(200, 1); + CHECK_EQUAL(201, f.GetLength()); + + Keyframe g; + g.AddPoint(200, 1); + CHECK_EQUAL(1, g.GetLength()); + g.AddPoint(1,1); + CHECK_EQUAL(201, g.GetLength()); +} + +TEST(Keyframe_Use_Interpolation_of_Segment_End_Point) +{ + Keyframe f; + f.AddPoint(1,0, CONSTANT); + f.AddPoint(100,155, BEZIER); + CHECK_CLOSE(75.9, f.GetValue(50), 0.1); +} From 3b2e262821e2c8cf0485385390c73ffffcf65250 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Fri, 22 Nov 2019 22:37:06 +0100 Subject: [PATCH 14/36] Keyframe: New implementation calculating values ondemand - GetValue() calculates the value at the given position now ondemand, without caching values as previously. First, it uses binary search to find the segment (start and end point) containing the given position. Constant and linear interpolation are straightforward, bezier curves are calculating by binary search between the end points until a point on the curve is found with a X coordinate close enough to the given position. That points Y coordinate is returned. - IsIncreasing(), GetRepeatFraction(), GetDelta() use GetValue() instead of accessing the (removed) Values vector. - Removed now unused private functions, and the unused public Process(). First test results show good performance improvements for the "rendering case" (setting up the keyframe points at once, then getting all values) and drastic performance improvements for the "preview case" (changing keyframe points, mixed with getting values). --- include/KeyFrame.h | 26 --- src/KeyFrame.cpp | 491 ++++++++++----------------------------------- 2 files changed, 105 insertions(+), 412 deletions(-) diff --git a/include/KeyFrame.h b/include/KeyFrame.h index 4ea076d7..aafd6c4b 100644 --- a/include/KeyFrame.h +++ b/include/KeyFrame.h @@ -63,25 +63,7 @@ namespace openshot { */ class Keyframe { private: - bool needs_update; - double FactorialLookup[4]; std::vector Points; ///< Vector of all Points - std::vector Values; ///< Vector of all Values (i.e. the processed coordinates from the curve) - - // Process an individual segment - void ProcessSegment(int Segment, Point p1, Point p2); - - // create lookup table for fast factorial calculation - void CreateFactorialTable(); - - // Get a factorial for a coordinate - double Factorial(int64_t n); - - // Calculate the factorial function for Bernstein basis - double Ni(int64_t n, int64_t i); - - // Calculate Bernstein Basis - double Bernstein(int64_t n, int64_t i, double t); public: @@ -155,14 +137,6 @@ namespace openshot { void SetJson(std::string value); ///< Load JSON string into this object void SetJsonValue(Json::Value root); ///< Load Json::JsonValue into this object - /** - * @brief Calculate all of the values for this keyframe. - * - * This clears any existing data in the "values" vector. This method is automatically called - * by AddPoint(), so you don't typically need to call this method. - */ - void Process(); - /// Remove a point by matching a coordinate void RemovePoint(Point p); diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index 79e3ca45..2cd0b35a 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -37,26 +37,18 @@ using namespace openshot; // Constructor which sets the default point & coordinate at X=1 -Keyframe::Keyframe(double value) : needs_update(true) { - // Init the factorial table, needed by bezier curves - CreateFactorialTable(); - +Keyframe::Keyframe(double value) { // Add initial point AddPoint(Point(value)); } // Keyframe constructor -Keyframe::Keyframe() : needs_update(true) { - // Init the factorial table, needed by bezier curves - CreateFactorialTable(); +Keyframe::Keyframe() { } // Add a new point on the key-frame. Each point has a primary coordinate, // a left handle, and a right handle. void Keyframe::AddPoint(Point p) { - // mark as dirty - needs_update = true; - // candidate is not less (greater or equal) than the new point in // the X coordinate. std::vector::iterator candidate = @@ -221,23 +213,78 @@ Point Keyframe::GetMaxPoint() { // Get the value at a specific index double Keyframe::GetValue(int64_t index) { - // Check if it needs to be processed - if (needs_update) - Process(); + if (Points.empty()) { + return 0; + } + std::vector::iterator candidate = + std::lower_bound(begin(Points), end(Points), Point(index, -1), [](Point const & l, Point const & r) { + return l.co.X < r.co.X; + }); - // Is index a valid point? - if (index >= 0 && index < Values.size()) - // Return value - return Values[index].Y; - else if (index < 0 && Values.size() > 0) - // Return the minimum value - return Values[0].Y; - else if (index >= Values.size() && Values.size() > 0) - // return the maximum value - return Values[Values.size() - 1].Y; - else - // return a blank coordinate (0,0) - return 0.0; + if (candidate == end(Points)) { + // index is behind last point + return Points.back().co.Y; + } + if (candidate == begin(Points)) { + // index is at or before first point + return Points.front().co.Y; + } + if (candidate->co.X == index) { + // index is directly on a point + return candidate->co.Y; + } + std::vector::iterator predecessor = candidate - 1; + assert(predecessor->co.X < index); + assert(index < candidate->co.X); + + // CONSTANT and LINEAR interpolations are fast to compute! + if (candidate->interpolation == CONSTANT) { + return predecessor->co.Y; + } + if (candidate->interpolation == LINEAR) { + double const diff_Y = candidate->co.Y - predecessor->co.Y; + double const diff_X = candidate->co.X - predecessor->co.X; + double const slope = diff_Y / diff_X; + return predecessor->co.Y + slope * (index - predecessor->co.X); + } + + // BEZIER curve! + // TODO: use switch instead of if for compiler warning support! + assert(candidate->interpolation == BEZIER); + + double const X_diff = candidate->co.X - predecessor->co.X; + double const Y_diff = candidate->co.Y - predecessor->co.Y; + Coordinate const p0 = predecessor->co; + Coordinate const p1 = Coordinate(p0.X + predecessor->handle_right.X * X_diff, p0.Y + predecessor->handle_right.Y * Y_diff); + Coordinate const p2 = Coordinate(p0.X + candidate->handle_left.X * X_diff, p0.Y + candidate->handle_left.Y * Y_diff); + Coordinate const p3 = candidate->co; + + double t = 0.5; + double t_step = 0.25; + do { + // Bernstein polynoms + double B[4] = {1, 3, 3, 1}; + double oneMinTExp = 1; + double tExp = 1; + for (int i = 0; i < 4; ++i, tExp *= t) { + B[i] *= tExp; + } + for (int i = 0; i < 4; ++i, oneMinTExp *= 1 - t) { + B[4 - i - 1] *= oneMinTExp; + } + double const x = p0.X * B[0] + p1.X * B[1] + p2.X * B[2] + p3.X * B[3]; + double const y = p0.Y * B[0] + p1.Y * B[1] + p2.Y * B[2] + p3.Y * B[3]; + if (abs(index - x) < 0.01) { + return y; + } + if (x > index) { + t -= t_step; + } + else { + t += t_step; + } + t_step /= 2; + } while (true); } // Get the rounded INT value at a specific index @@ -255,30 +302,17 @@ int64_t Keyframe::GetLong(int64_t index) // Get the direction of the curve at a specific index (increasing or decreasing) bool Keyframe::IsIncreasing(int index) { - // Check if it needs to be processed - if (needs_update) - Process(); - - // Is index a valid point? - if (index >= 1 && (index + 1) < Values.size()) { - int64_t current_value = GetLong(index); - int64_t next_value = 0; - - // Loop forwards and look for the next unique value - for (std::vector::iterator forwards_it = Values.begin() + (index + 1); forwards_it != Values.end(); forwards_it++) { - next_value = long(round((*forwards_it).Y)); - if (next_value != current_value) { - break; - } - } - - if (current_value >= next_value) { - // Decreasing - return false; - } + if (index < 1 || (index + 1) >= GetLength()) { + return true; } - // return default true (since most curves increase) - return true; + int64_t const current = GetLong(index); + // TODO: skip over constant sections. + do { + int64_t const next = GetLong(++index); + if (next > current) return true; + if (next < current) return false; + } while (index < GetLength()); + return false; } // Generate JSON string of this object @@ -337,10 +371,6 @@ void Keyframe::SetJson(std::string value) { // Load Json::JsonValue into this object void Keyframe::SetJsonValue(Json::Value root) { - - // mark as dirty - needs_update = true; - // Clear existing points Points.clear(); @@ -365,22 +395,15 @@ void Keyframe::SetJsonValue(Json::Value root) { // This is depreciated and will be removed soon. Fraction Keyframe::GetRepeatFraction(int64_t index) { - // Check if it needs to be processed - if (needs_update) - Process(); - // Is index a valid point? - if (index >= 1 && (index + 1) < Values.size()) { + if (index >= 1 && (index + 1) < GetLength()) { int64_t current_value = GetLong(index); - int64_t previous_value = 0; - int64_t next_value = 0; int64_t previous_repeats = 0; int64_t next_repeats = 0; // Loop backwards and look for the next unique value - for (std::vector::iterator backwards_it = Values.begin() + index; backwards_it != Values.begin(); backwards_it--) { - previous_value = long(round((*backwards_it).Y)); - if (previous_value == current_value) { + for (int64_t i = index; i > 0; --i) { + if (GetLong(i) == current_value) { // Found same value previous_repeats++; } else { @@ -390,9 +413,8 @@ Fraction Keyframe::GetRepeatFraction(int64_t index) } // Loop forwards and look for the next unique value - for (std::vector::iterator forwards_it = Values.begin() + (index + 1); forwards_it != Values.end(); forwards_it++) { - next_value = long(round((*forwards_it).Y)); - if (next_value == current_value) { + for (int64_t i = index + 1; i < GetLength(); ++i) { + if (GetLong(i) == current_value) { // Found same value next_repeats++; } else { @@ -412,40 +434,10 @@ Fraction Keyframe::GetRepeatFraction(int64_t index) // Get the change in Y value (from the previous Y value) double Keyframe::GetDelta(int64_t index) { - // Check if it needs to be processed - if (needs_update) - Process(); - - // Is index a valid point? - if (index >= 1 && (index + 1) < Values.size()) { - int64_t current_value = GetLong(index); - int64_t previous_value = 0; - int64_t previous_repeats = 0; - - // Loop backwards and look for the next unique value - for (std::vector::iterator backwards_it = Values.begin() + index; backwards_it != Values.begin(); backwards_it--) { - previous_value = long(round((*backwards_it).Y)); - if (previous_value == current_value) { - // Found same value - previous_repeats++; - } else { - // Found non repeating value, no more repeats found - break; - } - } - - // Check for matching previous value (special case for 1st element) - if (current_value == previous_value) - previous_value = 0; - - if (previous_repeats == 1) - return current_value - previous_value; - else - return 0.0; - } - else - // return a blank coordinate - return 0.0; + if (index < 1) return 0; + if (index == 1 && ! Points.empty()) return Points[0].co.Y; + if (index >= GetLength()) return 0; + return GetLong(index) - GetLong(index - 1); } // Get a point at a specific index @@ -460,18 +452,14 @@ Point const & Keyframe::GetPoint(int64_t index) { // Get the number of values (i.e. coordinates on the X axis) int64_t Keyframe::GetLength() { - // Check if it needs to be processed - if (needs_update) - Process(); - - // return the size of the Values vector - return Values.size(); + if (Points.empty()) return 0; + if (Points.size() == 1) return 1; + return round(Points.back().co.X) + 1; } // Get the number of points (i.e. # of points) int64_t Keyframe::GetCount() { - // return the size of the Values vector return Points.size(); } @@ -486,8 +474,6 @@ void Keyframe::RemovePoint(Point p) { if (p.co.X == existing_point.co.X && p.co.Y == existing_point.co.Y) { // Remove the matching point, and break out of loop Points.erase(Points.begin() + x); - // mark as dirty - needs_update = true; return; } } @@ -501,8 +487,6 @@ void Keyframe::RemovePoint(int64_t index) { // Is index a valid point? if (index >= 0 && index < Points.size()) { - // mark as dirty - needs_update = true; // Remove a specific point by index Points.erase(Points.begin() + index); } @@ -520,10 +504,6 @@ void Keyframe::UpdatePoint(int64_t index, Point p) { } void Keyframe::PrintPoints() { - // Check if it needs to be processed - if (needs_update) - Process(); - cout << fixed << setprecision(4); for (std::vector::iterator it = Points.begin(); it != Points.end(); it++) { Point p = *it; @@ -532,300 +512,39 @@ void Keyframe::PrintPoints() { } void Keyframe::PrintValues() { - // Check if it needs to be processed - if (needs_update) - Process(); - cout << fixed << setprecision(4); - cout << "Frame Number (X)\tValue (Y)\tIs Increasing\tRepeat Numerator\tRepeat Denominator\tDelta (Y Difference)" << endl; + cout << "Frame Number (X)\tValue (Y)\tIs Increasing\tRepeat Numerator\tRepeat Denominator\tDelta (Y Difference)\n"; - for (std::vector::iterator it = Values.begin() + 1; it != Values.end(); it++) { - Coordinate c = *it; - cout << long(round(c.X)) << "\t" << c.Y << "\t" << IsIncreasing(c.X) << "\t" << GetRepeatFraction(c.X).num << "\t" << GetRepeatFraction(c.X).den << "\t" << GetDelta(c.X) << endl; + for (uint64_t i = 1; i < GetLength(); ++i) { + cout << i << "\t" << GetValue(i) << "\t" << IsIncreasing(i) << "\t" ; + cout << GetRepeatFraction(i).num << "\t" << GetRepeatFraction(i).den << "\t" << GetDelta(i) << "\n"; } } -void Keyframe::Process() { - - #pragma omp critical (keyframe_process) - { - // only process if needed - if (needs_update && Points.size() == 0) { - // Clear all values - Values.clear(); - } - else if (needs_update && Points.size() > 0) - { - // Clear all values - Values.clear(); - - // fill in all values between 1 and 1st point's co.X - Point p1 = Points[0]; - if (Points.size() > 1) - // Fill in previous X values (before 1st point) - for (int64_t x = 0; x < p1.co.X; x++) - Values.push_back(Coordinate(Values.size(), p1.co.Y)); - else - // Add a single value (since we only have 1 point) - Values.push_back(Coordinate(Values.size(), p1.co.Y)); - - // Loop through each pair of points (1 less than the max points). Each - // pair of points is used to process a segment of the keyframe. - Point p2(0, 0); - for (int64_t x = 0; x < Points.size() - 1; x++) { - p1 = Points[x]; - p2 = Points[x + 1]; - - // process segment p1,p2 - ProcessSegment(x, p1, p2); - } - } - - // reset flag - needs_update = false; - } -} - -void Keyframe::ProcessSegment(int Segment, Point p1, Point p2) { - // Determine the number of values for this segment - int64_t number_of_values = round(p2.co.X) - round(p1.co.X); - - // Exit function if no values - if (number_of_values == 0) - return; - - // Based on the interpolation mode, fill the "values" vector with the coordinates - // for this segment - switch (p2.interpolation) { - - // Calculate the "values" for this segment in with a LINEAR equation, effectively - // creating a straight line with coordinates. - case LINEAR: { - // Get the difference in value - double current_value = p1.co.Y; - double value_difference = p2.co.Y - p1.co.Y; - double value_increment = 0.0f; - - // Get the increment value, but take into account the - // first segment has 1 extra value - value_increment = value_difference / (double) (number_of_values); - - if (Segment == 0) - // Add an extra value to the first segment - number_of_values++; - else - // If not 1st segment, skip the first value - current_value += value_increment; - - // Add each increment to the values vector - for (int64_t x = 0; x < number_of_values; x++) { - // add value as a coordinate to the "values" vector - Values.push_back(Coordinate(Values.size(), current_value)); - - // increment value - current_value += value_increment; - } - - break; - } - - // Calculate the "values" for this segment using a quadratic Bezier curve. This creates a - // smooth curve. - case BEZIER: { - - // Always increase the number of points by 1 (need all possible points - // to correctly calculate the curve). - number_of_values++; - number_of_values *= 4; // We need a higher resolution curve (4X) - - // Diff between points - double X_diff = p2.co.X - p1.co.X; - double Y_diff = p2.co.Y - p1.co.Y; - - std::vector segment_coordinates; - segment_coordinates.push_back(p1.co); - segment_coordinates.push_back(Coordinate(p1.co.X + (p1.handle_right.X * X_diff), p1.co.Y + (p1.handle_right.Y * Y_diff))); - segment_coordinates.push_back(Coordinate(p1.co.X + (p2.handle_left.X * X_diff), p1.co.Y + (p2.handle_left.Y * Y_diff))); - segment_coordinates.push_back(p2.co); - - std::vector raw_coordinates; - int64_t npts = segment_coordinates.size(); - int64_t icount, jcount; - double step, t; - double last_x = -1; // small number init, to track the last used x - - // Calculate points on curve - icount = 0; - t = 0; - - step = (double) 1.0 / (number_of_values - 1); - - for (int64_t i1 = 0; i1 < number_of_values; i1++) { - if ((1.0 - t) < 5e-6) - t = 1.0; - - jcount = 0; - - double new_x = 0.0f; - double new_y = 0.0f; - - for (int64_t i = 0; i < npts; i++) { - Coordinate co = segment_coordinates[i]; - double basis = Bernstein(npts - 1, i, t); - new_x += basis * co.X; - new_y += basis * co.Y; - } - - // Add new value to the vector - Coordinate current_value(new_x, new_y); - - // Add all values for 1st segment - raw_coordinates.push_back(current_value); - - // increment counters - icount += 2; - t += step; - } - - // Loop through the raw coordinates, and map them correctly to frame numbers. For example, - // we can't have duplicate X values, since X represents our frame numbers. - int64_t current_frame = p1.co.X; - double current_value = p1.co.Y; - for (int64_t i = 0; i < raw_coordinates.size(); i++) - { - // Get the raw coordinate - Coordinate raw = raw_coordinates[i]; - - if (current_frame == round(raw.X)) - // get value of raw coordinate - current_value = raw.Y; - else - { - // Missing X values (use last known Y values) - int64_t number_of_missing = round(raw.X) - current_frame; - for (int64_t missing = 0; missing < number_of_missing; missing++) - { - // Add new value to the vector - Coordinate new_coord(current_frame, current_value); - - if (Segment == 0 || (Segment > 0 && current_frame > p1.co.X)) - // Add to "values" vector - Values.push_back(new_coord); - - // Increment frame - current_frame++; - } - - // increment the current value - current_value = raw.Y; - } - } - - // Add final coordinate - Coordinate new_coord(current_frame, current_value); - Values.push_back(new_coord); - - break; - } - - // Calculate the "values" of this segment by maintaining the value of p1 until the - // last point, and then make the value jump to p2. This effectively just jumps - // the value, instead of ramping up or down the value. - case CONSTANT: { - - if (Segment == 0) - // first segment has 1 extra value - number_of_values++; - - // Add each increment to the values vector - for (int64_t x = 0; x < number_of_values; x++) { - if (x < (number_of_values - 1)) { - // Not the last value of this segment - // add coordinate to "values" - Values.push_back(Coordinate(Values.size(), p1.co.Y)); - } else { - // This is the last value of this segment - // add coordinate to "values" - Values.push_back(Coordinate(Values.size(), p2.co.Y)); - } - } - break; - } - - } -} - -// Create lookup table for fast factorial calculation -void Keyframe::CreateFactorialTable() { - // Only 4 lookups are needed, because we only support 4 coordinates per curve - FactorialLookup[0] = 1.0; - FactorialLookup[1] = 1.0; - FactorialLookup[2] = 2.0; - FactorialLookup[3] = 6.0; -} - -// Get a factorial for a coordinate -double Keyframe::Factorial(int64_t n) { - assert(n >= 0 && n <= 3); - return FactorialLookup[n]; /* returns the value n! as a SUMORealing point number */ -} - -// Calculate the factorial function for Bernstein basis -double Keyframe::Ni(int64_t n, int64_t i) { - double ni; - double a1 = Factorial(n); - double a2 = Factorial(i); - double a3 = Factorial(n - i); - ni = a1 / (a2 * a3); - return ni; -} - -// Calculate Bernstein basis -double Keyframe::Bernstein(int64_t n, int64_t i, double t) { - double basis; - double ti; /* t^i */ - double tni; /* (1 - t)^i */ - - /* Prevent problems with pow */ - if (t == 0.0 && i == 0) - ti = 1.0; - else - ti = pow(t, i); - - if (n == i && t == 1.0) - tni = 1.0; - else - tni = pow((1 - t), (n - i)); - - // Bernstein basis - basis = Ni(n, i) * ti * tni; - return basis; -} // Scale all points by a percentage (good for evenly lengthening or shortening an openshot::Keyframe) // 1.0 = same size, 1.05 = 5% increase, etc... void Keyframe::ScalePoints(double scale) { + // TODO: What if scale is small so that two points land on the + // same X coordinate? + // TODO: What if scale < 0? + // Loop through each point (skipping the 1st point) for (int64_t point_index = 1; point_index < Points.size(); point_index++) { // Scale X value Points[point_index].co.X = round(Points[point_index].co.X * scale); - - // Mark for re-processing - needs_update = true; } } // Flip all the points in this openshot::Keyframe (useful for reversing an effect or transition, etc...) void Keyframe::FlipPoints() { - // Loop through each point for (int64_t point_index = 0, reverse_index = Points.size() - 1; point_index < reverse_index; point_index++, reverse_index--) { // Flip the points using std::swap; swap(Points[point_index].co.Y, Points[reverse_index].co.Y); + // TODO: check that this has the desired effect even with + // regards to handles! } - - // Mark for re-processing - needs_update = true; } From 86c1df211a6fd75853bd84b55ff737918862a5f0 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Sun, 24 Nov 2019 21:00:37 +0100 Subject: [PATCH 15/36] Update Keyframe test curve values; new curves are smoother The curves generated by the new code are much smoother than the old curves and align much better with what (at least gnuplot) says are the "correct curves". Some values therefore needed updating. --- tests/Color_Tests.cpp | 6 ++--- tests/KeyFrame_Tests.cpp | 50 ++++++++++++++++++++-------------------- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/tests/Color_Tests.cpp b/tests/Color_Tests.cpp index 210cca59..82c7d180 100644 --- a/tests/Color_Tests.cpp +++ b/tests/Color_Tests.cpp @@ -79,7 +79,7 @@ TEST(Color_HEX_Value) c.blue.AddPoint(100, 255); CHECK_EQUAL("#000000", c.GetColorHex(1)); - CHECK_EQUAL("#7f7f7f", c.GetColorHex(50)); + CHECK_EQUAL("#7d7d7d", c.GetColorHex(50)); CHECK_EQUAL("#ffffff", c.GetColorHex(100)); } @@ -93,7 +93,7 @@ TEST(Color_HEX_Constructor) c.blue.AddPoint(100, 255); CHECK_EQUAL("#4586db", c.GetColorHex(1)); - CHECK_EQUAL("#a2c2ed", c.GetColorHex(50)); + CHECK_EQUAL("#a0c1ed", c.GetColorHex(50)); CHECK_EQUAL("#ffffff", c.GetColorHex(100)); } @@ -118,7 +118,7 @@ TEST(Color_RGBA_Constructor) c.blue.AddPoint(100, 255); CHECK_EQUAL("#4586db", c.GetColorHex(1)); - CHECK_EQUAL("#a2c2ed", c.GetColorHex(50)); + CHECK_EQUAL("#a0c1ed", c.GetColorHex(50)); CHECK_EQUAL("#ffffff", c.GetColorHex(100)); // Color with alpha diff --git a/tests/KeyFrame_Tests.cpp b/tests/KeyFrame_Tests.cpp index 511283e8..826abe10 100644 --- a/tests/KeyFrame_Tests.cpp +++ b/tests/KeyFrame_Tests.cpp @@ -92,10 +92,10 @@ TEST(Keyframe_GetValue_For_Bezier_Curve_2_Points) // Spot check values from the curve CHECK_CLOSE(1.0f, kf.GetValue(-1), 0.0001); CHECK_CLOSE(1.0f, kf.GetValue(0), 0.0001); - CHECK_CLOSE(1.00023f, kf.GetValue(1), 0.0001); - CHECK_CLOSE(1.14025f, kf.GetValue(9), 0.0001); - CHECK_CLOSE(1.91492f, kf.GetValue(20), 0.0001); - CHECK_CLOSE(3.81602f, kf.GetValue(40), 0.0001); + CHECK_CLOSE(1.0f, kf.GetValue(1), 0.0001); + CHECK_CLOSE(1.12414f, kf.GetValue(9), 0.0001); + CHECK_CLOSE(1.86370f, kf.GetValue(20), 0.0001); + CHECK_CLOSE(3.79733f, kf.GetValue(40), 0.0001); CHECK_CLOSE(4.0f, kf.GetValue(50), 0.0001); // Check the expected number of values CHECK_EQUAL(kf.GetLength(), 51); @@ -114,11 +114,11 @@ TEST(Keyframe_GetValue_For_Bezier_Curve_5_Points_40_Percent_Handle) // Spot check values from the curve CHECK_CLOSE(kf.GetValue(-1), 1.0f, 0.0001); CHECK_CLOSE(1.0f, kf.GetValue(0), 0.0001); - CHECK_CLOSE(1.00023f, kf.GetValue(1), 0.0001); - CHECK_CLOSE(2.73656f, kf.GetValue(27), 0.0001); - CHECK_CLOSE(7.55139f, kf.GetValue(77), 0.0001); - CHECK_CLOSE(4.08102f, kf.GetValue(127), 0.0001); - CHECK_CLOSE(1.77569f, kf.GetValue(177), 0.0001); + CHECK_CLOSE(1.0f, kf.GetValue(1), 0.0001); + CHECK_CLOSE(2.68197f, kf.GetValue(27), 0.0001); + CHECK_CLOSE(7.47719f, kf.GetValue(77), 0.0001); + CHECK_CLOSE(4.20468f, kf.GetValue(127), 0.0001); + CHECK_CLOSE(1.73860f, kf.GetValue(177), 0.0001); CHECK_CLOSE(3.0f, kf.GetValue(200), 0.0001); // Check the expected number of values CHECK_EQUAL(kf.GetLength(), 201); @@ -137,11 +137,11 @@ TEST(Keyframe_GetValue_For_Bezier_Curve_5_Points_25_Percent_Handle) // Spot check values from the curve CHECK_CLOSE(1.0f, kf.GetValue(-1), 0.0001); CHECK_CLOSE(1.0f, kf.GetValue(0), 0.0001); - CHECK_CLOSE(1.00023f, kf.GetValue(1), 0.0001); - CHECK_CLOSE(2.73656f, kf.GetValue(27), 0.0001); - CHECK_CLOSE(7.55139f, kf.GetValue(77), 0.0001); - CHECK_CLOSE(4.08102f, kf.GetValue(127), 0.0001); - CHECK_CLOSE(1.77569f, kf.GetValue(177), 0.0001); + CHECK_CLOSE(1.0f, kf.GetValue(1), 0.0001); + CHECK_CLOSE(2.68197f, kf.GetValue(27), 0.0001); + CHECK_CLOSE(7.47719f, kf.GetValue(77), 0.0001); + CHECK_CLOSE(4.20468f, kf.GetValue(127), 0.0001); + CHECK_CLOSE(1.73860f, kf.GetValue(177), 0.0001); CHECK_CLOSE(3.0f, kf.GetValue(200), 0.0001); // Check the expected number of values CHECK_EQUAL(kf.GetLength(), 201); @@ -200,7 +200,7 @@ TEST(Keyframe_Check_Direction_and_Repeat_Fractions) CHECK_EQUAL(kf.GetInt(1), 500); CHECK_EQUAL(kf.IsIncreasing(1), false); CHECK_EQUAL(kf.GetRepeatFraction(1).num, 1); - CHECK_EQUAL(kf.GetRepeatFraction(1).den, 12); + CHECK_EQUAL(kf.GetRepeatFraction(1).den, 13); CHECK_EQUAL(kf.GetDelta(1), 500); CHECK_EQUAL(kf.GetInt(24), 498); @@ -212,13 +212,13 @@ TEST(Keyframe_Check_Direction_and_Repeat_Fractions) CHECK_EQUAL(kf.GetLong(390), 100); CHECK_EQUAL(kf.IsIncreasing(390), true); CHECK_EQUAL(kf.GetRepeatFraction(390).num, 3); - CHECK_EQUAL(kf.GetRepeatFraction(390).den, 15); + CHECK_EQUAL(kf.GetRepeatFraction(390).den, 16); CHECK_EQUAL(kf.GetDelta(390), 0); CHECK_EQUAL(kf.GetLong(391), 100); CHECK_EQUAL(kf.IsIncreasing(391), true); CHECK_EQUAL(kf.GetRepeatFraction(391).num, 4); - CHECK_EQUAL(kf.GetRepeatFraction(391).den, 15); + CHECK_EQUAL(kf.GetRepeatFraction(391).den, 16); CHECK_EQUAL(kf.GetDelta(388), -1); } @@ -307,8 +307,8 @@ TEST(Keyframe_Scale_Keyframe) CHECK_CLOSE(1.0f, kf.GetValue(1), 0.01); CHECK_CLOSE(7.99f, kf.GetValue(24), 0.01); CHECK_CLOSE(8.0f, kf.GetValue(25), 0.01); - CHECK_CLOSE(3.68f, kf.GetValue(40), 0.01); - CHECK_CLOSE(2.0f, kf.GetValue(49), 0.01); + CHECK_CLOSE(3.85f, kf.GetValue(40), 0.01); + CHECK_CLOSE(2.01f, kf.GetValue(49), 0.01); CHECK_CLOSE(2.0f, kf.GetValue(50), 0.01); // Resize / Scale the keyframe @@ -316,12 +316,12 @@ TEST(Keyframe_Scale_Keyframe) // Spot check values from the curve CHECK_CLOSE(1.0f, kf.GetValue(1), 0.01); - CHECK_CLOSE(4.21f, kf.GetValue(24), 0.01); - CHECK_CLOSE(4.47f, kf.GetValue(25), 0.01); - CHECK_CLOSE(7.57f, kf.GetValue(40), 0.01); + CHECK_CLOSE(4.08f, kf.GetValue(24), 0.01); + CHECK_CLOSE(4.36f, kf.GetValue(25), 0.01); + CHECK_CLOSE(7.53f, kf.GetValue(40), 0.01); CHECK_CLOSE(7.99f, kf.GetValue(49), 0.01); CHECK_CLOSE(8.0f, kf.GetValue(50), 0.01); - CHECK_CLOSE(2.35f, kf.GetValue(90), 0.01); + CHECK_CLOSE(2.39f, kf.GetValue(90), 0.01); CHECK_CLOSE(2.0f, kf.GetValue(100), 0.01); // Resize / Scale the keyframe @@ -331,8 +331,8 @@ TEST(Keyframe_Scale_Keyframe) CHECK_CLOSE(1.0f, kf.GetValue(1), 0.01); CHECK_CLOSE(7.99f, kf.GetValue(24), 0.01); CHECK_CLOSE(8.0f, kf.GetValue(25), 0.01); - CHECK_CLOSE(3.68f, kf.GetValue(40), 0.01); - CHECK_CLOSE(2.0f, kf.GetValue(49), 0.01); + CHECK_CLOSE(3.85f, kf.GetValue(40), 0.01); + CHECK_CLOSE(2.01f, kf.GetValue(49), 0.01); CHECK_CLOSE(2.0f, kf.GetValue(50), 0.01); } From 504fd0e3fae1f763566be4920775fa30662e5f4d Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Sun, 24 Nov 2019 21:12:33 +0100 Subject: [PATCH 16/36] KeyFrame_Tests.cpp: Correct usage for CHECK_EQUAL It's CHECK_EQUAL(expected, actual), not the other way around! --- tests/KeyFrame_Tests.cpp | 112 +++++++++++++++++++-------------------- 1 file changed, 56 insertions(+), 56 deletions(-) diff --git a/tests/KeyFrame_Tests.cpp b/tests/KeyFrame_Tests.cpp index 826abe10..1f03fa80 100644 --- a/tests/KeyFrame_Tests.cpp +++ b/tests/KeyFrame_Tests.cpp @@ -98,7 +98,7 @@ TEST(Keyframe_GetValue_For_Bezier_Curve_2_Points) CHECK_CLOSE(3.79733f, kf.GetValue(40), 0.0001); CHECK_CLOSE(4.0f, kf.GetValue(50), 0.0001); // Check the expected number of values - CHECK_EQUAL(kf.GetLength(), 51); + CHECK_EQUAL(51, kf.GetLength()); } TEST(Keyframe_GetValue_For_Bezier_Curve_5_Points_40_Percent_Handle) @@ -121,7 +121,7 @@ TEST(Keyframe_GetValue_For_Bezier_Curve_5_Points_40_Percent_Handle) CHECK_CLOSE(1.73860f, kf.GetValue(177), 0.0001); CHECK_CLOSE(3.0f, kf.GetValue(200), 0.0001); // Check the expected number of values - CHECK_EQUAL(kf.GetLength(), 201); + CHECK_EQUAL(201, kf.GetLength()); } TEST(Keyframe_GetValue_For_Bezier_Curve_5_Points_25_Percent_Handle) @@ -144,7 +144,7 @@ TEST(Keyframe_GetValue_For_Bezier_Curve_5_Points_25_Percent_Handle) CHECK_CLOSE(1.73860f, kf.GetValue(177), 0.0001); CHECK_CLOSE(3.0f, kf.GetValue(200), 0.0001); // Check the expected number of values - CHECK_EQUAL(kf.GetLength(), 201); + CHECK_EQUAL(201, kf.GetLength()); } TEST(Keyframe_GetValue_For_Linear_Curve_3_Points) @@ -164,7 +164,7 @@ TEST(Keyframe_GetValue_For_Linear_Curve_3_Points) CHECK_CLOSE(4.4f, kf.GetValue(40), 0.0001); CHECK_CLOSE(2.0f, kf.GetValue(50), 0.0001); // Check the expected number of values - CHECK_EQUAL(kf.GetLength(), 51); + CHECK_EQUAL(51, kf.GetLength()); } TEST(Keyframe_GetValue_For_Constant_Curve_3_Points) @@ -185,7 +185,7 @@ TEST(Keyframe_GetValue_For_Constant_Curve_3_Points) CHECK_CLOSE(8.0f, kf.GetValue(49), 0.0001); CHECK_CLOSE(2.0f, kf.GetValue(50), 0.0001); // Check the expected number of values - CHECK_EQUAL(kf.GetLength(), 51); + CHECK_EQUAL(51, kf.GetLength()); } TEST(Keyframe_Check_Direction_and_Repeat_Fractions) @@ -197,29 +197,29 @@ TEST(Keyframe_Check_Direction_and_Repeat_Fractions) kf.AddPoint(500, 500); // Spot check values from the curve - CHECK_EQUAL(kf.GetInt(1), 500); - CHECK_EQUAL(kf.IsIncreasing(1), false); - CHECK_EQUAL(kf.GetRepeatFraction(1).num, 1); - CHECK_EQUAL(kf.GetRepeatFraction(1).den, 13); - CHECK_EQUAL(kf.GetDelta(1), 500); + CHECK_EQUAL(500, kf.GetInt(1)); + CHECK_EQUAL(false, kf.IsIncreasing(1)); + CHECK_EQUAL(1, kf.GetRepeatFraction(1).num); + CHECK_EQUAL(13, kf.GetRepeatFraction(1).den); + CHECK_EQUAL(500, kf.GetDelta(1)); - CHECK_EQUAL(kf.GetInt(24), 498); - CHECK_EQUAL(kf.IsIncreasing(24), false); - CHECK_EQUAL(kf.GetRepeatFraction(24).num, 3); - CHECK_EQUAL(kf.GetRepeatFraction(24).den, 6); - CHECK_EQUAL(kf.GetDelta(24), 0); + CHECK_EQUAL(498, kf.GetInt(24)); + CHECK_EQUAL(false, kf.IsIncreasing(24)); + CHECK_EQUAL(3, kf.GetRepeatFraction(24).num); + CHECK_EQUAL(6, kf.GetRepeatFraction(24).den); + CHECK_EQUAL(0, kf.GetDelta(24)); - CHECK_EQUAL(kf.GetLong(390), 100); - CHECK_EQUAL(kf.IsIncreasing(390), true); - CHECK_EQUAL(kf.GetRepeatFraction(390).num, 3); - CHECK_EQUAL(kf.GetRepeatFraction(390).den, 16); - CHECK_EQUAL(kf.GetDelta(390), 0); + CHECK_EQUAL(100, kf.GetLong(390)); + CHECK_EQUAL(true, kf.IsIncreasing(390)); + CHECK_EQUAL(3, kf.GetRepeatFraction(390).num); + CHECK_EQUAL(16, kf.GetRepeatFraction(390).den); + CHECK_EQUAL(0, kf.GetDelta(390)); - CHECK_EQUAL(kf.GetLong(391), 100); - CHECK_EQUAL(kf.IsIncreasing(391), true); - CHECK_EQUAL(kf.GetRepeatFraction(391).num, 4); - CHECK_EQUAL(kf.GetRepeatFraction(391).den, 16); - CHECK_EQUAL(kf.GetDelta(388), -1); + CHECK_EQUAL(100, kf.GetLong(391)); + CHECK_EQUAL(true, kf.IsIncreasing(391)); + CHECK_EQUAL(4, kf.GetRepeatFraction(391).num); + CHECK_EQUAL(16, kf.GetRepeatFraction(391).den); + CHECK_EQUAL(-1, kf.GetDelta(388)); } @@ -232,22 +232,22 @@ TEST(Keyframe_Get_Closest_Point) kf.AddPoint(2500, 0.0); // Spot check values from the curve (to the right) - CHECK_EQUAL(kf.GetClosestPoint(openshot::Point(900, 900)).co.X, 1000); - CHECK_EQUAL(kf.GetClosestPoint(openshot::Point(1, 1)).co.X, 1); - CHECK_EQUAL(kf.GetClosestPoint(openshot::Point(5, 5)).co.X, 1000); - CHECK_EQUAL(kf.GetClosestPoint(openshot::Point(1000, 1000)).co.X, 1000); - CHECK_EQUAL(kf.GetClosestPoint(openshot::Point(1001, 1001)).co.X, 2500); - CHECK_EQUAL(kf.GetClosestPoint(openshot::Point(2500, 2500)).co.X, 2500); - CHECK_EQUAL(kf.GetClosestPoint(openshot::Point(3000, 3000)).co.X, 2500); + CHECK_EQUAL(1000, kf.GetClosestPoint(openshot::Point(900, 900)).co.X); + CHECK_EQUAL(1, kf.GetClosestPoint(openshot::Point(1, 1)).co.X); + CHECK_EQUAL(1000, kf.GetClosestPoint(openshot::Point(5, 5)).co.X); + CHECK_EQUAL(1000, kf.GetClosestPoint(openshot::Point(1000, 1000)).co.X); + CHECK_EQUAL(2500, kf.GetClosestPoint(openshot::Point(1001, 1001)).co.X); + CHECK_EQUAL(2500, kf.GetClosestPoint(openshot::Point(2500, 2500)).co.X); + CHECK_EQUAL(2500, kf.GetClosestPoint(openshot::Point(3000, 3000)).co.X); // Spot check values from the curve (to the left) - CHECK_EQUAL(kf.GetClosestPoint(openshot::Point(900, 900), true).co.X, 1); - CHECK_EQUAL(kf.GetClosestPoint(openshot::Point(1, 1), true).co.X, 1); - CHECK_EQUAL(kf.GetClosestPoint(openshot::Point(5, 5), true).co.X, 1); - CHECK_EQUAL(kf.GetClosestPoint(openshot::Point(1000, 1000), true).co.X, 1); - CHECK_EQUAL(kf.GetClosestPoint(openshot::Point(1001, 1001), true).co.X, 1000); - CHECK_EQUAL(kf.GetClosestPoint(openshot::Point(2500, 2500), true).co.X, 1000); - CHECK_EQUAL(kf.GetClosestPoint(openshot::Point(3000, 3000), true).co.X, 2500); + CHECK_EQUAL(1, kf.GetClosestPoint(openshot::Point(900, 900), true).co.X); + CHECK_EQUAL(1, kf.GetClosestPoint(openshot::Point(1, 1), true).co.X); + CHECK_EQUAL(1, kf.GetClosestPoint(openshot::Point(5, 5), true).co.X); + CHECK_EQUAL(1, kf.GetClosestPoint(openshot::Point(1000, 1000), true).co.X); + CHECK_EQUAL(1000, kf.GetClosestPoint(openshot::Point(1001, 1001), true).co.X); + CHECK_EQUAL(1000, kf.GetClosestPoint(openshot::Point(2500, 2500), true).co.X); + CHECK_EQUAL(2500, kf.GetClosestPoint(openshot::Point(3000, 3000), true).co.X); } @@ -260,13 +260,13 @@ TEST(Keyframe_Get_Previous_Point) kf.AddPoint(2500, 0.0); // Spot check values from the curve - CHECK_EQUAL(kf.GetPreviousPoint(kf.GetClosestPoint(openshot::Point(900, 900))).co.X, 1); - CHECK_EQUAL(kf.GetPreviousPoint(kf.GetClosestPoint(openshot::Point(1, 1))).co.X, 1); - CHECK_EQUAL(kf.GetPreviousPoint(kf.GetClosestPoint(openshot::Point(5, 5))).co.X, 1); - CHECK_EQUAL(kf.GetPreviousPoint(kf.GetClosestPoint(openshot::Point(1000, 1000))).co.X, 1); - CHECK_EQUAL(kf.GetPreviousPoint(kf.GetClosestPoint(openshot::Point(1001, 1001))).co.X, 1000); - CHECK_EQUAL(kf.GetPreviousPoint(kf.GetClosestPoint(openshot::Point(2500, 2500))).co.X, 1000); - CHECK_EQUAL(kf.GetPreviousPoint(kf.GetClosestPoint(openshot::Point(3000, 3000))).co.X, 1000); + CHECK_EQUAL(1, kf.GetPreviousPoint(kf.GetClosestPoint(openshot::Point(900, 900))).co.X); + CHECK_EQUAL(1, kf.GetPreviousPoint(kf.GetClosestPoint(openshot::Point(1, 1))).co.X); + CHECK_EQUAL(1, kf.GetPreviousPoint(kf.GetClosestPoint(openshot::Point(5, 5))).co.X); + CHECK_EQUAL(1, kf.GetPreviousPoint(kf.GetClosestPoint(openshot::Point(1000, 1000))).co.X); + CHECK_EQUAL(1000, kf.GetPreviousPoint(kf.GetClosestPoint(openshot::Point(1001, 1001))).co.X); + CHECK_EQUAL(1000, kf.GetPreviousPoint(kf.GetClosestPoint(openshot::Point(2500, 2500))).co.X); + CHECK_EQUAL(1000, kf.GetPreviousPoint(kf.GetClosestPoint(openshot::Point(3000, 3000))).co.X); } @@ -277,22 +277,22 @@ TEST(Keyframe_Get_Max_Point) kf.AddPoint(1, 1.0); // Spot check values from the curve - CHECK_EQUAL(kf.GetMaxPoint().co.Y, 1.0); + CHECK_EQUAL(1.0, kf.GetMaxPoint().co.Y); kf.AddPoint(2, 0.0); // Spot check values from the curve - CHECK_EQUAL(kf.GetMaxPoint().co.Y, 1.0); + CHECK_EQUAL(1.0, kf.GetMaxPoint().co.Y); kf.AddPoint(3, 2.0); // Spot check values from the curve - CHECK_EQUAL(kf.GetMaxPoint().co.Y, 2.0); + CHECK_EQUAL(2.0, kf.GetMaxPoint().co.Y); kf.AddPoint(4, 1.0); // Spot check values from the curve - CHECK_EQUAL(kf.GetMaxPoint().co.Y, 2.0); + CHECK_EQUAL(2.0, kf.GetMaxPoint().co.Y); } TEST(Keyframe_Scale_Keyframe) @@ -380,14 +380,14 @@ TEST(Keyframe_Remove_Duplicate_Point) kf.AddPoint(1, 2.0); // Spot check values from the curve - CHECK_EQUAL(kf.GetLength(), 1); - CHECK_CLOSE(kf.GetPoint(0).co.Y, 2.0, 0.01); + CHECK_EQUAL(1, kf.GetLength()); + CHECK_CLOSE(2.0, kf.GetPoint(0).co.Y, 0.01); } TEST(Keyframe_Large_Number_Values) { // Large value - int64_t large_value = 30 * 60 * 90; + int64_t const large_value = 30 * 60 * 90; // Create a keyframe curve with 2 points Keyframe kf; @@ -395,9 +395,9 @@ TEST(Keyframe_Large_Number_Values) kf.AddPoint(large_value, 100.0); // 90 minutes long // Spot check values from the curve - CHECK_EQUAL(kf.GetLength(), large_value + 1); - CHECK_CLOSE(kf.GetPoint(0).co.Y, 1.0, 0.01); - CHECK_CLOSE(kf.GetPoint(1).co.Y, 100.0, 0.01); + CHECK_EQUAL(large_value + 1, kf.GetLength()); + CHECK_CLOSE(1.0, kf.GetPoint(0).co.Y, 0.01); + CHECK_CLOSE(100.0, kf.GetPoint(1).co.Y, 0.01); } TEST(Keyframe_Remove_Point) From edf85dda783c610c43d466704618944e6cbbb9af Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Mon, 25 Nov 2019 10:23:45 +0100 Subject: [PATCH 17/36] Keyframe: use = default to specify default constructor --- include/KeyFrame.h | 2 +- src/KeyFrame.cpp | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/include/KeyFrame.h b/include/KeyFrame.h index aafd6c4b..4466dcaa 100644 --- a/include/KeyFrame.h +++ b/include/KeyFrame.h @@ -68,7 +68,7 @@ namespace openshot { public: /// Default constructor for the Keyframe class - Keyframe(); + Keyframe() = default; /// Constructor which sets the default point & coordinate at X=1 Keyframe(double value); diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index 2cd0b35a..ec9a64ba 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -42,10 +42,6 @@ Keyframe::Keyframe(double value) { AddPoint(Point(value)); } -// Keyframe constructor -Keyframe::Keyframe() { -} - // Add a new point on the key-frame. Each point has a primary coordinate, // a left handle, and a right handle. void Keyframe::AddPoint(Point p) { From 6f71736c6a14b5c3c0cb099d63cf9f0eb3a438d9 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Mon, 25 Nov 2019 10:24:37 +0100 Subject: [PATCH 18/36] Keyframe: mark all non-modifying member functions const Member functions that do not (need to) modify any members or internal state of the Keyframe class should be declared as const. --- include/KeyFrame.h | 38 ++++++++++++++++----------------- src/KeyFrame.cpp | 52 ++++++++++++++++++++-------------------------- 2 files changed, 42 insertions(+), 48 deletions(-) diff --git a/include/KeyFrame.h b/include/KeyFrame.h index 4466dcaa..6ed39553 100644 --- a/include/KeyFrame.h +++ b/include/KeyFrame.h @@ -83,57 +83,57 @@ namespace openshot { void AddPoint(double x, double y, InterpolationType interpolate); /// Does this keyframe contain a specific point - bool Contains(Point p); + bool Contains(Point p) const; /// Flip all the points in this openshot::Keyframe (useful for reversing an effect or transition, etc...) void FlipPoints(); /// Get the index of a point by matching a coordinate - int64_t FindIndex(Point p); + int64_t FindIndex(Point p) const; /// Get the value at a specific index - double GetValue(int64_t index); + double GetValue(int64_t index) const; /// Get the rounded INT value at a specific index - int GetInt(int64_t index); + int GetInt(int64_t index) const; /// Get the rounded LONG value at a specific index - int64_t GetLong(int64_t index); + int64_t GetLong(int64_t index) const; /// Get the fraction that represents how many times this value is repeated in the curve - Fraction GetRepeatFraction(int64_t index); + Fraction GetRepeatFraction(int64_t index) const; /// Get the change in Y value (from the previous Y value) - double GetDelta(int64_t index); + double GetDelta(int64_t index) const; /// Get a point at a specific index - Point const & GetPoint(int64_t index); + Point const & GetPoint(int64_t index) const; /// Get current point (or closest point to the right) from the X coordinate (i.e. the frame number) - Point GetClosestPoint(Point p); + Point GetClosestPoint(Point p) const; /// Get current point (or closest point) from the X coordinate (i.e. the frame number) /// Either use the closest left point, or right point - Point GetClosestPoint(Point p, bool useLeft); + Point GetClosestPoint(Point p, bool useLeft) const; /// Get previous point ( - Point GetPreviousPoint(Point p); + Point GetPreviousPoint(Point p) const; /// Get max point (by Y coordinate) - Point GetMaxPoint(); + Point GetMaxPoint() const; // Get the number of values (i.e. coordinates on the X axis) - int64_t GetLength(); + int64_t GetLength() const; /// Get the number of points (i.e. # of points) - int64_t GetCount(); + int64_t GetCount() const; /// Get the direction of the curve at a specific index (increasing or decreasing) - bool IsIncreasing(int index); + bool IsIncreasing(int index) const; /// Get and Set JSON methods - std::string Json(); ///< Generate JSON string of this object - Json::Value JsonValue(); ///< Generate Json::JsonValue for this object + std::string Json() const; ///< Generate JSON string of this object + Json::Value JsonValue() const; ///< Generate Json::JsonValue for this object void SetJson(std::string value); ///< Load JSON string into this object void SetJsonValue(Json::Value root); ///< Load Json::JsonValue into this object @@ -151,10 +151,10 @@ namespace openshot { void UpdatePoint(int64_t index, Point p); /// Print a list of points - void PrintPoints(); + void PrintPoints() const; /// Print just the Y value of the point's primary coordinate - void PrintValues(); + void PrintValues() const; }; diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index ec9a64ba..cc72881a 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -91,7 +91,7 @@ void Keyframe::AddPoint(double x, double y, InterpolationType interpolate) } // Get the index of a point by matching a coordinate -int64_t Keyframe::FindIndex(Point p) { +int64_t Keyframe::FindIndex(Point p) const { // loop through points, and find a matching coordinate for (int64_t x = 0; x < Points.size(); x++) { // Get each point @@ -109,7 +109,7 @@ int64_t Keyframe::FindIndex(Point p) { } // Determine if point already exists -bool Keyframe::Contains(Point p) { +bool Keyframe::Contains(Point p) const { // loop through points, and find a matching coordinate for (int64_t x = 0; x < Points.size(); x++) { // Get each point @@ -127,7 +127,7 @@ bool Keyframe::Contains(Point p) { } // Get current point (or closest point) from the X coordinate (i.e. the frame number) -Point Keyframe::GetClosestPoint(Point p, bool useLeft) { +Point Keyframe::GetClosestPoint(Point p, bool useLeft) const { Point closest(-1, -1); // loop through points, and find a matching coordinate @@ -164,12 +164,12 @@ Point Keyframe::GetClosestPoint(Point p, bool useLeft) { } // Get current point (or closest point to the right) from the X coordinate (i.e. the frame number) -Point Keyframe::GetClosestPoint(Point p) { +Point Keyframe::GetClosestPoint(Point p) const { return GetClosestPoint(p, false); } // Get previous point (if any) -Point Keyframe::GetPreviousPoint(Point p) { +Point Keyframe::GetPreviousPoint(Point p) const { // Lookup the index of this point try { @@ -188,7 +188,7 @@ Point Keyframe::GetPreviousPoint(Point p) { } // Get max point (by Y coordinate) -Point Keyframe::GetMaxPoint() { +Point Keyframe::GetMaxPoint() const { Point maxPoint(-1, -1); // loop through points, and find the largest Y value @@ -207,12 +207,11 @@ Point Keyframe::GetMaxPoint() { } // Get the value at a specific index -double Keyframe::GetValue(int64_t index) -{ +double Keyframe::GetValue(int64_t index) const { if (Points.empty()) { return 0; } - std::vector::iterator candidate = + std::vector::const_iterator candidate = std::lower_bound(begin(Points), end(Points), Point(index, -1), [](Point const & l, Point const & r) { return l.co.X < r.co.X; }); @@ -229,7 +228,7 @@ double Keyframe::GetValue(int64_t index) // index is directly on a point return candidate->co.Y; } - std::vector::iterator predecessor = candidate - 1; + std::vector::const_iterator predecessor = candidate - 1; assert(predecessor->co.X < index); assert(index < candidate->co.X); @@ -284,19 +283,17 @@ double Keyframe::GetValue(int64_t index) } // Get the rounded INT value at a specific index -int Keyframe::GetInt(int64_t index) -{ +int Keyframe::GetInt(int64_t index) const { return int(round(GetValue(index))); } // Get the rounded INT value at a specific index -int64_t Keyframe::GetLong(int64_t index) -{ +int64_t Keyframe::GetLong(int64_t index) const { return long(round(GetValue(index))); } // Get the direction of the curve at a specific index (increasing or decreasing) -bool Keyframe::IsIncreasing(int index) +bool Keyframe::IsIncreasing(int index) const { if (index < 1 || (index + 1) >= GetLength()) { return true; @@ -312,14 +309,14 @@ bool Keyframe::IsIncreasing(int index) } // Generate JSON string of this object -std::string Keyframe::Json() { +std::string Keyframe::Json() const { // Return formatted string return JsonValue().toStyledString(); } // Generate Json::JsonValue for this object -Json::Value Keyframe::JsonValue() { +Json::Value Keyframe::JsonValue() const { // Create root json object Json::Value root; @@ -389,8 +386,7 @@ void Keyframe::SetJsonValue(Json::Value root) { // Get the fraction that represents how many times this value is repeated in the curve // This is depreciated and will be removed soon. -Fraction Keyframe::GetRepeatFraction(int64_t index) -{ +Fraction Keyframe::GetRepeatFraction(int64_t index) const { // Is index a valid point? if (index >= 1 && (index + 1) < GetLength()) { int64_t current_value = GetLong(index); @@ -428,8 +424,7 @@ Fraction Keyframe::GetRepeatFraction(int64_t index) } // Get the change in Y value (from the previous Y value) -double Keyframe::GetDelta(int64_t index) -{ +double Keyframe::GetDelta(int64_t index) const { if (index < 1) return 0; if (index == 1 && ! Points.empty()) return Points[0].co.Y; if (index >= GetLength()) return 0; @@ -437,7 +432,7 @@ double Keyframe::GetDelta(int64_t index) } // Get a point at a specific index -Point const & Keyframe::GetPoint(int64_t index) { +Point const & Keyframe::GetPoint(int64_t index) const { // Is index a valid point? if (index >= 0 && index < Points.size()) return Points[index]; @@ -447,14 +442,14 @@ Point const & Keyframe::GetPoint(int64_t index) { } // Get the number of values (i.e. coordinates on the X axis) -int64_t Keyframe::GetLength() { +int64_t Keyframe::GetLength() const { if (Points.empty()) return 0; if (Points.size() == 1) return 1; return round(Points.back().co.X) + 1; } // Get the number of points (i.e. # of points) -int64_t Keyframe::GetCount() { +int64_t Keyframe::GetCount() const { return Points.size(); } @@ -499,15 +494,15 @@ void Keyframe::UpdatePoint(int64_t index, Point p) { AddPoint(p); } -void Keyframe::PrintPoints() { +void Keyframe::PrintPoints() const { cout << fixed << setprecision(4); - for (std::vector::iterator it = Points.begin(); it != Points.end(); it++) { + for (std::vector::const_iterator it = Points.begin(); it != Points.end(); it++) { Point p = *it; cout << p.co.X << "\t" << p.co.Y << endl; } } -void Keyframe::PrintValues() { +void Keyframe::PrintValues() const { cout << fixed << setprecision(4); cout << "Frame Number (X)\tValue (Y)\tIs Increasing\tRepeat Numerator\tRepeat Denominator\tDelta (Y Difference)\n"; @@ -534,8 +529,7 @@ void Keyframe::ScalePoints(double scale) } // Flip all the points in this openshot::Keyframe (useful for reversing an effect or transition, etc...) -void Keyframe::FlipPoints() -{ +void Keyframe::FlipPoints() { for (int64_t point_index = 0, reverse_index = Points.size() - 1; point_index < reverse_index; point_index++, reverse_index--) { // Flip the points using std::swap; From b546b6a982346de7c93b0a396a3c9ad97499d665 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Mon, 25 Nov 2019 10:34:14 +0100 Subject: [PATCH 19/36] Keyframe: Dedicated Point comparision function instead of lambda's --- src/KeyFrame.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index cc72881a..7e4daeab 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -35,6 +35,12 @@ using namespace std; using namespace openshot; +namespace { + bool IsPointBeforeX(Point const & p, double const x) { + return p.co.X < x; + } +} + // Constructor which sets the default point & coordinate at X=1 Keyframe::Keyframe(double value) { @@ -48,9 +54,7 @@ void Keyframe::AddPoint(Point p) { // candidate is not less (greater or equal) than the new point in // the X coordinate. std::vector::iterator candidate = - std::lower_bound(begin(Points), end(Points), p, [](Point const & l, Point const & r) { - return l.co.X < r.co.X; - }); + std::lower_bound(begin(Points), end(Points), p.co.X, IsPointBeforeX); if (candidate == end(Points)) { // New point X is greater than all other points' X, add to // back. @@ -212,9 +216,7 @@ double Keyframe::GetValue(int64_t index) const { return 0; } std::vector::const_iterator candidate = - std::lower_bound(begin(Points), end(Points), Point(index, -1), [](Point const & l, Point const & r) { - return l.co.X < r.co.X; - }); + std::lower_bound(begin(Points), end(Points), static_cast(index), IsPointBeforeX); if (candidate == end(Points)) { // index is behind last point From a67fb9555cc4bd84f95b1c2ce0f6144c25621ccd Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Mon, 25 Nov 2019 10:37:51 +0100 Subject: [PATCH 20/36] Keyframe interpolation selection: Use switch instead of if Using switch allows (some) compilers to emit a warning if a possible enum value for the interpolation type has been forgotten. This is not the case now, but might guard against future errors (e.g. adding an interpolation type) --- src/KeyFrame.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index 7e4daeab..8f89d1da 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -235,18 +235,18 @@ double Keyframe::GetValue(int64_t index) const { assert(index < candidate->co.X); // CONSTANT and LINEAR interpolations are fast to compute! - if (candidate->interpolation == CONSTANT) { - return predecessor->co.Y; - } - if (candidate->interpolation == LINEAR) { + switch (candidate->interpolation) { + case CONSTANT: return predecessor->co.Y; + case LINEAR: { double const diff_Y = candidate->co.Y - predecessor->co.Y; double const diff_X = candidate->co.X - predecessor->co.X; double const slope = diff_Y / diff_X; return predecessor->co.Y + slope * (index - predecessor->co.X); } + case BEZIER: break; + } // BEZIER curve! - // TODO: use switch instead of if for compiler warning support! assert(candidate->interpolation == BEZIER); double const X_diff = candidate->co.X - predecessor->co.X; From 4b76c1eadce78382b90a8311806eaeee3db94a08 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Tue, 26 Nov 2019 00:56:13 +0100 Subject: [PATCH 21/36] Frame.cpp: Avoid unnecessary copy of image data As mentioned in issue #202 QImage::bits() and QImage::scanLine() make a deep copy of the source image. This is completely unnecessary when read-only access to the pixel data is required. Changing to QImage::constBits() and QImage::constScanLine() solves this. Both functions were introduced in Qt 4.7. https://doc.qt.io/qt-5/qimage.html#constBits --- src/Frame.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Frame.cpp b/src/Frame.cpp index 475d5c28..9ae3ca04 100644 --- a/src/Frame.cpp +++ b/src/Frame.cpp @@ -280,7 +280,7 @@ const unsigned char* Frame::GetWaveformPixels(int width, int height, int Red, in wave_image = GetWaveform(width, height, Red, Green, Blue, Alpha); // Return array of pixel packets - return wave_image->bits(); + return wave_image->constBits(); } // Display the wave form @@ -473,14 +473,14 @@ const unsigned char* Frame::GetPixels() AddColor(width, height, color); // Return array of pixel packets - return image->bits(); + return image->constBits(); } // Get pixel data (for only a single scan-line) const unsigned char* Frame::GetPixels(int row) { // Return array of pixel packets - return image->scanLine(row); + return image->constScanLine(row); } // Check a specific pixel color value (returns True/False) @@ -692,7 +692,7 @@ void Frame::Thumbnail(std::string path, int new_width, int new_height, std::stri // Get pixels unsigned char *pixels = (unsigned char *) thumbnail->bits(); - unsigned char *mask_pixels = (unsigned char *) mask->bits(); + unsigned char const *mask_pixels = (unsigned char *) mask->constBits(); // Convert the mask image to grayscale // Loop through pixels @@ -826,8 +826,8 @@ void Frame::AddImage(std::shared_ptr new_image, bool only_odd_lines) const GenericScopedLock lock(addingImageSection); #pragma omp critical (AddImage) { - const unsigned char *pixels = image->bits(); - const unsigned char *new_pixels = new_image->bits(); + const unsigned char *pixels = image->constBits(); + const unsigned char *new_pixels = new_image->constBits(); // Loop through the scanlines of the image (even or odd) int start = 0; @@ -922,7 +922,7 @@ std::shared_ptr Frame::GetMagickImage() AddColor(width, height, "#000000"); // Get the pixels from the frame image - QRgb const *tmpBits = (const QRgb*)image->bits(); + QRgb const *tmpBits = (const QRgb*)image->constBits(); // Create new image object, and fill with pixel data std::shared_ptr magick_image = std::shared_ptr(new Magick::Image(image->width(), image->height(),"RGBA", Magick::CharPixel, tmpBits)); From 7e2846039e2dceb5f5a5791e87590ef31b9cb19d Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Wed, 27 Nov 2019 23:57:58 +0100 Subject: [PATCH 22/36] More traditional placement of const specifier, matching casts As suggested in the code review: - More traditional placment of the const specifier, e.g. const unsigned char * instead of unsigned char const * - Matching casts to also cast to const unsigned char * instead of of unsigned char * Co-Authored-By: Frank Dana --- src/Frame.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Frame.cpp b/src/Frame.cpp index 9ae3ca04..40183422 100644 --- a/src/Frame.cpp +++ b/src/Frame.cpp @@ -692,7 +692,7 @@ void Frame::Thumbnail(std::string path, int new_width, int new_height, std::stri // Get pixels unsigned char *pixels = (unsigned char *) thumbnail->bits(); - unsigned char const *mask_pixels = (unsigned char *) mask->constBits(); + const unsigned char *mask_pixels = (const unsigned char *) mask->constBits(); // Convert the mask image to grayscale // Loop through pixels @@ -922,7 +922,7 @@ std::shared_ptr Frame::GetMagickImage() AddColor(width, height, "#000000"); // Get the pixels from the frame image - QRgb const *tmpBits = (const QRgb*)image->constBits(); + const QRgb *tmpBits = (const QRgb*)image->constBits(); // Create new image object, and fill with pixel data std::shared_ptr magick_image = std::shared_ptr(new Magick::Image(image->width(), image->height(),"RGBA", Magick::CharPixel, tmpBits)); From 27bfbbc24a4e95400c8c041c2742b02538fcbe44 Mon Sep 17 00:00:00 2001 From: chad3814 Date: Fri, 29 Nov 2019 16:07:09 -0800 Subject: [PATCH 23/36] FFmpegWriter: match option 'rc_buffer_size' (#377) --- src/FFmpegWriter.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/FFmpegWriter.cpp b/src/FFmpegWriter.cpp index 6af07d92..645c0cca 100644 --- a/src/FFmpegWriter.cpp +++ b/src/FFmpegWriter.cpp @@ -341,7 +341,7 @@ void FFmpegWriter::SetOption(StreamType stream, std::string name, std::string va // Was option found? if (option || (name == "g" || name == "qmin" || name == "qmax" || name == "max_b_frames" || name == "mb_decision" || name == "level" || name == "profile" || name == "slices" || name == "rc_min_rate" || name == "rc_max_rate" || - name == "crf" || name == "cqp")) { + name == "rc_buffer_size" || name == "crf" || name == "cqp")) { // Check for specific named options if (name == "g") // Set gop_size From 5e1b6fd7409cf938672bb859121a911af66cbcf8 Mon Sep 17 00:00:00 2001 From: Frank Dana Date: Fri, 29 Nov 2019 22:08:09 -0500 Subject: [PATCH 24/36] Minor adjustments to Doxygen API docs (#376) - Define `USE_IMAGEMAGICK` and `USE_BLACKMAGIC` unconditionally when building docs, so that the classes will be documented. - Improve handling of `std::`-prefixed types in doxygen output. --- CMakeLists.txt | 14 +++++++------- Doxyfile.in | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index acbb3195..5cfb678c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -97,14 +97,14 @@ if (TARGET doc) message(STATUS "Doxygen found, documentation target enabled") message("\nTo compile documentation in doc/html, run: 'make doc'") -# Install docs, if the user builds them with `make doc` -install(CODE "MESSAGE(\"Checking for documentation files to install...\")") -install(CODE "MESSAGE(\"(Compile with 'make doc' command, requires Doxygen)\")") + # Install docs, if the user builds them with `make doc` + install(CODE "MESSAGE(\"Checking for documentation files to install...\")") + install(CODE "MESSAGE(\"(Compile with 'make doc' command, requires Doxygen)\")") -install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/doc/html/ - DESTINATION ${CMAKE_INSTALL_DOCDIR}/API - MESSAGE_NEVER # Don't spew about file copies - OPTIONAL ) # No error if the docs aren't found + install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/doc/html/ + DESTINATION ${CMAKE_INSTALL_DOCDIR}/API + MESSAGE_NEVER # Don't spew about file copies + OPTIONAL ) # No error if the docs aren't found endif() ############# PROCESS tests/ DIRECTORY ############## diff --git a/Doxyfile.in b/Doxyfile.in index 7b54a9c0..c47d6e65 100644 --- a/Doxyfile.in +++ b/Doxyfile.in @@ -340,7 +340,7 @@ AUTOLINK_SUPPORT = YES # diagrams that involve STL classes more complete and accurate. # The default value is: NO. -BUILTIN_STL_SUPPORT = NO +BUILTIN_STL_SUPPORT = YES # If you use Microsoft's C++/CLI language, you should set this option to YES to # enable parsing support. @@ -2096,7 +2096,7 @@ INCLUDE_FILE_PATTERNS = # recursively expanded use the := operator instead of the = operator. # This tag requires that the tag ENABLE_PREPROCESSING is set to YES. -PREDEFINED = +PREDEFINED = USE_BLACKMAGIC USE_IMAGEMAGICK # If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then this # tag can be used to specify a list of macro names that should be expanded. The From 54e8e37d2db141158e2cf10572ad96c57462e140 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Sat, 30 Nov 2019 11:32:52 +0100 Subject: [PATCH 25/36] Keyframe::Contains(): Use binary search instead of linear search --- src/KeyFrame.cpp | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index 8f89d1da..686626b0 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -114,20 +114,9 @@ int64_t Keyframe::FindIndex(Point p) const { // Determine if point already exists bool Keyframe::Contains(Point p) const { - // loop through points, and find a matching coordinate - for (int64_t x = 0; x < Points.size(); x++) { - // Get each point - Point existing_point = Points[x]; - - // find a match - if (p.co.X == existing_point.co.X) { - // Remove the matching point, and break out of loop - return true; - } - } - - // no matching point found - return false; + std::vector::const_iterator i = + std::lower_bound(begin(Points), end(Points), p.co.X, IsPointBeforeX); + return i != end(Points) && i->co.X == p.co.X; } // Get current point (or closest point) from the X coordinate (i.e. the frame number) From 65cb3dfde90bb8ae1c5751b86f006ecf67c1af22 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Sat, 30 Nov 2019 11:58:51 +0100 Subject: [PATCH 26/36] Keyframe::GetClosestPoint(): Use binary search --- src/KeyFrame.cpp | 53 +++++++++++++++++++++--------------------------- 1 file changed, 23 insertions(+), 30 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index 686626b0..6b186d37 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -121,39 +121,32 @@ bool Keyframe::Contains(Point p) const { // Get current point (or closest point) from the X coordinate (i.e. the frame number) Point Keyframe::GetClosestPoint(Point p, bool useLeft) const { - Point closest(-1, -1); - - // loop through points, and find a matching coordinate - for (int64_t x = 0; x < Points.size(); x++) { - // Get each point - Point existing_point = Points[x]; - - // find a match - if (existing_point.co.X >= p.co.X && !useLeft) { - // New closest point found (to the Right) - closest = existing_point; - break; - } else if (existing_point.co.X < p.co.X && useLeft) { - // New closest point found (to the Left) - closest = existing_point; - } else if (existing_point.co.X >= p.co.X && useLeft) { - // We've gone past the left point... so break - break; - } + if (Points.size() == 0) { + return Point(-1, -1); } - // Handle edge cases (if no point was found) - if (closest.co.X == -1) { - if (p.co.X <= 1 && Points.size() > 0) - // Assign 1st point - closest = Points[0]; - else if (Points.size() > 0) - // Assign last point - closest = Points[Points.size() - 1]; - } + // Finds a point with an X coordinate which is "not less" (greater + // or equal) than the queried X coordinate. + std::vector::const_iterator candidate = + std::lower_bound(begin(Points), end(Points), p.co.X, IsPointBeforeX); - // no matching point found - return closest; + if (candidate == end(Points)) { + // All points are before the queried point. + // + // Note: Behavior the same regardless of useLeft! + return Points.back(); + } + if (candidate == begin(Points)) { + // First point is greater or equal to the queried point. + // + // Note: Behavior the same regardless of useLeft! + return Points.front(); + } + if (useLeft) { + return *(candidate - 1); + } else { + return *candidate; + } } // Get current point (or closest point to the right) from the X coordinate (i.e. the frame number) From c04dc94cc82ce3afe5d0b35e032f63fe4b3ef9b2 Mon Sep 17 00:00:00 2001 From: Frank Dana Date: Mon, 2 Dec 2019 10:45:06 -0500 Subject: [PATCH 27/36] Wrap assignment in conditional with () (#379) --- src/Clip.cpp | 2 +- src/Timeline.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Clip.cpp b/src/Clip.cpp index e3f33a18..1a86c644 100644 --- a/src/Clip.cpp +++ b/src/Clip.cpp @@ -914,7 +914,7 @@ void Clip::SetJsonValue(Json::Value root) { if (!existing_effect["type"].isNull()) { // Create instance of effect - if (e = EffectInfo().CreateEffect(existing_effect["type"].asString())) { + if ( (e = EffectInfo().CreateEffect(existing_effect["type"].asString())) ) { // Load Json into Effect e->SetJsonValue(existing_effect); diff --git a/src/Timeline.cpp b/src/Timeline.cpp index 51868157..2cb948a4 100644 --- a/src/Timeline.cpp +++ b/src/Timeline.cpp @@ -1098,7 +1098,7 @@ void Timeline::SetJsonValue(Json::Value root) { if (!existing_effect["type"].isNull()) { // Create instance of effect - if (e = EffectInfo().CreateEffect(existing_effect["type"].asString())) { + if ( (e = EffectInfo().CreateEffect(existing_effect["type"].asString())) ) { // Load Json into Effect e->SetJsonValue(existing_effect); @@ -1364,7 +1364,7 @@ void Timeline::apply_json_to_effects(Json::Value change, EffectBase* existing_ef EffectBase *e = NULL; // Init the matching effect object - if (e = EffectInfo().CreateEffect(effect_type)) { + if ( (e = EffectInfo().CreateEffect(effect_type)) ) { // Load Json into Effect e->SetJsonValue(change["value"]); From 79cb8483f3a64f9278ab27ade83899dcf0d2e5dd Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Tue, 3 Dec 2019 16:56:53 +0100 Subject: [PATCH 28/36] Keyframe: Move Bezier code into extra function, parameterise Bezier interpolation code is now in a dedicated function and can be used to either find a Y from a known X or a X from a known Y, with a given allowed error. --- src/KeyFrame.cpp | 72 ++++++++++++++++++++++++++---------------------- 1 file changed, 39 insertions(+), 33 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index 6b186d37..68ea1ef1 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -39,6 +39,44 @@ namespace { bool IsPointBeforeX(Point const & p, double const x) { return p.co.X < x; } + + double InterpolateBezierCurve(Point const & left, Point const & right, bool const needY, double const target, double const allowed_error) { + double const X_diff = right.co.X - left.co.X; + double const Y_diff = right.co.Y - left.co.Y; + Coordinate const p0 = left.co; + Coordinate const p1 = Coordinate(p0.X + left.handle_right.X * X_diff, p0.Y + left.handle_right.Y * Y_diff); + Coordinate const p2 = Coordinate(p0.X + right.handle_left.X * X_diff, p0.Y + right.handle_left.Y * Y_diff); + Coordinate const p3 = right.co; + + double t = 0.5; + double t_step = 0.25; + do { + // Bernstein polynoms + double B[4] = {1, 3, 3, 1}; + double oneMinTExp = 1; + double tExp = 1; + for (int i = 0; i < 4; ++i, tExp *= t) { + B[i] *= tExp; + } + for (int i = 0; i < 4; ++i, oneMinTExp *= 1 - t) { + B[4 - i - 1] *= oneMinTExp; + } + double const x = p0.X * B[0] + p1.X * B[1] + p2.X * B[2] + p3.X * B[3]; + double const y = p0.Y * B[0] + p1.Y * B[1] + p2.Y * B[2] + p3.Y * B[3]; + bool const stop = abs(target - (needY ? x : y)) < allowed_error; + bool const move_left = (needY ? x : y) > target; + if (stop) { + return needY ? y : x; + } + if (move_left) { + t -= t_step; + } + else { + t += t_step; + } + t_step /= 2; + } while (true); + } } @@ -231,39 +269,7 @@ double Keyframe::GetValue(int64_t index) const { // BEZIER curve! assert(candidate->interpolation == BEZIER); - double const X_diff = candidate->co.X - predecessor->co.X; - double const Y_diff = candidate->co.Y - predecessor->co.Y; - Coordinate const p0 = predecessor->co; - Coordinate const p1 = Coordinate(p0.X + predecessor->handle_right.X * X_diff, p0.Y + predecessor->handle_right.Y * Y_diff); - Coordinate const p2 = Coordinate(p0.X + candidate->handle_left.X * X_diff, p0.Y + candidate->handle_left.Y * Y_diff); - Coordinate const p3 = candidate->co; - - double t = 0.5; - double t_step = 0.25; - do { - // Bernstein polynoms - double B[4] = {1, 3, 3, 1}; - double oneMinTExp = 1; - double tExp = 1; - for (int i = 0; i < 4; ++i, tExp *= t) { - B[i] *= tExp; - } - for (int i = 0; i < 4; ++i, oneMinTExp *= 1 - t) { - B[4 - i - 1] *= oneMinTExp; - } - double const x = p0.X * B[0] + p1.X * B[1] + p2.X * B[2] + p3.X * B[3]; - double const y = p0.Y * B[0] + p1.Y * B[1] + p2.Y * B[2] + p3.Y * B[3]; - if (abs(index - x) < 0.01) { - return y; - } - if (x > index) { - t -= t_step; - } - else { - t += t_step; - } - t_step /= 2; - } while (true); + return InterpolateBezierCurve(*predecessor, *candidate, true, index, 0.01); } // Get the rounded INT value at a specific index From b40fa6922e0c4b9ac78527db14cc77ed21f082b1 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Tue, 3 Dec 2019 16:58:53 +0100 Subject: [PATCH 29/36] Keyframe::GetMaxPoint() simplify loop --- src/KeyFrame.cpp | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index 68ea1ef1..8982f29d 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -215,14 +215,8 @@ Point Keyframe::GetPreviousPoint(Point p) const { Point Keyframe::GetMaxPoint() const { Point maxPoint(-1, -1); - // loop through points, and find the largest Y value - for (int64_t x = 0; x < Points.size(); x++) { - // Get each point - Point existing_point = Points[x]; - - // Is point larger than max point + for (Point const & existing_point: Points) { if (existing_point.co.Y >= maxPoint.co.Y) { - // New max point found maxPoint = existing_point; } } From ed0b0818033cab39da39b37c7bfe5237dd37954a Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Tue, 3 Dec 2019 17:27:28 +0100 Subject: [PATCH 30/36] Keyframe::IsIncreasing(): Search over points, not values Searching over the keyframe points is considerably faster than calculating interpolating values and searching over them. --- src/KeyFrame.cpp | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index 8982f29d..ba2fda32 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -282,13 +282,23 @@ bool Keyframe::IsIncreasing(int index) const if (index < 1 || (index + 1) >= GetLength()) { return true; } - int64_t const current = GetLong(index); - // TODO: skip over constant sections. + std::vector::const_iterator candidate = + std::lower_bound(begin(Points), end(Points), static_cast(index), IsPointBeforeX); + if (candidate == end(Points)) { + return false; // After the last point, thus constant. + } + if ((candidate->co.X == index) || (candidate == begin(Points))) { + ++candidate; + } + int64_t const value = GetLong(index); do { - int64_t const next = GetLong(++index); - if (next > current) return true; - if (next < current) return false; - } while (index < GetLength()); + if (value < round(candidate->co.Y)) { + return true; + } else if (value > round(candidate->co.Y)) { + return false; + } + ++candidate; + } while (candidate != end(Points)); return false; } From 4a5eb20202e01ee4cbde7ad62029dc13e8452a33 Mon Sep 17 00:00:00 2001 From: "FeRD (Frank Dana)" Date: Mon, 2 Dec 2019 22:27:52 -0500 Subject: [PATCH 31/36] Add __repr__ to openshot.Version --- src/bindings/python/openshot.i | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/bindings/python/openshot.i b/src/bindings/python/openshot.i index a2228d75..44b1953e 100644 --- a/src/bindings/python/openshot.i +++ b/src/bindings/python/openshot.i @@ -153,10 +153,16 @@ } %extend openshot::OpenShotVersion { - // Give the struct a string representation + // Give the struct a string representation const std::string __str__() { return std::string(OPENSHOT_VERSION_FULL); } + // And a repr for interactive use + const std::string __repr__() { + std::ostringstream result; + result << "OpenShotVersion('" << OPENSHOT_VERSION_FULL << "')"; + return result.str(); + } } %include "OpenShotVersion.h" From f00edbad7e193ebb5c9ed78cd18474d2fc3dbc1a Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Fri, 6 Dec 2019 01:03:56 +0100 Subject: [PATCH 32/36] Keyframe interpolation: In own function; only for Y coordinate --- src/KeyFrame.cpp | 48 +++++++++++++++++++++++------------------------- 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index ba2fda32..0e850b07 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -40,7 +40,14 @@ namespace { return p.co.X < x; } - double InterpolateBezierCurve(Point const & left, Point const & right, bool const needY, double const target, double const allowed_error) { + double InterpolateLinearCurve(Point const & left, Point const & right, double const target) { + double const diff_Y = right.co.Y - left.co.Y; + double const diff_X = right.co.X - left.co.X; + double const slope = diff_Y / diff_X; + return left.co.Y + slope * (target - left.co.X); + } + + double InterpolateBezierCurve(Point const & left, Point const & right, double const target, double const allowed_error) { double const X_diff = right.co.X - left.co.X; double const Y_diff = right.co.Y - left.co.Y; Coordinate const p0 = left.co; @@ -63,12 +70,10 @@ namespace { } double const x = p0.X * B[0] + p1.X * B[1] + p2.X * B[2] + p3.X * B[3]; double const y = p0.Y * B[0] + p1.Y * B[1] + p2.Y * B[2] + p3.Y * B[3]; - bool const stop = abs(target - (needY ? x : y)) < allowed_error; - bool const move_left = (needY ? x : y) > target; - if (stop) { - return needY ? y : x; + if (abs(target - x) < allowed_error) { + return y; } - if (move_left) { + if (x > target) { t -= t_step; } else { @@ -77,6 +82,17 @@ namespace { t_step /= 2; } while (true); } + + + double InterpolateBetween(Point const & left, Point const & right, double target, double allowed_error) { + assert(left.co.X < target); + assert(target <= right.co.X); + switch (right.interpolation) { + case CONSTANT: return left.co.Y; + case LINEAR: return InterpolateLinearCurve(left, right, target); + case BEZIER: return InterpolateBezierCurve(left, right, target, allowed_error); + } + } } @@ -245,25 +261,7 @@ double Keyframe::GetValue(int64_t index) const { return candidate->co.Y; } std::vector::const_iterator predecessor = candidate - 1; - assert(predecessor->co.X < index); - assert(index < candidate->co.X); - - // CONSTANT and LINEAR interpolations are fast to compute! - switch (candidate->interpolation) { - case CONSTANT: return predecessor->co.Y; - case LINEAR: { - double const diff_Y = candidate->co.Y - predecessor->co.Y; - double const diff_X = candidate->co.X - predecessor->co.X; - double const slope = diff_Y / diff_X; - return predecessor->co.Y + slope * (index - predecessor->co.X); - } - case BEZIER: break; - } - - // BEZIER curve! - assert(candidate->interpolation == BEZIER); - - return InterpolateBezierCurve(*predecessor, *candidate, true, index, 0.01); + return InterpolateBetween(*predecessor, *candidate, index, 0.01); } // Get the rounded INT value at a specific index From 1fbdc521ca7b3373f7c7abdb7a8139543bd78da5 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Fri, 6 Dec 2019 01:04:47 +0100 Subject: [PATCH 33/36] Keyframe::GetRepeatFraction(): Binary search, skipping when constant The old implementation did a linear scan over the values. This was slow with slowly changing keyframes. This new implementation skips over constant (when rounded) segments and performs binary search in (possibly long) interpolated segments to find the X coordinates where a change occurs quickly. --- src/KeyFrame.cpp | 149 ++++++++++++++++++++++++++++++++++++----------- 1 file changed, 116 insertions(+), 33 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index 0e850b07..c59d5bd8 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -379,40 +379,123 @@ void Keyframe::SetJsonValue(Json::Value root) { // Get the fraction that represents how many times this value is repeated in the curve // This is depreciated and will be removed soon. Fraction Keyframe::GetRepeatFraction(int64_t index) const { - // Is index a valid point? - if (index >= 1 && (index + 1) < GetLength()) { - int64_t current_value = GetLong(index); - int64_t previous_repeats = 0; - int64_t next_repeats = 0; - - // Loop backwards and look for the next unique value - for (int64_t i = index; i > 0; --i) { - if (GetLong(i) == current_value) { - // Found same value - previous_repeats++; - } else { - // Found non repeating value, no more repeats found - break; - } - } - - // Loop forwards and look for the next unique value - for (int64_t i = index + 1; i < GetLength(); ++i) { - if (GetLong(i) == current_value) { - // Found same value - next_repeats++; - } else { - // Found non repeating value, no more repeats found - break; - } - } - - int64_t total_repeats = previous_repeats + next_repeats; - return Fraction(previous_repeats, total_repeats); - } - else - // return a blank coordinate + // Frame numbers (index) outside of the "defined" range of this + // keyframe result in a 1/1 default value. + if (index < 1 || (index + 1) >= GetLength()) { return Fraction(1,1); + } + assert(Points.size() > 1); // Due to ! ((index + 1) >= GetLength) there are at least two points! + + // First, get the value at the given frame and the closest point + // to the right. + int64_t const current_value = GetLong(index); + std::vector::const_iterator const candidate = + std::lower_bound(begin(Points), end(Points), static_cast(index), IsPointBeforeX); + assert(candidate != end(Points)); // Due to the (index + 1) >= GetLength check above! + + // Calculate how many of the next values are going to be the same: + int64_t next_repeats = 0; + std::vector::const_iterator i = candidate; + // If the index (frame number) is the X coordinate of the closest + // point, then look at the segment to the right; the "current" + // segement is not interesting because we're already at the last + // value of it. + if (i->co.X == index) { + ++i; + } + // Skip over "constant" (when rounded) segments. + bool all_constant = true; + for (; i != end(Points); ++i) { + if (current_value != round(i->co.Y)) { + all_constant = false; + break; + } + } + if (! all_constant) { + // Found a point which defines a segment which will give a + // different value than the current value. This means we + // moved at least one segment to the right, thus we cannot be + // at the first point. + assert(i != begin(Points)); + Point const left = *(i - 1); + Point const right = *i; + // Binary search for the first value which is different from + // the current value. + bool const increasing = current_value < round(i->co.Y); + int64_t start = left.co.X; + int64_t stop = right.co.X; + while (start < stop) { + int64_t const mid = (start + stop + 1) / 2; + double const value = InterpolateBetween(left, right, mid, 0.01); + bool const smaller = round(value) <= current_value; + bool const larger = round(value) >= current_value; + if ((increasing && smaller) || (!increasing && larger)) { + start = mid; + } else { + stop = mid - 1; + } + } + next_repeats = start - index; + } else { + // All values to the right are the same! + next_repeats = Points.back().co.X - index; + } + + // Now look to the left, to the previous values. + all_constant = true; + i = candidate; + if (i != begin(Points)) { + // The binary search below assumes i to be the left point; + // candidate is the right point of the current segment + // though. So change this if possible. If this branch is NOT + // taken, then we're at/before the first point and all is + // constant! + --i; + } + int64_t previous_repeats = 0; + // Skip over constant (when rounded) segments! + for (; i != begin(Points); --i) { + if (current_value != round(i->co.Y)) { + all_constant = false; + break; + } + } + // Special case when skipped until the first point, but the first + // point is actually different. Will not happen if index is + // before the first point! + if (current_value != round(i->co.Y)) { + assert(i != candidate); + all_constant = false; + } + if (! all_constant) { + // There are at least two points, and we're not at the end, + // thus the following is safe! + Point const left = *i; + Point const right = *(i + 1); + // Binary search for the last value (seen from the left to + // right) to be different than the current value. + bool const increasing = current_value > round(left.co.Y); + int64_t start = left.co.X; + int64_t stop = right.co.X; + while (start < stop) { + int64_t const mid = (start + stop + 1) / 2; + double const value = InterpolateBetween(left, right, mid, 0.01); + bool const smaller = round(value) < current_value; + bool const larger = round(value) > current_value; + if ((increasing && smaller) || (!increasing && larger)) { + start = mid; + } else { + stop = mid - 1; + } + } + previous_repeats = index - start; + } else { + // Every previous value is the same (rounded) as the current + // value. + previous_repeats = index; + } + int64_t total_repeats = previous_repeats + next_repeats; + return Fraction(previous_repeats, total_repeats); } // Get the change in Y value (from the previous Y value) From c940c1f42b65696a8154bb67ea2f3cf11c0c0b61 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Fri, 6 Dec 2019 01:21:25 +0100 Subject: [PATCH 34/36] Keyframe: Cleanup duplicate binary search code GetRepeatFraction uses two binary searches; reuse that code! --- src/KeyFrame.cpp | 64 ++++++++++++++++++++++++------------------------ 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/src/KeyFrame.cpp b/src/KeyFrame.cpp index c59d5bd8..d16348a2 100644 --- a/src/KeyFrame.cpp +++ b/src/KeyFrame.cpp @@ -30,6 +30,7 @@ #include "../include/KeyFrame.h" #include +#include #include using namespace std; @@ -93,6 +94,23 @@ namespace { case BEZIER: return InterpolateBezierCurve(left, right, target, allowed_error); } } + + + template + int64_t SearchBetweenPoints(Point const & left, Point const & right, int64_t const current, Check check) { + int64_t start = left.co.X; + int64_t stop = right.co.X; + while (start < stop) { + int64_t const mid = (start + stop + 1) / 2; + double const value = InterpolateBetween(left, right, mid, 0.01); + if (check(round(value), current)) { + start = mid; + } else { + stop = mid - 1; + } + } + return start; + } } @@ -419,23 +437,14 @@ Fraction Keyframe::GetRepeatFraction(int64_t index) const { assert(i != begin(Points)); Point const left = *(i - 1); Point const right = *i; - // Binary search for the first value which is different from - // the current value. - bool const increasing = current_value < round(i->co.Y); - int64_t start = left.co.X; - int64_t stop = right.co.X; - while (start < stop) { - int64_t const mid = (start + stop + 1) / 2; - double const value = InterpolateBetween(left, right, mid, 0.01); - bool const smaller = round(value) <= current_value; - bool const larger = round(value) >= current_value; - if ((increasing && smaller) || (!increasing && larger)) { - start = mid; - } else { - stop = mid - 1; - } + int64_t change_at; + if (current_value < round(i->co.Y)) { + change_at = SearchBetweenPoints(left, right, current_value, std::less_equal{}); + } else { + assert(current_value > round(i->co.Y)); + change_at = SearchBetweenPoints(left, right, current_value, std::greater_equal{}); } - next_repeats = start - index; + next_repeats = change_at - index; } else { // All values to the right are the same! next_repeats = Points.back().co.X - index; @@ -472,23 +481,14 @@ Fraction Keyframe::GetRepeatFraction(int64_t index) const { // thus the following is safe! Point const left = *i; Point const right = *(i + 1); - // Binary search for the last value (seen from the left to - // right) to be different than the current value. - bool const increasing = current_value > round(left.co.Y); - int64_t start = left.co.X; - int64_t stop = right.co.X; - while (start < stop) { - int64_t const mid = (start + stop + 1) / 2; - double const value = InterpolateBetween(left, right, mid, 0.01); - bool const smaller = round(value) < current_value; - bool const larger = round(value) > current_value; - if ((increasing && smaller) || (!increasing && larger)) { - start = mid; - } else { - stop = mid - 1; - } + int64_t change_at; + if (current_value > round(left.co.Y)) { + change_at = SearchBetweenPoints(left, right, current_value, std::less{}); + } else { + assert(current_value < round(left.co.Y)); + change_at = SearchBetweenPoints(left, right, current_value, std::greater{}); } - previous_repeats = index - start; + previous_repeats = index - change_at; } else { // Every previous value is the same (rounded) as the current // value. From 89479bb01a4130b8f3b0e92a177104cb9d777b20 Mon Sep 17 00:00:00 2001 From: Daniel Jour Date: Fri, 6 Dec 2019 01:35:31 +0100 Subject: [PATCH 35/36] Keyframe tests: Add test about large segment, including performance This new test makes sure that a large segment is handled correctly and also with reasonable performance. The timeout of 10ms is still relatively slow (on a mid-class laptop it takes 0.001ms currently) but the test shouldn't fail when e.g. the build machine is under (mild to heavy) load. --- tests/KeyFrame_Tests.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/KeyFrame_Tests.cpp b/tests/KeyFrame_Tests.cpp index 1f03fa80..dbd81500 100644 --- a/tests/KeyFrame_Tests.cpp +++ b/tests/KeyFrame_Tests.cpp @@ -482,3 +482,15 @@ TEST(Keyframe_Use_Interpolation_of_Segment_End_Point) f.AddPoint(100,155, BEZIER); CHECK_CLOSE(75.9, f.GetValue(50), 0.1); } + +TEST(Keyframe_Handle_Large_Segment) +{ + Keyframe kf; + kf.AddPoint(1, 0, CONSTANT); + kf.AddPoint(1000000, 1, LINEAR); + UNITTEST_TIME_CONSTRAINT(10); // 10 milliseconds would still be relatively slow, but need to think about slower build machines! + CHECK_CLOSE(0.5, kf.GetValue(500000), 0.01); + CHECK_EQUAL(true, kf.IsIncreasing(10)); + Fraction fr = kf.GetRepeatFraction(250000); + CHECK_CLOSE(0.5, (double)fr.num / fr.den, 0.01); +} From 7b7f2cc574e238aef779da548c1ccbb870d1e85b Mon Sep 17 00:00:00 2001 From: "FeRD (Frank Dana)" Date: Sat, 7 Dec 2019 13:01:55 -0500 Subject: [PATCH 36/36] Move feature summary to root CMakeLists --- CMakeLists.txt | 15 +++++++++++++++ src/CMakeLists.txt | 12 ------------ 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5cfb678c..574ff8bb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -66,6 +66,15 @@ Generating build files for OpenShot with CMake ${CMAKE_VERSION} # in order to properly configure CMAKE_INSTALL_LIBDIR path include(GNUInstallDirs) +# Collect and display summary of options/dependencies +include(FeatureSummary) + +################ OPTIONS ################## +# Optional build settings for libopenshot +option(USE_SYSTEM_JSONCPP "Use system installed JsonCpp, if found" ON) +option(DISABLE_BUNDLED_JSONCPP "Don't fall back to bundled JsonCpp" OFF) +option(ENABLE_IWYU "Enable 'Include What You Use' scanner (CMake 3.3+)" OFF) + ########## Configure Version.h header ############## configure_file(include/OpenShotVersion.h.in include/OpenShotVersion.h @ONLY) # We'll want that installed later @@ -111,3 +120,9 @@ endif() if(NOT DISABLE_TESTS) add_subdirectory(tests) endif() + +########### PRINT FEATURE SUMMARY ############## +feature_summary(WHAT ALL + INCLUDE_QUIET_PACKAGES + FATAL_ON_MISSING_REQUIRED_PACKAGES + DESCRIPTION "Displaying feature summary\n\nBuild configuration:") diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index fd61ef50..b684aaec 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -27,12 +27,6 @@ # Collect and display summary of options/dependencies include(FeatureSummary) -################ OPTIONS ################## -# Optional build settings for libopenshot -option(USE_SYSTEM_JSONCPP "Use system installed JsonCpp, if found" ON) -option(DISABLE_BUNDLED_JSONCPP "Don't fall back to bundled JsonCpp" OFF) -option(ENABLE_IWYU "Enable 'Include What You Use' scanner (CMake 3.3+)" OFF) - # Automatically process Qt classes with meta-object compiler set(CMAKE_AUTOMOC True) @@ -426,12 +420,6 @@ ENDIF (BLACKMAGIC_FOUND) ############### INCLUDE SWIG BINDINGS ################ add_subdirectory(bindings) -########### PRINT FEATURE SUMMARY ############## -feature_summary(WHAT ALL - INCLUDE_QUIET_PACKAGES - FATAL_ON_MISSING_REQUIRED_PACKAGES - DESCRIPTION "Displaying feature summary\n\nBuild configuration:") - ############### INSTALL HEADERS & LIBRARY ################ set(LIB_INSTALL_DIR lib${LIB_SUFFIX}) # determine correct lib folder