Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid a copy in EncodeImage #19504

Merged
merged 1 commit into from
Jul 10, 2020
Merged

Conversation

zanderso
Copy link
Member

@zanderso zanderso commented Jul 5, 2020

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 ensures that we have the only reference to the SkData and creates an external typed data object instead.

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.

@auto-assign auto-assign bot requested a review from GaryQian July 5, 2020 05:43
@zanderso zanderso force-pushed the external-encode-image branch 2 times, most recently from 859124b to 3f37b23 Compare July 6, 2020 23:16
@zanderso zanderso changed the title WIP: Avoid a copy in EncodeImage Avoid a copy in EncodeImage Jul 7, 2020
@zanderso zanderso requested review from chinmaygarde and dnfield July 7, 2020 15:26
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

nice, LGTM

@zanderso zanderso force-pushed the external-encode-image branch from 3f37b23 to f9cd4ca Compare July 7, 2020 18:34
}
FML_DCHECK(buffer->unique());
Copy link
Member

Choose a reason for hiding this comment

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

SkData buffers are immutable. It should be safe to wrap the SkData in a Dart external typed data even if someone else is also holding a reference to the SkData.

Copy link
Member Author

Choose a reason for hiding this comment

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

The writable_data() getter below will do a uniqueness check. I opted to hoist it out to here with a comment.

Copy link
Member

Choose a reason for hiding this comment

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

Skia will never write into a clients SkData buffer (I'll CC you the clarification the Skia folks made about this). I am not sure why you need to invoke writable_data() at all though. The docs for Dart_NewExternalTypedDataWithFinalizer say "A data array. This array must not move." for the data argument. So it seems like Dart won't write to it either. In that case, call data() on SkData and const_cast it to to suit the requirements of the Dart API (I am not sure why that API does not say const void * but maybe you do.

Copy link
Member Author

@zanderso zanderso Jul 7, 2020

Choose a reason for hiding this comment

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

Ah, okay. The external Uint8List is generally writable, but I can wrap it in a UnmodifiableUint8ListView (here) before returning it from the runtime call. I'll do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. ptal.

@zanderso zanderso force-pushed the external-encode-image branch 3 times, most recently from c9ada9c to b091402 Compare July 8, 2020 18:21
@zanderso zanderso force-pushed the external-encode-image branch from b091402 to 5aaf1b1 Compare July 10, 2020 15:13
@zanderso
Copy link
Member Author

Updated the Dart code to return an UnmodifiableByteDataView and updated comments to match. ptal.

@zanderso zanderso merged commit f9acd08 into flutter:master Jul 10, 2020
@zanderso zanderso deleted the external-encode-image branch July 10, 2020 21:36
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 14, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 14, 2020
jason-simmons added a commit to jason-simmons/flutter that referenced this pull request Jul 14, 2020
… views

The engine is now wrapping image byte data in an UnmodifiableByteDataView
(see flutter/engine#19504).  The golden image
comparator must make a copy before modifying the buffer.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 14, 2020
GaryQian pushed a commit to flutter/flutter that referenced this pull request Jul 14, 2020
* 160b268 Set locale in Linux shell (flutter/engine#19470)

* c479b93 Fix documentation of unset platform view ID (flutter/engine#19320)

* f3ab78d Fix clipboard paste functionality not working (flutter/engine#19489)

* 9345347 Added the ability to set properties in interface builder for FlutterViewController. (flutter/engine#19458)

* 69fdf6d Use identical in hashValues instead of operator== (flutter/engine#19615)

* 3dc8163 [android] Pass synthesized eventType to VirtualDisplay platform views and fix memory leak (flutter/engine#19620)

* b16c47d using text capitalization value in web (flutter/engine#19564)

* 49c0161 Roll Skia from 16bf7d31c819 to 4d48bb35972f (32 revisions) (flutter/engine#19635)

* 2d6afa7 Roll Skia from 4d48bb35972f to 9f821489c9f3 (4 revisions) (flutter/engine#19638)

* 35f59b9 Roll Fuchsia Linux SDK from NC9pp... to 5R9a0... (flutter/engine#19639)

* 53038e2 Roll Fuchsia Mac SDK from iwQCA... to Wym-S... (flutter/engine#19640)

* d9e68f4 Roll Skia from 9f821489c9f3 to f8a6b5b4b0d0 (1 revision) (flutter/engine#19643)

* d329617 Changes to fix build errors in google3. (flutter/engine#19616)

* dd0a6c4 Roll Skia from f8a6b5b4b0d0 to 5160e8caa226 (9 revisions) (flutter/engine#19647)

* d98d539 Manual Roll of Dart dfdc7e45c5...06cb010247 (flutter/engine#19649)

* dfd0727 don't throw error for the new autofill request (flutter/engine#19633)

* 39e98d2 Manual Skia roll to c91db040ad18b9cc3236e342e9acca020eaafd10 (flutter/engine#19650)

* f9acd08 Avoid a copy in EncodeImage (flutter/engine#19504)

* 7e101f1 Roll Fuchsia Linux SDK from 5R9a0... to GOf1K... (flutter/engine#19656)

* f3be9f1 Incorporate compat info changes into flutter engine (flutter/engine#19606)

* 9188ff6 Manual Roll of Dart 707c6404f9...dfdc7e45c5 (flutter/engine#19657)

* cbdd3e2 Roll Fuchsia Mac SDK from Wym-S... to -v4bL... (flutter/engine#19661)

* 6efb152 Roll Dart SDK from 707c6404f969 to 0e25306d3f78 (4 revisions) (flutter/engine#19666)

* 22a440c Roll Dart SDK from 0e25306d3f78 to 6d06476bae6b (1 revision) (flutter/engine#19669)

* 8063923 Roll Fuchsia Linux SDK from GOf1K... to QWI76... (flutter/engine#19671)

* c99deb0 CkPaint uses SkPaint (flutter/engine#19562)

* 015f2ea Use the main bundle if the App bundle is not found (flutter/engine#18749)

* f82d30b Roll Dart SDK from 6d06476bae6b to a9e67d81941a (3 revisions) (flutter/engine#19677)

* 61b2fd2 Roll Fuchsia Mac SDK from -v4bL... to phQJt... (flutter/engine#19681)

* 74b541d Roll Fuchsia Linux SDK from QWI76... to mrlGt... (flutter/engine#19682)

* 25ce3db Roll Skia from c91db040ad18 to e4f36d7ac8f5 (20 revisions) (flutter/engine#19686)

* 162dba1 Roll Skia from e4f36d7ac8f5 to ed15b1c39b8b (1 revision) (flutter/engine#19687)

* f97c38b Roll Fuchsia Mac SDK from phQJt... to kEVc6... (flutter/engine#19688)

* 40c4ee8 Roll Fuchsia Linux SDK from mrlGt... to YfVT0... (flutter/engine#19690)

* ec944b0 Roll Skia from ed15b1c39b8b to 1434ce1aa94d (1 revision) (flutter/engine#19692)

* 7ce988a Roll Skia from 1434ce1aa94d to f64be13cbf84 (2 revisions) (flutter/engine#19695)

* 5f7ca41 Roll Fuchsia Mac SDK from kEVc6... to hRul_... (flutter/engine#19696)

* d24549c Roll Skia from f64be13cbf84 to 041796e60364 (3 revisions) (flutter/engine#19697)

* 617bd88 Roll Dart SDK from a9e67d81941a to fecc8163afc7 (3 revisions) (flutter/engine#19703)

* 8d241e4 [Android] Prevent FlutterRenderer listener from calling JNI after detach (flutter/engine#19558)

* 309e514 Roll Skia from 041796e60364 to d1ce4cb2beb8 (2 revisions) (flutter/engine#19704)

* 211f18e Roll Skia from d1ce4cb2beb8 to 7c1967700b44 (6 revisions) (flutter/engine#19707)

* 0532227 Roll Skia from 7c1967700b44 to 439709a97dfd (8 revisions) (flutter/engine#19712)

* 1e02bfd Support decimal information on the TextInputType (flutter/engine#19664)

* 7a95e32 Linux: Use a hash table to map cursors (flutter/engine#19561)

* df23044 [web] Implement ulps for path ops (flutter/engine#19711)

* 4a3aa4d Fixes typo in android_context_gl.h (flutter/engine#19700)

* 5b966eb Roll Skia from 439709a97dfd to 2604a89d3353 (3 revisions) (flutter/engine#19715)

* d024ae4 [fuchsia] Use memory_requirements_2 extension. (flutter/engine#19678)

* 4392fbf Roll Fuchsia Linux SDK from YfVT0... to 2ct5j... (flutter/engine#19720)

* ab05c79 Roll Fuchsia Mac SDK from hRul_... to ZXLvD... (flutter/engine#19721)

* c05ca7e Roll Dart SDK from fecc8163afc7 to f997d62a6d29 (13 revisions) (flutter/engine#19722)

* ae37971 Remove xcpretty from unit tests to see full output on test failures (flutter/engine#19667)

* 91f80ef Moved to RMSE for image comparison to account for slight variations in golden image tests (flutter/engine#19658)

* 1e23309 Wait for platform view to appear in iOS UI tests (flutter/engine#19725)
zanderso added a commit to zanderso/engine that referenced this pull request Jul 14, 2020
zanderso added a commit that referenced this pull request Jul 14, 2020
Pragya007 pushed a commit to Pragya007/flutter that referenced this pull request Aug 11, 2020
* 160b268 Set locale in Linux shell (flutter/engine#19470)

* c479b93 Fix documentation of unset platform view ID (flutter/engine#19320)

* f3ab78d Fix clipboard paste functionality not working (flutter/engine#19489)

* 9345347 Added the ability to set properties in interface builder for FlutterViewController. (flutter/engine#19458)

* 69fdf6d Use identical in hashValues instead of operator== (flutter/engine#19615)

* 3dc8163 [android] Pass synthesized eventType to VirtualDisplay platform views and fix memory leak (flutter/engine#19620)

* b16c47d using text capitalization value in web (flutter/engine#19564)

* 49c0161 Roll Skia from 16bf7d31c819 to 4d48bb35972f (32 revisions) (flutter/engine#19635)

* 2d6afa7 Roll Skia from 4d48bb35972f to 9f821489c9f3 (4 revisions) (flutter/engine#19638)

* 35f59b9 Roll Fuchsia Linux SDK from NC9pp... to 5R9a0... (flutter/engine#19639)

* 53038e2 Roll Fuchsia Mac SDK from iwQCA... to Wym-S... (flutter/engine#19640)

* d9e68f4 Roll Skia from 9f821489c9f3 to f8a6b5b4b0d0 (1 revision) (flutter/engine#19643)

* d329617 Changes to fix build errors in google3. (flutter/engine#19616)

* dd0a6c4 Roll Skia from f8a6b5b4b0d0 to 5160e8caa226 (9 revisions) (flutter/engine#19647)

* d98d539 Manual Roll of Dart dfdc7e45c5...06cb010247 (flutter/engine#19649)

* dfd0727 don't throw error for the new autofill request (flutter/engine#19633)

* 39e98d2 Manual Skia roll to c91db040ad18b9cc3236e342e9acca020eaafd10 (flutter/engine#19650)

* f9acd08 Avoid a copy in EncodeImage (flutter/engine#19504)

* 7e101f1 Roll Fuchsia Linux SDK from 5R9a0... to GOf1K... (flutter/engine#19656)

* f3be9f1 Incorporate compat info changes into flutter engine (flutter/engine#19606)

* 9188ff6 Manual Roll of Dart 707c6404f9...dfdc7e45c5 (flutter/engine#19657)

* cbdd3e2 Roll Fuchsia Mac SDK from Wym-S... to -v4bL... (flutter/engine#19661)

* 6efb152 Roll Dart SDK from 707c6404f969 to 0e25306d3f78 (4 revisions) (flutter/engine#19666)

* 22a440c Roll Dart SDK from 0e25306d3f78 to 6d06476bae6b (1 revision) (flutter/engine#19669)

* 8063923 Roll Fuchsia Linux SDK from GOf1K... to QWI76... (flutter/engine#19671)

* c99deb0 CkPaint uses SkPaint (flutter/engine#19562)

* 015f2ea Use the main bundle if the App bundle is not found (flutter/engine#18749)

* f82d30b Roll Dart SDK from 6d06476bae6b to a9e67d81941a (3 revisions) (flutter/engine#19677)

* 61b2fd2 Roll Fuchsia Mac SDK from -v4bL... to phQJt... (flutter/engine#19681)

* 74b541d Roll Fuchsia Linux SDK from QWI76... to mrlGt... (flutter/engine#19682)

* 25ce3db Roll Skia from c91db040ad18 to e4f36d7ac8f5 (20 revisions) (flutter/engine#19686)

* 162dba1 Roll Skia from e4f36d7ac8f5 to ed15b1c39b8b (1 revision) (flutter/engine#19687)

* f97c38b Roll Fuchsia Mac SDK from phQJt... to kEVc6... (flutter/engine#19688)

* 40c4ee8 Roll Fuchsia Linux SDK from mrlGt... to YfVT0... (flutter/engine#19690)

* ec944b0 Roll Skia from ed15b1c39b8b to 1434ce1aa94d (1 revision) (flutter/engine#19692)

* 7ce988a Roll Skia from 1434ce1aa94d to f64be13cbf84 (2 revisions) (flutter/engine#19695)

* 5f7ca41 Roll Fuchsia Mac SDK from kEVc6... to hRul_... (flutter/engine#19696)

* d24549c Roll Skia from f64be13cbf84 to 041796e60364 (3 revisions) (flutter/engine#19697)

* 617bd88 Roll Dart SDK from a9e67d81941a to fecc8163afc7 (3 revisions) (flutter/engine#19703)

* 8d241e4 [Android] Prevent FlutterRenderer listener from calling JNI after detach (flutter/engine#19558)

* 309e514 Roll Skia from 041796e60364 to d1ce4cb2beb8 (2 revisions) (flutter/engine#19704)

* 211f18e Roll Skia from d1ce4cb2beb8 to 7c1967700b44 (6 revisions) (flutter/engine#19707)

* 0532227 Roll Skia from 7c1967700b44 to 439709a97dfd (8 revisions) (flutter/engine#19712)

* 1e02bfd Support decimal information on the TextInputType (flutter/engine#19664)

* 7a95e32 Linux: Use a hash table to map cursors (flutter/engine#19561)

* df23044 [web] Implement ulps for path ops (flutter/engine#19711)

* 4a3aa4d Fixes typo in android_context_gl.h (flutter/engine#19700)

* 5b966eb Roll Skia from 439709a97dfd to 2604a89d3353 (3 revisions) (flutter/engine#19715)

* d024ae4 [fuchsia] Use memory_requirements_2 extension. (flutter/engine#19678)

* 4392fbf Roll Fuchsia Linux SDK from YfVT0... to 2ct5j... (flutter/engine#19720)

* ab05c79 Roll Fuchsia Mac SDK from hRul_... to ZXLvD... (flutter/engine#19721)

* c05ca7e Roll Dart SDK from fecc8163afc7 to f997d62a6d29 (13 revisions) (flutter/engine#19722)

* ae37971 Remove xcpretty from unit tests to see full output on test failures (flutter/engine#19667)

* 91f80ef Moved to RMSE for image comparison to account for slight variations in golden image tests (flutter/engine#19658)

* 1e23309 Wait for platform view to appear in iOS UI tests (flutter/engine#19725)
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
* 160b268 Set locale in Linux shell (flutter/engine#19470)

* c479b93 Fix documentation of unset platform view ID (flutter/engine#19320)

* f3ab78d Fix clipboard paste functionality not working (flutter/engine#19489)

* 9345347 Added the ability to set properties in interface builder for FlutterViewController. (flutter/engine#19458)

* 69fdf6d Use identical in hashValues instead of operator== (flutter/engine#19615)

* 3dc8163 [android] Pass synthesized eventType to VirtualDisplay platform views and fix memory leak (flutter/engine#19620)

* b16c47d using text capitalization value in web (flutter/engine#19564)

* 49c0161 Roll Skia from 16bf7d31c819 to 4d48bb35972f (32 revisions) (flutter/engine#19635)

* 2d6afa7 Roll Skia from 4d48bb35972f to 9f821489c9f3 (4 revisions) (flutter/engine#19638)

* 35f59b9 Roll Fuchsia Linux SDK from NC9pp... to 5R9a0... (flutter/engine#19639)

* 53038e2 Roll Fuchsia Mac SDK from iwQCA... to Wym-S... (flutter/engine#19640)

* d9e68f4 Roll Skia from 9f821489c9f3 to f8a6b5b4b0d0 (1 revision) (flutter/engine#19643)

* d329617 Changes to fix build errors in google3. (flutter/engine#19616)

* dd0a6c4 Roll Skia from f8a6b5b4b0d0 to 5160e8caa226 (9 revisions) (flutter/engine#19647)

* d98d539 Manual Roll of Dart dfdc7e45c5...06cb010247 (flutter/engine#19649)

* dfd0727 don't throw error for the new autofill request (flutter/engine#19633)

* 39e98d2 Manual Skia roll to c91db040ad18b9cc3236e342e9acca020eaafd10 (flutter/engine#19650)

* f9acd08 Avoid a copy in EncodeImage (flutter/engine#19504)

* 7e101f1 Roll Fuchsia Linux SDK from 5R9a0... to GOf1K... (flutter/engine#19656)

* f3be9f1 Incorporate compat info changes into flutter engine (flutter/engine#19606)

* 9188ff6 Manual Roll of Dart 707c6404f9...dfdc7e45c5 (flutter/engine#19657)

* cbdd3e2 Roll Fuchsia Mac SDK from Wym-S... to -v4bL... (flutter/engine#19661)

* 6efb152 Roll Dart SDK from 707c6404f969 to 0e25306d3f78 (4 revisions) (flutter/engine#19666)

* 22a440c Roll Dart SDK from 0e25306d3f78 to 6d06476bae6b (1 revision) (flutter/engine#19669)

* 8063923 Roll Fuchsia Linux SDK from GOf1K... to QWI76... (flutter/engine#19671)

* c99deb0 CkPaint uses SkPaint (flutter/engine#19562)

* 015f2ea Use the main bundle if the App bundle is not found (flutter/engine#18749)

* f82d30b Roll Dart SDK from 6d06476bae6b to a9e67d81941a (3 revisions) (flutter/engine#19677)

* 61b2fd2 Roll Fuchsia Mac SDK from -v4bL... to phQJt... (flutter/engine#19681)

* 74b541d Roll Fuchsia Linux SDK from QWI76... to mrlGt... (flutter/engine#19682)

* 25ce3db Roll Skia from c91db040ad18 to e4f36d7ac8f5 (20 revisions) (flutter/engine#19686)

* 162dba1 Roll Skia from e4f36d7ac8f5 to ed15b1c39b8b (1 revision) (flutter/engine#19687)

* f97c38b Roll Fuchsia Mac SDK from phQJt... to kEVc6... (flutter/engine#19688)

* 40c4ee8 Roll Fuchsia Linux SDK from mrlGt... to YfVT0... (flutter/engine#19690)

* ec944b0 Roll Skia from ed15b1c39b8b to 1434ce1aa94d (1 revision) (flutter/engine#19692)

* 7ce988a Roll Skia from 1434ce1aa94d to f64be13cbf84 (2 revisions) (flutter/engine#19695)

* 5f7ca41 Roll Fuchsia Mac SDK from kEVc6... to hRul_... (flutter/engine#19696)

* d24549c Roll Skia from f64be13cbf84 to 041796e60364 (3 revisions) (flutter/engine#19697)

* 617bd88 Roll Dart SDK from a9e67d81941a to fecc8163afc7 (3 revisions) (flutter/engine#19703)

* 8d241e4 [Android] Prevent FlutterRenderer listener from calling JNI after detach (flutter/engine#19558)

* 309e514 Roll Skia from 041796e60364 to d1ce4cb2beb8 (2 revisions) (flutter/engine#19704)

* 211f18e Roll Skia from d1ce4cb2beb8 to 7c1967700b44 (6 revisions) (flutter/engine#19707)

* 0532227 Roll Skia from 7c1967700b44 to 439709a97dfd (8 revisions) (flutter/engine#19712)

* 1e02bfd Support decimal information on the TextInputType (flutter/engine#19664)

* 7a95e32 Linux: Use a hash table to map cursors (flutter/engine#19561)

* df23044 [web] Implement ulps for path ops (flutter/engine#19711)

* 4a3aa4d Fixes typo in android_context_gl.h (flutter/engine#19700)

* 5b966eb Roll Skia from 439709a97dfd to 2604a89d3353 (3 revisions) (flutter/engine#19715)

* d024ae4 [fuchsia] Use memory_requirements_2 extension. (flutter/engine#19678)

* 4392fbf Roll Fuchsia Linux SDK from YfVT0... to 2ct5j... (flutter/engine#19720)

* ab05c79 Roll Fuchsia Mac SDK from hRul_... to ZXLvD... (flutter/engine#19721)

* c05ca7e Roll Dart SDK from fecc8163afc7 to f997d62a6d29 (13 revisions) (flutter/engine#19722)

* ae37971 Remove xcpretty from unit tests to see full output on test failures (flutter/engine#19667)

* 91f80ef Moved to RMSE for image comparison to account for slight variations in golden image tests (flutter/engine#19658)

* 1e23309 Wait for platform view to appear in iOS UI tests (flutter/engine#19725)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants