Improve validation of control points

Signed-off-by: Doug Walker <[email protected]>

Author: doug-walker

include/OpenColorIO/OpenColorTransforms.h

@@ -1309,14 +1309,18 @@ extern OCIOEXPORT std::ostream & operator<<(std::ostream &, const GradingPrimary
  * The domain of the curves is [0,1] and control points outside that domain are mapped into it.
  * A hue of 0 or 1 corresponds to a magenta hue.
  * 
+ * The control points are dynamic, so they may be adjusted even after the Transform is included
+ * in a Processor. However, creating a curve or setting the parameters will call the
+ * GradingBSplineCurveImpl::validate function, which will throw an exception if the control
+ * points do not meet certain requirements, for example that the X-coordinates are non-decreasing
+ * Please review that function for details on the validation. Applications that provide a UI to
+ * edit curves must ensure that they prevent users from creating control points that are not valid.
+ * 
  * The transform is invertible as long as the curves allow it. For example, if saturation is
  * mapped to zero, obviously that cannot be resaturated. Care should be taken with the Hue-FX
  * curve because it is possible to fold hues over on themselves, which also cannot be inverted.
  * In most cases the Hue-FX curve is not necessary since the Hue-Hue curve provides similar
  * functionality with the added benefit of being strictly invertible.
- * 
- * The control points are dynamic, so they may be adjusted even after the Transform is included
- * in a Processor.
  */
 class OCIOEXPORT GradingHueCurveTransform : public Transform
 {

src/OpenColorIO/DynamicProperty.cpp

@@ -137,6 +137,8 @@ DynamicPropertyDoubleImplRcPtr DynamicPropertyDoubleImpl::createEditableCopy() c
     return std::make_shared<DynamicPropertyDoubleImpl>(getType(), getValue(), isDynamic());
 }
 
+//========================================================================================
+
 DynamicPropertyGradingPrimaryImpl::DynamicPropertyGradingPrimaryImpl(GradingStyle style,
                                                                      TransformDirection dir,
                                                                      const GradingPrimary & value,
@@ -195,6 +197,8 @@ void DynamicPropertyGradingPrimaryImpl::setDirection(TransformDirection dir) noe
     }
 }
 
+//========================================================================================
+
 DynamicPropertyGradingRGBCurveImpl::DynamicPropertyGradingRGBCurveImpl(
     const ConstGradingRGBCurveRcPtr & value, bool dynamic)
     : DynamicPropertyImpl(DYNAMIC_PROPERTY_GRADING_RGBCURVE, dynamic)
@@ -288,6 +292,7 @@ DynamicPropertyGradingRGBCurveImplRcPtr DynamicPropertyGradingRGBCurveImpl::crea
     return res;
 }
 
+//========================================================================================
 
 DynamicPropertyGradingHueCurveImpl::DynamicPropertyGradingHueCurveImpl(
     const ConstGradingHueCurveRcPtr & value, bool dynamic)
@@ -383,6 +388,8 @@ DynamicPropertyGradingHueCurveImplRcPtr DynamicPropertyGradingHueCurveImpl::crea
     return res;
 }
 
+//========================================================================================
+
 DynamicPropertyGradingToneImpl::DynamicPropertyGradingToneImpl(const GradingTone & value,
                                                                GradingStyle style,
                                                                bool dynamic)

src/OpenColorIO/ops/gradingrgbcurve/GradingBSplineCurve.cpp

@@ -73,6 +73,7 @@ void PrepHueCurveData(const std::vector<GradingControlPoint>& ctrlPnts,
      }
      if (!isHorizontal)
      {
+         // Ensure that there is a minimum space between the y values.
          const float y_span = outCtrlPnts[numCtrlPnts - 1].m_y - outCtrlPnts[0].m_y;
          for (unsigned i = 1; i < outCtrlPnts.size(); ++i)
          {
@@ -665,29 +666,72 @@ void GradingBSplineCurveImpl::validate() const
         throw Exception("The slopes array must be the same length as the control points.");
     }
 
-    // Make sure the points are non-decreasing.
+    // Make sure the x-coordinates are non-decreasing.
     float lastX = -std::numeric_limits<float>::max();
     for (size_t i = 0; i < numPoints; ++i)
     {
-        // Test x values only.
         const float x = m_controlPoints[i].m_x;
         if (x < lastX)
         {
             std::ostringstream oss;
             oss << "Control point at index " << i << " has a x coordinate '" << x << "' that is ";
-            oss << "less than previous control point x cooordinate '" << lastX << "'.";
+            oss << "less than previous control point x coordinate '" << lastX << "'.";
             throw Exception(oss.str().c_str());
         }
         lastX = x;
     }
 
+    // The x-coordinates for a hue-hue spline must be in [0,1].
+    if (m_splineType == HUE_HUE_B_SPLINE)
+    {
+        if (m_controlPoints[0].m_x < 0.f)
+        {
+            std::ostringstream oss;
+            oss << "The HUE-HUE spline may not have negative x coordinates.";
+            throw Exception(oss.str().c_str());
+        }
+        else if (m_controlPoints[numPoints - 1].m_x > 1.f)
+        {
+            std::ostringstream oss;
+            oss << "The HUE-HUE spline may not have x coordinates greater than one.";
+            throw Exception(oss.str().c_str());
+        }
+    }
+
+    // Make sure the y-coordinates are non-decreasing, for diagonal spline types.
+    if ( m_splineType == B_SPLINE          || 
+         m_splineType == DIAGONAL_B_SPLINE ||
+         m_splineType == HUE_HUE_B_SPLINE )
+    {
+        float lastY = -std::numeric_limits<float>::max();
+        if (m_splineType == HUE_HUE_B_SPLINE)
+        {
+            // The curve is diagonal but continues in a periodic way, so wrap the last point
+            // around and ensure the first point would preserve monotonicity.
+            lastY = m_controlPoints[numPoints - 1].m_y - 1.f;
+        }
+
+        for (size_t i = 0; i < numPoints; ++i)
+        {
+            const float y = m_controlPoints[i].m_y;
+            if (y < lastY)
+            {
+                std::ostringstream oss;
+                oss << "Control point at index " << i << " has a y coordinate '" << y << "' that is ";
+                oss << "less than previous control point y coordinate '" << lastY << "'.";
+                throw Exception(oss.str().c_str());
+            }
+            lastY = y;
+        }
+    }
+
     // Don't allow only x values of 0 and 1 for periodic curves (since they are essentially only one point).
     if (numPoints == 2)
     {    
         // Periodic curve types only.
-        if( m_splineType == PERIODIC_1_B_SPLINE || 
-            m_splineType == PERIODIC_0_B_SPLINE || 
-            m_splineType == HUE_HUE_B_SPLINE )
+        if ( m_splineType == PERIODIC_1_B_SPLINE || 
+             m_splineType == PERIODIC_0_B_SPLINE || 
+             m_splineType == HUE_HUE_B_SPLINE )
         {
             const float del_x = m_controlPoints[1].m_x - m_controlPoints[0].m_x;
             if (std::abs(1.f - del_x) < 1e-3)

src/OpenColorIO/ops/gradingrgbcurve/GradingRGBCurve.cpp

@@ -104,6 +104,15 @@ void GradingRGBCurveImpl::validate() const
                 << "with: " << e.what();
             throw Exception(oss.str().c_str());
         }
+
+        if (m_curves[c]->getSplineType() != B_SPLINE)
+        {
+            // TODO: Allow use of the hue curve diagonal spline types?
+            std::ostringstream oss;
+            oss << "GradingRGBCurve validation failed: '" << CurveType(c) << "' curve "
+                << "is of the wrong BSplineType.";
+            throw Exception(oss.str().c_str());
+        }
     }
 }
 

tests/cpu/Config_tests.cpp

@@ -4650,7 +4650,7 @@ OCIO_ADD_TEST(Config, grading_huecurve_serialization)
             "      children:\n"
             "        - !<GradingHueCurveTransform>\n"
             "          style: log\n"
-            "          hue_hue: {control_points: [0, 0, 0.5, 0.5, 1, 1.123456]}\n"
+            "          hue_hue: {control_points: [0, 0.15, 0.5, 0.5, 1, 1.123456]}\n"
             "        - !<GradingHueCurveTransform>\n"
             "          style: log\n"
             "          hue_sat: {control_points: [0, 0, 0.5, 0.5, 1, 1.5]}\n"
@@ -4663,7 +4663,7 @@ OCIO_ADD_TEST(Config, grading_huecurve_serialization)
             "          lum_lum: {control_points: [-1, -1, 0, 0.1, 0.5, 0.6, 1, 1.1]}\n"
             "        - !<GradingHueCurveTransform>\n"
             "          style: video\n"
-            "          hue_hue: {control_points: [-0.2, 0, 0.5, 0.5, 1.2, 1.5]}\n"
+            "          hue_hue: {control_points: [0.02, -0.1, 0.5, 0.5, 0.9, 0.8]}\n"
             "          hue_lum: {control_points: [0, 0, 0.2, 0.5, 1, 1.5]}\n"
             "          lum_sat: {control_points: [0, 0, 0.1, 0.5, 1, 1.5], slopes: [0, 1, 1.1]}\n"
             "          sat_lum: {control_points: [-1, -1, 0, 0.1, 0.5, 0.6, 1, 1.1]}\n"

tests/cpu/DynamicProperty_tests.cpp

@@ -257,6 +257,55 @@ OCIO_ADD_TEST(DynamicPropertyImpl, equal_grading_rgb_curve)
     OCIO_CHECK_ASSERT(!(*dp0 == *dpImplDouble));
 }
 
+OCIO_ADD_TEST(DynamicPropertyImpl, setter_validation)
+{
+    // Make an identity dynamic transform.
+    OCIO::GradingHueCurveTransformRcPtr gct = OCIO::GradingHueCurveTransform::Create(OCIO::GRADING_LOG);
+    gct->makeDynamic();
+
+    // Apply it on CPU.
+    OCIO::ConfigRcPtr config = OCIO::Config::Create();
+    OCIO::ConstProcessorRcPtr processor = config->getProcessor(gct);
+    OCIO::ConstCPUProcessorRcPtr cpuProcessor = processor->getDefaultCPUProcessor();
+
+    float pixel[3] = { 0.4f, 0.3f, 0.2f };
+    cpuProcessor->applyRGB(pixel);
+
+    const float error = 1e-5f;
+    OCIO_CHECK_CLOSE(pixel[0], pixel[0], error);
+    OCIO_CHECK_CLOSE(pixel[1], pixel[1], error);
+    OCIO_CHECK_CLOSE(pixel[2], pixel[2], error);
+
+    // Get a handle to the dynamic property.
+    OCIO::DynamicPropertyRcPtr dp;
+    OCIO_CHECK_NO_THROW(dp = cpuProcessor->getDynamicProperty(OCIO::DYNAMIC_PROPERTY_GRADING_HUECURVE));
+    auto dpVal = OCIO::DynamicPropertyValue::AsGradingHueCurve(dp);
+    OCIO_REQUIRE_ASSERT(dpVal);
+
+    // Set a non-identity value.
+    OCIO::GradingHueCurveRcPtr hueCurve = dpVal->getValue()->createEditableCopy();
+    OCIO::GradingBSplineCurveRcPtr huehue = hueCurve->getCurve(OCIO::HUE_HUE);
+    huehue->setNumControlPoints(3);
+    huehue->getControlPoint(0) = OCIO::GradingControlPoint(0.f, -0.1f);
+    huehue->getControlPoint(1) = OCIO::GradingControlPoint(0.5f, 0.5f);
+    huehue->getControlPoint(2) = OCIO::GradingControlPoint(0.8f, 0.8f);
+    dpVal->setValue(hueCurve);
+    cpuProcessor->applyRGB(pixel);
+
+    OCIO_CHECK_CLOSE(pixel[0], 0.4385873675f, error);
+    OCIO_CHECK_CLOSE(pixel[1], 0.2829087377f, error);
+    OCIO_CHECK_CLOSE(pixel[2], 0.2556785941f, error);
+
+    // Ensure that validation of control points is happening as expected. Set the last point
+    // so that it is no longer monotonic with respect to the first point. Because it is periodic,
+    // the last point Y value becomes -0.05 when wrapped around to an X value of -0.2.
+    huehue->getControlPoint(2) = OCIO::GradingControlPoint(0.8f, 0.95f);
+    OCIO_CHECK_THROW_WHAT(dpVal->setValue(hueCurve),
+                          OCIO::Exception,
+                          "GradingHueCurve validation failed for 'hue_hue' curve with: Control point at index 0 "
+                          "has a y coordinate '-0.1' that is less than previous control point y coordinate '-0.05'.");
+}
+
 OCIO_ADD_TEST(DynamicPropertyImpl, grading_rgb_curve_knots_coefs)
 {
     auto curve11 = OCIO::GradingBSplineCurve::Create({ { 0.f, 10.f },{ 2.f, 10.f },{ 3.f, 10.f },
@@ -431,8 +480,8 @@ void checkKnotsAndCoefs(
 
 OCIO_ADD_TEST(DynamicPropertyImpl, grading_hue_curve_knots_coefs)
 {
-    auto hh = OCIO::GradingBSplineCurve::Create(
-        { {-0.1f, -0.15f}, {0.2f, 0.3f}, {0.5f, 0.25f}, {0.8f, 0.7f}, {0.85f, 0.8f}, {1.05f, 0.9f} },
+     auto hh = OCIO::GradingBSplineCurve::Create(
+        { {0.1f, 0.05f}, {0.2f, 0.3f}, {0.5f, 0.4f}, {0.8f, 0.7f}, {0.9f, 0.75f}, {1.0f, 0.9f} },
         OCIO::HUE_HUE);
     auto hs = OCIO::GradingBSplineCurve::Create(
         { {-0.15f, 1.25f}, {0.f, 0.8f}, {0.2f, 0.9f}, {0.4f, 1.8f}, {0.6f, 1.4f}, {0.8f, 1.3f}, {0.9f, 1.1f}, {1.1f, 0.7f} },
@@ -505,20 +554,20 @@ OCIO_ADD_TEST(DynamicPropertyImpl, grading_hue_curve_knots_coefs)
 
     {
         // Hue-Hue
+        const float true_knots[15] = {-0.1f, -0.06928571f, 0.0f, 0.05642857f, 0.1f, 0.17549634f, 0.2f, 0.33714286f,
+                                       0.5f,  0.62499860f, 0.8f, 0.85261905f, 0.9f, 0.93071429f, 1.f };
 
-        const float true_knots[15] = {-0.1f, -0.01816964f, 0.05f, 0.125f, 0.2f, 0.35f, 0.5f, 0.57558291f, 0.8f, 0.82731918f, 
-                                       0.85f, 0.87808036f, 0.9f, 0.98183036f, 1.05f };
         // Quadratic coefs.
-        const float true_coefsA[14] = { -2.29385141e+00f, 3.43265663e+00f, 2.95979024e+01f, -3.34850652e+01f, -2.11943922e-02f,
-                                         2.11186821e-02f, 9.58311056e+00f, 3.05897301e-01f, 1.69849435e+01f, -2.62364216e+01f,
-                                        -5.36565978e+00f, -1.21350984e+01f, -2.29385141e+00f, 3.43265663e+00f};
+        const float true_coefsA[14] = { 15.95930233f, -1.66237113f, -1.44778481f, 6.17827869f, 10.39930009f,
+                                       -58.70626575f, -1.54375789f,  1.03834397f, 3.7077401f,  -2.12344738f,
+                                        -3.54260935f,  4.81365159f, 15.95930233f,-1.66237113f };
         // Linear coefs.
-        const float true_coefsB[14] = {  5.00000000e-01f, 1.24586640e-01f, 5.92592593e-01f, 5.03227795e+00f, 9.51817043e-03f,
-                                         3.15985276e-03f, 9.49545739e-03f, 1.45813415e+00f, 1.59543132e+00f, 2.52346065e+00f,
-                                         1.33333333e+00f, 1.03199405e+00f, 5.00000000e-01f, 1.24586640e-01f };
+        const float true_coefsB[14] = { 0.75f, 1.73035714f, 1.5f, 1.33660714f, 1.875f, 3.44521825f, 0.56818182f,
+                                        0.14475108f, 0.48295455f, 1.40987919f, 0.66666667f, 0.29384921f, 0.75f, 1.73035714f };
+
         // Constant coefs.
-        const float true_coefsC[14] = { -0.15f, -0.12444493f, -0.1f, 0.11093265f, 0.3f, 0.30095085f, 0.3019f, 0.35736386f,
-                                         0.7f, 0.75626237f, 0.8f, 0.83320962f, 0.85f, 0.87555507f };
+        const float true_coefsC[14] = { -0.25f, -0.2119088f, -0.1f, -0.01996716f, 0.05f, 0.25082851f, 0.3f, 
+                                         0.34888683f, 0.4f, 0.51830078f, 0.7f, 0.72527072f, 0.75f, 0.7880912f };
 
         checkKnotsAndCoefs(dp, 0, true_knots, true_coefsA, true_coefsB, true_coefsC, __LINE__);
     }