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

Reland: Avoid a copy in EncodeImage #20003

Merged
merged 1 commit into from
Jul 31, 2020

Conversation

zanderso
Copy link
Member

Description

While working on a hobby project, I noticed that image.toByteData(format: ui.ImageByteFormat.png) is quite slow for large images (>4s for a 1080p image on a Nexus 5x). Most of that work is not on the Dart thread, but the final copy into a Dart heap Uint8List appeared to be, and was taking ~1.5s.

Instead of doing that last copy, this PR creates an external typed data object instead.

This is a reland of #19504. The buffer is not allocated in read-only memory, skia will not write it, and it is owned by Dart on return, so it does not need to be an UnmodifiableByteDataView. It can still be Uint8List.

Tests

I added the following tests:

image_encoding_unittests.cc

Breaking Change

Did any tests fail when you ran them? Please read [handling breaking changes].

  • No, no existing tests failed, so this is not a breaking change.

@zanderso zanderso added the Work in progress (WIP) Not ready (yet) for review! label Jul 24, 2020
@auto-assign auto-assign bot requested a review from liyuqian July 24, 2020 16:30
@zanderso zanderso force-pushed the reland-external-encode-image branch from 5a7e3b2 to 8945ad2 Compare July 24, 2020 17:33
@liyuqian
Copy link
Contributor

This seems to be a big improvement similar to #18838 (this PR seems to speed up the image exporting while #18838 speeds up the image importing). I wonder if we could have a benchmark in the engine similar to #18945 to quantify this improvement?

@liyuqian liyuqian added perf: speed Performance issues related to (mostly rendering) speed severe: performance Relates to speed or footprint issues. labels Jul 24, 2020
@zanderso zanderso force-pushed the reland-external-encode-image branch from 8945ad2 to e479fb1 Compare July 28, 2020 20:06
@zanderso zanderso force-pushed the reland-external-encode-image branch from e479fb1 to 400aa98 Compare July 29, 2020 17:31
@zanderso
Copy link
Member Author

This seems to be a big improvement similar to #18838 (this PR seems to speed up the image exporting while #18838 speeds up the image importing). I wonder if we could have a benchmark in the engine similar to #18945 to quantify this improvement?

I tried this, setting up an isolate as in #18945, but hit a crash in Picture::RasterizeToImage, presumably because more setup is required. @liyuqian are there any examples of benchmarks that do something like that?

@zanderso zanderso removed the Work in progress (WIP) Not ready (yet) for review! label Jul 29, 2020
@liyuqian
Copy link
Contributor

@zanderso : you may need to setup the whole shell in order to benchmark Picture::RasterizeToImage. An example of setting up shell in benchmarks could be found in

shell = Shell::Create(

You may also be able to utilize the ShellTest fixture to create the shell and run the engine.

std::unique_ptr<Shell> shell = CreateShell(settings);

Once the shell is properly set up, you should be able to call Picture::RasterizeToImage on the UI thread using

shell->GetTaskRunners().GetUITaskRunner()->PostTask(...)

@zanderso
Copy link
Member Author

Re-run of a flaky Fuchsia job succeeded, so I will land this on red to get things going again.

I have a benchmark for this running locally, but I will submit a separate PR for it to keep this one from getting too big.

@zanderso zanderso merged commit 5513273 into flutter:master Jul 31, 2020
@zanderso zanderso deleted the reland-external-encode-image branch July 31, 2020 15:24
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 31, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 31, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 31, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 31, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 3, 2020
zanderso pushed a commit to flutter/flutter that referenced this pull request Aug 3, 2020
* 7f5d044 Wait before switching surfaces (flutter/engine#20100)

* 5513273 Reland: Avoid a copy in EncodeImage (flutter/engine#20003)

* 1efdd95 Roll Dart SDK from bd528bfbd69d to ea6bde577d1c (19 revisions) (flutter/engine#20172)

* 3b0e697 Roll Skia from 8cc118dce813 to c3794dd52778 (27 revisions) (flutter/engine#20173)

* cb1a374 Roll Fuchsia Mac SDK from T2xc0OuiK... to i0zTcQ8Qb... (flutter/engine#20175)

* ed36b1a Roll Skia from c3794dd52778 to 2d01ed94605a (10 revisions) (flutter/engine#20179)

* fcc1eaf Fix iOS Keyboard stuck as UIKeyboardTypeNamePhonePad (flutter/engine#20181)

* 9c6837c Roll Skia from 2d01ed94605a to 7225788b9070 (6 revisions) (flutter/engine#20183)

* 13e993e Fix Typos (flutter/engine#19691)

* 98cfd1d Move platform specific information to `PlatformConfiguration` class (flutter/engine#19652)

* 22fb58b update nullability of drawAtlas methods and flesh out docs (flutter/engine#20176)

* bcc43df Roll Dart SDK from ea6bde577d1c to 033a81d924b9 (23 revisions) (flutter/engine#20186)

* cb4bb93 [web] increase number of shards. sync engine web tests same as flutter repo (flutter/engine#20164)

* d986b8d Enable linting in several files (flutter/engine#20134)

* 7dd092d Enable more linting (flutter/engine#20187)

* 3cc86ac Roll Dart SDK from 033a81d924b9 to ad5bcf16f1c8 (9 revisions) (flutter/engine#20191)

* 5ca8a2a Roll Dart SDK from ad5bcf16f1c8 to d169af6f7d8f (1 revision) (flutter/engine#20192)

* 4de0c04 Roll Dart SDK from d169af6f7d8f to 7e6c55e3aaf5 (1 revision) (flutter/engine#20196)

* 908fe01 Fix navigation message relay. (flutter/engine#20193)

* f1b3b69 Roll Dart SDK from 7e6c55e3aaf5 to 365525432a70 (2 revisions) (flutter/engine#20197)

* 8fbdd3f Fix parameter names

* 083282e Fix Implments typo
Pragya007 pushed a commit to Pragya007/flutter that referenced this pull request Aug 11, 2020
* 7f5d044 Wait before switching surfaces (flutter/engine#20100)

* 5513273 Reland: Avoid a copy in EncodeImage (flutter/engine#20003)

* 1efdd95 Roll Dart SDK from bd528bfbd69d to ea6bde577d1c (19 revisions) (flutter/engine#20172)

* 3b0e697 Roll Skia from 8cc118dce813 to c3794dd52778 (27 revisions) (flutter/engine#20173)

* cb1a374 Roll Fuchsia Mac SDK from T2xc0OuiK... to i0zTcQ8Qb... (flutter/engine#20175)

* ed36b1a Roll Skia from c3794dd52778 to 2d01ed94605a (10 revisions) (flutter/engine#20179)

* fcc1eaf Fix iOS Keyboard stuck as UIKeyboardTypeNamePhonePad (flutter/engine#20181)

* 9c6837c Roll Skia from 2d01ed94605a to 7225788b9070 (6 revisions) (flutter/engine#20183)

* 13e993e Fix Typos (flutter/engine#19691)

* 98cfd1d Move platform specific information to `PlatformConfiguration` class (flutter/engine#19652)

* 22fb58b update nullability of drawAtlas methods and flesh out docs (flutter/engine#20176)

* bcc43df Roll Dart SDK from ea6bde577d1c to 033a81d924b9 (23 revisions) (flutter/engine#20186)

* cb4bb93 [web] increase number of shards. sync engine web tests same as flutter repo (flutter/engine#20164)

* d986b8d Enable linting in several files (flutter/engine#20134)

* 7dd092d Enable more linting (flutter/engine#20187)

* 3cc86ac Roll Dart SDK from 033a81d924b9 to ad5bcf16f1c8 (9 revisions) (flutter/engine#20191)

* 5ca8a2a Roll Dart SDK from ad5bcf16f1c8 to d169af6f7d8f (1 revision) (flutter/engine#20192)

* 4de0c04 Roll Dart SDK from d169af6f7d8f to 7e6c55e3aaf5 (1 revision) (flutter/engine#20196)

* 908fe01 Fix navigation message relay. (flutter/engine#20193)

* f1b3b69 Roll Dart SDK from 7e6c55e3aaf5 to 365525432a70 (2 revisions) (flutter/engine#20197)

* 8fbdd3f Fix parameter names

* 083282e Fix Implments typo
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
* 7f5d044 Wait before switching surfaces (flutter/engine#20100)

* 5513273 Reland: Avoid a copy in EncodeImage (flutter/engine#20003)

* 1efdd95 Roll Dart SDK from bd528bfbd69d to ea6bde577d1c (19 revisions) (flutter/engine#20172)

* 3b0e697 Roll Skia from 8cc118dce813 to c3794dd52778 (27 revisions) (flutter/engine#20173)

* cb1a374 Roll Fuchsia Mac SDK from T2xc0OuiK... to i0zTcQ8Qb... (flutter/engine#20175)

* ed36b1a Roll Skia from c3794dd52778 to 2d01ed94605a (10 revisions) (flutter/engine#20179)

* fcc1eaf Fix iOS Keyboard stuck as UIKeyboardTypeNamePhonePad (flutter/engine#20181)

* 9c6837c Roll Skia from 2d01ed94605a to 7225788b9070 (6 revisions) (flutter/engine#20183)

* 13e993e Fix Typos (flutter/engine#19691)

* 98cfd1d Move platform specific information to `PlatformConfiguration` class (flutter/engine#19652)

* 22fb58b update nullability of drawAtlas methods and flesh out docs (flutter/engine#20176)

* bcc43df Roll Dart SDK from ea6bde577d1c to 033a81d924b9 (23 revisions) (flutter/engine#20186)

* cb4bb93 [web] increase number of shards. sync engine web tests same as flutter repo (flutter/engine#20164)

* d986b8d Enable linting in several files (flutter/engine#20134)

* 7dd092d Enable more linting (flutter/engine#20187)

* 3cc86ac Roll Dart SDK from 033a81d924b9 to ad5bcf16f1c8 (9 revisions) (flutter/engine#20191)

* 5ca8a2a Roll Dart SDK from ad5bcf16f1c8 to d169af6f7d8f (1 revision) (flutter/engine#20192)

* 4de0c04 Roll Dart SDK from d169af6f7d8f to 7e6c55e3aaf5 (1 revision) (flutter/engine#20196)

* 908fe01 Fix navigation message relay. (flutter/engine#20193)

* f1b3b69 Roll Dart SDK from 7e6c55e3aaf5 to 365525432a70 (2 revisions) (flutter/engine#20197)

* 8fbdd3f Fix parameter names

* 083282e Fix Implments typo
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes perf: speed Performance issues related to (mostly rendering) speed severe: performance Relates to speed or footprint issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants