In a single frame codec, release the encoded image buffer after giving it to the decoder #9825

jason-simmons · 2019-07-15T17:07:51Z

The codec will retain a reference to the decoded frame so it can be returned
in repeated calls to getNextFrame.

mklim

New logic LGTM. Could this be tested at all?

mklim · 2019-07-15T17:21:38Z

lib/ui/painting/single_frame_codec.cc

  const auto& data = descriptor_.data;
  auto data_byte_size = data ? data->size() : 0;
-  return data_byte_size + sizeof(this);
+  auto frame_byte_size =


nit:

Suggested change

auto frame_byte_size =

const auto frame_byte_size =

chinmaygarde

Can you add a unit-test for this in image_decoding_unittests.cc in the ui_unittests harness? You should be able to just duplicate TEST_F(ImageDecoderFixtureTest, ValidImageResultsInSuccess) and request the same image multiple (say 10) times. Thanks.

chinmaygarde · 2019-07-15T18:53:15Z

lib/ui/painting/single_frame_codec.h


 private:
  ImageDecoder::ImageDescriptor descriptor_;
+  fml::RefPtr<FrameInfo> frame_;


cached_frame_ maybe to indicate that this is not something that is implicitly created?

jason-simmons · 2019-07-16T23:00:33Z

Changed this to use anonymous mappings to hold compressed image buffers. This is a workaround for Android malloc multithreading behavior.

@chinmaygarde PTAL

chinmaygarde · 2019-07-16T23:11:59Z

lib/ui/painting/codec.cc

+  void* mapping = ::mmap(nullptr, mapping_length, PROT_READ | PROT_WRITE,
+                         MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+
+  FML_CHECK(mapping != MAP_FAILED);


I think we should return a nullptr SkData here to indicate OOM. We can check for a nullptr in the caller to return an error to Dart code.

chinmaygarde · 2019-07-16T23:13:01Z

lib/ui/painting/codec.cc

+
+  SkData::ReleaseProc proc = [](const void* ptr, void* context) {
+    size_t map_size = *reinterpret_cast<size_t*>(context);
+    ::munmap(const_cast<void*>(context), map_size);


Can we FML_LOG(ERROR) here in case of unmap failures?

chinmaygarde · 2019-07-16T23:14:15Z

lib/ui/painting/codec.cc

+// different from the freeing thread.  To work around this, create an SkData
+// backed by an anonymous mapping.
+sk_sp<SkData> MakeSkDataWithCopy(const void* data, size_t length) {
+  size_t mapping_length = length + sizeof(size_t);


Check for zero sized mappings here. I think those will cause EINVALs.

chinmaygarde · 2019-07-16T23:17:06Z

lib/ui/painting/codec.cc

+  ::memcpy(mapping_data, data, length);
+
+  SkData::ReleaseProc proc = [](const void* ptr, void* context) {
+    size_t map_size = *reinterpret_cast<size_t*>(context);


I'd feel more comfortable with a DCHECK here that checks context + sizeof(size_t) is the ptr here.

chinmaygarde · 2019-07-16T23:19:38Z

lib/ui/painting/single_frame_codec.h

  size_t GetAllocationSize() override;

 private:
+  enum { STATUS_NEW, STATUS_IN_PROGRESS, STATUS_COMPLETE } status_;


We follow the enum names as constants pattern as described here https://google.github.io/styleguide/cppguide.html#Enumerator_Names. Please use an enum class instead.

So,

enum class Status { kNew, kInProgress, kComplete, };

…ving it to the decoder The codec will retain a reference to the decoded frame so it can be returned in repeated calls to getNextFrame. Also change the compressed image buffer to use an anonymous mapping on Android. This is a workaround for high consumption of native heap pages by Android's malloc when buffers are allocated on one thread and freed on another thread. See flutter/flutter#36079

jason-simmons · 2019-07-16T23:46:47Z

Done - PTAL

…after giving it to the decoder (flutter/engine#9825)

flutter/engine@f348270...5467f6f git log f348270..5467f6f --no-merges --oneline 5467f6f Roll src/third_party/skia bf1d4effe171..19547c91b983 (4 commits) (flutter/engine#9942) 15797da Roll src/third_party/skia de71a74fc40e..bf1d4effe171 (5 commits) (flutter/engine#9940) db0839a Copy over fuchsia dev key from buildroot (flutter/engine#9936) 934e225 Build fuchsia artifacts from the engine (flutter/engine#9933) 9e04776 Update the exception thrown for invalid data in the codec test (flutter/engine#9929) 678ce2e Fix reentrancy handling in SingleFrameCodec (flutter/engine#9931) 9bb1b89 Update libcxx & libcxxabi to HEAD in prep for compiler upgrade. (flutter/engine#9906) cad5cc2 Roll src/third_party/skia 3e6aa9f52839..de71a74fc40e (11 commits) (flutter/engine#9930) 5ed71f1 Handle decompressed images in InstantiateImageCodec (flutter/engine#9901) ad5ae0f Update Buildroot Version (flutter/engine#9927) f20e935 Fix failure of the onReportTimings window hook test (flutter/engine#9923) eaf1f33 Don't try to use unset assets_dir setting (flutter/engine#9924) ae14f04 Add the isMultiline semantics flag to values (flutter/engine#9894) cf3fd6d Roll src/third_party/skia 83cfe4fa24d9..3e6aa9f52839 (6 commits) (flutter/engine#9921) cf40c24 Removed unused method. (flutter/engine#9919) 7bd8fc3 Roll fuchsia/sdk/core/mac-amd64 from GcUOj20BDDGW4Sz4cnsI4_Lf9qte_6OCgQBmDQLnGNcC to xNAaLqZJk8Bkz00BaHGzE8hCpiohggO7KabM3g2wdsQC (flutter/engine#9918) 68ae872 Made the persistent cache's directory a const pointer. (flutter/engine#9815) 8720043 Roll src/third_party/skia a2e7d5e2b63c..83cfe4fa24d9 (3 commits) (flutter/engine#9916) b28ccd8 Roll fuchsia/sdk/core/mac-amd64 from 9XAYIkrdh9JQjy22gyni7VbK2yYurszww1k9zRQ_jU8C to GcUOj20BDDGW4Sz4cnsI4_Lf9qte_6OCgQBmDQLnGNcC (flutter/engine#9915) 89a9a95 Roll fuchsia/sdk/core/mac-amd64 from lXCuLh2YGWM641A5Io3ASt3Uy70e_YGRKFLf46new08C to 9XAYIkrdh9JQjy22gyni7VbK2yYurszww1k9zRQ_jU8C (flutter/engine#9911) c8f35b9 Roll src/third_party/skia ea6da6909624..a2e7d5e2b63c (3 commits) (flutter/engine#9910) 8704d61 Roll src/third_party/dart 6bf1f8e280..63120303a7 (4 commits) 866d057 Roll src/third_party/dart 0506882b37..6bf1f8e280 (9 commits) 7289354 Roll src/third_party/dart 8cb7e4c237..0506882b37 (3 commits) d84b938 Roll src/third_party/dart 41d3971e83..8cb7e4c237 (2 commits) 52b226c Roll src/third_party/dart 2b3336b51e..41d3971e83 (3 commits) cf4129b Roll fuchsia/sdk/core/mac-amd64 from 0NcHg3_AYcxrkseoO6xmXrQ-GZ82gy8CE5NU-SDJq_QC to lXCuLh2YGWM641A5Io3ASt3Uy70e_YGRKFLf46new08C (flutter/engine#9908) c2133b4 Roll src/third_party/skia d7639aff1001..ea6da6909624 (7 commits) (flutter/engine#9907) ca91c66 Roll fuchsia/sdk/core/mac-amd64 from JDPk4JFZX16IXpzzjQH5KFf0vRALbOtJYiMHCqFLFOQC to 0NcHg3_AYcxrkseoO6xmXrQ-GZ82gy8CE5NU-SDJq_QC (flutter/engine#9904) fd2cb81 Respect EXIF information while decompressing images. (flutter/engine#9905) dd06cda Fix justify for RTL paragraphs. (flutter/engine#9859) be3e2ed Fix fuchsia license detection (flutter/engine#9857) b7b791b In a single frame codec, release the compressed image buffer after giving it to the decoder (flutter/engine#9825) 1af19ae Roll fuchsia/sdk/core/mac-amd64 from PHtpiJGexJFgd7sgPTUbFphKES09fzotmtrO2kTHI08C to JDPk4JFZX16IXpzzjQH5KFf0vRALbOtJYiMHCqFLFOQC (flutter/engine#9892) 5d9f7b1 Log dlopen errors only in debug mode (flutter/engine#9890) 8f060b9 Add clang version to Info.plist (flutter/engine#9873) 0fcf3b3 Roll src/third_party/skia e574f1e409aa..d7639aff1001 (16 commits) (flutter/engine#9889) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary. ...

flutter/engine@f348270...5467f6f git log f348270..5467f6f --no-merges --oneline 5467f6f Roll src/third_party/skia bf1d4effe171..19547c91b983 (4 commits) (flutter/engine#9942) 15797da Roll src/third_party/skia de71a74fc40e..bf1d4effe171 (5 commits) (flutter/engine#9940) db0839a Copy over fuchsia dev key from buildroot (flutter/engine#9936) 934e225 Build fuchsia artifacts from the engine (flutter/engine#9933) 9e04776 Update the exception thrown for invalid data in the codec test (flutter/engine#9929) 678ce2e Fix reentrancy handling in SingleFrameCodec (flutter/engine#9931) 9bb1b89 Update libcxx & libcxxabi to HEAD in prep for compiler upgrade. (flutter/engine#9906) cad5cc2 Roll src/third_party/skia 3e6aa9f52839..de71a74fc40e (11 commits) (flutter/engine#9930) 5ed71f1 Handle decompressed images in InstantiateImageCodec (flutter/engine#9901) ad5ae0f Update Buildroot Version (flutter/engine#9927) f20e935 Fix failure of the onReportTimings window hook test (flutter/engine#9923) eaf1f33 Don&flutter#39;t try to use unset assets_dir setting (flutter/engine#9924) ae14f04 Add the isMultiline semantics flag to values (flutter/engine#9894) cf3fd6d Roll src/third_party/skia 83cfe4fa24d9..3e6aa9f52839 (6 commits) (flutter/engine#9921) cf40c24 Removed unused method. (flutter/engine#9919) 7bd8fc3 Roll fuchsia/sdk/core/mac-amd64 from GcUOj20BDDGW4Sz4cnsI4_Lf9qte_6OCgQBmDQLnGNcC to xNAaLqZJk8Bkz00BaHGzE8hCpiohggO7KabM3g2wdsQC (flutter/engine#9918) 68ae872 Made the persistent cache&flutter#39;s directory a const pointer. (flutter/engine#9815) 8720043 Roll src/third_party/skia a2e7d5e2b63c..83cfe4fa24d9 (3 commits) (flutter/engine#9916) b28ccd8 Roll fuchsia/sdk/core/mac-amd64 from 9XAYIkrdh9JQjy22gyni7VbK2yYurszww1k9zRQ_jU8C to GcUOj20BDDGW4Sz4cnsI4_Lf9qte_6OCgQBmDQLnGNcC (flutter/engine#9915) 89a9a95 Roll fuchsia/sdk/core/mac-amd64 from lXCuLh2YGWM641A5Io3ASt3Uy70e_YGRKFLf46new08C to 9XAYIkrdh9JQjy22gyni7VbK2yYurszww1k9zRQ_jU8C (flutter/engine#9911) c8f35b9 Roll src/third_party/skia ea6da6909624..a2e7d5e2b63c (3 commits) (flutter/engine#9910) 8704d61 Roll src/third_party/dart 6bf1f8e280..63120303a7 (4 commits) 866d057 Roll src/third_party/dart 0506882b37..6bf1f8e280 (9 commits) 7289354 Roll src/third_party/dart 8cb7e4c237..0506882b37 (3 commits) d84b938 Roll src/third_party/dart 41d3971e83..8cb7e4c237 (2 commits) 52b226c Roll src/third_party/dart 2b3336b51e..41d3971e83 (3 commits) cf4129b Roll fuchsia/sdk/core/mac-amd64 from 0NcHg3_AYcxrkseoO6xmXrQ-GZ82gy8CE5NU-SDJq_QC to lXCuLh2YGWM641A5Io3ASt3Uy70e_YGRKFLf46new08C (flutter/engine#9908) c2133b4 Roll src/third_party/skia d7639aff1001..ea6da6909624 (7 commits) (flutter/engine#9907) ca91c66 Roll fuchsia/sdk/core/mac-amd64 from JDPk4JFZX16IXpzzjQH5KFf0vRALbOtJYiMHCqFLFOQC to 0NcHg3_AYcxrkseoO6xmXrQ-GZ82gy8CE5NU-SDJq_QC (flutter/engine#9904) fd2cb81 Respect EXIF information while decompressing images. (flutter/engine#9905) dd06cda Fix justify for RTL paragraphs. (flutter/engine#9859) be3e2ed Fix fuchsia license detection (flutter/engine#9857) b7b791b In a single frame codec, release the compressed image buffer after giving it to the decoder (flutter/engine#9825) 1af19ae Roll fuchsia/sdk/core/mac-amd64 from PHtpiJGexJFgd7sgPTUbFphKES09fzotmtrO2kTHI08C to JDPk4JFZX16IXpzzjQH5KFf0vRALbOtJYiMHCqFLFOQC (flutter/engine#9892) 5d9f7b1 Log dlopen errors only in debug mode (flutter/engine#9890) 8f060b9 Add clang version to Info.plist (flutter/engine#9873) 0fcf3b3 Roll src/third_party/skia e574f1e409aa..d7639aff1001 (16 commits) (flutter/engine#9889) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary. ...

googlebot added the cla: yes label Jul 15, 2019

jason-simmons requested a review from chinmaygarde July 15, 2019 17:08

mklim reviewed Jul 15, 2019

View reviewed changes

chinmaygarde approved these changes Jul 15, 2019

View reviewed changes

jason-simmons force-pushed the bug_36079 branch from 5d19e77 to e1105d6 Compare July 16, 2019 22:57

chinmaygarde reviewed Jul 16, 2019

View reviewed changes

jason-simmons force-pushed the bug_36079 branch from e1105d6 to 9fc16a2 Compare July 16, 2019 23:46

chinmaygarde approved these changes Jul 16, 2019

View reviewed changes

jason-simmons merged commit b7b791b into flutter:master Jul 17, 2019

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 17, 2019

b7b791b In a single frame codec, release the compressed image buffer …

614fa3e

…after giving it to the decoder (flutter/engine#9825)

engine-flutter-autoroll mentioned this pull request Jul 17, 2019

Roll engine 1af19ae67dd1..dd06cda54bf7 (3 commits) flutter/flutter#36406

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 18, 2019

b7b791b In a single frame codec, release the compressed image buffer …

71f373f

…after giving it to the decoder (flutter/engine#9825)

engine-flutter-autoroll mentioned this pull request Jul 18, 2019

Roll engine f3482700474a..68ae872e22d3 (21 commits) flutter/flutter#36467

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 18, 2019

b7b791b In a single frame codec, release the compressed image buffer …

9f4155a

…after giving it to the decoder (flutter/engine#9825)

engine-flutter-autoroll mentioned this pull request Jul 18, 2019

Roll engine f3482700474a..7bd8fc3177c9 (22 commits) flutter/flutter#36472

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 18, 2019

b7b791b In a single frame codec, release the compressed image buffer …

8349c25

…after giving it to the decoder (flutter/engine#9825)

engine-flutter-autoroll mentioned this pull request Jul 18, 2019

Roll engine f3482700474a..cf40c243a023 (23 commits) flutter/flutter#36480

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 18, 2019

b7b791b In a single frame codec, release the compressed image buffer …

826f80d

…after giving it to the decoder (flutter/engine#9825)

engine-flutter-autoroll mentioned this pull request Jul 18, 2019

Roll engine f3482700474a..cf3fd6df0bbc (24 commits) flutter/flutter#36483

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 18, 2019

b7b791b In a single frame codec, release the compressed image buffer …

ba9f18d

…after giving it to the decoder (flutter/engine#9825)

engine-flutter-autoroll mentioned this pull request Jul 18, 2019

Roll engine f3482700474a..ae14f04613f9 (25 commits) flutter/flutter#36489

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 18, 2019

b7b791b In a single frame codec, release the compressed image buffer …

9d513fa

…after giving it to the decoder (flutter/engine#9825)

engine-flutter-autoroll mentioned this pull request Jul 18, 2019

Roll engine f3482700474a..eaf1f33e5692 (26 commits) flutter/flutter#36495

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 18, 2019

b7b791b In a single frame codec, release the compressed image buffer …

bd3770d

…after giving it to the decoder (flutter/engine#9825)

engine-flutter-autoroll mentioned this pull request Jul 18, 2019

Roll engine f3482700474a..5ed71f19b525 (29 commits) flutter/flutter#36497

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 18, 2019

b7b791b In a single frame codec, release the compressed image buffer …

a168921

…after giving it to the decoder (flutter/engine#9825)

engine-flutter-autoroll mentioned this pull request Jul 18, 2019

Roll engine f3482700474a..cad5cc29623b (30 commits) flutter/flutter#36500

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 19, 2019

b7b791b In a single frame codec, release the compressed image buffer …

1c4353f

…after giving it to the decoder (flutter/engine#9825)

engine-flutter-autoroll mentioned this pull request Jul 19, 2019

Roll engine f3482700474a..9e0477643ec5 (33 commits) flutter/flutter#36502

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 19, 2019

b7b791b In a single frame codec, release the compressed image buffer …

3350cdc

…after giving it to the decoder (flutter/engine#9825)

engine-flutter-autoroll mentioned this pull request Jul 19, 2019

Roll engine f3482700474a..934e22571c22 (34 commits) flutter/flutter#36504

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 19, 2019

b7b791b In a single frame codec, release the compressed image buffer …

a7cf857

…after giving it to the decoder (flutter/engine#9825)

engine-flutter-autoroll mentioned this pull request Jul 19, 2019

Roll engine f3482700474a..db0839ab92c6 (35 commits) flutter/flutter#36508

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 19, 2019

b7b791b In a single frame codec, release the compressed image buffer …

90e1b60

…after giving it to the decoder (flutter/engine#9825)

This was referenced Jul 19, 2019

Roll engine f3482700474a..15797da86fa2 (36 commits) flutter/flutter#36514

Closed

Roll engine f3482700474a..5467f6f63046 (37 commits) flutter/flutter#36521

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 19, 2019

b7b791b In a single frame codec, release the compressed image buffer …

41ed739

…after giving it to the decoder (flutter/engine#9825)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

In a single frame codec, release the encoded image buffer after giving it to the decoder #9825

In a single frame codec, release the encoded image buffer after giving it to the decoder #9825

Uh oh!

jason-simmons commented Jul 15, 2019

Uh oh!

mklim left a comment

Uh oh!

mklim Jul 15, 2019

Uh oh!

chinmaygarde left a comment

Uh oh!

chinmaygarde Jul 15, 2019

Uh oh!

jason-simmons commented Jul 16, 2019

Uh oh!

chinmaygarde Jul 16, 2019

Uh oh!

chinmaygarde Jul 16, 2019

Uh oh!

chinmaygarde Jul 16, 2019

Uh oh!

chinmaygarde Jul 16, 2019

Uh oh!

chinmaygarde Jul 16, 2019

Uh oh!

jason-simmons commented Jul 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

In a single frame codec, release the encoded image buffer after giving it to the decoder #9825

In a single frame codec, release the encoded image buffer after giving it to the decoder #9825

Uh oh!

Conversation

jason-simmons commented Jul 15, 2019

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

mklim Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

jason-simmons commented Jul 16, 2019

Uh oh!

chinmaygarde Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

jason-simmons commented Jul 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants