Directly use 4x4 matrices with surface textures instead of converting to and from the 3x3 variants. (#54126)

SurfaceTextureGetTransformMatrix returns a col-major 4x4 matrix. We used to convert it to a 3x3 matrix. Then when applying the transformation in the shader, I'd convert it back to a 4x4 matrix.

Instead of all this (potentially lossy) flip-flopping, store the matrix just as we get it in 4x4 form and perform the conversion just once if necessary. Today, it is necessary only in the Skia backend. SkM44 already has a converter to convert to and from a 3x3 SkMatrix. So, use that instead of rolling our own.

I spent a lot of time debugging these conversions and transformations because we had rolled our own and the printers seemed to dump in row-major order irrespective of storage order. This caused a lot of confusion.

No change in functionality. Hopefully, the next person debugging transformations has an easier go at this.

Author: chinmaygarde

shell/platform/android/android_shell_holder_unittests.cc

@@ -48,9 +48,9 @@ class MockPlatformViewAndroidJNI : public PlatformViewAndroidJNI {
               SurfaceTextureUpdateTexImage,
               (JavaLocalRef surface_texture),
               (override));
-  MOCK_METHOD(void,
+  MOCK_METHOD(SkM44,
               SurfaceTextureGetTransformMatrix,
-              (JavaLocalRef surface_texture, SkMatrix& transform),
+              (JavaLocalRef surface_texture),
               (override));
   MOCK_METHOD(void,
               SurfaceTextureDetachFromGLContext,

shell/platform/android/jni/jni_mock.h

@@ -59,9 +59,9 @@ class JNIMock final : public PlatformViewAndroidJNI {
               (JavaLocalRef surface_texture),
               (override));
 
-  MOCK_METHOD(void,
+  MOCK_METHOD(SkM44,
               SurfaceTextureGetTransformMatrix,
-              (JavaLocalRef surface_texture, SkMatrix& transform),
+              (JavaLocalRef surface_texture),
               (override));
 
   MOCK_METHOD(JavaLocalRef,

shell/platform/android/jni/platform_view_android_jni.h

@@ -107,8 +107,8 @@ class PlatformViewAndroidJNI {
   ///             Then, it updates the `transform` matrix, so it fill the canvas
   ///             and preserve the aspect ratio.
   ///
-  virtual void SurfaceTextureGetTransformMatrix(JavaLocalRef surface_texture,
-                                                SkMatrix& transform) = 0;
+  virtual SkM44 SurfaceTextureGetTransformMatrix(
+      JavaLocalRef surface_texture) = 0;
 
   //----------------------------------------------------------------------------
   /// @brief      Detaches a SurfaceTexture from the OpenGL ES context.

shell/platform/android/platform_view_android_jni_impl.cc

@@ -1502,20 +1502,19 @@ void PlatformViewAndroidJNIImpl::SurfaceTextureUpdateTexImage(
   FML_CHECK(fml::jni::CheckException(env));
 }
 
-void PlatformViewAndroidJNIImpl::SurfaceTextureGetTransformMatrix(
-    JavaLocalRef surface_texture,
-    SkMatrix& transform) {
+SkM44 PlatformViewAndroidJNIImpl::SurfaceTextureGetTransformMatrix(
+    JavaLocalRef surface_texture) {
   JNIEnv* env = fml::jni::AttachCurrentThread();
 
   if (surface_texture.is_null()) {
-    return;
+    return {};
   }
 
   fml::jni::ScopedJavaLocalRef<jobject> surface_texture_local_ref(
       env, env->CallObjectMethod(surface_texture.obj(),
                                  g_java_weak_reference_get_method));
   if (surface_texture_local_ref.is_null()) {
-    return;
+    return {};
   }
 
   fml::jni::ScopedJavaLocalRef<jfloatArray> transformMatrix(
@@ -1527,36 +1526,12 @@ void PlatformViewAndroidJNIImpl::SurfaceTextureGetTransformMatrix(
 
   float* m = env->GetFloatArrayElements(transformMatrix.obj(), nullptr);
 
-  // SurfaceTexture 4x4 Column Major -> Skia 3x3 Row Major
-
-  // SurfaceTexture 4x4 (Column Major):
-  // | m[0] m[4] m[ 8] m[12] |
-  // | m[1] m[5] m[ 9] m[13] |
-  // | m[2] m[6] m[10] m[14] |
-  // | m[3] m[7] m[11] m[15] |
-
-  // According to Android documentation, the 4x4 matrix returned should be used
-  // with texture coordinates in the form (s, t, 0, 1). Since the z component is
-  // always 0.0, we are free to ignore any element that multiplies with the z
-  // component. Converting this to a 3x3 matrix is easy:
-
-  // SurfaceTexture 3x3 (Column Major):
-  // | m[0] m[4] m[12] |
-  // | m[1] m[5] m[13] |
-  // | m[3] m[7] m[15] |
-
-  // Skia (Row Major):
-  // | m[0] m[1] m[2] |
-  // | m[3] m[4] m[5] |
-  // | m[6] m[7] m[8] |
-
-  SkScalar matrix3[] = {
-      m[0], m[4], m[12],  //
-      m[1], m[5], m[13],  //
-      m[3], m[7], m[15],  //
-  };
+  static_assert(sizeof(SkScalar) == sizeof(float));
+  const auto transform = SkM44::ColMajor(m);
+
   env->ReleaseFloatArrayElements(transformMatrix.obj(), m, JNI_ABORT);
-  transform.set9(matrix3);
+
+  return transform;
 }
 
 void PlatformViewAndroidJNIImpl::SurfaceTextureDetachFromGLContext(

shell/platform/android/platform_view_android_jni_impl.h

@@ -49,8 +49,7 @@ class PlatformViewAndroidJNIImpl final : public PlatformViewAndroidJNI {
 
   void SurfaceTextureUpdateTexImage(JavaLocalRef surface_texture) override;
 
-  void SurfaceTextureGetTransformMatrix(JavaLocalRef surface_texture,
-                                        SkMatrix& transform) override;
+  SkM44 SurfaceTextureGetTransformMatrix(JavaLocalRef surface_texture) override;
 
   void SurfaceTextureDetachFromGLContext(JavaLocalRef surface_texture) override;
 

shell/platform/android/surface_texture_external_texture.cc

@@ -67,7 +67,7 @@ void SurfaceTextureExternalTexture::DrawFrame(
     PaintContext& context,
     const SkRect& bounds,
     const DlImageSampling sampling) const {
-  auto transform = GetCurrentUVTransformation();
+  auto transform = GetCurrentUVTransformation().asM33();
 
   // Android's SurfaceTexture transform matrix works on texture coordinate
   // lookups in the range 0.0-1.0, while Skia's Shader transform matrix works on
@@ -136,12 +136,11 @@ bool SurfaceTextureExternalTexture::ShouldUpdate() {
 void SurfaceTextureExternalTexture::Update() {
   jni_facade_->SurfaceTextureUpdateTexImage(
       fml::jni::ScopedJavaLocalRef<jobject>(surface_texture_));
-  jni_facade_->SurfaceTextureGetTransformMatrix(
-      fml::jni::ScopedJavaLocalRef<jobject>(surface_texture_), transform_);
+  transform_ = jni_facade_->SurfaceTextureGetTransformMatrix(
+      fml::jni::ScopedJavaLocalRef<jobject>(surface_texture_));
 }
 
-const SkMatrix& SurfaceTextureExternalTexture::GetCurrentUVTransformation()
-    const {
+const SkM44& SurfaceTextureExternalTexture::GetCurrentUVTransformation() const {
   return transform_;
 }
 

shell/platform/android/surface_texture_external_texture.h

@@ -9,6 +9,7 @@
 
 #include "flutter/common/graphics/texture.h"
 #include "flutter/shell/platform/android/platform_view_android_jni_impl.h"
+#include "flutter/third_party/skia/include/core/SkM44.h"
 
 namespace flutter {
 
@@ -64,7 +65,7 @@ class SurfaceTextureExternalTexture : public flutter::Texture {
   ///
   /// @return     The current uv transformation.
   ///
-  const SkMatrix& GetCurrentUVTransformation() const;
+  const SkM44& GetCurrentUVTransformation() const;
 
   //----------------------------------------------------------------------------
   /// @brief      Provides an opportunity for the subclasses to sever the
@@ -111,7 +112,7 @@ class SurfaceTextureExternalTexture : public flutter::Texture {
   sk_sp<flutter::DlImage> dl_image_;
 
  private:
-  SkMatrix transform_;
+  SkM44 transform_;
 
   // |Texture|
   void Paint(PaintContext& context,

shell/platform/android/surface_texture_external_texture_vk_impeller.cc

@@ -148,9 +148,9 @@ void SurfaceTextureExternalTextureVKImpeller::ProcessFrame(
                    vk::ImageLayout::eColorAttachmentOptimal,
                    LayoutUpdateMode::kSync);
 
-  SkM44 transformation(GetCurrentUVTransformation());
   impeller::Matrix uv_transformation;
-  transformation.getColMajor(reinterpret_cast<SkScalar*>(&uv_transformation));
+  GetCurrentUVTransformation().getColMajor(
+      reinterpret_cast<SkScalar*>(&uv_transformation));
 
   glvk::Trampoline::GLTextureInfo src_texture;
   src_texture.texture = src_gl_texture;