Make upscaling opt in for image decoding #19067

dnfield · 2020-06-16T19:01:12Z

Description

Make upscaling images using the targetWidth and targetHeight opt-in.

Users are encouraged to use this API to have better memory management of their images. They don't typically want the image to be upscaled - this patch adds a parameter for that and defaults it to false.

It would be nice to land #19062 before this to avoid merge conflicts, but this is otherwise ready for review.

Related Issues

flutter/flutter#59578
flutter/flutter#48885

Tests

I added the following tests:

C++ test for image decoding using the parameter set to true and false
Dart tests opted in and out

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.

Based on my understanding of the current compatibility policy, this is not breaking - although I did change some existing tests. However, this will not break third policies or google3 AFAICT.

…_codec

zanderso

lgtm w/ nits.

zanderso · 2020-06-17T15:51:36Z

lib/ui/painting.dart

+  _ImageInfo? imageInfo,
+  int targetWidth,
+  int targetHeight,
+  bool allowUpscaling) native 'instantiateImageCodec';


Style nit: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#if-you-have-a-newline-after-some-opening-punctuation-match-it-on-the-closing-punctuation

zanderso · 2020-06-17T15:59:48Z

lib/ui/painting/image_decoder.cc

@@ -42,11 +42,22 @@ static double AspectRatio(const SkISize& size) {
 // intrinsic dimensions of the image.
 static SkISize GetResizedDimensions(SkISize current_size,
                                    std::optional<uint32_t> target_width,
-                                    std::optional<uint32_t> target_height) {
+                                    std::optional<uint32_t> target_height,
+                                    bool allow_upscaling) {


style nit: Consider replacing bool argument with an enum: https://google.github.io/styleguide/cppguide.html#Function_Argument_Comments

chinmaygarde

Other than the undefined behavior around explicitly aspect-ratio overriding decodes, looks good. Thanks.

chinmaygarde · 2020-06-17T17:32:44Z

lib/ui/painting/image_decoder.cc

+      target_width = std::min(current_size.width(),
+                              static_cast<int32_t>(target_width.value()));
+    }
+    if (target_height)


Nit: Missing braces.

chinmaygarde · 2020-06-17T17:58:14Z

lib/ui/painting/image_decoder.cc

  if (current_size.isEmpty()) {
    return SkISize::MakeEmpty();
  }

+  if (image_upscaling == ImageUpscalingMode::kNotAllowed) {
+    if (target_width) {


This is a breaking change for this one (admittedly obscure) use case: If the user explicitly wants to decode the image at a non-native aspect ratio, they would specify both the target width and height. Before, the decoder would always return images at that aspect ratio. Now, that aspect ratio will not be preserved if the dimensions along either axis exceed the intrinsic value.

I think a clarification of whether the scaling will be done in an aspect ratio preserving manner is warranted in the documentation as well as a test case in image_decoder_unittests.cc (let's add a test even if decide not to preserve the aspect ratio just so the behavior is not undefined).

If you do decide to preserve scale, you can account the specification of both in another conditional check here.

I know this is an obscure case, but let's avoid undefined behavior as much as possible.

This is a good point.

My grand plan is to follow this up with another PR to add something to control how aspect ratios are or are not preserved. I was originally thinking that this could be separated out from that, but I think you're right that it can't.

I'll take a quick look at how much more complicated this PR gets to add that extra parameter, otherwise I'll look to clarify the docs.

Actually, if a user wants that case they have to just set the allowUpscaling bit to true right?

But yes, we should document and test this.

Yeah, we have sort of tacked on stuff to this original simple API in bits and pieces as we went along. This has understandably become a bit unwieldy.

I'll take a quick look at how much more complicated this PR gets to add that extra parameter...

Instead of doing all the design work to further update this API with additional options, I am fine if we just say "this will/won't preserve aspect ratio if both dimensions are specified" and add a test for this. As I said, my comment was more about the undefined behavior. This is still an improvement for a vast majority of the cases.

In the long run, an API that works with image descriptors without decompression is the way to go. But that is a lot more work to write and migrate to.

if a user wants that case they have to just set the allowUpscaling bit to true right?

Yeah, there is a workaround. Let's just document and test the default. My only concern still is that since the default has changed, earlier, they didn't need to specify the argument but now they have to.

How about just preserving the aspect ratio while scaling down if both width and height are specified? That ought to be really straightforward.

Ok, the more I look at this, the more I think it's just a doc and test thing. Whether or how we add other aspect ratio parameters here will still be relevant to this case.

Sounds good.

chinmaygarde · 2020-06-17T17:59:27Z

lib/ui/painting/image_decoder_unittests.cc

                ->dimensions(),
            SkISize::Make(6, 2));
 }

+TEST(ImageDecoderTest, VerifySimpleDecodingNoUpscaling) {


Per my comment earlier, a test case here about aspect ratio overwriting scales. So something like decode at 1200x100 with ImageUpscalingMode::kNotAllowed.

chinmaygarde · 2020-06-17T18:00:46Z

lib/ui/painting.dart

  int? targetWidth,
  int? targetHeight,
+  bool allowUpscaling = false,


This is going to be a breaking change. Not sure we want to do the dance of keeping it true and flip it later. Your call.

For the Flutter compatibility policy, we would have to break a contributed test (including internal). I did not do a full tgp but I believe this is not breaking by that definition. I understand that is a change in default behavior though.

I suppose to prevent any need to roll back I can default it to true for now, and then we can go back and default it to false once we can test it internally a bit more easily.

This is a new API, so there can't be a test that breaks right? Just felt it might be in violation of the "spirit" of the compatibility policy if not the letter. Either way, your call. It was just an observation about the defaults changing.

There could be a test somewhere that's using the targetWidth/Height to upscale the image currently, and that test would start failing. If that test is registered in flutter/tests or in google3, it counts as a breaking change.

I'm going to see if I can run this through a TGP and make sure it doesn't break anything. If not, I'm going to avoid the dance of changing defaults around - this is clearly what most users want.

Sounds good.

chinmaygarde · 2020-06-18T01:39:26Z

lib/ui/painting/image_decoder.cc

  if (current_size.isEmpty()) {
    return SkISize::MakeEmpty();
  }

+  if (image_upscaling == ImageUpscalingMode::kNotAllowed) {
+    if (target_width) {


Sounds good.

dnfield · 2020-06-18T15:43:05Z

This is failing a framework side test that explicitly upscales the image. I'm going to swap this so that it's defaulting to true, then we can opt that test in, and change this to false.

dnfield added 4 commits June 9, 2020 20:46

fix build

48a1245

Fix doc

8040abd

Merge remote-tracking branch 'upstream/master' into instantiate_image…

9edb828

…_codec

Make upscaling opt-in

4f55dae

googlebot added the cla: yes label Jun 16, 2020

dnfield requested review from cbracken, chinmaygarde and zanderso June 16, 2020 19:01

dnfield changed the title ~~Instantiate image codec upscale~~ Make upscaling opt in for image decoding Jun 16, 2020

dnfield added 2 commits June 16, 2020 15:17

skip assertion related tests if asserts are disabled

67fe8c6

merge

b0f95d5

dnfield marked this pull request as ready for review June 16, 2020 22:18

auto-assign bot requested a review from gw280 June 16, 2020 22:19

save before merging!

b02feff

zanderso approved these changes Jun 17, 2020

View reviewed changes

dnfield added 2 commits June 17, 2020 09:54

merge and nit

bf4afdb

enum

f86b7e1

dnfield added perf: memory Performance issues related to memory waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. labels Jun 17, 2020

chinmaygarde reviewed Jun 17, 2020

View reviewed changes

dnfield removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jun 17, 2020

review

c18d4f4

chinmaygarde approved these changes Jun 18, 2020

View reviewed changes

change default to true

2c3adfa

dnfield merged commit 5157a6b into flutter:master Jun 18, 2020

dnfield deleted the instantiate_image_codec_upscale branch June 18, 2020 16:42

engine-flutter-autoroll mentioned this pull request Jun 18, 2020

Roll Engine from 965fbbed1776 to 5157a6b2107d (122 revisions) flutter/flutter#59790

Closed

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

5157a6b Parameterize upscaling for image decoding (flutter/engine#19067)

96d0cab

engine-flutter-autoroll mentioned this pull request Jun 18, 2020

Roll Engine from 965fbbed1776 to b7f8d2d11040 (125 revisions) flutter/flutter#59795

Closed

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

5157a6b Parameterize upscaling for image decoding (flutter/engine#19067)

4732400

engine-flutter-autoroll mentioned this pull request Jun 18, 2020

Roll Engine from 965fbbed1776 to b5f5e6332cb4 (127 revisions) flutter/flutter#59806

Closed

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

5157a6b Parameterize upscaling for image decoding (flutter/engine#19067)

7584349

engine-flutter-autoroll mentioned this pull request Jun 18, 2020

Roll Engine from 965fbbed1776 to 4a4142c06ffe (130 revisions) flutter/flutter#59812

Closed

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

5157a6b Parameterize upscaling for image decoding (flutter/engine#19067)

120f8d4

dnfield mentioned this pull request Jun 19, 2020

Make upscaling images opt-in flutter/flutter#59856

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make upscaling opt in for image decoding #19067

Make upscaling opt in for image decoding #19067

dnfield commented Jun 16, 2020 •

edited

Loading

zanderso left a comment

zanderso Jun 17, 2020

dnfield Jun 17, 2020

zanderso Jun 17, 2020

dnfield Jun 17, 2020

chinmaygarde left a comment

chinmaygarde Jun 17, 2020

dnfield Jun 17, 2020

chinmaygarde Jun 17, 2020

dnfield Jun 17, 2020

dnfield Jun 17, 2020

chinmaygarde Jun 17, 2020

chinmaygarde Jun 17, 2020

dnfield Jun 17, 2020

chinmaygarde Jun 18, 2020

chinmaygarde Jun 17, 2020 •

edited

Loading

dnfield Jun 17, 2020

chinmaygarde Jun 17, 2020 •

edited

Loading

dnfield Jun 17, 2020

chinmaygarde Jun 17, 2020

dnfield Jun 17, 2020

chinmaygarde Jun 17, 2020

chinmaygarde Jun 18, 2020

dnfield commented Jun 18, 2020

Make upscaling opt in for image decoding #19067

Make upscaling opt in for image decoding #19067

Conversation

dnfield commented Jun 16, 2020 • edited Loading

Description

Related Issues

Tests

Breaking Change

zanderso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield commented Jun 18, 2020

dnfield commented Jun 16, 2020 •

edited

Loading

chinmaygarde Jun 17, 2020 •

edited

Loading

chinmaygarde Jun 17, 2020 •

edited

Loading