Refactor instantiateImageCodec/decodeImageFromPixels to provide an ImageDescriptor by dnfield · Pull Request #19537 · flutter-team-archive/engine

dnfield · 2020-07-06T18:50:01Z

Description

Refactors instantiateImageCodec and decodeImageFromPixels to provide intermediate data about the image width, height, and bytes per pixel. This allows for more fine grained control from Dart, particularly when trying to reason about how or whether to maintain aspect ratio in the targetWidth/targetHeight parameters.

This leaves the existing methods functional by re-implementing them to use the new ImmutableBuffer/ImageDescriptor class.

The ImmutableBuffer class is provided so that callers can avoid copying the image data multiple times if they wish to re-recreate the image descriptor.

Related Issues

#19385
flutter/flutter#59578

Tests

I added the following tests:

Tests to exercise the new classes/behaviors.

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.

dnfield · 2020-07-06T19:56:20Z

+  final PixelFormat? _format;
+
+  /// The width, in pixels, of the image.
+  int get width => _width != null ? _width : throw UnsupportedError('ImageDescriptor.width is not supported on web.');


@jonahwilliams - is there some way we can implement these? Should I just be filing a bug?

There is no way these can be implemented on the html backend as far as I know. Fyi @yjbanov

For the canvaskit backend, it might be feasible? @hterkelsen

chinmaygarde · 2020-07-06T19:24:51Z

+    }
+  }
+  return descriptor.instantiateCodec(
+    targetWidth: targetWidth ?? _kDoNotResizeDimension,


IMO, this API can be made even simpler if the engine mandates that the target width and height can never be unspecified. So, instead of _kDoNotResizeDimension, return descriptor.width or height since we now have this information in Dart code already. You can then remove _kDoNotResizeDimension and all the places where the engine has to deal with optionals.

Ditto elsewhere on all uses of _kDoNotResizeDimension.

To be clear - this means we always check for GetResizedDimension, and that we'd want to remove the std::optionalness on the C++ side?

Right. There is already a fast path where the resized dimensions are the same as the intrinsic dimensions.

Ok. This turned out to be a bit of a hassle but I think it's better this way. The aspect ratio code that used to live in C++ now lives in Dart.

chinmaygarde · 2020-07-06T19:34:16Z

+  final int length;
+
+  /// Retreives a single byte from the underlying data.
+  int operator [](int index) => elementAt(index);


Is there a current use case for this? The caller can already work on the source buffer before creating the immutable variant. Keeping it super simple, I would just make this an opaque handle for now. We can add to it as the use cases arise. The engine can get more creative with where it puts the buffer if it doesn't have to worry about also providing synchronous access pixel data.

No, I can remove this.

chinmaygarde · 2020-07-06T19:40:25Z

+
+  /// Creates a [Codec] object which is suitable for decoding the data in the
+  /// buffer to an [Image].
+  Future<Codec> instantiateCodec({int? targetWidth, int? targetHeight, bool disposeBuffer = true}) {


Please document disposeBuffer and its semantics.

Also, it's not used so maybe patch that too :)

That was meant to be removed - it's been removed in the latest commit.

Sound good. I think I submitted the review after you amended the patch and I was confused where the comment thread went.

chinmaygarde · 2020-07-06T19:47:27Z

+  /// pixels in the data buffer. If unspecified, it defaults to `width` multiplied
+  /// by the number of bytes per pixel in the provided `format`.
+  // Not async because there's no expensive work to do here.
+  ImageDescriptor.raw(


Minor nit: Is it conventional for "raw" to mean from decompressed image data. I always found that terminology to be a bit terse. How about "imageData"? Either way is fine. Whatever you think is clearer.

How about decompressed?

Or just pixels?

I like pixels. But really, it was a just a minor observation. Fine either way.

chinmaygarde · 2020-07-06T19:55:07Z

+
+namespace flutter {
+
+class ImageDescriptor : public RefCountedDartWrappable<ImageDescriptor> {


Header docs here please. Specifically, descriptors on other platforms/APIs are extremely lightweight objects. They usually they just "describe" a thing but don't own it necessarily. This is not the case with this implementation. I'd make that clear upfront. Other stuff is mostly self explanatory.

On that note, maybe there is a better a better name for this class since it owns the underlying allocation as well as describing it. ImageCodec maybe?

I think that's confusing given we already have Codec and SingleFrameCodec/MultiFrameCodec. It's also a bit confusing to have ImageCodec::instantiateCodec.

I can add the docs. I can't think of a good way to make this object more light weight than it is, or to avoid retaining the underlying data for it.

chinmaygarde · 2020-07-06T20:23:12Z

+  }
+
+  size_t GetAllocationSize() const override {
+    return sizeof(ImmutableBuffer) + (data_->size() * 4);


The * 4 is inaccurate. The data size is already in bytes.

chinmaygarde · 2020-07-06T20:23:28Z

+    return data_->size();
+  }
+
+  int elementAt(size_t index) const {


See my comment about this being unnecessary.

chinmaygarde · 2020-07-06T20:24:26Z

+#include "third_party/tonic/typed_data/typed_list.h"
+
+namespace flutter {
+class ImmutableBuffer : public RefCountedDartWrappable<ImmutableBuffer> {


Header docs.

chinmaygarde · 2020-07-06T20:24:45Z

+namespace flutter {
+class ImmutableBuffer : public RefCountedDartWrappable<ImmutableBuffer> {
+ public:
+  ~ImmutableBuffer() override = default;


OOL this please. This applies to all virtual methods elsewhere as well.

chinmaygarde · 2020-07-06T20:26:21Z

+
+  static void init(Dart_NativeArguments args);
+
+  int length() const {


size_t length() const;

zanderso · 2020-07-06T21:06:45Z

+  void _initRaw(ImageDescriptor outDescriptor, ImmutableBuffer buffer, int width, int height, int rowBytes, int pixelFormat) native 'ImageDescriptor_initRaw';
+
+  /// The width, in pixels, of the image.
+  int get width native 'ImageDescriptor_width';


Can width, height, or bytesPerPixel change? If not, then you can cache them in Dart fields, which will have faster access than a runtime call.

int? _width; int get width => _width ??= _getWidth(); int _getWidth() native 'ImageDescriptor_width';

These are immutable - we can cache them.

Do we have any benchmarks demonstrating this is faster though? At one point I thought I measured this and it didn't make a big difference.

Either way, done.

I doubt we have any benchmarks doing an A/B comparison, but in general the difference is a null check + load (3-4 instructions) vs. saving live registers, doing the runtime call, looking up the native call by string, setting up handle and allocation arenas in the runtime, etc., etc.., and then popping back out undoing all that. E.g.:

https://github.com/dart-lang/sdk/blob/master/runtime/vm/compiler/assembler/assembler_arm64.cc#L1488,
https://github.com/dart-lang/sdk/blob/master/runtime/vm/compiler/stub_code_compiler_arm64.cc#L74

and so on.

zanderso · 2020-07-06T21:23:03Z

+    }
+    return _elementAt(index);
+  }
+  int _elementAt(int index) native 'ImmutableBuffer_elementAt';


A runtime call per byte access will be very inefficient. If this were staying here, I'd recommend caching a Uint8List in a field of this object that is backed by an external typed data.

Per the comment thread above, this is being removed completely as there is no use case for it.

dnfield · 2020-07-09T19:24:29Z

@zanderso @chinmaygarde I believe all feedback has been addressed, and tests are passing locally (we'll see if CI agrees). PTAL.

zanderso · 2020-07-09T20:04:39Z

Presubmit check failures look related.

dnfield · 2020-07-09T20:33:58Z

Yeah, the include was too early (OS_ANDROID wasn't defined yet), and the web had some merge issues to fix for NNBD. Newest commit should build .. crosses fingers

dnfield · 2020-07-09T20:35:37Z

-      SkISize::Make(600, 200));
-}
-
-TEST(ImageDecoderTest, VerifySimpleDecodingNoUpscalingOneDimension) {


This functionality has been moved over to the dart side and is tested in resize_image_test.dart

jonahwilliams

I would clearly indicate on the dart:ui dartdocs which APIs (if any) are supported on the web

dnfield · 2020-07-09T20:57:20Z

@jonahwilliams - are we doing that for other APIs?

These seem like they should be possible to implement on the Skia and the DOM backends... /cc @ferhatb @yjbanov @hterkelsen

dnfield · 2020-07-09T22:26:08Z

Mac iOS engine was a flake, rerun here: https://ci.chromium.org/p/flutter/builders/try/Mac%20iOS%20Engine/5393

jonahwilliams · 2020-07-09T22:32:08Z

@jonahwilliams - are we doing that for other APIs?

Not always, but we should :)

jonahwilliams · 2020-07-09T22:37:16Z

in other words, it would generally be good to let users know ahead of time that an API isn't implemented, rather than have them fail to use it or misunderstand the error message

dnfield · 2020-07-10T23:19:25Z

Added those docs @jonahwilliams

dnfield · 2020-07-11T05:39:49Z

Updated this to make use of new Skia API from https://skia.googlesource.com/skia/+/d9eb219e809ac2e282f36c1e99a52cc1ae7522a8

dnfield · 2020-07-13T17:26:16Z

Once https://skia-review.googlesource.com/c/skia/+/302270 rolls in I can clean up my latest patch a little further to get rid of some of the variant weirdness and just have everything go through SkCodecImageGenerator.

jonahwilliams · 2020-07-13T22:40:59Z

+  int _getWidth() native 'ImageDescriptor_width';
+  /// The width, in pixels, of the image.
+  ///
+  /// On web, this is only supported for [raw] images.


uber-nit: "the web"

jonahwilliams · 2020-07-13T22:41:40Z

+///
+/// Use this class to determine the height, width, and byte size of image data
+/// before decoding it.
+class ImageDescriptor extends NativeFieldWrapperClass2 {


Is the a web_ui implementation of this Type?

https://github.com/flutter/engine/pull/19537/files#diff-b5fcf9e3adba0987e06621830060e310R1880

jonahwilliams

All of my comments have been addressed, LGTM (if LG to others...)

…de an ImageDescriptor (flutter-team-archive/engine#19537)

dnfield added 3 commits July 6, 2020 11:11

image descriptor

1387ef9

Merge remote-tracking branch 'upstream/master' into image_descriptor

495d2fa

test

8caf876

googlebot added cla: yes labels Jul 6, 2020

auto-assign Bot requested a review from liyuqian July 6, 2020 18:50

dnfield requested review from chinmaygarde and zanderso July 6, 2020 19:28

unused param, web impl

a2bdc0f

dnfield requested a review from jonahwilliams July 6, 2020 19:55

dnfield commented Jul 6, 2020

View reviewed changes

chinmaygarde reviewed Jul 6, 2020

View reviewed changes

zanderso reviewed Jul 6, 2020

View reviewed changes

dnfield added 8 commits July 7, 2020 16:57

review started

35caeb7

Merge remote-tracking branch 'upstream/master' into image_descriptor

b768bfd

more review work

87b58eb

close

d62c255

Merge remote-tracking branch 'upstream/master' into image_descriptor

2e26e76

done

b92858f

missing return, fix message, make sync

3af3c48

format :(

3b13911

fix merge/build/order issues

e9a4586

dnfield commented Jul 9, 2020

View reviewed changes

jonahwilliams reviewed Jul 9, 2020

View reviewed changes

missing include for windows

80feec9

docs for web

5bc83fa

dnfield added 2 commits July 10, 2020 20:53

Merge remote-tracking branch 'upstream/master' into image_descriptor

d483241

Refactor for new skia methods

81f298d

dnfield added 3 commits July 13, 2020 12:11

Merge remote-tracking branch 'upstream/master' into image_descriptor

a7e96f9

latest skia

7f97ba2

missing line

a08b811

jonahwilliams reviewed Jul 13, 2020

View reviewed changes

the Web

0855054

jonahwilliams approved these changes Jul 14, 2020

View reviewed changes

zanderso requested a review from chinmaygarde July 14, 2020 19:48

Merge remote-tracking branch 'upstream/master' into image_descriptor

0abced1

chinmaygarde approved these changes Jul 21, 2020

View reviewed changes

dnfield merged commit 5281322 into flutter-team-archive:master Jul 21, 2020

dnfield deleted the image_descriptor branch July 21, 2020 21:59

dnfield mentioned this pull request Jul 21, 2020

Utilize new ImageDescriptor logic to allow for aspect ratio maintaining ResizeImages flutter/flutter#61987

Closed

engine-flutter-autoroll mentioned this pull request Jul 22, 2020

Roll Engine from 5a73d4dc2503 to 602c2d81fa16 (21 revisions) flutter/flutter#62051

Closed

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

5281322 Refactor instantiateImageCodec/decodeImageFromPixels to provi…

93ff054

…de an ImageDescriptor (flutter-team-archive/engine#19537)

engine-flutter-autoroll mentioned this pull request Jul 23, 2020

Roll Engine from dcc9a4048da8 to 626244a72c5d (67 revisions) flutter/flutter#62080

Closed

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

5281322 Refactor instantiateImageCodec/decodeImageFromPixels to provi…

32e539e

…de an ImageDescriptor (flutter-team-archive/engine#19537)

walley892 mentioned this pull request Aug 11, 2026

Use round() for height inference in ImageDescriptor.instantiateCodec flutter/flutter#190925

Merged


		namespace flutter {

		class ImageDescriptor : public RefCountedDartWrappable<ImageDescriptor> {


		static void init(Dart_NativeArguments args);

		int length() const {

Conversation

dnfield commented Jul 6, 2020

Description

Related Issues

Tests

Breaking Change

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!

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!

dnfield commented Jul 9, 2020

Uh oh!

zanderso commented Jul 9, 2020