Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
2 changes: 2 additions & 0 deletions lib/ui/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ source_set("ui") {
"painting/gradient.h",
"painting/image.cc",
"painting/image.h",
"painting/image_encoding.cc",
"painting/image_encoding.h",
"painting/image_filter.cc",
"painting/image_filter.h",
"painting/image_shader.cc",
Expand Down
2 changes: 2 additions & 0 deletions lib/ui/dart_ui.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "flutter/lib/ui/painting/frame_info.h"
#include "flutter/lib/ui/painting/gradient.h"
#include "flutter/lib/ui/painting/image.h"
#include "flutter/lib/ui/painting/image_encoding.h"
#include "flutter/lib/ui/painting/image_filter.h"
#include "flutter/lib/ui/painting/image_shader.h"
#include "flutter/lib/ui/painting/path.h"
Expand Down Expand Up @@ -56,6 +57,7 @@ void DartUI::InitForGlobal() {
Codec::RegisterNatives(g_natives);
DartRuntimeHooks::RegisterNatives(g_natives);
FrameInfo::RegisterNatives(g_natives);
ImageEncoding::RegisterNatives(g_natives);
ImageFilter::RegisterNatives(g_natives);
ImageShader::RegisterNatives(g_natives);
Paragraph::RegisterNatives(g_natives);
Expand Down
20 changes: 20 additions & 0 deletions lib/ui/painting.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1316,6 +1316,26 @@ Future<Null> _decodeImageFromListAsync(Uint8List list, ImageDecoderCallback call
callback(frameInfo.image);
}

/// The encoding formats supported by the [encodeImage].
enum EncodedImageFormat {
// Be conservative with the formats we expose. We can increase the list in future but reducing
// it is more difficult.
JPEG,
PNG,
WEBP,
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.


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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed. Using a Future instead.


/// Convert an [Image] object into a byte array/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: / -> .

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

void encodeImage(Image image, EncodedImageFormat format, int quality, ImageEncoderCallback callback)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be a method on Image.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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;
   }
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yup, that's it exactly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

native 'encodeImage';

/// Determines the winding rule that decides how the interior of a [Path] is
/// calculated.
///
Expand Down
136 changes: 136 additions & 0 deletions lib/ui/painting/image_encoding.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
// Copyright 2018 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "flutter/lib/ui/painting/image_encoding.h"

#include "flutter/common/threads.h"
#include "flutter/lib/ui/painting/image.h"
#include "flutter/lib/ui/painting/resource_context.h"
#include "lib/fxl/build_config.h"
#include "lib/fxl/functional/make_copyable.h"
#include "lib/tonic/dart_persistent_value.h"
#include "lib/tonic/dart_state.h"
#include "lib/tonic/logging/dart_invoke.h"
#include "lib/tonic/typed_data/uint8_list.h"
#include "third_party/skia/include/core/SkEncodedImageFormat.h"
#include "third_party/skia/include/core/SkImage.h"

using tonic::DartInvoke;
using tonic::DartPersistentValue;
using tonic::ToDart;

namespace blink {
namespace {

void InvokeDataCallback(std::unique_ptr<DartPersistentValue> callback,
sk_sp<SkData> buffer) {
tonic::DartState* dart_state = callback->dart_state().get();
if (!dart_state) {
return;
}
tonic::DartState::Scope scope(dart_state);
if (!buffer) {
DartInvoke(callback->value(), {Dart_Null()});
} else {
Dart_Handle dart_data = tonic::DartConverter<tonic::Uint8List>::ToDart(
buffer->bytes(), buffer->size());
DartInvoke(callback->value(), {dart_data});
}
}

sk_sp<SkData> EncodeImage(sk_sp<SkImage> image,
SkEncodedImageFormat format,
int quality) {
if (image == nullptr) {
return nullptr;
}
return image->encodeToData(format, quality);
}

void EncodeImageAndInvokeDataCallback(
std::unique_ptr<DartPersistentValue> callback,
sk_sp<SkImage> image,
SkEncodedImageFormat format,
int quality) {
sk_sp<SkData> encoded = EncodeImage(std::move(image), format, quality);

Threads::UI()->PostTask(
fxl::MakeCopyable([callback = std::move(callback), encoded]() mutable {
InvokeDataCallback(std::move(callback), std::move(encoded));
}));
}

SkEncodedImageFormat ToSkEncodedImageFormat(int format) {
// Map the formats exposed in flutter to formats supported in Skia.
// See:
// https://github.com/google/skia/blob/master/include/core/SkEncodedImageFormat.h
switch (format) {
case 0:
return SkEncodedImageFormat::kJPEG;
case 1:
return SkEncodedImageFormat::kPNG;
case 2:
return SkEncodedImageFormat::kWEBP;
default:
/* NOTREACHED */
return SkEncodedImageFormat::kWEBP;
}
}

void EncodeImage(Dart_NativeArguments args) {
Dart_Handle exception = nullptr;

CanvasImage* canvas_image =
tonic::DartConverter<CanvasImage*>::FromArguments(args, 0, exception);

if (exception) {
Dart_ThrowException(exception);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

return;
}
if (!canvas_image)
Dart_ThrowException(ToDart("encodeImage called with non-genuine Image."));

int format = tonic::DartConverter<int>::FromArguments(args, 1, exception);
if (exception) {
Dart_ThrowException(exception);
return;
}
SkEncodedImageFormat image_format = ToSkEncodedImageFormat(format);

int quality = tonic::DartConverter<int>::FromArguments(args, 2, exception);
if (exception) {
Dart_ThrowException(exception);
return;
}
if (quality > 100)
quality = 100;
if (quality < 0)
quality = 0;

Dart_Handle callback_handle = Dart_GetNativeArgument(args, 3);
if (!Dart_IsClosure(callback_handle)) {
Dart_ThrowException(ToDart("Callback must be a function"));
return;
}

auto callback = std::make_unique<DartPersistentValue>(
tonic::DartState::Current(), callback_handle);
sk_sp<SkImage> image = canvas_image->image();

Threads::IO()->PostTask(fxl::MakeCopyable(
[callback = std::move(callback), image, image_format, quality]() mutable {
EncodeImageAndInvokeDataCallback(std::move(callback), std::move(image),
image_format, quality);
}));
}

} // namespace

void ImageEncoding::RegisterNatives(tonic::DartLibraryNatives* natives) {
natives->Register({
{"encodeImage", EncodeImage, 4, true},
});
}

} // namespace blink
19 changes: 19 additions & 0 deletions lib/ui/painting/image_encoding.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright 2018 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef FLUTTER_LIB_UI_PAINTING_IMAGE_ENCODING_H_
#define FLUTTER_LIB_UI_PAINTING_IMAGE_ENCODING_H_

#include "lib/tonic/dart_library_natives.h"

namespace blink {

class ImageEncoding {
public:
static void RegisterNatives(tonic::DartLibraryNatives* natives);
};

} // namespace blink

#endif // FLUTTER_LIB_UI_PAINTING_IMAGE_ENCODING_H_