From ff0997d0b24473740ca082d3364b98ef5cd9f51a Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Thu, 25 Jul 2024 19:20:04 -0700 Subject: [PATCH] Directly use 4x4 matrices with surface textures instead of converting to and from the 3x3 variants. (flutter/engine#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. --- .../android/android_shell_holder_unittests.cc | 4 +- .../shell/platform/android/jni/jni_mock.h | 4 +- .../android/jni/platform_view_android_jni.h | 4 +- .../android/platform_view_android_jni_impl.cc | 41 ++++--------------- .../android/platform_view_android_jni_impl.h | 3 +- .../surface_texture_external_texture.cc | 9 ++-- .../surface_texture_external_texture.h | 5 ++- ...ce_texture_external_texture_vk_impeller.cc | 4 +- 8 files changed, 24 insertions(+), 50 deletions(-) diff --git a/engine/src/flutter/shell/platform/android/android_shell_holder_unittests.cc b/engine/src/flutter/shell/platform/android/android_shell_holder_unittests.cc index 5b1d3f0abe..0a1f1ed3fc 100644 --- a/engine/src/flutter/shell/platform/android/android_shell_holder_unittests.cc +++ b/engine/src/flutter/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, diff --git a/engine/src/flutter/shell/platform/android/jni/jni_mock.h b/engine/src/flutter/shell/platform/android/jni/jni_mock.h index dd6c9737ea..cf5b329256 100644 --- a/engine/src/flutter/shell/platform/android/jni/jni_mock.h +++ b/engine/src/flutter/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, diff --git a/engine/src/flutter/shell/platform/android/jni/platform_view_android_jni.h b/engine/src/flutter/shell/platform/android/jni/platform_view_android_jni.h index dcce4fd569..356b6776fc 100644 --- a/engine/src/flutter/shell/platform/android/jni/platform_view_android_jni.h +++ b/engine/src/flutter/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. diff --git a/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc b/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc index 978a56e24e..8a3f0d7bd5 100644 --- a/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc +++ b/engine/src/flutter/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 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 transformMatrix( @@ -1527,36 +1526,12 @@ void PlatformViewAndroidJNIImpl::SurfaceTextureGetTransformMatrix( float* m = env->GetFloatArrayElements(transformMatrix.obj(), nullptr); - // SurfaceTexture 4x4 Column Major -> Skia 3x3 Row Major + static_assert(sizeof(SkScalar) == sizeof(float)); + const auto transform = SkM44::ColMajor(m); - // 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], // - }; env->ReleaseFloatArrayElements(transformMatrix.obj(), m, JNI_ABORT); - transform.set9(matrix3); + + return transform; } void PlatformViewAndroidJNIImpl::SurfaceTextureDetachFromGLContext( diff --git a/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.h b/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.h index 5cf432559f..73cb6a43cf 100644 --- a/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.h +++ b/engine/src/flutter/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; diff --git a/engine/src/flutter/shell/platform/android/surface_texture_external_texture.cc b/engine/src/flutter/shell/platform/android/surface_texture_external_texture.cc index 64fea9837b..5a14e01ad9 100644 --- a/engine/src/flutter/shell/platform/android/surface_texture_external_texture.cc +++ b/engine/src/flutter/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(surface_texture_)); - jni_facade_->SurfaceTextureGetTransformMatrix( - fml::jni::ScopedJavaLocalRef(surface_texture_), transform_); + transform_ = jni_facade_->SurfaceTextureGetTransformMatrix( + fml::jni::ScopedJavaLocalRef(surface_texture_)); } -const SkMatrix& SurfaceTextureExternalTexture::GetCurrentUVTransformation() - const { +const SkM44& SurfaceTextureExternalTexture::GetCurrentUVTransformation() const { return transform_; } diff --git a/engine/src/flutter/shell/platform/android/surface_texture_external_texture.h b/engine/src/flutter/shell/platform/android/surface_texture_external_texture.h index e4dec1062a..92c3fc7919 100644 --- a/engine/src/flutter/shell/platform/android/surface_texture_external_texture.h +++ b/engine/src/flutter/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 dl_image_; private: - SkMatrix transform_; + SkM44 transform_; // |Texture| void Paint(PaintContext& context, diff --git a/engine/src/flutter/shell/platform/android/surface_texture_external_texture_vk_impeller.cc b/engine/src/flutter/shell/platform/android/surface_texture_external_texture_vk_impeller.cc index bd2bc7b07c..679d289e63 100644 --- a/engine/src/flutter/shell/platform/android/surface_texture_external_texture_vk_impeller.cc +++ b/engine/src/flutter/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(&uv_transformation)); + GetCurrentUVTransformation().getColMajor( + reinterpret_cast(&uv_transformation)); glvk::Trampoline::GLTextureInfo src_texture; src_texture.texture = src_gl_texture;