Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Make upscaling opt in for image decoding #19067

Merged
merged 11 commits into from
Jun 18, 2020
Merged

Make upscaling opt in for image decoding #19067

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
48a1245
fix build
dnfield Jun 10, 2020
8040abd
Fix doc
dnfield Jun 16, 2020
9edb828
Merge remote-tracking branch 'upstream/master' into instantiate_image…
dnfield Jun 16, 2020
4f55dae
Make upscaling opt-in
dnfield Jun 16, 2020
67fe8c6
skip assertion related tests if asserts are disabled
dnfield Jun 16, 2020
b0f95d5
merge
dnfield Jun 16, 2020
b02feff
save before merging!
dnfield Jun 16, 2020
bf4afdb
merge and nit
dnfield Jun 17, 2020
f86b7e1
enum
dnfield Jun 17, 2020
c18d4f4
review
dnfield Jun 17, 2020
2c3adfa
change default to true
dnfield Jun 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 79 additions & 30 deletions lib/ui/painting.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1695,28 +1695,45 @@ 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 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.
/// 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<Codec> instantiateImageCodec(Uint8List list, {
Future<Codec> instantiateImageCodec(
Uint8List list, {
int? targetWidth,
int? targetHeight,
bool allowUpscaling = true,
}) {
return _futurize(
(_Callback<Codec> callback) => _instantiateImageCodec(list, callback, null, targetWidth ?? _kDoNotResizeDimension, targetHeight ?? _kDoNotResizeDimension)
);
return _futurize((_Callback<Codec> callback) {
return _instantiateImageCodec(
list,
callback,
null,
targetWidth ?? _kDoNotResizeDimension,
targetHeight ?? _kDoNotResizeDimension,
allowUpscaling,
);
});
}

/// Instantiates a [Codec] object for an image binary data.
Expand All @@ -1730,8 +1747,14 @@ Future<Codec> 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<Codec> callback, _ImageInfo? imageInfo, int targetWidth, int targetHeight)
native 'instantiateImageCodec';
String? _instantiateImageCodec(
Uint8List list,
_Callback<Codec> callback,
_ImageInfo? imageInfo,
int targetWidth,
int targetHeight,
bool allowUpscaling,
) native 'instantiateImageCodec';

/// Loads a single image frame from a byte array into an [Image] object.
///
Expand All @@ -1751,30 +1774,56 @@ Future<void> _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 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.
/// 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 = true,
}) {
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<Codec> codecFuture = _futurize(
(_Callback<Codec> callback) => _instantiateImageCodec(pixels, callback, imageInfo, targetWidth ?? _kDoNotResizeDimension, targetHeight ?? _kDoNotResizeDimension)
);
final Future<Codec> codecFuture = _futurize((_Callback<Codec> callback) {
return _instantiateImageCodec(
pixels,
callback,
imageInfo,
targetWidth ?? _kDoNotResizeDimension,
targetHeight ?? _kDoNotResizeDimension,
allowUpscaling,
);
});
codecFuture.then((Codec codec) => codec.getNextFrame())
.then((FrameInfo frameInfo) => callback(frameInfo.image));
}
Expand Down
19 changes: 12 additions & 7 deletions lib/ui/painting/codec.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,10 +191,12 @@ static void InstantiateImageCodec(Dart_NativeArguments args) {
}
}

const int targetWidth =
const int target_width =
tonic::DartConverter<int>::FromDart(Dart_GetNativeArgument(args, 3));
const int targetHeight =
const int target_height =
tonic::DartConverter<int>::FromDart(Dart_GetNativeArgument(args, 4));
const bool allow_upscaling =
tonic::DartConverter<bool>::FromDart(Dart_GetNativeArgument(args, 5));

std::unique_ptr<SkCodec> codec;
bool single_frame;
Expand All @@ -215,12 +217,15 @@ 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.image_upscaling = allow_upscaling
? ImageUpscalingMode::kAllowed
: ImageUpscalingMode::kNotAllowed;
descriptor.data = std::move(buffer);

ui_codec = fml::MakeRefCounted<SingleFrameCodec>(std::move(descriptor));
Expand All @@ -247,7 +252,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)});
}
Expand Down
40 changes: 23 additions & 17 deletions lib/ui/painting/image_decoder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,23 @@ 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,
ImageUpscalingMode image_upscaling) {
if (current_size.isEmpty()) {
return SkISize::MakeEmpty();
}

if (image_upscaling == ImageUpscalingMode::kNotAllowed) {
if (target_width) {
Copy link
Member

@chinmaygarde chinmaygarde Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dnfield dnfield Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dnfield dnfield Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@chinmaygarde chinmaygarde Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@chinmaygarde chinmaygarde Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dnfield dnfield Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@chinmaygarde chinmaygarde Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

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

if (target_width && target_height) {
return SkISize::Make(target_width.value(), target_height.value());
}
Expand Down Expand Up @@ -83,18 +95,8 @@ static sk_sp<SkImage> ResizeRasterImage(sk_sp<SkImage> 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;
Expand Down Expand Up @@ -140,6 +142,7 @@ static sk_sp<SkImage> ImageFromDecompressedData(
ImageDecoder::ImageInfo info,
std::optional<uint32_t> target_width,
std::optional<uint32_t> target_height,
ImageUpscalingMode image_upscaling,
const fml::tracing::TraceFlow& flow) {
TRACE_EVENT0("flutter", __FUNCTION__);
flow.Step(__FUNCTION__);
Expand All @@ -155,15 +158,16 @@ static sk_sp<SkImage> ImageFromDecompressedData(
return image->makeRasterImage();
}

auto resized_dimensions =
GetResizedDimensions(image->dimensions(), target_width, target_height);
auto resized_dimensions = GetResizedDimensions(
image->dimensions(), target_width, target_height, image_upscaling);

return ResizeRasterImage(std::move(image), resized_dimensions, flow);
}

sk_sp<SkImage> ImageFromCompressedData(sk_sp<SkData> data,
std::optional<uint32_t> target_width,
std::optional<uint32_t> target_height,
ImageUpscalingMode image_upscaling,
const fml::tracing::TraceFlow& flow) {
TRACE_EVENT0("flutter", __FUNCTION__);
flow.Step(__FUNCTION__);
Expand All @@ -185,8 +189,8 @@ sk_sp<SkImage> ImageFromCompressedData(sk_sp<SkData> 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, image_upscaling);

// No resize needed.
if (resized_dimensions == source_dimensions) {
Expand Down Expand Up @@ -344,11 +348,13 @@ void ImageDecoder::Decode(ImageDescriptor descriptor,
descriptor.decompressed_image_info.value(), //
descriptor.target_width, //
descriptor.target_height, //
descriptor.image_upscaling, //
flow //
)
: ImageFromCompressedData(std::move(descriptor.data), //
descriptor.target_width, //
descriptor.target_height, //
descriptor.image_upscaling, //
flow);

if (!decompressed) {
Expand Down
5 changes: 5 additions & 0 deletions lib/ui/painting/image_decoder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -47,6 +50,7 @@ class ImageDecoder {
std::optional<ImageInfo> decompressed_image_info;
std::optional<uint32_t> target_width;
std::optional<uint32_t> target_height;
ImageUpscalingMode image_upscaling = ImageUpscalingMode::kNotAllowed;
};

using ImageResult = std::function<void(SkiaGPUObject<SkImage>)>;
Expand All @@ -72,6 +76,7 @@ class ImageDecoder {
sk_sp<SkImage> ImageFromCompressedData(sk_sp<SkData> data,
std::optional<uint32_t> target_width,
std::optional<uint32_t> target_height,
ImageUpscalingMode image_upscaling,
const fml::tracing::TraceFlow& flow);

} // namespace flutter
Expand Down
45 changes: 44 additions & 1 deletion lib/ui/painting/image_decoder_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.image_upscaling = ImageUpscalingMode::kNotAllowed;
image_descriptor.data = OpenFixtureAsSkData("DashInNooglerHat.jpg");

ASSERT_TRUE(image_descriptor.data);
Expand Down Expand Up @@ -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.image_upscaling = ImageUpscalingMode::kNotAllowed;
image_descriptor.data = decompressed_data;
image_descriptor.decompressed_image_info = info;

Expand Down Expand Up @@ -530,11 +532,51 @@ 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, ImageUpscalingMode::kNotAllowed,
fml::tracing::TraceFlow(""))
->dimensions(),
SkISize::Make(6, 2));
}

TEST(ImageDecoderTest, VerifySimpleDecodingNoUpscaling) {
Copy link
Member

@chinmaygarde chinmaygarde Jun 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dnfield dnfield Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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, ImageUpscalingMode::kNotAllowed,
fml::tracing::TraceFlow(""))
->dimensions(),
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);
ASSERT_TRUE(image != nullptr);
ASSERT_EQ(SkISize::Make(600, 200), image->dimensions());

ASSERT_EQ(
ImageFromCompressedData(data, 900, 300, ImageUpscalingMode::kAllowed,
fml::tracing::TraceFlow(""))
->dimensions(),
SkISize::Make(900, 300));
}

TEST(ImageDecoderTest, VerifySubpixelDecodingPreservesExifOrientation) {
auto data = OpenFixtureAsSkData("Horizontal.jpg");
auto image = SkImage::MakeFromEncoded(data);
Expand All @@ -544,6 +586,7 @@ TEST(ImageDecoderTest, VerifySubpixelDecodingPreservesExifOrientation) {
auto decode = [data](std::optional<uint32_t> target_width,
std::optional<uint32_t> target_height) {
return ImageFromCompressedData(data, target_width, target_height,
ImageUpscalingMode::kNotAllowed,
fml::tracing::TraceFlow(""));
};

Expand Down
Loading