From c1a3e24918f99fc0b975111afb39dca38c50eb5c Mon Sep 17 00:00:00 2001 From: msarett Date: Thu, 23 Jun 2016 12:42:29 -0700 Subject: [PATCH] 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 --- include/core/SkMatrix44.h | 32 ++++++++++++++++++++++---------- src/codec/SkPngCodec.cpp | 4 ++-- src/core/SkColorSpace.cpp | 6 +++--- src/core/SkMatrix44.cpp | 4 ++-- tests/ColorSpaceTest.cpp | 2 +- tests/Matrix44Test.cpp | 18 ++++++++++++++++++ 6 files changed, 48 insertions(+), 18 deletions(-) diff --git a/include/core/SkMatrix44.h b/include/core/SkMatrix44.h index 715ee78..6b5e65d 100644 --- 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 --- a/src/codec/SkPngCodec.cpp +++ b/src/codec/SkPngCodec.cpp @@ -230,7 +230,7 @@ sk_sp 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 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 --- a/src/core/SkColorSpace.cpp +++ b/src/core/SkColorSpace.cpp @@ -142,7 +142,7 @@ sk_sp 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::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::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 --- 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 --- 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 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 --- 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); -- 2.7.4