Use full 4x4 matrix transforms in TransformLayer (#43536)

Fixes: flutter/flutter#82961
Fixes: flutter/flutter#113346

The fix was a simple fallout from the previous work to add support for SkM44 throughout the DL and Diff mechanisms (see flutter/flutter#82955, flutter/flutter#116198, #37394)

Tested with its own existing and new unit tests as well as the test case from flutter/flutter#113346

Author: flar

display_list/utils/dl_matrix_clip_tracker.cc

@@ -92,7 +92,7 @@ class Data3x3 : public DisplayListMatrixClipTracker::Data {
   SkMatrix matrix_;
 };
 
-static bool is_3x3(const SkM44& m) {
+bool DisplayListMatrixClipTracker::is_3x3(const SkM44& m) {
   // clang-format off
   return (                                      m.rc(0, 2) == 0 &&
                                                 m.rc(1, 2) == 0 &&

display_list/utils/dl_matrix_clip_tracker.h

@@ -27,6 +27,8 @@ class DisplayListMatrixClipTracker {
   DisplayListMatrixClipTracker(const SkRect& cull_rect, const SkMatrix& matrix);
   DisplayListMatrixClipTracker(const SkRect& cull_rect, const SkM44& matrix);
 
+  static bool is_3x3(const SkM44& m44);
+
   SkRect base_device_cull_rect() const { return saved_[0]->device_cull_rect(); }
 
   bool using_4x4_matrix() const { return current_->is_4x4(); }

flow/diff_context.cc

@@ -57,6 +57,10 @@ void DiffContext::PushTransform(const SkMatrix& transform) {
   clip_tracker_.transform(transform);
 }
 
+void DiffContext::PushTransform(const SkM44& transform) {
+  clip_tracker_.transform(transform);
+}
+
 void DiffContext::MakeCurrentTransformIntegral() {
   // TODO(knopp): This is duplicated from LayerStack. Maybe should be part of
   // clip tracker?

flow/diff_context.h

@@ -12,6 +12,7 @@
 #include "display_list/utils/dl_matrix_clip_tracker.h"
 #include "flutter/flow/paint_region.h"
 #include "flutter/fml/macros.h"
+#include "third_party/skia/include/core/SkM44.h"
 #include "third_party/skia/include/core/SkMatrix.h"
 #include "third_party/skia/include/core/SkRect.h"
 
@@ -70,6 +71,7 @@ class DiffContext {
 
   // Pushes additional transform for current subtree
   void PushTransform(const SkMatrix& transform);
+  void PushTransform(const SkM44& transform);
 
   // Pushes cull rect for current subtree
   bool PushCullRect(const SkRect& clip);

flow/layers/layer_state_stack.cc

@@ -577,9 +577,13 @@ void MutatorContext::transform(const SkMatrix& matrix) {
 }
 
 void MutatorContext::transform(const SkM44& m44) {
-  layer_state_stack_->maybe_save_layer_for_transform(save_needed_);
-  save_needed_ = false;
-  layer_state_stack_->push_transform(m44);
+  if (DisplayListMatrixClipTracker::is_3x3(m44)) {
+    transform(m44.asM33());
+  } else {
+    layer_state_stack_->maybe_save_layer_for_transform(save_needed_);
+    save_needed_ = false;
+    layer_state_stack_->push_transform(m44);
+  }
 }
 
 void MutatorContext::integralTransform() {

flow/layers/transform_layer.cc

@@ -8,9 +8,8 @@
 
 namespace flutter {
 
-TransformLayer::TransformLayer(const SkMatrix& transform)
-    : transform_(transform) {
-  // Checks (in some degree) that SkMatrix transform_ is valid and initialized.
+TransformLayer::TransformLayer(const SkM44& transform) : transform_(transform) {
+  // Checks (in some degree) that SkM44 transform_ is valid and initialized.
   //
   // If transform_ is uninitialized, this assert may look flaky as it doesn't
   // fail all the time, and some rerun may make it pass. But don't ignore it and
@@ -47,7 +46,7 @@ void TransformLayer::Preroll(PrerollContext* context) {
   SkRect child_paint_bounds = SkRect::MakeEmpty();
   PrerollChildren(context, &child_paint_bounds);
 
-  transform_.mapRect(&child_paint_bounds);
+  transform_.asM33().mapRect(&child_paint_bounds);
   set_paint_bounds(child_paint_bounds);
 }
 

flow/layers/transform_layer.h

@@ -13,7 +13,9 @@ namespace flutter {
 // at all. Hence |set_transform| must be called with an initialized SkMatrix.
 class TransformLayer : public ContainerLayer {
  public:
-  explicit TransformLayer(const SkMatrix& transform);
+  explicit TransformLayer(const SkMatrix& transform)
+      : TransformLayer(SkM44(transform)) {}
+  explicit TransformLayer(const SkM44& transform);
 
   void Diff(DiffContext* context, const Layer* old_layer) override;
 
@@ -22,7 +24,7 @@ class TransformLayer : public ContainerLayer {
   void Paint(PaintContext& context) const override;
 
  private:
-  SkMatrix transform_;
+  SkM44 transform_;
 
   FML_DISALLOW_COPY_AND_ASSIGN(TransformLayer);
 };

flow/layers/transform_layer_unittests.cc

@@ -56,6 +56,7 @@ TEST_F(TransformLayerTest, Identity) {
   EXPECT_TRUE(layer->needs_painting(paint_context()));
   EXPECT_EQ(mock_layer->parent_matrix(), SkMatrix());  // identity
   EXPECT_EQ(mock_layer->parent_cull_rect(), cull_rect);
+  EXPECT_EQ(mock_layer->parent_mutators().stack_count(), 0u);
   EXPECT_EQ(mock_layer->parent_mutators(), MutatorsStack());
 
   layer->Paint(display_list_paint_context());
@@ -118,6 +119,102 @@ TEST_F(TransformLayerTest, Simple) {
   EXPECT_TRUE(DisplayListsEQ_Verbose(display_list(), expected_builder.Build()));
 }
 
+TEST_F(TransformLayerTest, SimpleM44) {
+  SkPath child_path;
+  child_path.addRect(5.0f, 6.0f, 20.5f, 21.5f);
+  SkMatrix initial_transform = SkMatrix::Translate(-0.5f, -0.5f);
+  SkRect local_cull_rect = SkRect::MakeXYWH(2.0f, 2.0f, 14.0f, 14.0f);
+  SkRect device_cull_rect = initial_transform.mapRect(local_cull_rect);
+  SkMatrix layer_transform = SkMatrix::Translate(2.5f, 3.5f);
+  SkMatrix inverse_layer_transform;
+  EXPECT_TRUE(layer_transform.invert(&inverse_layer_transform));
+
+  auto mock_layer = std::make_shared<MockLayer>(child_path, DlPaint());
+  auto layer = std::make_shared<TransformLayer>(SkM44::Translate(2.5f, 3.5f));
+  layer->Add(mock_layer);
+
+  preroll_context()->state_stack.set_preroll_delegate(device_cull_rect,
+                                                      initial_transform);
+  layer->Preroll(preroll_context());
+  EXPECT_EQ(mock_layer->paint_bounds(), child_path.getBounds());
+  EXPECT_EQ(layer->paint_bounds(),
+            layer_transform.mapRect(mock_layer->paint_bounds()));
+  EXPECT_EQ(layer->child_paint_bounds(), mock_layer->paint_bounds());
+  EXPECT_TRUE(mock_layer->needs_painting(paint_context()));
+  EXPECT_TRUE(layer->needs_painting(paint_context()));
+  EXPECT_EQ(mock_layer->parent_matrix(),
+            SkMatrix::Concat(initial_transform, layer_transform));
+  EXPECT_EQ(mock_layer->parent_cull_rect(),
+            inverse_layer_transform.mapRect(local_cull_rect));
+  EXPECT_EQ(mock_layer->parent_mutators(),
+            std::vector({Mutator(layer_transform)}));
+
+  layer->Paint(display_list_paint_context());
+  DisplayListBuilder expected_builder;
+  /* (Transform)layer::Paint */ {
+    expected_builder.Save();
+    {
+      expected_builder.Transform(layer_transform);
+      /* mock_layer::Paint */ {
+        expected_builder.DrawPath(child_path, DlPaint());
+      }
+    }
+    expected_builder.Restore();
+  }
+  EXPECT_TRUE(DisplayListsEQ_Verbose(display_list(), expected_builder.Build()));
+}
+
+TEST_F(TransformLayerTest, ComplexM44) {
+  SkPath child_path;
+  child_path.addRect(5.0f, 6.0f, 20.5f, 21.5f);
+  SkMatrix initial_transform = SkMatrix::Translate(-0.5f, -0.5f);
+  SkRect local_cull_rect = SkRect::MakeXYWH(2.0f, 2.0f, 14.0f, 14.0f);
+  SkRect device_cull_rect = initial_transform.mapRect(local_cull_rect);
+  SkM44 layer_transform44 = SkM44();
+  layer_transform44.preTranslate(2.5f, 2.5f);
+  // 20 degrees around the X axis
+  layer_transform44.preConcat(SkM44::Rotate({1.0f, 0.0f, 0.0f}, M_PI / 9.0f));
+  // 10 degrees around the Y axis
+  layer_transform44.preConcat(SkM44::Rotate({0.0f, 1.0f, 0.0f}, M_PI / 18.0f));
+  SkMatrix layer_transform = layer_transform44.asM33();
+  SkMatrix inverse_layer_transform;
+  EXPECT_TRUE(layer_transform.invert(&inverse_layer_transform));
+
+  auto mock_layer = std::make_shared<MockLayer>(child_path, DlPaint());
+  auto layer = std::make_shared<TransformLayer>(layer_transform44);
+  layer->Add(mock_layer);
+
+  preroll_context()->state_stack.set_preroll_delegate(device_cull_rect,
+                                                      initial_transform);
+  layer->Preroll(preroll_context());
+  EXPECT_EQ(mock_layer->paint_bounds(), child_path.getBounds());
+  EXPECT_EQ(layer->paint_bounds(),
+            layer_transform.mapRect(mock_layer->paint_bounds()));
+  EXPECT_EQ(layer->child_paint_bounds(), mock_layer->paint_bounds());
+  EXPECT_TRUE(mock_layer->needs_painting(paint_context()));
+  EXPECT_TRUE(layer->needs_painting(paint_context()));
+  EXPECT_EQ(mock_layer->parent_matrix(),
+            SkMatrix::Concat(initial_transform, layer_transform));
+  EXPECT_EQ(mock_layer->parent_cull_rect(),
+            inverse_layer_transform.mapRect(local_cull_rect));
+  EXPECT_EQ(mock_layer->parent_mutators(),
+            std::vector({Mutator(layer_transform)}));
+
+  layer->Paint(display_list_paint_context());
+  DisplayListBuilder expected_builder;
+  /* (Transform)layer::Paint */ {
+    expected_builder.Save();
+    {
+      expected_builder.Transform(layer_transform44);
+      /* mock_layer::Paint */ {
+        expected_builder.DrawPath(child_path, DlPaint());
+      }
+    }
+    expected_builder.Restore();
+  }
+  EXPECT_TRUE(DisplayListsEQ_Verbose(display_list(), expected_builder.Build()));
+}
+
 TEST_F(TransformLayerTest, Nested) {
   SkPath child_path;
   child_path.addRect(5.0f, 6.0f, 20.5f, 21.5f);

lib/ui/compositing/scene_builder.cc

@@ -44,7 +44,7 @@ SceneBuilder::~SceneBuilder() = default;
 void SceneBuilder::pushTransform(Dart_Handle layer_handle,
                                  tonic::Float64List& matrix4,
                                  const fml::RefPtr<EngineLayer>& oldLayer) {
-  SkMatrix sk_matrix = ToSkMatrix(matrix4);
+  SkM44 sk_matrix = ToSkM44(matrix4);
   auto layer = std::make_shared<flutter::TransformLayer>(sk_matrix);
   PushLayer(layer);
   // matrix4 has to be released before we can return another Dart object

lib/ui/painting/matrix.cc

@@ -18,6 +18,17 @@ static const int kSkMatrixIndexToMatrix4Index[] = {
     // clang-format on
 };
 
+SkM44 ToSkM44(const tonic::Float64List& matrix4) {
+  // clang-format off
+  return SkM44(
+    SafeNarrow(matrix4[ 0]), SafeNarrow(matrix4[ 4]), SafeNarrow(matrix4[ 8]), SafeNarrow(matrix4[12]),
+    SafeNarrow(matrix4[ 1]), SafeNarrow(matrix4[ 5]), SafeNarrow(matrix4[ 9]), SafeNarrow(matrix4[13]),
+    SafeNarrow(matrix4[ 2]), SafeNarrow(matrix4[ 6]), SafeNarrow(matrix4[10]), SafeNarrow(matrix4[14]),
+    SafeNarrow(matrix4[ 3]), SafeNarrow(matrix4[ 7]), SafeNarrow(matrix4[11]), SafeNarrow(matrix4[15])
+  );
+  // clang-format on
+}
+
 SkMatrix ToSkMatrix(const tonic::Float64List& matrix4) {
   FML_DCHECK(matrix4.data());
   SkMatrix sk_matrix;

lib/ui/painting/matrix.h

@@ -5,11 +5,13 @@
 #ifndef FLUTTER_LIB_UI_PAINTING_MATRIX_H_
 #define FLUTTER_LIB_UI_PAINTING_MATRIX_H_
 
+#include "third_party/skia/include/core/SkM44.h"
 #include "third_party/skia/include/core/SkMatrix.h"
 #include "third_party/tonic/typed_data/typed_list.h"
 
 namespace flutter {
 
+SkM44 ToSkM44(const tonic::Float64List& matrix4);
 SkMatrix ToSkMatrix(const tonic::Float64List& matrix4);
 tonic::Float64List ToMatrix4(const SkMatrix& sk_matrix);