From 7ad1cf3453bfad7c7a48e3d2d6a3e7b797cc3c0b Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Thu, 25 Jul 2024 14:37:22 -0700 Subject: [PATCH 1/2] Directly use 44 matrices with surface textures instead of converting to and from the 33 variants. 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 this conversions and transformation because we had rolled our own and the printers seemed to dump in col-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 +- .../android/jni/platform_view_android_jni.h | 4 +- .../android/platform_view_android_jni_impl.cc | 43 ++++--------------- .../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 +- 7 files changed, 23 insertions(+), 49 deletions(-) diff --git a/shell/platform/android/android_shell_holder_unittests.cc b/shell/platform/android/android_shell_holder_unittests.cc index 5b1d3f0abe55d..0a1f1ed3fcf5d 100644 --- a/shell/platform/android/android_shell_holder_unittests.cc +++ b/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/shell/platform/android/jni/platform_view_android_jni.h b/shell/platform/android/jni/platform_view_android_jni.h index dcce4fd569ecc..356b6776fc631 100644 --- a/shell/platform/android/jni/platform_view_android_jni.h +++ b/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/shell/platform/android/platform_view_android_jni_impl.cc b/shell/platform/android/platform_view_android_jni_impl.cc index 978a56e24ebcb..8a3f0d7bd5989 100644 --- a/shell/platform/android/platform_view_android_jni_impl.cc +++ b/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 - - // 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( diff --git a/shell/platform/android/platform_view_android_jni_impl.h b/shell/platform/android/platform_view_android_jni_impl.h index 5cf432559f980..73cb6a43cffdb 100644 --- a/shell/platform/android/platform_view_android_jni_impl.h +++ b/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/shell/platform/android/surface_texture_external_texture.cc b/shell/platform/android/surface_texture_external_texture.cc index 64fea9837bb6b..5a14e01ad9731 100644 --- a/shell/platform/android/surface_texture_external_texture.cc +++ b/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/shell/platform/android/surface_texture_external_texture.h b/shell/platform/android/surface_texture_external_texture.h index e4dec1062ac7b..92c3fc7919a8b 100644 --- a/shell/platform/android/surface_texture_external_texture.h +++ b/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/shell/platform/android/surface_texture_external_texture_vk_impeller.cc b/shell/platform/android/surface_texture_external_texture_vk_impeller.cc index bd2bc7b07cdc8..679d289e63b3a 100644 --- a/shell/platform/android/surface_texture_external_texture_vk_impeller.cc +++ b/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; From c59695ffd817a8e30573ff6fa3b224910166ec51 Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Thu, 25 Jul 2024 14:55:39 -0700 Subject: [PATCH 2/2] Fixes --- shell/platform/android/jni/jni_mock.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/shell/platform/android/jni/jni_mock.h b/shell/platform/android/jni/jni_mock.h index dd6c9737eae33..cf5b329256e6c 100644 --- a/shell/platform/android/jni/jni_mock.h +++ b/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,