Enable Image encoding by leveraging existing Skia functionality #4762

majido · 2018-03-09T03:06:51Z

Enable Image encoding by leveraging existing Skia functionality.

This patch introduces the following:

Add a new encodeImage to painting library that takes Image, format, quality parameters.
Asynchronously Encode underlying SkImage with given format and quality on IO thread.
Post encoded result to the callback.

At the moment, it only exposes JPG, PNG, WEBP formats since it is easier to add more later
when there is a need.

Given this functionality it becomes easy to use the provided Canvas API in a flutter app to
draw and then export the result to an encoded image for sharing/download etc.

Fixes flutter/flutter#11648

majido · 2018-03-09T03:18:52Z

I needed this functionality on my own project and thought this may be of general value since
there was already an issue filed for it.

Hixie · 2018-03-10T00:57:39Z

lib/ui/painting.dart

+  JPEG,
+  PNG,
+  WEBP,
+}


these will need documentation, ideally including a pointer to the relevant specification for each one.

Also for each one we should say what the quality argument does.

These map to Skia values. Skia's does not document the supported format in much details. I was not able to even find link to relevant specification from them. So I have not added any spec links since I was not confident. If you have authoritative links I am happy to add.

Here is the Skia documentation on quality. It has no details on exact impact of quality for each format beyond "it is format specific and may be ignored".

My tests using the demo in the above link confirms that the quality actually works for this three
formats. So I have updated the wording to reflect this with more detail on low vs high.

Hixie · 2018-03-10T00:58:20Z

lib/ui/painting.dart

+}
+
+/// Callback signature for [encodeImage].
+typedef void ImageEncoderCallback(Uint8List result);


Please document what the "result" argument is (presumably, bytes representing the encoded image in the requested format?).

(this can just be removed if we go the route suggested below of returning a future)

Removed. Using a Future instead.

Hixie · 2018-03-10T00:58:30Z

lib/ui/painting.dart

+/// Callback signature for [encodeImage].
+typedef void ImageEncoderCallback(Uint8List result);
+
+/// Convert an [Image] object into a byte array/


typo: / -> .

Hixie · 2018-03-10T00:58:57Z

lib/ui/painting.dart

+/// [format] is encoding format to be used.
+///
+/// [quality] is a value in [0, 100] where 0 corresponds to the lowest quality.
+void encodeImage(Image image, EncodedImageFormat format, int quality, ImageEncoderCallback callback)


This should be a method on Image.

We should probably make the format and quality arguments named optional arguments with defaults.
It's probably also a good idea to have this return a Future<ByteData> rather than using the callback approach. That would be more idiomatic. We can use the callback approach internally in this function in order to implement the Future, though, since dealing with Futures and the engine is non-trivial otherwise.

Also, if we make this a method on Image, we should probably call it toByteData, to be more idiomatic.

+1. FYI for context, see flutter/flutter#11648 (comment)

Ack. I think it makes sense for this to be a method on Image.

Can you please provide my an example of how a Future returning method is implemented in
engine?
I presume Hixie's suggestion was to have a private method implemented natively that uses
callback and then call this private method from public dart method. Effectively creating
the future in dart and resolving that future via callback roughly:

class Image { Future<Uint8List> toByteData(format, quality) { final Completer<Uint8List> completer = new Completer<Uint8List>(); _encodeImage(this, format, quality, completer.complete); // this is natively implemented. return completer.future; } }

Yup, that's it exactly.

You can just use the _futurize method in this file to do it for you though.

Done.

Turned into an async method on Image. Format and quality are now names arguments with defaults (jpeg, 80). I choose jpeg since it is very common and 80 since it is what I think
mac os preview export defaults to. It is supposed to give a reasonable quality/size trade off. Happy to change if you have other ideas.

Most implementation is still kept in image_encoding.h.

Hixie · 2018-03-10T01:02:15Z

lib/ui/painting.dart

+///
+/// [format] is encoding format to be used.
+///
+/// [quality] is a value in [0, 100] where 0 corresponds to the lowest quality.


Please start each paragraph with a capital letter, e.g. by saying The [format] argument is...

Rather than in [0, 100] let's say something like in the range 0 to 100.

Done. Improved the wording and provide more information of what lowest and highest quality mean.

Hixie · 2018-03-10T01:04:24Z

cc @chinmaygarde for review, especially of the C++ side with which I am less familiar.

Fixes flutter/flutter#11648.

Hixie · 2018-03-10T01:06:59Z

Thanks for your contribution! This is a super-popular request, so you will definitely receive the gratitude of many for implementing this!

Hixie · 2018-03-10T01:08:10Z

It would be great if you could include a test of this (just verify that some trivial Image, created with a Canvas, returns the expected bytes). Other relevant tests are in https://github.com/flutter/engine/blob/master/testing/dart/ .

Hixie · 2018-03-10T01:08:51Z

The Travis failure is some minor thing with our license script -- you just need to edit the file as suggested in the Travis output to add your FILE lines.

chinmaygarde · 2018-03-10T17:46:51Z

lib/ui/painting/image_encoding.cc

+      tonic::DartConverter<CanvasImage*>::FromArguments(args, 0, exception);
+
+  if (exception) {
+    Dart_ThrowException(exception);


We are trying to reduce/remove our use of Dart_ThrowException because it does not return and hence will not call destructors of C++ objects on the stack (not that this is an issue in your patch). Can you make this method return an error flag and message that is sent back to the Dart method? On error, you can then throw a Dart exception from that wrapper to get similar behavior.

Done. This is no longer uses Dart_ThrowException instead relies on return values.

The error value is handled via existing _futurize method in the dart side and turned into
and exception.

eseidelGoogle · 2018-03-11T00:10:24Z

<3 This will make a lot of people happy. Thank you!

nsreenath · 2018-03-12T16:39:41Z

Good work @majido
Will this also allow us to save a widget tree as PNG ?

tvolkert · 2018-03-21T20:13:30Z

@majido just checking in to see if there's anything you need to address the review comments.

majido · 2018-03-21T23:22:15Z

@tvolkert Thanks for checking in. I think all comments are clear and make sense. I just asked some clarification on how to return a future from native method. I just got around to implement a simple test per suggestion before embarking on the necessary refactors. I plan to make more progress this weekend.

- Add tests - Move to make encode a method of Image - futurize - Avoid using Dart_ThrowException instead use return value - Improve formatting and comments

majido · 2018-04-01T19:27:53Z

PR is now updated. I believe all issues should have been addressed.

I also added a simple test which compares a generated canvas image against its encoded
versions.

majido · 2018-04-01T20:32:53Z

@nsreenath

Will this also allow us to save a widget tree as PNG ?

This patch allows one to convert any canvas into an encoded image. My usage is mainly based on directly drawing into a canvas. Something similar to this example. So if there is a way to paint the widget tree into a canvas, then perhaps yes.

I haven't look at the internals of flutter rendering system, but each render object seems to have
a paint function. So there maybe a way to trigger a paint on the widget tree root and obtain
the resulting canvas and then save that. The recursive paint appears to be creating new canvases for
each layer so there is definitely more complexities I am overlooking. Hopefully real experts here may be able to shed more light.

Hixie · 2018-04-01T21:56:50Z

lib/ui/painting.dart

+  JPEG,
+  PNG,
+  WEBP,
+}


These should be lowercase.

Here are some doc strings for each one:

/// JPEG format. /// /// This format, strictly speaking called JFIF, is a lossy compression graphics format that /// can handle images up to 65,535 pixels in either dimension. The `quality` metric controls /// the compression ratio. Values in the range of about 50 to 90 are somewhat reasonable; /// values above 95 increase the file size with little noticeable improvement to the quality, /// values below 50 drop the quality substantially. /// /// This format is well suited for photographs. It is very poorly suited for images with hard /// edges or text. It does not support transparency. /// /// JPEG images normally use the `.jpeg` file extension and the `image/jpeg` MIME type. /// /// See also: /// /// * <https://en.wikipedia.org/wiki/JPEG>, the Wikipedia page on JPEG. jpeg, /// PNG format. /// /// A loss-less compression format for images. This format is well suited for images /// with hard edges, such as screenshots or sprites, and images with text. Transparency /// is supported. The `quality` metric is ignored for this format. The PNG format supports /// images up to 2,147,483,647 pixels in either dimension, though in practice available /// memory provides a more immediate limitation on maximum image size. /// /// PNG images normally use the `.png` file extension and the `image/png` MIME type. /// /// See also: /// /// * <https://en.wikipedia.org/wiki/Portable_Network_Graphics>, the Wikipedia page on PNG. /// * <https://tools.ietf.org/rfc/rfc2083.txt>, the PNG standard. png, /// WebP format. /// /// The WebP format supports both lossy and lossless compression; however, the /// [Image.toByteData] method always uses lossy compression when [webp] is /// specified. The `quality` metric determines the compression ratio; higher values /// result in better quality but larger file sizes, and vice versa. WebP images are /// limited to 16,383 pixels in each direction (width and height). /// /// WebP images normally use the `.webp` file extension and the `image/webp` MIME type. /// /// See also: /// /// * <https://en.wikipedia.org/wiki/WebP>, the Wikipedia page on WebP. webp,

Hixie · 2018-04-01T21:58:14Z

testing/dart/encoding_test.dart

+    PictureRecorder recorder = new PictureRecorder();
+    Canvas canvas = new Canvas(recorder, new Rect.fromLTWH(0.0, 0.0, 10.0, 10.0));
+
+    var black = new Paint()


rather than using var, please specify explicit types

Hixie · 2018-04-01T21:58:33Z

testing/dart/encoding_test.dart

+
+void main() {
+
+  Image createSquareTestImage() {


for clarity, please move this and readFile out of main

tvolkert · 2018-04-02T19:13:06Z

Not sure how easy it is to get from a Uint8List to a ByteData, off-hand

It looks like this is the way to do so, though @rmacnak-google would know if there's a better way.

new ByteData.view(uint8List.buffer);

tvolkert · 2018-04-02T19:36:17Z

lib/ui/painting.dart

+  ///
+  /// Returns a future which complete with the binary image data (e.g a PNG or JPEG binary data) or
+  /// an error if encoding fails.
+  Future<Uint8List> toByteData({EncodeFormat format: EncodeFormat.JPEG, int quality: 80}) {


It feels a little weird to pass quality when it doesn't always make sense depending on the format. What if the API were as follows:

/// The encoding formats supported by the [Image.toByteData]. class EncodingFormat { const EncodingFormat._(this._format, [this._quality = 0]); final int _format; final int _quality; // Be conservative with the formats we expose. It is easy to add new formats in future but // difficult to remove. // These values must be kept in sync with the logic in ToSkEncodedImageFormat. static const int _jpegFormat = 0; static const int _pngFormat = 1; static const int _webpFormat = 2; /// PNG format. /// /// A loss-less compression format for images. This format is well suited for images /// with hard edges, such as screenshots or sprites, and images with text. Transparency /// is supported. The `quality` metric is ignored for this format. The PNG format supports /// images up to 2,147,483,647 pixels in either dimension, though in practice available /// memory provides a more immediate limitation on maximum image size. /// /// PNG images normally use the `.png` file extension and the `image/png` MIME type. /// /// See also: /// /// * <https://en.wikipedia.org/wiki/Portable_Network_Graphics>, the Wikipedia page on PNG. /// * <https://tools.ietf.org/rfc/rfc2083.txt>, the PNG standard. static const EncodingFormat png = const EncodingFormat._(_pngFormat); /// JPEG format. /// /// This format, strictly speaking called JFIF, is a lossy compression graphics format that /// can handle images up to 65,535 pixels in either dimension. The [quality] metric controls /// the compression ratio. Values in the range of about 50 to 90 are somewhat reasonable; /// values above 95 increase the file size with little noticeable improvement to the quality, /// values below 50 drop the quality substantially. /// /// This format is well suited for photographs. It is very poorly suited for images with hard /// edges or text. It does not support transparency. /// /// JPEG images normally use the `.jpeg` file extension and the `image/jpeg` MIME type. /// /// See also: /// /// * <https://en.wikipedia.org/wiki/JPEG>, the Wikipedia page on JPEG. factory EncodingFormat.jpeg([int quality = 80]) => new EncodingFormat._(_jpegFormat, quality); /// WebP format. /// /// The WebP format supports both lossy and lossless compression; however, the /// [Image.toByteData] method always uses lossy compression when [webp] is /// specified. The [quality] metric determines the compression ratio; higher values /// result in better quality but larger file sizes, and vice versa. WebP images are /// limited to 16,383 pixels in each direction (width and height). /// /// WebP images normally use the `.webp` file extension and the `image/webp` MIME type. /// /// See also: /// /// * <https://en.wikipedia.org/wiki/WebP>, the Wikipedia page on WebP. factory EncodingFormat.webp([int quality = 80]) => new EncodingFormat._(_webpFormat, quality); } Future<Uint8List> toByteData({EncodingFormat format: EncodingFormat.png}) { return _futurize( (_Callback<Uint8List> callback) => _toByteData(format._format, format._quality, callback) );

We could even then provide some suggested values...

static const EncodingFormat jpegHigh = const EncodingFormat._(_jpegFormat, 95); static const EncodingFormat jpegMedium = const EncodingFormat._(_jpegFormat, 80); static const EncodingFormat jpegLow = const EncodingFormat._(_jpegFormat, 60);

I like this quite a bit more. It provides a much nicer API and enables to have different default quality values for different formats.

nit: I will probably default the quality to be '100' when unspecified. It is safer if Skia ever decides to respect quality for a format in future.

nit: If we decide to provide additional (Low,Medium, High) suggested values we probably should
do it consistently for all formats where quality is supported.

I like this proposal, though please use named arguments not optional arguments for the quality (easier to extend, easier to understand -- a number alone could be any number of things).

We may want to consider making the PNG case a constructor too, so that we can add arguments later.

We don't have to use factory constructors, we could just use real constructors. Then we don't have to have the private constructor.

I think if we're going to have a fallback default, 0 is safer, because it's more likely to cause noticeable problems (like crashes, divide by zero, horrible output, etc), whereas 100 is more likely to cause very subtle problems (like very large file sizes).

I probably wouldn't bother with the low/medium/high defaults. I'm not sure what problem they really solve.

Done. Used a class with three regular constructors, one for each supported format.

tvolkert

This is looking great. Just one comment from me, then I think we're good to go.

tvolkert · 2018-04-05T03:59:09Z

lib/ui/painting.dart

+  ///
+  /// Returns a future which complete with the binary image data (e.g a PNG or JPEG binary data) or
+  /// an error if encoding fails.
+  Future<Uint8List> toByteData({EncodingFormat format: const EncodingFormat.jpeg()}) {


As @Hixie said, let's make this return a Future<ByteData>.

Unfortunately, it doesn't look like here's a tonic::DartConverter<tonic::DartByteData>::ToDart(), so we can just do this in Dart here:

Future<ByteData> toByteData({EncodingFormat format: const EncodingFormat.jpeg()}) { return _futurize((_Callback<ByteData> callback) { return _toByteData(format._format, format._quality, (Uint8List uint8List) { callback(uint8List.buffer.asByteData()); }); }); }

... then the test will have to be updated to match too

tvolkert

LGTM.

Waiting for an LGTM from @Hixie before merging.

Hixie · 2018-04-05T17:43:32Z

lib/ui/painting.dart

+  const EncodingFormat.webp({int quality = 80})
+      : _format = _webpFormat,
+        _quality = quality;
+}


the constructors should come first

Hixie · 2018-04-05T17:44:22Z

lib/ui/painting.dart

+  /// an error if encoding fails.
+  Future<ByteData> toByteData({EncodingFormat format: const EncodingFormat.jpeg()}) {
+    return _futurize((_Callback<ByteData> callback) {
+      _toByteData(format._format, format._quality, (Uint8List encoded) {


we're discarding the string returned by toByteData here, which seems bad

Good catch. We just need to return _toByteData(... here

tvolkert · 2018-04-05T19:33:40Z

@nsreenath

Will this also allow us to save a widget tree as PNG ?

flutter/flutter#16274 is the next piece of the puzzle required to do what you ask.

tvolkert · 2018-04-05T20:51:45Z

lib/ui/painting.dart

+  Future<ByteData> toByteData({EncodingFormat format: const EncodingFormat.jpeg()}) {
+    return _futurize((_Callback<ByteData> callback) {
+      _toByteData(format._format, format._quality, (Uint8List encoded) {
+        callback(new ByteData.view(encoded.buffer));


Nit: encoded.buffer.asByteData() is equivalent and slightly more concise.

tvolkert

LGTM

chinmaygarde · 2018-04-06T00:18:17Z

Can we land this? The underlying API for accessing the task runners is going to change significantly in #4932 and I want to fix the build after I rebase the contents of this patch. Thanks.

tvolkert · 2018-04-06T00:28:08Z

@chinmaygarde, done! @majido, thanks for the contribution!

nsreenath · 2018-04-06T15:00:54Z

lib/ui/painting.dart

+  ///
+  /// Returns a future which complete with the binary image data (e.g a PNG or JPEG binary data) or
+  /// an error if encoding fails.
+  Future<ByteData> toByteData({EncodingFormat format: const EncodingFormat.jpeg()}) {


https://github.com/google/skia/blob/master/src/image/SkImage.cpp#L117

Skia seem to be using lossless PNG format as the default when encoding format is not specified.
Here, are we using JPEG as the default ?
Or should we pass null and use Skia's default ?

Pass const EncodingFormat.png() as the format to get lossless PNG encoding.

tvolkert · 2018-04-11T06:29:17Z

It looks like this increased our release binary size by ~88K. Is that expected?

/cc @Hixie @chinmaygarde @majido

jason-simmons · 2018-04-11T18:03:27Z

Which binary size increased?

I tried doing android_release builds of libflutter.so before and after this commit and saw a 4KB difference

tvolkert · 2018-04-11T18:17:55Z

Hmm, this only seems to substantially affect iOS release binaries. complex_layout_ios__compile::release_size_bytes is the benchmark that regressed, whereas complex_layout_android__compile::release_size_bytes only went up by ~4K as you say.

tvolkert · 2018-04-12T22:35:05Z

Looks like the binary size increase is due to a newly introduced dependency on image encoding. The reason it only surfaced on iOS and not Android is that we only do LTO on iOS. As a result, Android's binary already had the image encoding support linked in before this change and was thus already inflated.

@majido I think the right answer is to make Image.toByteData() return unencoded (raw) bytes (RGBA form, row-primary) and leave it up to callers to encode the raw data however they see fit (using package:image or a platform plugin).

This change first landed in v0.2.10, meaning that it hasn't hit the beta channel yet. We should make sure we update the API to remove the dependency on image encoding asap so it lands before we cut the next beta release.

tvolkert · 2018-04-12T22:47:25Z

Filed flutter/flutter#16537 to fix this.

ghost · 2018-04-17T14:30:17Z

Do you think that video encinh is possible also at this level , rather than having to write a plug-in ?

tvolkert · 2018-04-17T14:36:26Z

@gedw99 see flutter/flutter#8245

ghost · 2018-04-18T22:38:03Z

@tvolkert thanks !

Add basic image encoding with limited format

f7ce006

googlebot added the cla: yes label Mar 9, 2018

prefer single quote

5472da9

Hixie reviewed Mar 10, 2018

View reviewed changes

chinmaygarde reviewed Mar 10, 2018

View reviewed changes

Hixie added the needs love label Mar 19, 2018

jonahwilliams mentioned this pull request Mar 29, 2018

Getting pixel values of an Image? flutter/flutter#15931

Closed

majido added 3 commits April 1, 2018 14:55

Address review feedback including:

a4e3b63

- Add tests - Move to make encode a method of Image - futurize - Avoid using Dart_ThrowException instead use return value - Improve formatting and comments

Revert changes to run_tests

4a39f8d

Update quality documentation

447d8d5

majido added 2 commits April 1, 2018 15:39

Update license file to make Travis happy

2ecab34

minor cleanup

7202b16

Hixie reviewed Apr 1, 2018

View reviewed changes

tvolkert reviewed Apr 2, 2018

View reviewed changes

majido added 2 commits April 4, 2018 22:53

Make EncodingFormat a class

e8a8820

fix formatting of encoding_test.dart

813e9ec

tvolkert reviewed Apr 5, 2018

View reviewed changes

Switch to returning ByteData

cd5f265

tvolkert approved these changes Apr 5, 2018

View reviewed changes

tvolkert removed the needs love label Apr 5, 2018

Hixie reviewed Apr 5, 2018

View reviewed changes

tvolkert reviewed Apr 5, 2018

View reviewed changes

Address feedback

c89adb9

tvolkert approved these changes Apr 6, 2018

View reviewed changes

tvolkert merged commit b6f466b into flutter:master Apr 6, 2018

nsreenath reviewed Apr 6, 2018

View reviewed changes

tvolkert mentioned this pull request Apr 11, 2018

Roll engine to d2b69fec751d6d11f62019c863b2cba4ce575bfe flutter/flutter#16307

Merged

tvolkert mentioned this pull request Apr 12, 2018

Performing encoding in Image.toByteData() bloats engine binary size flutter/flutter#16537

Closed

tvolkert mentioned this pull request Apr 16, 2018

Support different encodings in Image.toByteData() flutter/flutter#16635

Closed

e-simpson mentioned this pull request Dec 16, 2020

Image.toByteData() flutter/flutter#11648

Closed

Enable Image encoding by leveraging existing Skia functionality #4762

Enable Image encoding by leveraging existing Skia functionality #4762

Uh oh!

Conversation

majido commented Mar 9, 2018 • edited by tvolkert Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

majido commented Mar 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Mar 10, 2018

Uh oh!

Hixie commented Mar 10, 2018

Uh oh!

Hixie commented Mar 10, 2018

Uh oh!

Hixie commented Mar 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eseidelGoogle commented Mar 11, 2018

Uh oh!

nsreenath commented Mar 12, 2018

Uh oh!

tvolkert commented Mar 21, 2018

Uh oh!

majido commented Mar 21, 2018

Uh oh!

majido commented Apr 1, 2018

Uh oh!

majido commented Apr 1, 2018

Uh oh!

Hixie Apr 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvolkert commented Apr 2, 2018

majido commented Mar 9, 2018 •

edited by tvolkert

Loading

Hixie Apr 1, 2018 •

edited

Loading