[Impeller] match rrect_blur math to the gaussian math

[gaaclarke]

fixes flutter/flutter#149276

before

after

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[gaaclarke]

When we land this we should revisit the math for the rrect_blur halo since we should be able to make it even more constricted.

[jonahwilliams]

Beautiful!

[flar]

How do either of those 2 results compare to our legacy behavior on these shapes?

[gaaclarke]

How do either of those 2 results compare to our legacy behavior on these shapes?

The guassian blur matches skia's blur. That work happened here: #47405

[flutter-dashboard]

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #53130 at sha 2466518

[gaaclarke]

Oh awesome, the results on CI show that the scalar is related to DPI. I guess my golden images were running at 2x. I thought they were all 1x. I'll make that change. That wraps everything up in a nice bow.

[flar]

How do either of those 2 results compare to our legacy behavior on these shapes?

The guassian blur matches skia's blur. That work happened here: #47405

(Hang on, I was running this on a May 11 version of the framework repo, need to double check...)
(Reconfirmed on To(framework)T)

That's not what I'm seeing on the current ToT - the fast blur matches Skia, not the other way around:
(Note, the RRect fast blur should be on the left, the right is a Path that simulates a round rect and isn't optimized to an RRect)

test program

import 'package:flutter/material.dart';

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

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

  @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: Column(
          mainAxisAlignment: MainAxisAlignment.spaceEvenly,
          children: <Widget>[
            Row(
              mainAxisAlignment: MainAxisAlignment.spaceEvenly,
              children: <Widget>[
                CustomPaint(
                  painter: _RRectPainter(
                    cornerSize: 10,
                    blurSize: 10,
                  ),
                  size: const Size(150, 150),
                ),
                CustomPaint(
                  painter: _RRectPathPainter(
                    cornerSize: 10,
                    blurSize: 10,
                  ),
                  size: const Size(150, 150),
                ),
              ],
            ),
            Row(
              mainAxisAlignment: MainAxisAlignment.spaceEvenly,
              children: <Widget>[
                CustomPaint(
                  painter: _RRectPainter(
                    cornerSize: 10,
                    blurSize: 30,
                  ),
                  size: const Size(150, 150),
                ),
                CustomPaint(
                  painter: _RRectPathPainter(
                    cornerSize: 10,
                    blurSize: 30,
                  ),
                  size: const Size(150, 150),
                ),
              ],
            ),
            Row(
              mainAxisAlignment: MainAxisAlignment.spaceEvenly,
              children: <Widget>[
                CustomPaint(
                  painter: _RRectPainter(
                    cornerSize: 10,
                    blurSize: 50,
                  ),
                  size: const Size(150, 150),
                ),
                CustomPaint(
                  painter: _RRectPathPainter(
                    cornerSize: 10,
                    blurSize: 50,
                  ),
                  size: const Size(150, 150),
                ),
              ],
            ),
          ],
        ),
      ),
    );
  }
}

class _RRectPainter extends CustomPainter {
  _RRectPainter({required this.cornerSize, required this.blurSize});

  final double cornerSize;
  final double blurSize;

  @override
  void paint(Canvas canvas, Size size) {
    Rect rect = Rect.fromLTRB(0, 0, size.width, size.height);
    RRect rrect = RRect.fromRectXY(rect, cornerSize, cornerSize);

    Paint paint = Paint()
      ..maskFilter = MaskFilter.blur(BlurStyle.normal, blurSize)
      ..color = Colors.blue;
    canvas.drawRRect(rrect, paint);
  }

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

class _RRectPathPainter extends CustomPainter {
  _RRectPathPainter({required this.cornerSize, required this.blurSize});

  final double cornerSize;
  final double blurSize;

  @override
  void paint(Canvas canvas, Size size) {
    Path path = Path();
    path.addRect(Rect.fromLTRB(
        cornerSize, 0,
        size.width - cornerSize, size.height));
    path.addRect(Rect.fromLTRB(
        0, cornerSize,
        size.width, size.height - cornerSize));
    path.addOval(Rect.fromLTRB(
      0, 0, cornerSize * 2, cornerSize * 2,
    ));
    path.addOval(Rect.fromLTRB(
      size.width - cornerSize * 2, 0, size.width, cornerSize * 2,
    ));
    path.addOval(Rect.fromLTRB(
      0, size.height - cornerSize * 2, cornerSize * 2, size.height,
    ));
    path.addOval(Rect.fromLTRB(
      size.width - cornerSize * 2, size.height - cornerSize * 2, size.width, size.height,
    ));

    Paint paint = Paint()
      ..maskFilter = MaskFilter.blur(BlurStyle.normal, blurSize)
      ..color = Colors.blue;
    canvas.drawPath(path, paint);
  }

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

Skia output

Impeller output

[gaaclarke]

That's not what I'm seeing on the current ToT

Oh interesting, #53130 was working with larger sigmas. Yet sigma 50 is super blurry.
Did you check the reproduction for the bug that fixed?

There is obviously some issue with dpi that needs to be resolved. I'm going to move this back to a draft as we sort through this.

[flar]

That's not what I'm seeing on the current ToT

Oh interesting, #53130 was working with larger sigmas. Yet sigma 50 is super blurry. Did you check the reproduction for the bug that fixed?

There is obviously some issue with dpi that needs to be resolved. I'm going to move this back to a draft as we sort through this.

This is the framework commit that I was running against:

commit 488fb0958269a8553e400ac8724525ecf98ea88f (HEAD -> main, upstream/master, upstream/main)
Author: Kostia Sokolovskyi <sokolovskyi.konstantin@gmail.com>
Date:   Thu May 30 22:17:04 2024 +0200

[flutter-dashboard]

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

[flar]

In Paint::MaskBlurDescriptor::CreateMaskBlur I don't see anything that applies the transform to the sigmas...?

[gaaclarke]

I verified that the gaussian blur still looks correct for the reproduction code for issue flutter/flutter#127770. That has 164 for a sigma. But setting the sigma to 50 we see the same thing jim saw

skia sigma = 164

impeller sigma = 164

skia sigma = 50

impeller sigma = 50

doctor

[!] Flutter (Channel [user-branch], 3.23.0-13.0.pre.27, on macOS 14.5 23F79 darwin-arm64, locale en)
    ! Flutter version 3.23.0-13.0.pre.27 on channel [user-branch] at /Users/aaclarke/dev/flutter
      Currently on an unknown channel. Run `flutter channel` to switch to an official channel.
      If that doesn't fix the issue, reinstall Flutter by following instructions at https://flutter.dev/docs/get-started/install.
    ! Upstream repository unknown source is not a standard remote.
      Set environment variable "FLUTTER_GIT_URL" to unknown source to dismiss this error.
    • Framework revision 472a29d3f3 (6 hours ago), 2024-05-30 09:24:14 -0700
    • Engine revision fb64b9a4e6
    • Dart version 3.5.0 (build 3.5.0-203.0.dev)
    • DevTools version 2.36.0-dev.10
    • If those were intentional, you can disregard the above warnings; however it is recommended to use "git" directly to perform update checks and upgrades.

[flar]

I modified my test case to do 4 columns

• RRect with 0.5 scale
• Path with 0.5 scale
• RRect with 1.0 scale
• Path with 1.0 scale

Note that I adjust the RRect and Path geometry to counter-act the scale so that they will be the same size shape in both cases, but the blur will be scaled. The left pair should look the same with about 1/2 the blurring compared to the right pair. As you can see, the blurring changes size for the RRect fast path version, but not for the Path (gaussian) version, confirming that the gaussian path is ignoring the transform for the sigma...

(Updated the test case - I forgot to adjust the corner size for the fast RRect case - fixed now)

New test code

import 'package:flutter/material.dart';

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

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

  @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> {
  Widget makeRow(double cornerSize, double blurSize) {
    return Row(
      mainAxisAlignment: MainAxisAlignment.spaceEvenly,
      children: <Widget>[
        CustomPaint(
          painter: _RRectPainter(
            cornerSize: cornerSize,
            blurSize: blurSize,
            scale: 0.5,
          ),
          size: const Size(150, 150),
        ),
        CustomPaint(
          painter: _RRectPathPainter(
            cornerSize: cornerSize,
            blurSize: blurSize,
            scale: 0.5,
          ),
          size: const Size(150, 150),
        ),
        CustomPaint(
          painter: _RRectPainter(
            cornerSize: cornerSize,
            blurSize: blurSize,
            scale: 1.0,
          ),
          size: const Size(150, 150),
        ),
        CustomPaint(
          painter: _RRectPathPainter(
            cornerSize: cornerSize,
            blurSize: blurSize,
            scale: 1.0,
          ),
          size: const Size(150, 150),
        ),
      ],
    );
  }
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        backgroundColor: Theme.of(context).colorScheme.inversePrimary,
        title: Text(widget.title),
      ),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.spaceEvenly,
          children: <Widget>[
            makeRow(10, 10),
            makeRow(10, 30),
            makeRow(10, 50),
          ],
        ),
      ),
    );
  }
}

class _RRectPainter extends CustomPainter {
  _RRectPainter({required this.cornerSize, required this.blurSize, required this.scale});

  final double cornerSize;
  final double blurSize;
  final double scale;

  @override
  void paint(Canvas canvas, Size size) {
    canvas.scale(scale, scale);
    double w = size.width / scale;
    double h = size.height / scale;
    double c = cornerSize / scale;

    Rect rect = Rect.fromLTRB(0, 0, w, h);
    RRect rrect = RRect.fromRectXY(rect, c, c);

    Paint paint = Paint()
      ..maskFilter = MaskFilter.blur(BlurStyle.normal, blurSize)
      ..color = Colors.blue;
    canvas.drawRRect(rrect, paint);
  }

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

class _RRectPathPainter extends CustomPainter {
  _RRectPathPainter({required this.cornerSize, required this.blurSize, required this.scale});

  final double cornerSize;
  final double blurSize;
  final double scale;

  @override
  void paint(Canvas canvas, Size size) {
    canvas.scale(scale, scale);
    double w = size.width / scale;
    double h = size.height / scale;
    double c = cornerSize / scale;

    Path path = Path();
    path.addRect(Rect.fromLTRB(c, 0, w - c, h));
    path.addRect(Rect.fromLTRB(0, c, w, h - c));
    path.addOval(Rect.fromLTRB(0, 0, c * 2, c * 2));
    path.addOval(Rect.fromLTRB(w - c * 2, 0, w, c * 2));
    path.addOval(Rect.fromLTRB(0, h - c * 2, c * 2, h));
    path.addOval(Rect.fromLTRB(w - c * 2, h - c * 2, w, h));

    Paint paint = Paint()
      ..maskFilter = MaskFilter.blur(BlurStyle.normal, blurSize)
      ..color = Colors.blue;
    canvas.drawPath(path, paint);
  }

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

Results on Impeller

[flar]

These 2 do not look the same to me. The Skia version is much blurrier than the Impeller version. I can still see the basic approximate shape in the Impeller version, but the Skia version is much more obscured.

I verified that the gaussian blur still looks correct for the reproduction code for issue flutter/flutter#127770. That has 164 for a sigma. But setting the sigma to 50 we see the same thing jim saw

skia sigma = 164

impeller sigma = 164

[gaaclarke]

These 2 do not look the same to me. The Skia version is much blurrier than the Impeller version. I can still see the basic approximate shape in the Impeller version, but the Skia version is much more obscured.

Yea, they're not going to be the same. Check out flutter/flutter#127770 to see what incorrect looks like.

[gaaclarke]

Okay, I found the bug in the gaussian blur. We were scaling the sigma by the effect transform, but not the entity's transform. That made the rrect_blur and gaussian blur match so I could remove that 0.5 scalar from rrect_blur. I've also added a new golden that shows they are the same when scaled too.

I still need to make sure that we are still good for that issue against 164 sigma and also make sure that the golden images are correct.

[gaaclarke]

With the most recent change, here is skia vs impeller with flutter/flutter#127770. Although they appear to agree they seem much more dissipated than in the expected result in the original bug.

skia

impeller

[gaaclarke]

Okay, the golden results look as expected. This is ready for review again. The bug was missing the entity transform when scaling the blur sigma. PTAL.

[flutter-dashboard]

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #53130 at sha 713670f

[flar]

The 2 new images for 164 look similar, but they are so big it's hard to eyeball them due to having to scroll back and forth...

[flutter-dashboard]

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #53130 at sha 3a178f7

[gaaclarke]

This PR makes the halo the match the rrect_blur size, but it seems to also be diluting the blur as seen in flutter/flutter#149431 (comment)

[gaaclarke]

This PR makes the halo the match the rrect_blur size, but it seems to also be diluting the blur as seen in flutter/flutter#149431 (comment)

I mistakenly took out the effect transform from the calculation of blur radius. I thought that was included in the entity transform, it turns out it wasn't.

[flutter-dashboard]

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #53130 at sha 82a120b

[flutter-dashboard]

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #53130 at sha 985fb30