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

[Impeller] fix NPE caused by implicit sk_sp to fml::Status conversion. #53177

Merged
auto-submit merged 3 commits into from
Jun 3, 2024
Merged

[Impeller] fix NPE caused by implicit sk_sp to fml::Status conversion. #53177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d5fd6a7
[Impeller] fix NPE caused by implicit sk_sp to fml::Status conversion.
Jun 3, 2024
b4f7281
++
Jun 3, 2024
2c0d896
Update image_encoding.cc
Jun 3, 2024
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
37 changes: 20 additions & 17 deletions lib/ui/painting/image_encoding.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "flutter/fml/status_or.h"
#include "flutter/fml/trace_event.h"
#include "flutter/lib/ui/painting/image.h"
#include "fml/status.h"
#if IMPELLER_SUPPORTS_RENDERING
#include "flutter/lib/ui/painting/image_encoding_impeller.h"
#endif // IMPELLER_SUPPORTS_RENDERING
Expand Down Expand Up @@ -64,16 +65,17 @@ void InvokeDataCallback(std::unique_ptr<DartPersistentValue> callback,
DartInvoke(callback->value(), {dart_data, Dart_Null()});
}

sk_sp<SkData> CopyImageByteData(const sk_sp<SkImage>& raster_image,
SkColorType color_type,
SkAlphaType alpha_type) {
fml::StatusOr<sk_sp<SkData>> CopyImageByteData(
const sk_sp<SkImage>& raster_image,
SkColorType color_type,
SkAlphaType alpha_type) {
FML_DCHECK(raster_image);

SkPixmap pixmap;

if (!raster_image->peekPixels(&pixmap)) {
FML_LOG(ERROR) << "Could not copy pixels from the raster image.";
return nullptr;
return fml::Status(fml::StatusCode::kInternal,
"Could not copy pixels from the raster image.");
}

// The color types already match. No need to swizzle. Return early.
Expand All @@ -87,15 +89,15 @@ sk_sp<SkData> CopyImageByteData(const sk_sp<SkImage>& raster_image,
color_type, alpha_type, nullptr));

if (!surface) {
FML_LOG(ERROR) << "Could not set up the surface for swizzle.";
return nullptr;
fml::Status(fml::StatusCode::kInternal,
"Could not set up the surface for swizzle.");
}

surface->writePixels(pixmap, 0, 0);

if (!surface->peekPixels(&pixmap)) {
FML_LOG(ERROR) << "Pixel address is not available.";
return nullptr;
return fml::Status(fml::StatusCode::kInternal,
"Pixel address is not available.");
}

return SkData::MakeWithCopy(pixmap.addr(), pixmap.computeByteSize());
Expand Down Expand Up @@ -125,7 +127,8 @@ void EncodeImageAndInvokeDataCallback(
[callback_task = std::move(callback_task), format,
ui_task_runner](const fml::StatusOr<sk_sp<SkImage>>& raster_image) {
if (raster_image.ok()) {
sk_sp<SkData> encoded = EncodeImage(raster_image.value(), format);
fml::StatusOr<sk_sp<SkData>> encoded =
EncodeImage(raster_image.value(), format);
ui_task_runner->PostTask([callback_task = callback_task,
encoded = std::move(encoded)]() mutable {
callback_task(std::move(encoded));
Expand Down Expand Up @@ -199,21 +202,21 @@ Dart_Handle EncodeImage(CanvasImage* canvas_image,
return Dart_Null();
}

sk_sp<SkData> EncodeImage(const sk_sp<SkImage>& raster_image,
ImageByteFormat format) {
fml::StatusOr<sk_sp<SkData>> EncodeImage(const sk_sp<SkImage>& raster_image,
ImageByteFormat format) {
TRACE_EVENT0("flutter", __FUNCTION__);

if (!raster_image) {
return nullptr;
return fml::Status(fml::StatusCode::kInternal, "Missing raster image.");
}

switch (format) {
case kPNG: {
auto png_image = SkPngEncoder::Encode(nullptr, raster_image.get(), {});

if (png_image == nullptr) {
FML_LOG(ERROR) << "Could not convert raster image to PNG.";
Copy link
Member

@gaaclarke gaaclarke Jun 3, 2024

Choose a reason for hiding this comment

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

Wait, do we want to maintain these logs somewhere?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Jun 3, 2024

Choose a reason for hiding this comment

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

These strings get returned to the dart:ui layer and printed there as an error.

return nullptr;
return fml::Status(fml::StatusCode::kInternal,
"Could not convert raster image to PNG.");
};
return png_image;
}
Expand All @@ -233,8 +236,8 @@ sk_sp<SkData> EncodeImage(const sk_sp<SkImage>& raster_image,
kUnpremul_SkAlphaType);
}

FML_LOG(ERROR) << "Unknown error encoding image.";
return nullptr;
return fml::Status(fml::StatusCode::kInternal,
"Unknown error encoding image.");
}

} // namespace flutter
5 changes: 3 additions & 2 deletions lib/ui/painting/image_encoding.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#ifndef FLUTTER_LIB_UI_PAINTING_IMAGE_ENCODING_H_
#define FLUTTER_LIB_UI_PAINTING_IMAGE_ENCODING_H_

#include "fml/status_or.h"
#include "third_party/skia/include/core/SkImage.h"
#include "third_party/tonic/dart_library_natives.h"

Expand All @@ -25,8 +26,8 @@ Dart_Handle EncodeImage(CanvasImage* canvas_image,
int format,
Dart_Handle callback_handle);

sk_sp<SkData> EncodeImage(const sk_sp<SkImage>& raster_image,
ImageByteFormat format);
fml::StatusOr<sk_sp<SkData>> EncodeImage(const sk_sp<SkImage>& raster_image,
ImageByteFormat format);

} // namespace flutter

Expand Down
4 changes: 2 additions & 2 deletions lib/ui/painting/image_encoding_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,8 +465,8 @@ TEST(ImageEncodingImpellerTest, PngEncoding10XR) {

sk_sp<SkImage> image = surface->makeImageSnapshot();

sk_sp<SkData> png = EncodeImage(image, ImageByteFormat::kPNG);
EXPECT_TRUE(png);
fml::StatusOr<sk_sp<SkData>> png = EncodeImage(image, ImageByteFormat::kPNG);
EXPECT_TRUE(png.ok());
}

#endif // IMPELLER_SUPPORTS_RENDERING
Expand Down