From 48a124586a15c06873f9aaede99e32cdfcb47f44 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 9 Jun 2020 20:46:33 -0700 Subject: [PATCH 1/8] fix build --- shell/platform/darwin/ios/BUILD.gn | 1 - 1 file changed, 1 deletion(-) diff --git a/shell/platform/darwin/ios/BUILD.gn b/shell/platform/darwin/ios/BUILD.gn index 9bdc12115a708..d6ebeb35e9afb 100644 --- a/shell/platform/darwin/ios/BUILD.gn +++ b/shell/platform/darwin/ios/BUILD.gn @@ -132,7 +132,6 @@ source_set("flutter_framework_source") { "//flutter/runtime", "//flutter/runtime:libdart", "//flutter/shell/common", - "//flutter/shell/common:common_standalone", "//flutter/shell/platform/darwin/common", "//flutter/shell/platform/darwin/common:framework_shared", "//flutter/shell/profiling:profiling", From 8040abd19de1abb015facb0fbd2f6758c9a17a40 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 16 Jun 2020 10:04:20 -0700 Subject: [PATCH 2/8] Fix doc --- lib/ui/painting.dart | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index dd3ed4934feea..aeb03096ef8f4 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1714,8 +1714,9 @@ class Codec extends NativeFieldWrapperClass2 { /// The [targetWidth] and [targetHeight] arguments specify the size of the output /// image, in image pixels. If they are not equal to the intrinsic dimensions of the /// image, then the image will be scaled after being decoded. If only one dimension -/// is specified, the omitted dimension will remain its original size. If both are -/// not specified, then the image maintains its real size. +/// is specified, the omitted dimension will be scaled to maintain the original +/// aspect ratio. If both are not specified, then the image maintains its real +/// size. /// /// The returned future can complete with an error if the image decoding has /// failed. From 4f55daee50cf1f759dec11fab32dc3877901cd1a Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 16 Jun 2020 11:50:08 -0700 Subject: [PATCH 3/8] Make upscaling opt-in --- lib/ui/painting.dart | 103 +++++++++++++++------ lib/ui/painting/codec.cc | 17 ++-- lib/ui/painting/image_decoder.cc | 39 ++++---- lib/ui/painting/image_decoder.h | 2 + lib/ui/painting/image_decoder_unittests.cc | 33 ++++++- lib/web_ui/lib/src/ui/painting.dart | 10 +- testing/dart/image_resize_test.dart | 83 +++++++++++++++-- 7 files changed, 219 insertions(+), 68 deletions(-) diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index b78f9b476103c..6206c874d1568 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1695,28 +1695,42 @@ class Codec extends NativeFieldWrapperClass2 { void dispose() native 'Codec_dispose'; } -/// Instantiates an image codec [Codec] object. +/// Instantiates an image [Codec]. /// -/// [list] is the binary image data (e.g a PNG or GIF binary data). +/// The `list` parameter is the binary image data (e.g a PNG or GIF binary data). /// The data can be for either static or animated images. The following image /// formats are supported: {@macro flutter.dart:ui.imageFormats} /// -/// The [targetWidth] and [targetHeight] arguments specify the size of the output -/// image, in image pixels. If they are not equal to the intrinsic dimensions of the -/// image, then the image will be scaled after being decoded. If only one dimension -/// is specified, the omitted dimension will be scaled to maintain the original -/// aspect ratio. If both are not specified, then the image maintains its real -/// size. +/// The `targetWidth` and `targetHeight` arguments specify the size of the +/// output image, in image pixels. If they are not equal to the intrinsic +/// dimensions of the image, then the image will be scaled after being decoded. +/// If only one dimension is specified, the omitted dimension will be scaled to +/// maintain the original aspect ratio. If both are not specified, then the +/// image maintains its intrinsic size. +/// +/// Scaling the image to larger than its intrinsic size should usually be +/// avoided, since it causes the image to use more memory than necessary. +/// Instead, prefer scaling the [Canvas] transform. If the image must be scaled +/// up, the `allowUpscaling` parameter must be set to true. /// /// The returned future can complete with an error if the image decoding has /// failed. -Future instantiateImageCodec(Uint8List list, { +Future instantiateImageCodec( + Uint8List list, { int? targetWidth, int? targetHeight, + bool allowUpscaling = false, }) { - return _futurize( - (_Callback callback) => _instantiateImageCodec(list, callback, null, targetWidth ?? _kDoNotResizeDimension, targetHeight ?? _kDoNotResizeDimension) - ); + return _futurize((_Callback callback) { + return _instantiateImageCodec( + list, + callback, + null, + targetWidth ?? _kDoNotResizeDimension, + targetHeight ?? _kDoNotResizeDimension, + allowUpscaling, + ); + }); } /// Instantiates a [Codec] object for an image binary data. @@ -1730,8 +1744,13 @@ Future instantiateImageCodec(Uint8List list, { /// If both are equal to [_kDoNotResizeDimension], then the image maintains its real size. /// /// Returns an error message if the instantiation has failed, null otherwise. -String? _instantiateImageCodec(Uint8List list, _Callback callback, _ImageInfo? imageInfo, int targetWidth, int targetHeight) - native 'instantiateImageCodec'; +String? _instantiateImageCodec( + Uint8List list, + _Callback callback, + _ImageInfo? imageInfo, + int targetWidth, + int targetHeight, + bool allowUpscaling) native 'instantiateImageCodec'; /// Loads a single image frame from a byte array into an [Image] object. /// @@ -1751,30 +1770,54 @@ Future _decodeImageFromListAsync(Uint8List list, /// Convert an array of pixel values into an [Image] object. /// -/// [pixels] is the pixel data in the encoding described by [format]. +/// The `pixels` parameter is the pixel data in the encoding described by +/// `format`. /// -/// [rowBytes] is the number of bytes consumed by each row of pixels in the -/// data buffer. If unspecified, it defaults to [width] multiplied by the -/// number of bytes per pixel in the provided [format]. +/// The `rowBytes` parameter is the number of bytes consumed by each row of +/// pixels in the data buffer. If unspecified, it defaults to `width` multiplied +/// by the number of bytes per pixel in the provided `format`. /// -/// The [targetWidth] and [targetHeight] arguments specify the size of the output -/// image, in image pixels. If they are not equal to the intrinsic dimensions of the -/// image, then the image will be scaled after being decoded. If exactly one of -/// these two arguments is specified, then the aspect ratio will be maintained -/// while forcing the image to match the other given dimension. If neither is -/// specified, then the image maintains its real size. +/// The `targetWidth` and `targetHeight` arguments specify the size of the +/// output image, in image pixels. If they are not equal to the intrinsic +/// dimensions of the image, then the image will be scaled after being decoded. +/// If exactly one of these two arguments is specified, then the aspect ratio +/// will be maintained while forcing the image to match the other given +/// dimension. If neither is specified, then the image maintains its intrinsic +/// size. +/// +/// Scaling the image to larger than its intrinsic size should usually be +/// avoided, since it causes the image to use more memory than necessary. +/// Instead, prefer scaling the [Canvas] transform. If the image must be scaled +/// up, the `allowUpscaling` parameter must be set to true. void decodeImageFromPixels( Uint8List pixels, int width, int height, PixelFormat format, - ImageDecoderCallback callback, - {int? rowBytes, int? targetWidth, int? targetHeight} -) { + ImageDecoderCallback callback, { + int? rowBytes, + int? targetWidth, + int? targetHeight, + bool allowUpscaling = false, +}) { + if (targetWidth != null) { + assert(allowUpscaling || targetWidth <= width); + } + if (targetHeight != null) { + assert(allowUpscaling || targetHeight <= height); + } + final _ImageInfo imageInfo = _ImageInfo(width, height, format.index, rowBytes); - final Future codecFuture = _futurize( - (_Callback callback) => _instantiateImageCodec(pixels, callback, imageInfo, targetWidth ?? _kDoNotResizeDimension, targetHeight ?? _kDoNotResizeDimension) - ); + final Future codecFuture = _futurize((_Callback callback) { + return _instantiateImageCodec( + pixels, + callback, + imageInfo, + targetWidth ?? _kDoNotResizeDimension, + targetHeight ?? _kDoNotResizeDimension, + allowUpscaling, + ); + }); codecFuture.then((Codec codec) => codec.getNextFrame()) .then((FrameInfo frameInfo) => callback(frameInfo.image)); } diff --git a/lib/ui/painting/codec.cc b/lib/ui/painting/codec.cc index b283f124b863f..ecbfc1707cd76 100644 --- a/lib/ui/painting/codec.cc +++ b/lib/ui/painting/codec.cc @@ -191,10 +191,12 @@ static void InstantiateImageCodec(Dart_NativeArguments args) { } } - const int targetWidth = + const int target_width = tonic::DartConverter::FromDart(Dart_GetNativeArgument(args, 3)); - const int targetHeight = + const int target_height = tonic::DartConverter::FromDart(Dart_GetNativeArgument(args, 4)); + const bool allow_upscaling = + tonic::DartConverter::FromDart(Dart_GetNativeArgument(args, 5)); std::unique_ptr codec; bool single_frame; @@ -215,12 +217,13 @@ static void InstantiateImageCodec(Dart_NativeArguments args) { ImageDecoder::ImageDescriptor descriptor; descriptor.decompressed_image_info = image_info; - if (targetWidth > 0) { - descriptor.target_width = targetWidth; + if (target_width > 0) { + descriptor.target_width = target_width; } - if (targetHeight > 0) { - descriptor.target_height = targetHeight; + if (target_height > 0) { + descriptor.target_height = target_height; } + descriptor.allow_upscaling = allow_upscaling; descriptor.data = std::move(buffer); ui_codec = fml::MakeRefCounted(std::move(descriptor)); @@ -247,7 +250,7 @@ void Codec::dispose() { void Codec::RegisterNatives(tonic::DartLibraryNatives* natives) { natives->Register({ - {"instantiateImageCodec", InstantiateImageCodec, 5, true}, + {"instantiateImageCodec", InstantiateImageCodec, 6, true}, }); natives->Register({FOR_EACH_BINDING(DART_REGISTER_NATIVE)}); } diff --git a/lib/ui/painting/image_decoder.cc b/lib/ui/painting/image_decoder.cc index 5b322e1d8deb0..d8d41f45f83c6 100644 --- a/lib/ui/painting/image_decoder.cc +++ b/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 target_width, - std::optional target_height) { + std::optional target_height, + bool allow_upscaling) { if (current_size.isEmpty()) { return SkISize::MakeEmpty(); } + if (!allow_upscaling) { + if (target_width) { + target_width = std::min(current_size.width(), + static_cast(target_width.value())); + } + if (target_height) + target_height = std::min(current_size.height(), + static_cast(target_height.value())); + } + if (target_width && target_height) { return SkISize::Make(target_width.value(), target_height.value()); } @@ -83,18 +94,8 @@ static sk_sp ResizeRasterImage(sk_sp image, return image->makeRasterImage(); } - if (resized_dimensions.width() > image->dimensions().width() || - resized_dimensions.height() > image->dimensions().height()) { - FML_LOG(WARNING) << "Image is being upsized from " - << image->dimensions().width() << "x" - << image->dimensions().height() << " to " - << resized_dimensions.width() << "x" - << resized_dimensions.height() - << ". Are cache(Height|Width) used correctly?"; - // TOOD(48885): consider exiting here, there's no good reason to support - // upsampling in a "caching"-optimization context.. - } - + // TODO(dnfield): remove this in favor of clearer constraints. + // https://github.com/flutter/flutter/issues/59578 const bool aspect_ratio_changed = std::abs(AspectRatio(resized_dimensions) - AspectRatio(image->dimensions())) > kAspectRatioChangedThreshold; @@ -140,6 +141,7 @@ static sk_sp ImageFromDecompressedData( ImageDecoder::ImageInfo info, std::optional target_width, std::optional target_height, + bool allow_upscaling, const fml::tracing::TraceFlow& flow) { TRACE_EVENT0("flutter", __FUNCTION__); flow.Step(__FUNCTION__); @@ -155,8 +157,8 @@ static sk_sp ImageFromDecompressedData( return image->makeRasterImage(); } - auto resized_dimensions = - GetResizedDimensions(image->dimensions(), target_width, target_height); + auto resized_dimensions = GetResizedDimensions( + image->dimensions(), target_width, target_height, allow_upscaling); return ResizeRasterImage(std::move(image), resized_dimensions, flow); } @@ -164,6 +166,7 @@ static sk_sp ImageFromDecompressedData( sk_sp ImageFromCompressedData(sk_sp data, std::optional target_width, std::optional target_height, + bool allow_upscaling, const fml::tracing::TraceFlow& flow) { TRACE_EVENT0("flutter", __FUNCTION__); flow.Step(__FUNCTION__); @@ -185,8 +188,8 @@ sk_sp ImageFromCompressedData(sk_sp data, auto image_generator = SkCodecImageGenerator::MakeFromCodec(std::move(codec)); const auto& source_dimensions = image_generator->getInfo().dimensions(); - auto resized_dimensions = - GetResizedDimensions(source_dimensions, target_width, target_height); + auto resized_dimensions = GetResizedDimensions( + source_dimensions, target_width, target_height, allow_upscaling); // No resize needed. if (resized_dimensions == source_dimensions) { @@ -344,11 +347,13 @@ void ImageDecoder::Decode(ImageDescriptor descriptor, descriptor.decompressed_image_info.value(), // descriptor.target_width, // descriptor.target_height, // + descriptor.allow_upscaling, // flow // ) : ImageFromCompressedData(std::move(descriptor.data), // descriptor.target_width, // descriptor.target_height, // + descriptor.allow_upscaling, // flow); if (!decompressed) { diff --git a/lib/ui/painting/image_decoder.h b/lib/ui/painting/image_decoder.h index b8190acdf5271..8e76fcc3b80c6 100644 --- a/lib/ui/painting/image_decoder.h +++ b/lib/ui/painting/image_decoder.h @@ -47,6 +47,7 @@ class ImageDecoder { std::optional decompressed_image_info; std::optional target_width; std::optional target_height; + bool allow_upscaling = false; }; using ImageResult = std::function)>; @@ -72,6 +73,7 @@ class ImageDecoder { sk_sp ImageFromCompressedData(sk_sp data, std::optional target_width, std::optional target_height, + bool allow_upscaling, const fml::tracing::TraceFlow& flow); } // namespace flutter diff --git a/lib/ui/painting/image_decoder_unittests.cc b/lib/ui/painting/image_decoder_unittests.cc index d90378dd625fe..5c8af890c47a0 100644 --- a/lib/ui/painting/image_decoder_unittests.cc +++ b/lib/ui/painting/image_decoder_unittests.cc @@ -364,6 +364,7 @@ TEST_F(ImageDecoderFixtureTest, CanDecodeWithResizes) { ImageDecoder::ImageDescriptor image_descriptor; image_descriptor.target_width = target_width; image_descriptor.target_height = target_height; + image_descriptor.allow_upscaling = false; image_descriptor.data = OpenFixtureAsSkData("DashInNooglerHat.jpg"); ASSERT_TRUE(image_descriptor.data); @@ -463,6 +464,7 @@ TEST_F(ImageDecoderFixtureTest, CanResizeWithoutDecode) { ImageDecoder::ImageDescriptor image_descriptor; image_descriptor.target_width = target_width; image_descriptor.target_height = target_height; + image_descriptor.allow_upscaling = false; image_descriptor.data = decompressed_data; image_descriptor.decompressed_image_info = info; @@ -530,9 +532,34 @@ TEST(ImageDecoderTest, VerifySimpleDecoding) { ASSERT_TRUE(image != nullptr); ASSERT_EQ(SkISize::Make(600, 200), image->dimensions()); - ASSERT_EQ(ImageFromCompressedData(data, 6, 2, fml::tracing::TraceFlow("")) + ASSERT_EQ( + ImageFromCompressedData(data, 6, 2, false, fml::tracing::TraceFlow("")) + ->dimensions(), + SkISize::Make(6, 2)); +} + +TEST(ImageDecoderTest, VerifySimpleDecodingNoUpscaling) { + auto data = OpenFixtureAsSkData("Horizontal.jpg"); + auto image = SkImage::MakeFromEncoded(data); + ASSERT_TRUE(image != nullptr); + ASSERT_EQ(SkISize::Make(600, 200), image->dimensions()); + + ASSERT_EQ(ImageFromCompressedData(data, 900, 300, false, + fml::tracing::TraceFlow("")) ->dimensions(), - SkISize::Make(6, 2)); + SkISize::Make(600, 200)); +} + +TEST(ImageDecoderTest, VerifySimpleDecodingWithUpscaling) { + auto data = OpenFixtureAsSkData("Horizontal.jpg"); + auto image = SkImage::MakeFromEncoded(data); + ASSERT_TRUE(image != nullptr); + ASSERT_EQ(SkISize::Make(600, 200), image->dimensions()); + + ASSERT_EQ( + ImageFromCompressedData(data, 900, 300, true, fml::tracing::TraceFlow("")) + ->dimensions(), + SkISize::Make(900, 300)); } TEST(ImageDecoderTest, VerifySubpixelDecodingPreservesExifOrientation) { @@ -543,7 +570,7 @@ TEST(ImageDecoderTest, VerifySubpixelDecodingPreservesExifOrientation) { auto decode = [data](std::optional target_width, std::optional target_height) { - return ImageFromCompressedData(data, target_width, target_height, + return ImageFromCompressedData(data, target_width, target_height, false, fml::tracing::TraceFlow("")); }; diff --git a/lib/web_ui/lib/src/ui/painting.dart b/lib/web_ui/lib/src/ui/painting.dart index 51474bebd608f..fae4978c63504 100644 --- a/lib/web_ui/lib/src/ui/painting.dart +++ b/lib/web_ui/lib/src/ui/painting.dart @@ -1582,6 +1582,7 @@ Future/*!*/ instantiateImageCodec( Uint8List/*!*/ list, { int/*?*/ targetWidth, int/*?*/ targetHeight, + bool/*!*/ allowUpscaling = false, }) { return engine.futurize((engine.Callback callback) => // TODO: Implement targetWidth and targetHeight support. @@ -1648,9 +1649,12 @@ void decodeImageFromPixels( int/*!*/ width, int/*!*/ height, PixelFormat/*!*/ format, - ImageDecoderCallback/*!*/ callback, - {int/*?*/ rowBytes, int/*?*/ targetWidth, int/*?*/ targetHeight} -) { + ImageDecoderCallback/*!*/ callback, { + int/*?*/ rowBytes, + int/*?*/ targetWidth, + int/*?*/ targetHeight, + bool/*!*/ allowUpscaling, +}) { final _ImageInfo imageInfo = _ImageInfo(width, height, format.index, rowBytes); final Future codecFuture = engine.futurize( diff --git a/testing/dart/image_resize_test.dart b/testing/dart/image_resize_test.dart index 8ef592a25e354..7e3b4bcdfdfef 100644 --- a/testing/dart/image_resize_test.dart +++ b/testing/dart/image_resize_test.dart @@ -44,7 +44,7 @@ void main() { test('upscale image by 5x', () async { final Uint8List bytes = await readFile('4x4.png'); - final Codec codec = await instantiateImageCodec(bytes, targetWidth: 10); + final Codec codec = await instantiateImageCodec(bytes, targetWidth: 10, allowUpscaling: true); final FrameInfo frame = await codec.getNextFrame(); final int codecHeight = frame.image.height; final int codecWidth = frame.image.width; @@ -52,10 +52,20 @@ void main() { expect(codecWidth, 10); }); + test('upscale image by 5x - no upscaling', () async { + final Uint8List bytes = await readFile('4x4.png'); + final Codec codec = await instantiateImageCodec(bytes, targetWidth: 10); + final FrameInfo frame = await codec.getNextFrame(); + final int codecHeight = frame.image.height; + final int codecWidth = frame.image.width; + expect(codecHeight, 2); + expect(codecWidth, 2); + }); + test('upscale image varying width and height', () async { final Uint8List bytes = await readFile('4x4.png'); final Codec codec = - await instantiateImageCodec(bytes, targetWidth: 10, targetHeight: 1); + await instantiateImageCodec(bytes, targetWidth: 10, targetHeight: 1, allowUpscaling: true); final FrameInfo frame = await codec.getNextFrame(); final int codecHeight = frame.image.height; final int codecWidth = frame.image.width; @@ -63,6 +73,17 @@ void main() { expect(codecWidth, 10); }); + test('upscale image varying width and height - no upscaling', () async { + final Uint8List bytes = await readFile('4x4.png'); + final Codec codec = + await instantiateImageCodec(bytes, targetWidth: 10, targetHeight: 1); + final FrameInfo frame = await codec.getNextFrame(); + final int codecHeight = frame.image.height; + final int codecWidth = frame.image.width; + expect(codecHeight, 1); + expect(codecWidth, 2); + }); + test('pixels: no resize by default', () async { final BlackSquare blackSquare = BlackSquare.create(); final Image resized = await blackSquare.resize(); @@ -86,19 +107,58 @@ void main() { test('pixels: upscale image by 5x', () async { final BlackSquare blackSquare = BlackSquare.create(); - final Image resized = await blackSquare.resize(targetWidth: 10); + final Image resized = await blackSquare.resize(targetWidth: 10, allowUpscaling: true); expect(resized.height, 10); expect(resized.width, 10); }); + test('pixels: upscale image by 5x - no upscaling', () async { + final BlackSquare blackSquare = BlackSquare.create(); + bool threw = false; + try { + decodeImageFromPixels( + blackSquare.pixels, + blackSquare.width, + blackSquare.height, + PixelFormat.rgba8888, + (Image image) => null, + targetHeight: 10, + ); + } catch (e) { + expect(e is AssertionError, true); + threw = true; + } + expect(threw, true); + }); + test('pixels: upscale image varying width and height', () async { final BlackSquare blackSquare = BlackSquare.create(); final Image resized = - await blackSquare.resize(targetHeight: 1, targetWidth: 10); + await blackSquare.resize(targetHeight: 1, targetWidth: 10, allowUpscaling: true); expect(resized.height, 1); expect(resized.width, 10); }); + test('pixels: upscale image varying width and height - no upscaling', () async { + final BlackSquare blackSquare = BlackSquare.create(); + bool threw = false; + try { + decodeImageFromPixels( + blackSquare.pixels, + blackSquare.width, + blackSquare.height, + PixelFormat.rgba8888, + (Image image) => null, + targetHeight: 10, + targetWidth: 1, + ); + } catch (e) { + expect(e is AssertionError, true); + threw = true; + } + expect(threw, true); + }); + test('pixels: large negative dimensions', () async { final BlackSquare blackSquare = BlackSquare.create(); final Image resized = @@ -117,11 +177,18 @@ class BlackSquare { return BlackSquare._(width, height, pixels); } - Future resize({int targetWidth, int targetHeight}) async { + Future resize({int targetWidth, int targetHeight, bool allowUpscaling = false}) async { final Completer imageCompleter = Completer(); - decodeImageFromPixels(pixels, width, height, PixelFormat.rgba8888, - (Image image) => imageCompleter.complete(image), - targetHeight: targetHeight, targetWidth: targetWidth); + decodeImageFromPixels( + pixels, + width, + height, + PixelFormat.rgba8888, + (Image image) => imageCompleter.complete(image), + targetHeight: targetHeight, + targetWidth: targetWidth, + allowUpscaling: allowUpscaling, + ); return await imageCompleter.future; } From 67fe8c692b84211b08c5e082e67d1dada75e566d Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 16 Jun 2020 15:17:56 -0700 Subject: [PATCH 4/8] skip assertion related tests if asserts are disabled --- testing/dart/image_resize_test.dart | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/testing/dart/image_resize_test.dart b/testing/dart/image_resize_test.dart index 7e3b4bcdfdfef..75f553dba1ff7 100644 --- a/testing/dart/image_resize_test.dart +++ b/testing/dart/image_resize_test.dart @@ -12,6 +12,12 @@ import 'package:path/path.dart' as path; import 'package:test/test.dart'; void main() { + bool assertsEnabled = false; + assert(() { + assertsEnabled = true; + return true; + }()); + test('no resize by default', () async { final Uint8List bytes = await readFile('4x4.png'); final Codec codec = await instantiateImageCodec(bytes); @@ -129,7 +135,7 @@ void main() { threw = true; } expect(threw, true); - }); + }, skip: !assertsEnabled); test('pixels: upscale image varying width and height', () async { final BlackSquare blackSquare = BlackSquare.create(); @@ -157,7 +163,7 @@ void main() { threw = true; } expect(threw, true); - }); + }, skip: !assertsEnabled); test('pixels: large negative dimensions', () async { final BlackSquare blackSquare = BlackSquare.create(); From b02feff33d6a768637eb7d8d9ec9deaae799bb7b Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 16 Jun 2020 15:23:25 -0700 Subject: [PATCH 5/8] save before merging! --- lib/ui/painting.dart | 9 --------- 1 file changed, 9 deletions(-) diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index 3591659f6c187..6206c874d1568 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1701,7 +1701,6 @@ class Codec extends NativeFieldWrapperClass2 { /// The data can be for either static or animated images. The following image /// formats are supported: {@macro flutter.dart:ui.imageFormats} /// -<<<<<<< HEAD /// The `targetWidth` and `targetHeight` arguments specify the size of the /// output image, in image pixels. If they are not equal to the intrinsic /// dimensions of the image, then the image will be scaled after being decoded. @@ -1713,14 +1712,6 @@ class Codec extends NativeFieldWrapperClass2 { /// avoided, since it causes the image to use more memory than necessary. /// Instead, prefer scaling the [Canvas] transform. If the image must be scaled /// up, the `allowUpscaling` parameter must be set to true. -======= -/// The [targetWidth] and [targetHeight] arguments specify the size of the output -/// image, in image pixels. If they are not equal to the intrinsic dimensions of the -/// image, then the image will be scaled after being decoded. If only one dimension -/// is specified, the omitted dimension will be scaled to maintain the original -/// aspect ratio. If both are not specified, then the image maintains its real -/// size. ->>>>>>> upstream/master /// /// The returned future can complete with an error if the image decoding has /// failed. From f86b7e11f042590ecc7b517e3e894338933d808f Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 17 Jun 2020 10:28:20 -0700 Subject: [PATCH 6/8] enum --- lib/ui/painting/codec.cc | 4 +++- lib/ui/painting/image_decoder.cc | 16 ++++++------- lib/ui/painting/image_decoder.h | 7 ++++-- lib/ui/painting/image_decoder_unittests.cc | 27 ++++++++++++---------- 4 files changed, 31 insertions(+), 23 deletions(-) diff --git a/lib/ui/painting/codec.cc b/lib/ui/painting/codec.cc index ecbfc1707cd76..a2abdf29a0f7f 100644 --- a/lib/ui/painting/codec.cc +++ b/lib/ui/painting/codec.cc @@ -223,7 +223,9 @@ static void InstantiateImageCodec(Dart_NativeArguments args) { if (target_height > 0) { descriptor.target_height = target_height; } - descriptor.allow_upscaling = allow_upscaling; + descriptor.image_upscaling = allow_upscaling + ? ImageUpscalingMode::kAllowed + : ImageUpscalingMode::kNotAllowed; descriptor.data = std::move(buffer); ui_codec = fml::MakeRefCounted(std::move(descriptor)); diff --git a/lib/ui/painting/image_decoder.cc b/lib/ui/painting/image_decoder.cc index d8d41f45f83c6..7444d9ad16ea8 100644 --- a/lib/ui/painting/image_decoder.cc +++ b/lib/ui/painting/image_decoder.cc @@ -43,12 +43,12 @@ static double AspectRatio(const SkISize& size) { static SkISize GetResizedDimensions(SkISize current_size, std::optional target_width, std::optional target_height, - bool allow_upscaling) { + ImageUpscalingMode image_upscaling) { if (current_size.isEmpty()) { return SkISize::MakeEmpty(); } - if (!allow_upscaling) { + if (image_upscaling == ImageUpscalingMode::kNotAllowed) { if (target_width) { target_width = std::min(current_size.width(), static_cast(target_width.value())); @@ -141,7 +141,7 @@ static sk_sp ImageFromDecompressedData( ImageDecoder::ImageInfo info, std::optional target_width, std::optional target_height, - bool allow_upscaling, + ImageUpscalingMode image_upscaling, const fml::tracing::TraceFlow& flow) { TRACE_EVENT0("flutter", __FUNCTION__); flow.Step(__FUNCTION__); @@ -158,7 +158,7 @@ static sk_sp ImageFromDecompressedData( } auto resized_dimensions = GetResizedDimensions( - image->dimensions(), target_width, target_height, allow_upscaling); + image->dimensions(), target_width, target_height, image_upscaling); return ResizeRasterImage(std::move(image), resized_dimensions, flow); } @@ -166,7 +166,7 @@ static sk_sp ImageFromDecompressedData( sk_sp ImageFromCompressedData(sk_sp data, std::optional target_width, std::optional target_height, - bool allow_upscaling, + ImageUpscalingMode image_upscaling, const fml::tracing::TraceFlow& flow) { TRACE_EVENT0("flutter", __FUNCTION__); flow.Step(__FUNCTION__); @@ -189,7 +189,7 @@ sk_sp ImageFromCompressedData(sk_sp data, const auto& source_dimensions = image_generator->getInfo().dimensions(); auto resized_dimensions = GetResizedDimensions( - source_dimensions, target_width, target_height, allow_upscaling); + source_dimensions, target_width, target_height, image_upscaling); // No resize needed. if (resized_dimensions == source_dimensions) { @@ -347,13 +347,13 @@ void ImageDecoder::Decode(ImageDescriptor descriptor, descriptor.decompressed_image_info.value(), // descriptor.target_width, // descriptor.target_height, // - descriptor.allow_upscaling, // + descriptor.image_upscaling, // flow // ) : ImageFromCompressedData(std::move(descriptor.data), // descriptor.target_width, // descriptor.target_height, // - descriptor.allow_upscaling, // + descriptor.image_upscaling, // flow); if (!decompressed) { diff --git a/lib/ui/painting/image_decoder.h b/lib/ui/painting/image_decoder.h index 8e76fcc3b80c6..81e216a8a6d79 100644 --- a/lib/ui/painting/image_decoder.h +++ b/lib/ui/painting/image_decoder.h @@ -23,6 +23,9 @@ namespace flutter { +// Whether to allow for image upscaling when resizing an image. +enum class ImageUpscalingMode { kAllowed, kNotAllowed }; + // An object that coordinates image decompression and texture upload across // multiple threads/components in the shell. This object must be created, // accessed and collected on the UI thread (typically the engine or its runtime @@ -47,7 +50,7 @@ class ImageDecoder { std::optional decompressed_image_info; std::optional target_width; std::optional target_height; - bool allow_upscaling = false; + ImageUpscalingMode image_upscaling = ImageUpscalingMode::kNotAllowed; }; using ImageResult = std::function)>; @@ -73,7 +76,7 @@ class ImageDecoder { sk_sp ImageFromCompressedData(sk_sp data, std::optional target_width, std::optional target_height, - bool allow_upscaling, + ImageUpscalingMode image_upscaling, const fml::tracing::TraceFlow& flow); } // namespace flutter diff --git a/lib/ui/painting/image_decoder_unittests.cc b/lib/ui/painting/image_decoder_unittests.cc index 5c8af890c47a0..3ac923d171279 100644 --- a/lib/ui/painting/image_decoder_unittests.cc +++ b/lib/ui/painting/image_decoder_unittests.cc @@ -364,7 +364,7 @@ TEST_F(ImageDecoderFixtureTest, CanDecodeWithResizes) { ImageDecoder::ImageDescriptor image_descriptor; image_descriptor.target_width = target_width; image_descriptor.target_height = target_height; - image_descriptor.allow_upscaling = false; + image_descriptor.image_upscaling = ImageUpscalingMode::kNotAllowed; image_descriptor.data = OpenFixtureAsSkData("DashInNooglerHat.jpg"); ASSERT_TRUE(image_descriptor.data); @@ -464,7 +464,7 @@ TEST_F(ImageDecoderFixtureTest, CanResizeWithoutDecode) { ImageDecoder::ImageDescriptor image_descriptor; image_descriptor.target_width = target_width; image_descriptor.target_height = target_height; - image_descriptor.allow_upscaling = false; + image_descriptor.image_upscaling = ImageUpscalingMode::kNotAllowed; image_descriptor.data = decompressed_data; image_descriptor.decompressed_image_info = info; @@ -532,10 +532,10 @@ TEST(ImageDecoderTest, VerifySimpleDecoding) { ASSERT_TRUE(image != nullptr); ASSERT_EQ(SkISize::Make(600, 200), image->dimensions()); - ASSERT_EQ( - ImageFromCompressedData(data, 6, 2, false, fml::tracing::TraceFlow("")) - ->dimensions(), - SkISize::Make(6, 2)); + ASSERT_EQ(ImageFromCompressedData(data, 6, 2, ImageUpscalingMode::kNotAllowed, + fml::tracing::TraceFlow("")) + ->dimensions(), + SkISize::Make(6, 2)); } TEST(ImageDecoderTest, VerifySimpleDecodingNoUpscaling) { @@ -544,10 +544,11 @@ TEST(ImageDecoderTest, VerifySimpleDecodingNoUpscaling) { ASSERT_TRUE(image != nullptr); ASSERT_EQ(SkISize::Make(600, 200), image->dimensions()); - ASSERT_EQ(ImageFromCompressedData(data, 900, 300, false, - fml::tracing::TraceFlow("")) - ->dimensions(), - SkISize::Make(600, 200)); + ASSERT_EQ( + ImageFromCompressedData(data, 900, 300, ImageUpscalingMode::kNotAllowed, + fml::tracing::TraceFlow("")) + ->dimensions(), + SkISize::Make(600, 200)); } TEST(ImageDecoderTest, VerifySimpleDecodingWithUpscaling) { @@ -557,7 +558,8 @@ TEST(ImageDecoderTest, VerifySimpleDecodingWithUpscaling) { ASSERT_EQ(SkISize::Make(600, 200), image->dimensions()); ASSERT_EQ( - ImageFromCompressedData(data, 900, 300, true, fml::tracing::TraceFlow("")) + ImageFromCompressedData(data, 900, 300, ImageUpscalingMode::kAllowed, + fml::tracing::TraceFlow("")) ->dimensions(), SkISize::Make(900, 300)); } @@ -570,7 +572,8 @@ TEST(ImageDecoderTest, VerifySubpixelDecodingPreservesExifOrientation) { auto decode = [data](std::optional target_width, std::optional target_height) { - return ImageFromCompressedData(data, target_width, target_height, false, + return ImageFromCompressedData(data, target_width, target_height, + ImageUpscalingMode::kNotAllowed, fml::tracing::TraceFlow("")); }; From c18d4f4a6939672ad28726320812c4022c4b434d Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 17 Jun 2020 16:18:04 -0700 Subject: [PATCH 7/8] review --- lib/ui/painting.dart | 19 ++++++++++++------- lib/ui/painting/image_decoder.cc | 3 ++- lib/ui/painting/image_decoder_unittests.cc | 13 +++++++++++++ lib/web_ui/lib/src/ui/painting.dart | 12 ++++++------ 4 files changed, 33 insertions(+), 14 deletions(-) diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index c146ba622f91a..17dc200df09cb 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1704,9 +1704,12 @@ class Codec extends NativeFieldWrapperClass2 { /// The `targetWidth` and `targetHeight` arguments specify the size of the /// output image, in image pixels. If they are not equal to the intrinsic /// dimensions of the image, then the image will be scaled after being decoded. -/// If only one dimension is specified, the omitted dimension will be scaled to -/// maintain the original aspect ratio. If both are not specified, then the -/// image maintains its intrinsic size. +/// If the `allowUpscaling` parameter is not set to true, both dimensions will +/// be capped at the intrinsic dimensions of the image, even if only one of +/// them would have exceeded those intrinsic dimensions. If exactly one of these +/// two arguments is specified, then the aspect ratio will be maintained while +/// forcing the image to match the other given dimension. If neither is +/// specified, then the image maintains its intrinsic size. /// /// Scaling the image to larger than its intrinsic size should usually be /// avoided, since it causes the image to use more memory than necessary. @@ -1781,10 +1784,12 @@ Future _decodeImageFromListAsync(Uint8List list, /// The `targetWidth` and `targetHeight` arguments specify the size of the /// output image, in image pixels. If they are not equal to the intrinsic /// dimensions of the image, then the image will be scaled after being decoded. -/// If exactly one of these two arguments is specified, then the aspect ratio -/// will be maintained while forcing the image to match the other given -/// dimension. If neither is specified, then the image maintains its intrinsic -/// size. +/// If the `allowUpscaling` parameter is not set to true, both dimensions will +/// be capped at the intrinsic dimensions of the image, even if only one of +/// them would have exceeded those intrinsic dimensions. If exactly one of these +/// two arguments is specified, then the aspect ratio will be maintained while +/// forcing the image to match the other given dimension. If neither is +/// specified, then the image maintains its intrinsic size. /// /// Scaling the image to larger than its intrinsic size should usually be /// avoided, since it causes the image to use more memory than necessary. diff --git a/lib/ui/painting/image_decoder.cc b/lib/ui/painting/image_decoder.cc index 7444d9ad16ea8..c778d1ba22a56 100644 --- a/lib/ui/painting/image_decoder.cc +++ b/lib/ui/painting/image_decoder.cc @@ -53,9 +53,10 @@ static SkISize GetResizedDimensions(SkISize current_size, target_width = std::min(current_size.width(), static_cast(target_width.value())); } - if (target_height) + if (target_height) { target_height = std::min(current_size.height(), static_cast(target_height.value())); + } } if (target_width && target_height) { diff --git a/lib/ui/painting/image_decoder_unittests.cc b/lib/ui/painting/image_decoder_unittests.cc index 3ac923d171279..6c17157373fd6 100644 --- a/lib/ui/painting/image_decoder_unittests.cc +++ b/lib/ui/painting/image_decoder_unittests.cc @@ -551,6 +551,19 @@ TEST(ImageDecoderTest, VerifySimpleDecodingNoUpscaling) { SkISize::Make(600, 200)); } +TEST(ImageDecoderTest, VerifySimpleDecodingNoUpscalingOneDimension) { + auto data = OpenFixtureAsSkData("Horizontal.jpg"); + auto image = SkImage::MakeFromEncoded(data); + ASSERT_TRUE(image != nullptr); + ASSERT_EQ(SkISize::Make(600, 200), image->dimensions()); + + ASSERT_EQ( + ImageFromCompressedData(data, 1200, 200, ImageUpscalingMode::kNotAllowed, + fml::tracing::TraceFlow("")) + ->dimensions(), + SkISize::Make(600, 200)); +} + TEST(ImageDecoderTest, VerifySimpleDecodingWithUpscaling) { auto data = OpenFixtureAsSkData("Horizontal.jpg"); auto image = SkImage::MakeFromEncoded(data); diff --git a/lib/web_ui/lib/src/ui/painting.dart b/lib/web_ui/lib/src/ui/painting.dart index 1c0d63c028270..1a68ad3a5f5eb 100644 --- a/lib/web_ui/lib/src/ui/painting.dart +++ b/lib/web_ui/lib/src/ui/painting.dart @@ -1579,8 +1579,8 @@ class Codec { /// failed. Future instantiateImageCodec( Uint8List list, { - int targetWidth, - int targetHeight, + int? targetWidth, + int? targetHeight, bool allowUpscaling = false, }) { return _futurize((engine.Callback callback) => @@ -1649,10 +1649,10 @@ void decodeImageFromPixels( int height, PixelFormat format, ImageDecoderCallback callback, { - int rowBytes, - int targetWidth, - int targetHeight, - bool allowUpscaling, + int? rowBytes, + int? targetWidth, + int? targetHeight, + bool allowUpscaling = false, }) { final _ImageInfo imageInfo = _ImageInfo(width, height, format.index, rowBytes); From 2c3adfa380912434ced3178606d759cfa776645f Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 18 Jun 2020 08:45:40 -0700 Subject: [PATCH 8/8] change default to true --- lib/ui/painting.dart | 4 ++-- lib/web_ui/lib/src/ui/painting.dart | 4 ++-- testing/dart/image_resize_test.dart | 6 ++++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index 17dc200df09cb..2aaa5a08b6f91 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1722,7 +1722,7 @@ Future instantiateImageCodec( Uint8List list, { int? targetWidth, int? targetHeight, - bool allowUpscaling = false, + bool allowUpscaling = true, }) { return _futurize((_Callback callback) { return _instantiateImageCodec( @@ -1804,7 +1804,7 @@ void decodeImageFromPixels( int? rowBytes, int? targetWidth, int? targetHeight, - bool allowUpscaling = false, + bool allowUpscaling = true, }) { if (targetWidth != null) { assert(allowUpscaling || targetWidth <= width); diff --git a/lib/web_ui/lib/src/ui/painting.dart b/lib/web_ui/lib/src/ui/painting.dart index 1a68ad3a5f5eb..0facfaeee8d14 100644 --- a/lib/web_ui/lib/src/ui/painting.dart +++ b/lib/web_ui/lib/src/ui/painting.dart @@ -1581,7 +1581,7 @@ Future instantiateImageCodec( Uint8List list, { int? targetWidth, int? targetHeight, - bool allowUpscaling = false, + bool allowUpscaling = true, }) { return _futurize((engine.Callback callback) => // TODO: Implement targetWidth and targetHeight support. @@ -1652,7 +1652,7 @@ void decodeImageFromPixels( int? rowBytes, int? targetWidth, int? targetHeight, - bool allowUpscaling = false, + bool allowUpscaling = true, }) { final _ImageInfo imageInfo = _ImageInfo(width, height, format.index, rowBytes); diff --git a/testing/dart/image_resize_test.dart b/testing/dart/image_resize_test.dart index 75f553dba1ff7..484dfc48bb8c6 100644 --- a/testing/dart/image_resize_test.dart +++ b/testing/dart/image_resize_test.dart @@ -60,7 +60,7 @@ void main() { test('upscale image by 5x - no upscaling', () async { final Uint8List bytes = await readFile('4x4.png'); - final Codec codec = await instantiateImageCodec(bytes, targetWidth: 10); + final Codec codec = await instantiateImageCodec(bytes, targetWidth: 10, allowUpscaling: false); final FrameInfo frame = await codec.getNextFrame(); final int codecHeight = frame.image.height; final int codecWidth = frame.image.width; @@ -82,7 +82,7 @@ void main() { test('upscale image varying width and height - no upscaling', () async { final Uint8List bytes = await readFile('4x4.png'); final Codec codec = - await instantiateImageCodec(bytes, targetWidth: 10, targetHeight: 1); + await instantiateImageCodec(bytes, targetWidth: 10, targetHeight: 1, allowUpscaling: false); final FrameInfo frame = await codec.getNextFrame(); final int codecHeight = frame.image.height; final int codecWidth = frame.image.width; @@ -129,6 +129,7 @@ void main() { PixelFormat.rgba8888, (Image image) => null, targetHeight: 10, + allowUpscaling: false, ); } catch (e) { expect(e is AssertionError, true); @@ -157,6 +158,7 @@ void main() { (Image image) => null, targetHeight: 10, targetWidth: 1, + allowUpscaling: false, ); } catch (e) { expect(e is AssertionError, true);