Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

flar
Copy link
Contributor

@flar flar commented Nov 9, 2023

Impeller implements the DrawLine primitive as DrawPath on a path containing a single line. Benchmarks show that this can cost 30% overhead on apps that use a lot of DrawLine primitives. This PR creates a more direct Entity that can tesselate the geometry of a line directly.

The reduced overhead should help with flutter/flutter#138004

The current code will back off to Path rendering for round caps. When the circle geometry work is finished (#47845) we can come back and implement round caps using the code refactored in that PR.

@flar flar added the Work in progress (WIP) Not ready (yet) for review! label Nov 9, 2023
@flar
Copy link
Contributor Author

flar commented Nov 9, 2023

Here are some benchmarks I ran on an iPhone 6s using the example below. All times are the raster time average ms/frame on a profile build with LTO measured with the Performance Overlay with 10,000 lines per frame.

Running on Impeller now: 101
Running on Skia now: 17.2
Running on Impeller with this fix: 66

Test benchmark
import 'dart:math';

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 DrawLine Basher'),
    );
  }
}

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

  final String title;

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

class _MyHomePageState extends State<MyHomePage> with SingleTickerProviderStateMixin {
  List<Offset> points = List<Offset>.filled(10000, Offset.zero);
  int highlight = 0;
  late AnimationController _controller;

  static double W = 300;
  static double H = 300;

  @override
  void initState() {
    super.initState();
    Random r = Random();
    for (int i = 0; i < points.length; i++) {
      points[i] = Offset(r.nextDouble() * W, r.nextDouble() * H);
    }
    _controller = AnimationController(
      duration: const Duration(seconds: 10),
      vsync: this,
    );
    _controller.addListener(() {
      setState(() {
        highlight++;
        if (highlight >= points.length) {
          highlight = 0;
        }
      });
    });
    _controller.repeat(reverse: false);
  }

  @override
  void dispose() {
    _controller.dispose();
    super.dispose();
  }

  @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.center,
          children: <Widget>[
            CustomPaint(
              size: Size(W, H),
              painter: _MyPainter(points: points, highlight: highlight),
            ),
          ],
        ),
      ),
    );
  }
}

class _MyPainter extends CustomPainter {
  List<Offset> points;
  int highlight;

  _MyPainter({required this.points, this.highlight = -1});

  @override
  void paint(Canvas canvas, Size size) {
    Paint paint = Paint()
      ..color = Colors.green
      ..strokeWidth = 1
      ..strokeCap = StrokeCap.butt;
    Paint highlightPaint = Paint()
      ..color = Colors.blue
      ..strokeWidth = 4
      ..strokeCap = StrokeCap.square;
    for (int i = 1; i < points.length; i++) {
      if (i - 1 != highlight) {
        canvas.drawLine(points[i-1], points[i], paint);
      }
    }
    if (highlight > 0 && highlight < points.length) {
      canvas.drawLine(points[highlight-1], points[highlight], highlightPaint);
    }
  }

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

@jonahwilliams
Copy link
Contributor

As much of an improvement this is, Skia is still just so much better. I suspec what they're doing is some amount of batching either during cmd recording or playback?

@flar flar force-pushed the impeller-DrawRect-specialization branch from 4094fe6 to 07cdfbd Compare November 10, 2023 18:53
@flar flar marked this pull request as ready for review November 10, 2023 18:53
@flar flar removed the Work in progress (WIP) Not ready (yet) for review! label Nov 10, 2023
@flutter-dashboard
Copy link

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 #47846 at sha 07cdfbd

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nits

return coverage.has_value() ? coverage->Contains(rect) : false;
}

bool LineGeometry::IsAxisAlignedRect() const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if you're drawing a line, its only an axis aligned rect if it is a square cap?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

square or butt, I'll include this in the issue for handling round caps...

}

void Canvas::DrawLine(const Point& p0, const Point& p1, const Paint& paint) {
if (paint.stroke_cap == Cap::kRound) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: file an issue for round caps

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 10, 2023
@auto-submit auto-submit bot merged commit 77a12e4 into flutter:main Nov 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 10, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 11, 2023
…138266)

flutter/engine@9d8a112...00db306

2023-11-10 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Bump minSdk to 19 for Android tests" (flutter/engine#47935)
2023-11-10 [email protected] Roll Dart SDK from 91c4a92a64ea to 370145bbbd4f (1 revision) (flutter/engine#47930)
2023-11-10 [email protected] Roll Skia from 2c43bf002b7f to 96ce4d6f433d (3 revisions) (flutter/engine#47931)
2023-11-10 [email protected] [Impeller] implement Canvas::DrawLine to tesselate lines directly (flutter/engine#47846)
2023-11-10 [email protected] Roll Skia from d06840545bff to 2c43bf002b7f (1 revision) (flutter/engine#47928)
2023-11-10 [email protected] Bump minSdk to 19 for Android tests (flutter/engine#47686)
2023-11-10 [email protected] [Impeller] Reduce allocations for polyline generation (flutter/engine#47837)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller will affect goldens
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants