Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose API to decode images to specified dimensions #8596

Merged
merged 14 commits into from May 8, 2019

Conversation

Projects
None yet
5 participants
@iskakaushik
Copy link
Contributor

commented Apr 16, 2019

This PR allows us to specify the target dimensions when we decode an image.

Note: We do not yet support decoding uncompressed images.

Issue: flutter/flutter#26194

Framework side changes for an example of how this API will be used: flutter/flutter#31164

Kaushik Iska added some commits Apr 16, 2019

Kaushik Iska
Kaushik Iska

@googlebot googlebot added the cla: yes label Apr 16, 2019

@iskakaushik iskakaushik changed the title [WIP] Expose resizing api Expose API to decode images to specified dimensions Apr 17, 2019

@iskakaushik iskakaushik requested a review from jonahwilliams Apr 17, 2019

return _futurize(
(_Callback<Codec> callback) => _instantiateImageCodec(list, callback, null, decodedCacheRatioCap),
(_Callback<Codec> callback) => _instantiateImageCodec(

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Apr 17, 2019

Contributor

Here, and below this should be on one line

@@ -17,6 +17,8 @@
#include "third_party/tonic/logging/dart_invoke.h"
#include "third_party/tonic/typed_data/uint8_list.h"

#define fuzzyEquals(x, y) (fabs(x - y) < DBL_EPSILON)

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Apr 17, 2019

Contributor

This concerns me a bit, do we really need to resize if the size is scaled by 5%? Does skia allow doing this automatically?

This comment has been minimized.

Copy link
@liyuqian

liyuqian Apr 17, 2019

Contributor

Skia has mipmap which allows fast resizing to any size at a memory cost of ~1.3x. I think @chinmaygarde has done some experiments with it in the engine. Chinmay: what's your results and current status of using mipmap?

This comment has been minimized.

Copy link
@liyuqian

liyuqian Apr 17, 2019

Contributor

BTW, I think fuzzyEquals probably doesn't follow our naming style. All the #define I see has a name like FOO_BAR. See go/fluttercpp (which redirects to https://google.github.io/styleguide/cppguide.html)

This comment has been minimized.

Copy link
@iskakaushik

iskakaushik Apr 17, 2019

Author Contributor

I was hoping that someone would point me to an already existing implementation and I could delete this from here. Something similar to https://chromium.googlesource.com/angle/angle/+/master/src/common/mathutil.h, where I could just use AlmostEquals. I will rename this to FUZZY_EQUALS.

This comment has been minimized.

Copy link
@liyuqian

liyuqian Apr 17, 2019

Contributor

Oh, I just saw flutter/flutter#26194 which is about making thumbnails to avoid OOM. Mipmap won't help OOM.

In this case, I wouldn't worry about fuzzyEquals at all. If the developer asks to resize, let's always resize even if they differ just by 1 pixel. Otherwise, it will be very confusing: I'll probably file a bug if I asks Flutter to resize to (500, 500) but instead get an image of (501, 501). (Note that Skia's image only has integer width/height, so it doesn't make sense to me to take a double width/height input.)

If they're worried about the performance (decoding and resizing time), they can turn off resizing by checking the image size in their own code instead of us doing something opaque in the engine. Do we currently have an API to check image size without fully decoding? If not, we can probably add that to solve the problem.

BTW, here's another worry if developers can't know the original width/height: how could developers preserve the width/height ratio using this API if they don't know the original width/height?

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Apr 17, 2019

Contributor

We don't have an API to read image size without decoding the full image.

/// The returned future can complete with an error if the image decoding has
/// failed.
Future<Codec> instantiateImageCodec(Uint8List list, {
double decodedCacheRatioCap = double.infinity,
Size size = const Size.square(_kDoNotResizeImage),

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Apr 17, 2019

Contributor

I would do the null check here instead of in flutter/flutter.

@jonahwilliams jonahwilliams requested review from mklim and liyuqian Apr 17, 2019

@jonahwilliams

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

I'm not familiar enough with skia to review the resizing code, which is the more important part of this PR

const double currentHeight,
const double targetWidth,
const double targetHeight) {
if (fuzzyEquals(currentWidth, kDoNotResizeImage) ||

This comment has been minimized.

Copy link
@liyuqian

liyuqian Apr 17, 2019

Contributor

Should it be comparing targetWidth/Height?

This comment has been minimized.

Copy link
@iskakaushik

iskakaushik Apr 17, 2019

Author Contributor

Good catch!

const double targetWidth,
const double targetHeight) {
if (fuzzyEquals(currentWidth, kDoNotResizeImage) ||
fuzzyEquals(currentHeight, kDoNotResizeImage)) {

This comment has been minimized.

Copy link
@liyuqian

liyuqian Apr 17, 2019

Contributor

Instead of doing fuzzyEquals, I think checking targetWidth < 0 seems to be a more robust check.

This comment has been minimized.

Copy link
@iskakaushik

iskakaushik Apr 17, 2019

Author Contributor

I wanted to be consistent with the usage of kDoNotResizeImage, that way somebody in the future could trace the usage of it and replace it with std::optional if/once we have c++17.

This comment has been minimized.

Copy link
@liyuqian

liyuqian Apr 17, 2019

Contributor

Sure, see my new comments on integers vs doubles. Maybe just make it integer -1 and check != kDoNotResizeImage here. BTW, do you think that kNoResizeDimension is a better name? kDoNotResizeImage looks like a boolean to me.


// This indicates that we do not want a "linear blending" decode.
sk_sp<SkColorSpace> dstColorSpace = nullptr;
return SkImage::MakeCrossContextFromPixmap(context.get(), bitmap.pixmap(),

This comment has been minimized.

Copy link
@liyuqian

liyuqian Apr 17, 2019

Contributor

According to DecodeImage, the context might be nullptr here.

Kaushik Iska
Format _instantiateImageCodec calls to be single lined
Move null check for size to be inner

if (needsResize(width, height, targetWidth, targetHeight)) {
return DecodeAndResizeImageToExactSize(
context, imageInfo.makeWH(targetWidth, targetHeight), buffer, trace_id);

This comment has been minimized.

Copy link
@liyuqian

liyuqian Apr 17, 2019

Contributor

Note that makeWH only takes integers. We probably should only allow integers to be sent to the API.

@liyuqian
Copy link
Contributor

left a comment

LGTM. CC @Hixie maybe for another round of review for the API change.

if (targetWidth == kDoNotResizeDimension) {
newHeight = targetHeight;
const double aspectRatio = (double)currentWidth / currentHeight;
newWidth = (int)(aspectRatio * newHeight);

This comment has been minimized.

Copy link
@liyuqian

liyuqian Apr 17, 2019

Contributor

Nit: round the double instead of just truncate it? Same below.

Kaushik Iska

@iskakaushik iskakaushik requested a review from Hixie Apr 22, 2019

@@ -1023,6 +1023,48 @@ enum Clip {
// constant and can't propagate into the set/get calls.
const Endian _kFakeHostEndian = Endian.little;

/// An immutable image dimensions container that holds the target dimensions.

This comment has been minimized.

Copy link
@Hixie

Hixie Apr 24, 2019

Contributor

How is this not identical to Size?

This comment has been minimized.

Copy link
@iskakaushik

iskakaushik Apr 24, 2019

Author Contributor

Size passes dimensions in double not int. I was trying to avoid casts / rounding when converting them for skia.

@override
int get hashCode => hashValues(width, height);
}

This comment has been minimized.

Copy link
@Hixie

Hixie Apr 24, 2019

Contributor

Rather than introducing new API surface, we should use Size. I recommend using null to represent the "no fixed size" edges.

This comment has been minimized.

Copy link
@iskakaushik

iskakaushik Apr 24, 2019

Author Contributor

Answered in the previous comment. About using null, I was hoping that using -1 would let us reference the same placeholder value in both dart as well as c++ land. Else, we would be using null and some other optional placeholder value in c++.

This comment has been minimized.

Copy link
@Hixie

Hixie Apr 29, 2019

Contributor

In that case I would just have int width, int height arguments to instantiateImageCodec. Having a whole class and so on is a lot of API surface to add for something we're not going to use widely.

@iskakaushik

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@Hixie PTAL

return true;
}

if (targetHeight == kDoNotResizeDimension) {

This comment has been minimized.

Copy link
@Hixie

Hixie Apr 29, 2019

Contributor

this can be an else

This comment has been minimized.

Copy link
@Hixie

Hixie Apr 29, 2019

Contributor

(else if, i mean)

@Hixie

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

This needs tests.

@iskakaushik iskakaushik requested a review from Hixie May 2, 2019

@iskakaushik iskakaushik force-pushed the iskakaushik:expose-resizing-api branch from 9923f02 to 4495408 May 2, 2019

@iskakaushik

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Added tests, and removed ImageResizeDimensions from the api surface.

Kaushik Iska added some commits May 3, 2019

Kaushik Iska
Kaushik Iska
Merge branch 'master' of github.com:flutter/engine into expose-resizi…
…ng-api

 Conflicts:
	lib/stub_ui/lib/painting.dart
	lib/ui/painting.dart
@@ -1023,6 +1023,11 @@ enum Clip {
// constant and can't propagate into the set/get calls.
const Endian _kFakeHostEndian = Endian.little;

/// Indicates to the engine to not resize this dimension.
///
/// This needs to be kept in sync with "kDoNotResizeDimension" in codec.cc

This comment has been minimized.

Copy link
@Hixie

Hixie May 6, 2019

Contributor

that comment doesn't make sense to expose to our users.

this documentation should mention where this is used.

so e.g. something like:

/// Indicates that the image should not be resized in this dimensions.
///
/// Used by [instantiateImageCodec] as a magical value to disable resizing
/// in the given direction.
//
// This needs to be kept in sync with "kDoNotResizeDimension" in codec.cc

(notice the switch to // near the end)

@@ -1629,10 +1634,15 @@ class Codec {
/// unlikely that a factor that low will be sufficient to cache all decoded
/// frames. The default value is `25.0`.
///
/// If [targetWidth] or [targetHeight] are specified, image is resized to this. This is

This comment has been minimized.

Copy link
@Hixie

Hixie May 6, 2019

Contributor

"image is resized to this" is a bit gramatically dubious, and "container" isn't clear here (what container?). How about:

  /// 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 set to [kDoNotResizeDimension], then the aspect ratio
  /// will be maintained while forcing the image to match the other given dimension.
  /// If both are [kDoNotResizeDimension], then the image maintains its real size.

I still think it would be more idiomatic to use null rather than -1 in the API. It would avoid the need to introduce [kDoNotResizeDimension] to the public API. We would just pass targetWidth ?? _kDoNotResizeDimension in the implementation, so that the null doesn't make it to C++.

This comment has been minimized.

Copy link
@iskakaushik

iskakaushik May 6, 2019

Author Contributor

I like this idea. Will use null here.

@@ -1708,7 +1718,7 @@ void decodeImageFromPixels(
) {

This comment has been minimized.

Copy link
@Hixie

Hixie May 6, 2019

Contributor

why not support targetWidth and targetHeight here too?

This comment has been minimized.

Copy link
@iskakaushik

iskakaushik May 6, 2019

Author Contributor

Done

Kaushik Iska added some commits May 6, 2019

@iskakaushik

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

Made it null instead of -1 by default. Added support for decodePixels with targetWidth and targetHeight specified. Added tests for the same.

@iskakaushik iskakaushik requested a review from Hixie May 6, 2019

/// 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 specified dimension. If both are not
/// specified, then the image maintains its real size.

This comment has been minimized.

Copy link
@Hixie

Hixie May 8, 2019

Contributor

both are not -> neither is

}) {
return _futurize(
(_Callback<Codec> callback) => _instantiateImageCodec(list, callback, null, decodedCacheRatioCap),
(_Callback<Codec> callback) => _instantiateImageCodec(list, callback, null, decodedCacheRatioCap, targetWidth ?? _kDoNotResizeDimension, targetHeight ?? _kDoNotResizeDimension)
);
}

/// Instantiates a [Codec] object for an image binary data.
///
/// Returns an error message if the instantiation has failed, null otherwise.

This comment has been minimized.

Copy link
@Hixie

Hixie May 8, 2019

Contributor

let's mention _kDoNotResizeDimension here, so that next time we edit this code we remember what's going on

@Hixie

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

LGTM

Kaushik Iska

@iskakaushik iskakaushik merged commit e7e6689 into flutter:master May 8, 2019

17 checks passed

WIP Ready for review
Details
build_and_test_host Task Summary
Details
build_and_test_host
Details
build_and_test_host_profile Task Summary
Details
build_and_test_host_profile
Details
build_and_test_host_release Task Summary
Details
build_and_test_host_release
Details
build_android Task Summary
Details
build_android
Details
build_windows_debug Task Summary
Details
build_windows_debug
Details
build_windows_debug_unopt Task Summary
Details
build_windows_debug_unopt
Details
cla/google All necessary CLAs are signed
format_and_dart_test Task Summary
Details
format_and_dart_test
Details
luci-engine
Details

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 9, 2019

engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request May 9, 2019

Roll engine 026df8e46cde..3ef5007d90cf (2 commits) (#32355)
flutter/engine@026df8e...3ef5007

git log 026df8e46cde22dac687fdb8ed72310e5215be0b..3ef5007d90cf270ecd280db807c855e4b15d7f51 --no-merges --oneline
3ef5007d9 Roll buildroot to bb316a9e. (flutter/engine#8889)
e7e6689b7 Expose API to decode images to specified dimensions (flutter/engine#8596)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff (chinmaygarde@google.com), and stop
the roller if necessary.

huqiuser pushed a commit to huqiuser/engine that referenced this pull request Jun 12, 2019

Expose API to decode images to specified dimensions (flutter#8596)
* Dart side resize primitives exposed

* Write the codec side changes

* return un-scaled image if we can not allocate bitmap

* Format _instantiateImageCodec calls to be single lined

Move null check for size to be inner

* Address CR comments and make image resize dimensions container

* Round not trunc, also format

* Add tests, remove ImageResizeDims from api surface

* Make placeholder value public

* Make the api side changes

* Add a feature to resize pixels and also add tests

* Fix grammar and add more info

wxCUHK added a commit to wxCUHK/engine that referenced this pull request Jul 4, 2019

Expose API to decode images to specified dimensions (flutter#8596)
* Dart side resize primitives exposed

* Write the codec side changes

* return un-scaled image if we can not allocate bitmap

* Format _instantiateImageCodec calls to be single lined

Move null check for size to be inner

* Address CR comments and make image resize dimensions container

* Round not trunc, also format

* Add tests, remove ImageResizeDims from api surface

* Make placeholder value public

* Make the api side changes

* Add a feature to resize pixels and also add tests

* Fix grammar and add more info
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.