SkMatrix44 clarifications and clean-ups

Fixed row/col major bug and added comments to the
header.

Made SkMatrix::I() thread safe using constexpr.

Added a test set3x3RowMajorf().

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2098583002

Review-Url: https://codereview.chromium.org/2098583002

diff --git a/include/core/SkMatrix44.h b/include/core/SkMatrix44.h
index 715ee78..6b5e65d 100644 (file)
--- a/include/core/SkMatrix44.h
+++ b/include/core/SkMatrix44.h
@@ -136,8 +136,15 @@ public:
         kIdentity_Constructor
     };
 
-    SkMatrix44(Uninitialized_Constructor) { }
-    SkMatrix44(Identity_Constructor) { this->setIdentity(); }
+    SkMatrix44(Uninitialized_Constructor) {}
+
+    constexpr SkMatrix44(Identity_Constructor)
+        : fMat{{ 1, 0, 0, 0, },
+               { 0, 1, 0, 0, },
+               { 0, 0, 1, 0, },
+               { 0, 0, 0, 1, }}
+        , fTypeMask(kIdentity_Mask)
+    {}
 
     SK_ATTR_DEPRECATED("use the constructors that take an enum")
     SkMatrix44() { this->setIdentity(); }
@@ -281,6 +288,10 @@ public:
      *  array. The given array must have room for exactly 16 entries. Whenever
      *  possible, they will try to use memcpy rather than an entry-by-entry
      *  copy.
+     *
+     *  Col major indicates that consecutive elements of columns will be stored
+     *  contiguously in memory.  Row major indicates that consecutive elements
+     *  of rows will be stored contiguously in memory.
      */
     void asColMajorf(float[]) const;
     void asColMajord(double[]) const;
@@ -291,6 +302,11 @@ public:
      *  array. The given array must have room for exactly 16 entries. Whenever
      *  possible, they will try to use memcpy rather than an entry-by-entry
      *  copy.
+     *
+     *  Col major indicates that input memory will be treated as if consecutive
+     *  elements of columns are stored contiguously in memory.  Row major
+     *  indicates that input memory will be treated as if consecutive elements
+     *  of rows are stored contiguously in memory.
      */
     void setColMajorf(const float[]);
     void setColMajord(const double[]);
@@ -306,11 +322,12 @@ public:
 #endif
 
     /* This sets the top-left of the matrix and clears the translation and
-     * perspective components (with [3][3] set to 1). */
+     * perspective components (with [3][3] set to 1).  mXY is interpreted
+     * as the matrix entry at col = X, row = Y. */
     void set3x3(SkMScalar m00, SkMScalar m01, SkMScalar m02,
                 SkMScalar m10, SkMScalar m11, SkMScalar m12,
                 SkMScalar m20, SkMScalar m21, SkMScalar m22);
-    void set3x3ColMajorf(const float[]);
+    void set3x3RowMajorf(const float[]);
 
     void setTranslate(SkMScalar dx, SkMScalar dy, SkMScalar dz);
     void preTranslate(SkMScalar dx, SkMScalar dy, SkMScalar dz);
@@ -430,6 +447,7 @@ public:
     double determinant() const;
 
 private:
+    /* This is indexed by [col][row]. */
     SkMScalar           fMat[4][4];
     mutable unsigned    fTypeMask;
 
@@ -439,13 +457,7 @@ private:
         kAllPublic_Masks = 0xF
     };
 
-    /** Efficiently reads 12 matrix entries, ignoring the last col.
-     *  This is typically useful when we know the last col is (0, 0, 0, 1).
-     */
     void as4x3ColMajorf(float[]) const;
-
-    /* This sets the top-left of the matrix and clears the
-     * perspective components (with [3][3] set to 1). */
     void set4x3ColMajorf(const float[]);
 
     SkMScalar transX() const { return fMat[3][0]; }

diff --git a/src/codec/SkPngCodec.cpp b/src/codec/SkPngCodec.cpp
index 985d61a..32308b2 100644 (file)
--- a/src/codec/SkPngCodec.cpp
+++ b/src/codec/SkPngCodec.cpp
@@ -230,7 +230,7 @@ sk_sp<SkColorSpace> read_color_space(png_structp png_ptr, png_infop info_ptr) {
             toXYZD50[i] = png_fixed_point_to_float(XYZ[i]);
         }
         SkMatrix44 mat(SkMatrix44::kUninitialized_Constructor);
-        mat.set3x3ColMajorf(toXYZD50);
+        mat.set3x3RowMajorf(toXYZD50);
 
         if (PNG_INFO_gAMA == png_get_gAMA_fixed(png_ptr, info_ptr, &gamma)) {
             float value = png_inverted_fixed_point_to_float(gamma);
@@ -257,7 +257,7 @@ sk_sp<SkColorSpace> read_color_space(png_structp png_ptr, png_infop info_ptr) {
 
         // Since there is no cHRM, we will guess sRGB gamut.
         SkMatrix44 mat(SkMatrix44::kUninitialized_Constructor);
-        mat.set3x3ColMajorf(gSRGB_toXYZD50);
+        mat.set3x3RowMajorf(gSRGB_toXYZD50);
 
         return SkColorSpace_Base::NewRGB(gammas, mat);
     }

diff --git a/src/core/SkColorSpace.cpp b/src/core/SkColorSpace.cpp
index 95c38f2..3f93f16 100644 (file)
--- a/src/core/SkColorSpace.cpp
+++ b/src/core/SkColorSpace.cpp
@@ -142,7 +142,7 @@ sk_sp<SkColorSpace> SkColorSpace::NewNamed(Named named) {
         case kSRGB_Named: {
             sRGBOnce([] {
                 SkMatrix44 srgbToxyzD50(SkMatrix44::kUninitialized_Constructor);
-                srgbToxyzD50.set3x3ColMajorf(gSRGB_toXYZD50);
+                srgbToxyzD50.set3x3RowMajorf(gSRGB_toXYZD50);
                 sRGB = new SkColorSpace_Base(kSRGB_GammaNamed, srgbToxyzD50, kSRGB_Named);
             });
             return sk_ref_sp(sRGB);
@@ -150,7 +150,7 @@ sk_sp<SkColorSpace> SkColorSpace::NewNamed(Named named) {
         case kAdobeRGB_Named: {
             adobeRGBOnce([] {
                 SkMatrix44 adobergbToxyzD50(SkMatrix44::kUninitialized_Constructor);
-                adobergbToxyzD50.set3x3ColMajorf(gAdobeRGB_toXYZD50);
+                adobergbToxyzD50.set3x3RowMajorf(gAdobeRGB_toXYZD50);
                 adobeRGB = new SkColorSpace_Base(k2Dot2Curve_GammaNamed, adobergbToxyzD50,
                                                  kAdobeRGB_Named);
             });
@@ -897,7 +897,7 @@ sk_sp<SkColorSpace> SkColorSpace::NewICC(const void* input, size_t len) {
                     return_null("Need valid rgb tags for XYZ space");
                 }
                 SkMatrix44 mat(SkMatrix44::kUninitialized_Constructor);
-                mat.set3x3ColMajorf(toXYZ);
+                mat.set3x3RowMajorf(toXYZ);
 
                 // It is not uncommon to see missing or empty gamma tags.  This indicates
                 // that we should use unit gamma.

diff --git a/src/core/SkMatrix44.cpp b/src/core/SkMatrix44.cpp
index 56c2e8a..83c1adc 100644 (file)
--- a/src/core/SkMatrix44.cpp
+++ b/src/core/SkMatrix44.cpp
@@ -186,7 +186,7 @@ void SkMatrix44::setRowMajord(const double src[]) {
 ///////////////////////////////////////////////////////////////////////////////
 
 const SkMatrix44& SkMatrix44::I() {
-    static const SkMatrix44 gIdentity44(kIdentity_Constructor);
+    static constexpr SkMatrix44 gIdentity44(kIdentity_Constructor);
     return gIdentity44;
 }
 
@@ -220,7 +220,7 @@ void SkMatrix44::set3x3(SkMScalar m00, SkMScalar m01, SkMScalar m02,
     this->dirtyTypeMask();
 }
 
-void SkMatrix44::set3x3ColMajorf(const float src[]) {
+void SkMatrix44::set3x3RowMajorf(const float src[]) {
     fMat[0][0] = src[0]; fMat[0][1] = src[3]; fMat[0][2] = src[6]; fMat[0][3] = 0;
     fMat[1][0] = src[1]; fMat[1][1] = src[4]; fMat[1][2] = src[7]; fMat[1][3] = 0;
     fMat[2][0] = src[2]; fMat[2][1] = src[5]; fMat[2][2] = src[8]; fMat[2][3] = 0;

diff --git a/tests/ColorSpaceTest.cpp b/tests/ColorSpaceTest.cpp
index 853bdce..442ed3d 100644 (file)
--- a/tests/ColorSpaceTest.cpp
+++ b/tests/ColorSpaceTest.cpp
@@ -101,7 +101,7 @@ DEF_TEST(ColorSpaceSRGBCompare, r) {
 
     // Create an sRGB color space by value
     SkMatrix44 srgbToxyzD50(SkMatrix44::kUninitialized_Constructor);
-    srgbToxyzD50.set3x3ColMajorf(g_sRGB_XYZ);
+    srgbToxyzD50.set3x3RowMajorf(g_sRGB_XYZ);
     sk_sp<SkColorSpace> rgbColorSpace = SkColorSpace::NewRGB(SkColorSpace::kSRGB_GammaNamed,
                                                              srgbToxyzD50);
     REPORTER_ASSERT(r, rgbColorSpace == namedColorSpace);

diff --git a/tests/Matrix44Test.cpp b/tests/Matrix44Test.cpp
index f4b6783..75086b6 100644 (file)
--- a/tests/Matrix44Test.cpp
+++ b/tests/Matrix44Test.cpp
@@ -484,6 +484,23 @@ static void test_get_set_double(skiatest::Reporter* reporter) {
     }
 }
 
+static void test_set_3x3(skiatest::Reporter* r) {
+    static float vals[9] = { 1.0f, 2.0f, 3.0f, 4.0f, 5.0f, 6.0f, 7.0f, 8.0f, 9.0f, };
+
+    SkMatrix44 mat(SkMatrix44::kUninitialized_Constructor);
+    mat.set3x3RowMajorf(vals);
+
+    REPORTER_ASSERT(r, 1.0f == mat.getFloat(0, 0));
+    REPORTER_ASSERT(r, 2.0f == mat.getFloat(0, 1));
+    REPORTER_ASSERT(r, 3.0f == mat.getFloat(0, 2));
+    REPORTER_ASSERT(r, 4.0f == mat.getFloat(1, 0));
+    REPORTER_ASSERT(r, 5.0f == mat.getFloat(1, 1));
+    REPORTER_ASSERT(r, 6.0f == mat.getFloat(1, 2));
+    REPORTER_ASSERT(r, 7.0f == mat.getFloat(2, 0));
+    REPORTER_ASSERT(r, 8.0f == mat.getFloat(2, 1));
+    REPORTER_ASSERT(r, 9.0f == mat.getFloat(2, 2));
+}
+
 static void test_set_row_col_major(skiatest::Reporter* reporter) {
     SkMatrix44 a(SkMatrix44::kUninitialized_Constructor);
     SkMatrix44 b(SkMatrix44::kUninitialized_Constructor);
@@ -916,6 +933,7 @@ DEF_TEST(Matrix44, reporter) {
     test_transpose(reporter);
     test_get_set_double(reporter);
     test_set_row_col_major(reporter);
+    test_set_3x3(reporter);
     test_translate(reporter);
     test_scale(reporter);
     test_map2(reporter);