Add toImage to RenderRepaintBoundary by gspencergoog · Pull Request #16758 · flutter/flutter

gspencergoog · 2018-04-19T20:02:23Z

This adds a toImage function to RenderRepaintBoundary that returns an uncompressed raw image of the RenderRepaintBoundary and its children. A device pixel ratio different from the physical ratio may be specified for the captured image. A value of 1.0 will give an image in logical pixels.

tvolkert · 2018-04-20T21:32:19Z

packages/flutter/lib/src/rendering/object.dart

Don't say "multiplied by the [window.devicePixelRatio]" since it's covered below in the discussion of the [pixelRatio] named parameter?

Yeah, sorry, I guess that's irrelevant now that I added pixelRatio.

tvolkert · 2018-04-20T21:33:20Z

packages/flutter/lib/src/rendering/object.dart

the render object must not need layout, painting, or compositing

How does the caller know whether this is the case?

Also, can we assert it below?

Added asserts, and an explanation.

tvolkert · 2018-04-20T21:34:16Z

packages/flutter/lib/src/rendering/object.dart

Where is anti-aliasing usually done when the render object is drawn to the screen?

I think it's antialiased at the device level (in the GPU), but since the engine downsamples it with nearest neighbor, anything smaller than the device resolution is not effectively antialiased. At least I think that's the case, and Jason says that it can actually take one of several paths in the engine, so it's not really clear to me when it happens. I decided to take out the advice: they can look at the result and decide for themselves to increase the size or not.

I don't really understand how we're actually rendering this, but I would presume we're just uploading the whole scene to the GPU, having it render to a texture, then grabbing the texture? In which case I'd expect antialiasing to work fine?

cc @jason-simmons or @chinmaygarde

Yeah, what @Hixie said. We have to render to texture on the GPU first because the scene may reference images that are only resident on the GPU. AA should work just fine.

tvolkert · 2018-04-20T21:35:10Z

packages/flutter/lib/src/rendering/object.dart

Does this have to be [ui.Scene.toImage]?

Yes, but I wasn't sure if dartdoc would pick it up correctly or not if I did that.

search for [ui. or [dart:ui. and compare to actual output, I forget which one is the right one.

tvolkert · 2018-04-20T21:47:14Z

packages/flutter/lib/src/rendering/object.dart

Is it worth optimizing out this matrix multiply for the case where pixelRatio is 1.0?

No, not really. From what I can tell, calling Matrix4.identity() is slower than the diagonal3Values call.

tvolkert · 2018-04-20T21:48:35Z

packages/flutter/lib/src/rendering/object.dart

try/finally with a call to scene.dispose() in the finally?

Good call. Added.

tvolkert · 2018-04-20T21:52:20Z

packages/flutter/test/rendering/object_test.dart

Add a test that expects our assertion error if the render object isn't a repaint boundary?

No, we don't usually test asserts as far as I can tell, and if my call doesn't assert, then the layer getter will.

we do test asserts sometimes (check for takeException), but this point will be moot anyway if you move it to RenderRepaintBoundary.

tvolkert

LGTM

Hixie · 2018-04-20T22:46:48Z

packages/flutter/lib/src/rendering/object.dart

To avoid people depending on particular widgets that happen to be repaint boundaries but might not always be, I recommend putting this on RenderRepaintBoundary.

Yeah, I guess that makes sense. It means that they'll have to cast the result when they get the render object in order to call this function, but that's not horrible.

Hixie · 2018-04-20T22:53:41Z

packages/flutter/lib/src/rendering/object.dart

paintBounds is a bit of a dubious choice because it's just advisory really.
If you move this to RenderRepaintBoundary, you can just use size, which is much more authoritative.

Hixie · 2018-04-20T22:53:44Z

packages/flutter/lib/src/rendering/object.dart

We should replace "dimensions" with the actual property that we use to determine the dimensions (currently [paintBounds], though if you take my feedback below, [size]).

OK, size it is.

Hixie · 2018-04-20T22:54:41Z

packages/flutter/lib/src/rendering/object.dart

rather than "must not need painting" it might be more positive to say "must have gone through the paint phase" or some such (the parenthetical is fine though)

Hixie · 2018-04-20T22:57:43Z

packages/flutter/lib/src/rendering/object.dart

rather than using multiplied() and allocating a new matrix, you can just use translate.

Good idea. Done.

Hixie · 2018-04-20T22:58:30Z

packages/flutter/lib/src/rendering/object.dart

I wonder if we should change toImage to just take a Size... cc @tvolkert

For the use cases we've heard for this, I don't have a strong opinion either way.

Hixie · 2018-04-20T23:00:16Z

packages/flutter/test/rendering/object_test.dart

You should add a test that verifies that it works (gives the right pixels) with two layers, since that was a bug.

tvolkert · 2018-04-22T05:49:38Z

I tried this out in a simple widget test:

testWidgets('test', (WidgetTester tester) async {
  await tester.pumpWidget(new MyApp());

  RenderRepaintBoundary repaintRenderBoundary = tester
      .allRenderObjects
      .firstWhere((RenderObject object) => object is RenderRepaintBoundary);
  ui.Image image = await repaintRenderBoundary.toImage();
  ByteData bytes = await image.toByteData(format: ui.ImageByteFormat.png);
});

If I run that test on an Android device/emulator, I get a correct PNG capture of the app. However, if I run that on the host in flutter_tester, the PNG is all white.

Am I holding it wrong?

tvolkert · 2018-04-22T06:09:27Z

aha - indeed I was holding it wrong. I was calling:

await new File('/path/fo/foo.png').writeAsBytes(bytes);

Which doesn't play nicely with our fake async. When I call writeAsBytesSync(bytes), it all worked great.

…tter#16758) This adds a toImage function to RenderRepaintBoundary that returns an uncompressed raw image of the RenderRepaintBoundary and its children. A device pixel ratio different from the physical ratio may be specified for the captured image. A value of 1.0 will give an image in logical pixels.

googlebot added the cla: yes label Apr 19, 2018

gspencergoog force-pushed the widget_image branch 7 times, most recently from 13717d2 to 28caa4d Compare April 19, 2018 22:17

gspencergoog changed the title ~~Add toImage to RenderObject~~ [WIP] Add toImage to RenderObject Apr 19, 2018

gspencergoog force-pushed the widget_image branch 3 times, most recently from 573b758 to 6ef6eb7 Compare April 20, 2018 21:17

gspencergoog changed the title ~~[WIP] Add toImage to RenderObject~~ Add toImage to RenderObject Apr 20, 2018

tvolkert requested review from Hixie and tvolkert April 20, 2018 21:23

tvolkert reviewed Apr 20, 2018

View reviewed changes

tvolkert approved these changes Apr 20, 2018

View reviewed changes

gspencergoog force-pushed the widget_image branch 2 times, most recently from 15bce3f to ebf0fb8 Compare April 20, 2018 22:26

tvolkert approved these changes Apr 20, 2018

View reviewed changes

Hixie reviewed Apr 20, 2018

View reviewed changes

gspencergoog force-pushed the widget_image branch from 3675e9a to d162ccc Compare April 20, 2018 23:18

gspencergoog changed the title ~~Add toImage to RenderObject~~ Add toImage to RenderRepaintBoundary Apr 20, 2018

gspencergoog force-pushed the widget_image branch 5 times, most recently from 09eeb2b to 681a657 Compare April 21, 2018 02:12

tvolkert mentioned this pull request Apr 22, 2018

Support golden file image diffing in testWidgets #16859

Closed

Adding an API for capturing an image of a RenderRepaintBoundary.

0699d76

gspencergoog force-pushed the widget_image branch from 681a657 to 0699d76 Compare April 23, 2018 15:55

gspencergoog merged commit a043ac4 into flutter:master Apr 23, 2018

gspencergoog deleted the widget_image branch May 30, 2018 21:16

github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2021

Conversation

gspencergoog commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvolkert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvolkert commented Apr 22, 2018

Uh oh!

tvolkert commented Apr 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

gspencergoog commented Apr 19, 2018 •

edited

Loading