Add more flexible image loading API #38905

tvolkert · 2023-01-16T05:06:17Z

This adds a new instantiateImageCodecWithSize method, which can be used to decode an image into a size, where the target size isn't known until the caller is allowed to inspect the image descriptor.

This enables the use case of loading an image whose aspect ratio isn't known at load time, and which needs to be resized to fit within a bounding box while also maintaining its original aspect ratio.

flutter/flutter#118543

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the [CLA].
All existing and new tests are passing.

flutter-dashboard · 2023-01-16T05:06:19Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

tvolkert · 2023-01-16T19:11:34Z

@yjbanov I'm wondering how we can support this API in the web engine. Thoughts?

jonahwilliams · 2023-01-17T22:00:24Z

lib/ui/painting.dart

+///
+///  * [instantiateImageCodecWithSize], which used this signature for its
+///    `getTargetSize` argument.
+typedef TargetImageSizeProducer = TargetImageSize Function(ImageDescriptor descriptor);


I think we generally use "Callback" as the suffix for function types

jonahwilliams · 2023-01-17T22:02:54Z

lib/ui/painting.dart

 }) async {
+  getTargetSize ??= (ImageDescriptor descriptor) => const TargetImageSize();


Instead of using ??= here, create a static function and make it the argument default value:

TargetImageSize _defaultGetTargetSize(ImageDescriptor descriptor) { return const TargetImageSize(); }

TargetImageSizeProducer getTargetSize = _defaultGetTargetSize,

Heh - I did that at first, but then the package:flutter/painting.dart API wrapper has to do something like this:

Future<ui.Codec> instantiateImageCodecWithSize( ui.ImmutableBuffer buffer, { ui.TargetImageSizeProducer? getTargetSize, }) { if (getTargetSize == null) { return ui.instantiateImageCodecWithSize(buffer); } else { return ui.instantiateImageCodecWithSize(buffer, getTargetSize: getTargetSize); } }

Either that, or the static function has to be public (or worse: copied). After discussing with Bob, he suggested that he frequently resorts to the method I've done here. What do you think given those tradeoffs?

ahh yeah, that makes sense. I think the approach is fine. I would still use a static function instead of creating a new closure on each run though

jonahwilliams

I think overall this approach LGTM. How does this look on the web side of things?

tvolkert · 2023-01-19T06:21:34Z

How does this look on the web side of things?

I added a web impl. It's not pretty.... definitely interested in knowing if there's a better way.

jonahwilliams · 2023-01-19T18:41:23Z

lib/web_ui/lib/painting.dart

+  final int height;
+
+  @override
+  dynamic noSuchMethod(Invocation invocation) => throw UnsupportedError('ImageDescriptor.${invocation.memberName} is not supported on web within a TargetImageSizeCallback.');


Nooooooooo, can we avoid using noSuchMethod? :)

Yep, this can really blow up the JS bundle size.

TIL! I'll update it. Reason I went with this is because it was forcing me to implement all the private methods too, which was a bummer.

Done - I got rid of the need for the ImageDescriptor at all.

jonahwilliams · 2023-01-19T21:29:05Z

Talked about this a bit offline with @chinmaygarde . I think we can already accomplish this with our existing API, but its not very obvious how to do so. For example, you can set one of targetWidth and targetHeight to a maximum value, but don't set the other one - this will preserve the aspect ratio. Then set allowUpscalling to false, this will prevent the image from being resized to be larger.

Given that, I suspect you could probably implement this API in the framework without any dart:ui changes

alanwutang11 · 2023-01-19T21:29:21Z

lib/ui/painting.dart

+  /// must be positive.
+  const TargetImageSize({this.width, this.height})
+      : assert(width == null || width > 0),
+        assert(width == null || width > 0);


nit: I think you meant for this to be height

alanwutang11 · 2023-01-19T21:30:04Z

lib/web_ui/lib/painting.dart

+class TargetImageSize {
+  const TargetImageSize({this.width, this.height})
+      : assert(width == null || width > 0),
+        assert(width == null || width > 0);


nit: height here too

alanwutang11 · 2023-01-19T21:44:18Z

lib/ui/painting.dart

+/// Instead, prefer scaling the [Canvas] transform.
+///
+/// The returned future can complete with an error if the image decoding has
+/// failed.


maybe note here that image resizing capabilities are only available with the canvaskit renderer and not html renderer (we should probably update the api docs for instantiateImageCodec to note this too)

tvolkert · 2023-01-19T22:08:21Z

Talked about this a bit offline with @chinmaygarde . I think we can already accomplish this with our existing API, but its not very obvious how to do so. For example, you can set one of targetWidth and targetHeight to a maximum value, but don't set the other one - this will preserve the aspect ratio. Then set allowUpscalling to false, this will prevent the image from being resized to be larger.

Given that, I suspect you could probably implement this API in the framework without any dart:ui changes

That only works if you at least know the aspect ratio of the image. If I want to size an image to fit within e.g. a square bounding box of 100x100, but I don't know if the image is portrait or landscape orientation, then I don't know whether to say "load the image with width=100 and let height be flexible" or "load the image at height=100 and let the width be flexible".

This was the use case that drove this change. It comes about when you load a stream of random images over the network via a public API (e.g. Google Photos, Flickr, etc.).

jonahwilliams · 2023-01-19T22:47:22Z

so in that case you'd have to say maxWidth = 100, maxHeight = 100. but then that doesn't preserve the aspect ratio?

tvolkert · 2023-01-19T23:10:22Z

Correct - that stretches the image to a square.

tvolkert · 2023-01-19T23:10:52Z

Note that the current API doesn't specify maxWidth & maxHeight -- it specifies targetWidth & targetHeight

jonahwilliams · 2023-01-19T23:16:39Z

with allowUpscaling= false then targetSise becomes maxSize essentially

jonahwilliams · 2023-01-19T23:39:51Z

Could we accomplish something similar with a preserveAspectRatio flag that behaved like you expected?

tvolkert · 2023-01-19T23:40:30Z

No, allowUpscaling=false only ensures that you don't increase the target width beyond the intrinsic width (or the target height beyond the intrinsic height)

engine/lib/ui/painting.dart

Lines 2151 to 2158 in 7d40e77

    
           if (!allowUpscaling) { 
        
             if (targetWidth != null && targetWidth > descriptor.width) { 
        
               targetWidth = descriptor.width; 
        
             } 
        
             if (targetHeight != null && targetHeight > descriptor.height) { 
        
               targetHeight = descriptor.height; 
        
             } 
        
           }

tvolkert · 2023-01-19T23:44:02Z

Could we accomplish something similar with a preserveAspectRatio flag that behaved like you expected?

We already do preserve the aspect ratio if you only specify one of the target width or height. But the problem is, you don't know which one to pass if you don't already know its aspect ratio.

We could (and I considered) add a new parameter (or new signature) that had the caller pass a bounding box, and we'd fit the target image within the bounding box while maintaining the aspect ratio. But such an API change would be just as disruptive as the change in this PR, but less flexible.

This adds a new `instantiateImageCodecWithSize` method, which can be used to decode an image into a size, where the target size isn't known until the caller is allowed to inspect the image descriptor. This enables the use case of loading an image whose aspect ratio isn't known at load time, and which needs to be resized to fit within a bounding box while also maintaining its original aspect ratio. flutter/flutter#118543

* Respond to review comments

…needed

cbracken

Overall LGTM, and looks like all comments have been addressed. One nit about whether we could generalise TargetImageSize to avoid adding more such classes later elsewhere.

cbracken · 2023-01-22T17:40:42Z

lib/web_ui/lib/painting.dart

+
+typedef TargetImageSizeCallback = TargetImageSize Function(int intrinsicWidth, int intrinsicHeight);
+
+class TargetImageSize {


Are there other places in dart:ui we could use an int-based size? Might make sense to generalise the name so others could use it later, if so.

I can’t think of a great name though. Maybe something like PhysicalSize.

I thought about that, but a real PhysicalSize class would want both width & height to be non-null. This is unique in how it allows one or both of those to be null.

More similar to an IntConstraints type, not quite a size. We do really need an ISize type, but probably unrelated.

jonahwilliams · 2023-01-22T18:57:24Z

I think that this is a good feature, but I think it should actually be implemented in foundation instead of dart:ui. It does not require any new native bindings as far as I can tell.

tvolkert · 2023-01-22T20:31:05Z

There's an optimization I've discussed with @yjbanov and @alanwutang11 that will need to be done in the web engine, so it's probably best left here.

jonahwilliams · 2023-01-22T20:49:50Z

Which one?

tvolkert · 2023-01-22T21:55:11Z

If the user passes a callback, but it returns a TargetImageSize with either both width & height null or the same size as the intrinsic size, we can pass the existing codec without discarding it and having to re-create one. It relies on a web engine implementation detail whereby we can avoid getting the first frame in order to know the intrinsic size of the image.

jonahwilliams · 2023-01-22T22:12:56Z

Interesting! Please file a bug/todo for tracking this

jonahwilliams

LGTM

tvolkert · 2023-01-23T01:48:24Z

flutter/flutter#118985

…118989)

…e#38905) (#118989) Commit: 696a84b1ed7da6d8edc69e334a9ec1f3d4ad478f

…#38905) (flutter/flutter#118989)

* 044e344 a8522271c Roll Fuchsia Mac SDK from 5TQ9IL4-Yu3KHCR-H... to R4F4q-h902yt4s7ow... (flutter/engine#39058) (flutter/flutter#118984) * b974eac b3da52d8c Roll Fuchsia Linux SDK from l3c_b-vRr-o6ZFX_M... to f613tOkDB282hW2tA... (flutter/engine#39061) (flutter/flutter#118987) * 696a84b 1e4e11ad1 Add more flexible image loading API (flutter/engine#38905) (flutter/flutter#118989) * 6ae7ad7 92313596d Roll Dart SDK from 548678dd684c to 608a0691a1d7 (1 revision) (flutter/engine#39063) (flutter/flutter#118990) * bd7bee0 Roll Flutter Engine from 92313596d77b to 8e7bc509e0d7 (3 revisions) (flutter/flutter#119004)

* Add more flexible image loading API This adds a new `instantiateImageCodecWithSize` method, which can be used to decode an image into a size, where the target size isn't known until the caller is allowed to inspect the image descriptor. This enables the use case of loading an image whose aspect ratio isn't known at load time, and which needs to be resized to fit within a bounding box while also maintaining its original aspect ratio. flutter/flutter#118543 * Add test * Fixed test failure * Update * Respond to review comments * Add web implementation * Fixed typo * Review comments Also changed the TargetImageSizeCallback to just take intrinsic width & height, rather than the full image descriptor. * Forgot to remove the _SizeOnlyImageDescriptor class - it's no longer needed * Forgot to update test

* 044e344 a8522271c Roll Fuchsia Mac SDK from 5TQ9IL4-Yu3KHCR-H... to R4F4q-h902yt4s7ow... (flutter/engine#39058) (flutter/flutter#118984) * b974eac b3da52d8c Roll Fuchsia Linux SDK from l3c_b-vRr-o6ZFX_M... to f613tOkDB282hW2tA... (flutter/engine#39061) (flutter/flutter#118987) * 696a84b 1e4e11ad1 Add more flexible image loading API (flutter/engine#38905) (flutter/flutter#118989) * 6ae7ad7 92313596d Roll Dart SDK from 548678dd684c to 608a0691a1d7 (1 revision) (flutter/engine#39063) (flutter/flutter#118990) * bd7bee0 Roll Flutter Engine from 92313596d77b to 8e7bc509e0d7 (3 revisions) (flutter/flutter#119004)

flutter-dashboard bot added the needs tests label Jan 16, 2023

tvolkert added Work in progress (WIP) Not ready (yet) for review! and removed Work in progress (WIP) Not ready (yet) for review! needs tests labels Jan 16, 2023

tvolkert requested review from dnfield and yjbanov January 16, 2023 05:33

jonahwilliams reviewed Jan 17, 2023

View reviewed changes

jonahwilliams reviewed Jan 18, 2023

View reviewed changes

jonahwilliams reviewed Jan 19, 2023

View reviewed changes

yjbanov requested a review from alanwutang11 January 19, 2023 18:57

alanwutang11 reviewed Jan 19, 2023

View reviewed changes

tvolkert added 6 commits January 19, 2023 17:32

Add test

b095f72

Fixed test failure

e6371f3

Update

ca4dc78

* Respond to review comments

Add web implementation

1a026b3

Fixed typo

5c7d830

tvolkert added 2 commits January 19, 2023 17:32

Forgot to remove the _SizeOnlyImageDescriptor class - it's no longer …

d31fa58

…needed

Forgot to update test

3d79727

tvolkert force-pushed the load_image branch from 459c11a to 3d79727 Compare January 20, 2023 01:32

cbracken approved these changes Jan 22, 2023

View reviewed changes

jonahwilliams approved these changes Jan 22, 2023

View reviewed changes

tvolkert mentioned this pull request Jan 23, 2023

[web] [images] Optimize sized image loading flutter/flutter#118985

Open

tvolkert added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 23, 2023

auto-submit bot merged commit 1e4e11a into flutter:main Jan 23, 2023

engine-flutter-autoroll mentioned this pull request Jan 23, 2023

Roll Flutter Engine from b3da52d8c5c2 to 1e4e11ad1f05 (1 revision) flutter/flutter#118989

Merged

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

1e4e11ad1 Add more flexible image loading API (flutter/engine#38905)

397b7f8

auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 23, 2023

1e4e11ad1 Add more flexible image loading API (flutter/engine#38905) (#…

696a84b

…118989)

sourcegraph-bot pushed a commit to sgtest/megarepo that referenced this pull request Jan 23, 2023

flutter: 1e4e11ad1 Add more flexible image loading API (flutter/engin…

e4295f4

…e#38905) (#118989) Commit: 696a84b1ed7da6d8edc69e334a9ec1f3d4ad478f

This was referenced Jan 23, 2023

Roll Flutter from f33e8d3701b5 to bd7bee0f9eb8 (5 revisions) flutter/packages#3082

Merged

Roll Flutter from f33e8d3701b5 to bd7bee0f9eb8 (5 revisions) flutter/plugins#7010

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 23, 2023

696a84b 1e4e11ad1 Add more flexible image loading API (flutter/engine…

bf7bb34

…#38905) (flutter/flutter#118989)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 23, 2023

696a84b 1e4e11ad1 Add more flexible image loading API (flutter/engine…

ef54dbd

…#38905) (flutter/flutter#118989)

		}) async {
		getTargetSize ??= (ImageDescriptor descriptor) => const TargetImageSize();


		typedef TargetImageSizeCallback = TargetImageSize Function(int intrinsicWidth, int intrinsicHeight);

		class TargetImageSize {

Add more flexible image loading API #38905

Add more flexible image loading API #38905

Uh oh!

Conversation

tvolkert commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jan 16, 2023

Uh oh!

tvolkert commented Jan 16, 2023

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!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

tvolkert commented Jan 19, 2023

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!

jonahwilliams commented Jan 19, 2023

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 Jan 19, 2023

Uh oh!

jonahwilliams commented Jan 19, 2023

Uh oh!

tvolkert commented Jan 19, 2023

Uh oh!

tvolkert commented Jan 19, 2023

Uh oh!

jonahwilliams commented Jan 19, 2023

Uh oh!

jonahwilliams commented Jan 19, 2023

Uh oh!

tvolkert commented Jan 19, 2023

Uh oh!

tvolkert commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbracken 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!

jonahwilliams commented Jan 22, 2023

Uh oh!

tvolkert commented Jan 22, 2023

Uh oh!

jonahwilliams commented Jan 22, 2023

Uh oh!

tvolkert commented Jan 22, 2023

tvolkert commented Jan 16, 2023 •

edited

Loading

tvolkert commented Jan 19, 2023 •

edited

Loading