[Impeller] Use the new DisplayList depth info in the experimental canvas prototype

[flar]

Plumbing the new DL depth data into the experimental direct rendering Impeller canvas prototype.

[flar]

Not exactly the most thorough stress test, but this flutter app with a bunch of save/restored clips now restores the clip state correctly with this patch...

main.dart

import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple),
        useMaterial3: true,
      ),
      home: const MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  const MyHomePage({super.key, required this.title});

  final String title;

  @override
  State<MyHomePage> createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        backgroundColor: Theme.of(context).colorScheme.inversePrimary,
        title: Text(widget.title),
      ),
      body: Center(
        child: CustomPaint(
          size: const Size(200, 200),
          foregroundPainter: _MyPainter(),
        ),
      ),
    );
  }
}

class _MyPainter extends CustomPainter {
  @override
  void paint(Canvas canvas, Size size) {
    canvas.save();
    Rect bounds = Rect.fromLTWH(0, 0, size.width, size.height);
    double inset = size.longestSide / 50;
    bounds = bounds.deflate(inset * 20);
    bounds = bounds.shift(Offset(-inset * 10, 0));
    for (int i = 0; i < 20; i++) {
      canvas.save();
      canvas.clipRect(bounds);
      Paint paint = Paint()
        ..color = ((i & 1) == 0)
            ? Colors.lightGreenAccent
            : Colors.blueAccent;
      canvas.drawOval(bounds, paint);
      // bounds = bounds.deflate(inset);
      bounds = bounds.shift(Offset(inset, 0));
      canvas.restore();
    }
    canvas.restore();
    Paint paint = Paint()
      ..color = Colors.purple
      ..strokeWidth = 4.0;
    canvas.drawLine(Offset.zero, Offset(size.width, size.height), paint);
    canvas.drawLine(Offset(size.width, 0), Offset(0, size.height), paint);
  }

  @override
  bool shouldRepaint(covariant CustomPainter oldDelegate) => true;
}

[jonahwilliams]

When I run this on flutter gallery (dev/integration_tests/new_gallery) without the experimental canvas patch I'm getting a warning about an unresolved depth value:

E/flutter (14094): [ERROR:flutter/impeller/base/validation.cc(59)] Break on 'ImpellerValidationBreak' to inspect point of failure: EntityPass (Depth=1) contains one or more clips with an unresolved depth value.
E/flutter (14094): [ERROR:flutter/impeller/base/validation.cc(59)] Break on 'ImpellerValidationBreak' to inspect point of failure: EntityPass (Depth=1) contains one or more clips with an unresolved depth value.
E/flutter (14094): [ERROR:flutter/impeller/base/validation.cc(59)] Break on 'ImpellerValidationBreak' to inspect point of failure: EntityPass (Depth=1) contains one or more clips with an unresolved depth value.
E/flutter (14094): [ERROR:flutter/impeller/base/validation.cc(59)] Break on 'ImpellerValidationBreak' to inspect point of failure: EntityPass (Depth=1) contains one or more clips with an unresolved depth value.
E/flutter (14094): [ERROR:flutter/impeller/base/validation.cc(59)] Break on 'ImpellerValidationBreak' to inspect point of failure: EntityPass (Depth=1) contains one or more clips with an unresolved depth value.

[jonahwilliams]

This also crashes on gallery with the new canvas enabled:

F/flutter (14388): [FATAL:flutter/impeller/aiks/experimental_canvas.cc(134)] Check failed: entry.clip_depth <= transform_stack_.back().clip_depth. 
F/libc    (14388): Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 14486 (1.raster), pid 14388 (ple.new_gallery)
The Flutter DevTools debugger and profiler on Pixel 7 Pro is available at: http://127.0.0.1:9101?uri=http://127.0.0.1:62583/lfKqBuc0bjg=/
*** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
Build fingerprint: 'google/cheetah/cheetah:14/AP1A.240505.005.A1/11678256:userdebug/dev-keys'
Revision: 'MP1.0'
ABI: 'arm64'
Timestamp: 2024-04-18 10:33:06.655951217-0700
Process uptime: 3s
Cmdline: com.example.new_gallery
pid: 14388, tid: 14486, name: 1.raster  >>> com.example.new_gallery <<<
uid: 10307
tagged_addr_ctrl: 0000000000000001 (PR_TAGGED_ADDR_ENABLE)
signal 6 (SIGABRT), code -1 (SI_QUEUE), fault addr --------
Abort message: '[FATAL:flutter/impeller/aiks/experimental_canvas.cc(134)] Check failed: entry.clip_depth <= transform_stack_.back().clip_depth. '

[jonahwilliams]

This is happening while drawing a shadow:

      #01 pc 0000000001ce456c  /data/app/~~MxWAojzdDhoBYx7vkc_NFA==/com.example.new_gallery-EOFUbi9ms1rJGh8AnpR3SA==/lib/arm64/libflutter.so (std::__throw_bad_array_new_length[abi:v15000]()+4) (BuildId: c1230614699ea358ae3fb3baf3a0b0d9ef1190f5)
      #02 pc 0000000001d0e518  /data/app/~~MxWAojzdDhoBYx7vkc_NFA==/com.example.new_gallery-EOFUbi9ms1rJGh8AnpR3SA==/lib/arm64/libflutter.so (fml::LogMessage::~LogMessage()+208) (BuildId: c1230614699ea358ae3fb3baf3a0b0d9ef1190f5)
      #03 pc 00000000020ad0c0  /data/app/~~MxWAojzdDhoBYx7vkc_NFA==/com.example.new_gallery-EOFUbi9ms1rJGh8AnpR3SA==/lib/arm64/libflutter.so (impeller::ExperimentalCanvas::Save(unsigned int)+112) (BuildId: c1230614699ea358ae3fb3baf3a0b0d9ef1190f5)
      #04 pc 00000000020e9b6c  /data/app/~~MxWAojzdDhoBYx7vkc_NFA==/com.example.new_gallery-EOFUbi9ms1rJGh8AnpR3SA==/lib/arm64/libflutter.so (impeller::DlDispatcherBase::drawShadow(flutter::DlOpReceiver::CacheablePath const&, flutter::DlColor, float, bool, float)+376) (BuildId: c1230614699ea358ae3fb3baf3a0b0d9ef1190f5)
      #05 pc 0000000001d70680  /data/app/~~MxWAojzdDhoBYx7vkc_NFA==/com.example.new_gallery-EOFUbi9ms1rJGh8AnpR3SA==/lib/arm64/libflutter.so (flutter::DisplayList::Dispatch(flutter::DlOpReceiver&, unsigned char*, unsigned char*, flutter::Culler&) const+560) (BuildId: c1230614699ea358ae3fb3baf3a0b0d9ef1190f5)
      #06 pc 0000000001d70a18  /data/app/~~MxWAojzdDhoBYx7vkc_NFA==/com.example.new_gallery-EOFUbi9ms1rJGh8AnpR3SA==/lib/arm64/libflutter.so (flutter::DisplayList::Dispatch(flutter::DlOpReceiver&, SkRect const&) const+204) (BuildId: c1230614699ea358ae3fb3baf3a0b0d9ef1190f5)

[jonahwilliams]

It looks like the call to save in drawShadow is missing the depth value?

[jonahwilliams]

Here is my diff that I think fixes it at least for the gallery case:

diff --git a/impeller/aiks/canvas.cc b/impeller/aiks/canvas.cc
index 6c86ea3b5c..4feb71ee1f 100644
--- a/impeller/aiks/canvas.cc
+++ b/impeller/aiks/canvas.cc
@@ -21,6 +21,7 @@
 #include "impeller/entity/contents/text_contents.h"
 #include "impeller/entity/contents/texture_contents.h"
 #include "impeller/entity/contents/vertices_contents.h"
+#include "impeller/entity/entity_pass.h"
 #include "impeller/entity/geometry/geometry.h"
 #include "impeller/geometry/color.h"
 #include "impeller/geometry/constants.h"
@@ -411,13 +412,13 @@ bool Canvas::AttemptDrawBlurredRRect(const Rect& rect,
     // Defer the alpha, blend mode, and image filter to a separate layer.
     SaveLayer({.color = Color::White().WithAlpha(rrect_color.alpha),
                .blend_mode = paint.blend_mode,
-               .image_filter = paint.image_filter});
+               .image_filter = paint.image_filter}, /*bounds=*/std::nullopt, nullptr, ContentBoundsPromise::kContainsContents, 1); // TODO bounds + promise.
     rrect_paint.color = rrect_color.WithAlpha(1);
   } else {
     rrect_paint.color = rrect_color;
     rrect_paint.blend_mode = paint.blend_mode;
     rrect_paint.image_filter = paint.image_filter;
-    Save();
+    Save(1);
   }
 
   auto draw_blurred_rrect = [this, &rect, &corner_radii, &rrect_paint]() {
diff --git a/impeller/display_list/dl_dispatcher.cc b/impeller/display_list/dl_dispatcher.cc
index ab07ebdd9a..51a70068ca 100644
--- a/impeller/display_list/dl_dispatcher.cc
+++ b/impeller/display_list/dl_dispatcher.cc
@@ -1152,7 +1152,7 @@ void DlDispatcherBase::drawShadow(const CacheablePath& cache,
                       GetCanvas().GetCurrentTransform().GetScale().y},
   };
 
-  GetCanvas().Save();
+  GetCanvas().Save(1); // Only a single draw.
   GetCanvas().PreConcat(
       Matrix::MakeTranslation(Vector2(0, -occluder_z * light_position.y)));
 
diff --git a/shell/gpu/gpu_surface_vulkan_impeller.cc b/shell/gpu/gpu_surface_vulkan_impeller.cc
index e85ed212f9..e502834b38 100644
--- a/shell/gpu/gpu_surface_vulkan_impeller.cc
+++ b/shell/gpu/gpu_surface_vulkan_impeller.cc

[jonahwilliams]

I think the correct bounds for that SaveLayer is probably just the RRect bounds

[jonahwilliams]

There is also an error on the existing canvas when an image filter is applied, this can be seen when the stretch overscroll effect on the gallery runs - same error about unresolved depth value.

[jonahwilliams]

Actually since this is for the rrect blur we need to expand the coverage size up to some threshold. Doesn't need to be done now I'd just note it so we don't forget.

[jonahwilliams]

err not rrect blur, the slow gaussian blur path.

[flar]

The bounds for a SaveLayer are the content bounds, not for the filtered bounds, so it should not include any expansion by the image_filter in the SaveLayer paint...

[flar]

Or, are you saying that the contents of this SaveLayer will be the expanded blurred rrect?

[jonahwilliams]

Ahh right, if its the content bounds its just the rect.

[jonahwilliams]

loweing as in replacing things like drawShadow with saveLayer/blur drawShape restore. It would be specific to this renderer but that is good IMO

[flar]

Also, as we progress along with the experimental canvas we might start relying less on calling one method from another with depth implications and just insert the associated rendering structures more directly. Then we'll have more immediate control over the allocation of depths from each operation. Currently, the things that you suggest will have automatic allocations of depth implicit in calling "that other rendering operation". We could create a new layer, draw the blurred thing to it, and then render it back to the original render target all without needing to bump the depth more than the implicitly assigned single depth value. For now there are just a lot of side effects to each implementation because we're trying to leverage the regular canvas ops.

[flar]

I'm going to adjust for this by just having the DL allocate 2 depths per rendering op when there is a MaskFilter in effect. It's kind of Impeller-specific logic, but it's abstracted a bit to keep the object encapsulation intact. In reality, DL could always allocate 5 or 20 depths to each rendering op and we'd still never really run out of room in the depth buffer. Then we won't care how efficient or multi-staged the Impeller implementations are. For now, I'm trying to keep it mostly stingy with occasional conservative over-allocations...

[jonahwilliams]

Seems reasonable 👍

[flar]

The latest commit adjusts for this additional SaveLayer.

[flar]

In the latest commit I've added EXPERIMENTAL support for the playground...

[jonahwilliams]

LGTM