From 60a7bb2353b614d6b089fbcc60694e6e8da342ec Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Mon, 3 Jun 2024 13:43:07 -0700 Subject: [PATCH] [Impeller] fix NPE caused by implicit sk_sp to fml::Status conversion. (#53177) nullptr is converted to fml::Status::OK(nullptr), which we don't null check. Change this to consistently use fml::Status Discovered in https://github.com/flutter/flutter/issues/148851 --- lib/ui/painting/image_encoding.cc | 37 +++++++++++---------- lib/ui/painting/image_encoding.h | 5 +-- lib/ui/painting/image_encoding_unittests.cc | 4 +-- 3 files changed, 25 insertions(+), 21 deletions(-) diff --git a/lib/ui/painting/image_encoding.cc b/lib/ui/painting/image_encoding.cc index db60335007b79..152551ee26ffb 100644 --- a/lib/ui/painting/image_encoding.cc +++ b/lib/ui/painting/image_encoding.cc @@ -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 @@ -64,16 +65,17 @@ void InvokeDataCallback(std::unique_ptr callback, DartInvoke(callback->value(), {dart_data, Dart_Null()}); } -sk_sp CopyImageByteData(const sk_sp& raster_image, - SkColorType color_type, - SkAlphaType alpha_type) { +fml::StatusOr> CopyImageByteData( + const sk_sp& 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. @@ -87,15 +89,15 @@ sk_sp CopyImageByteData(const sk_sp& 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()); @@ -125,7 +127,8 @@ void EncodeImageAndInvokeDataCallback( [callback_task = std::move(callback_task), format, ui_task_runner](const fml::StatusOr>& raster_image) { if (raster_image.ok()) { - sk_sp encoded = EncodeImage(raster_image.value(), format); + fml::StatusOr> encoded = + EncodeImage(raster_image.value(), format); ui_task_runner->PostTask([callback_task = callback_task, encoded = std::move(encoded)]() mutable { callback_task(std::move(encoded)); @@ -199,12 +202,12 @@ Dart_Handle EncodeImage(CanvasImage* canvas_image, return Dart_Null(); } -sk_sp EncodeImage(const sk_sp& raster_image, - ImageByteFormat format) { +fml::StatusOr> EncodeImage(const sk_sp& raster_image, + ImageByteFormat format) { TRACE_EVENT0("flutter", __FUNCTION__); if (!raster_image) { - return nullptr; + return fml::Status(fml::StatusCode::kInternal, "Missing raster image."); } switch (format) { @@ -212,8 +215,8 @@ sk_sp EncodeImage(const sk_sp& raster_image, auto png_image = SkPngEncoder::Encode(nullptr, raster_image.get(), {}); if (png_image == nullptr) { - FML_LOG(ERROR) << "Could not convert raster image to PNG."; - return nullptr; + return fml::Status(fml::StatusCode::kInternal, + "Could not convert raster image to PNG."); }; return png_image; } @@ -233,8 +236,8 @@ sk_sp EncodeImage(const sk_sp& 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 diff --git a/lib/ui/painting/image_encoding.h b/lib/ui/painting/image_encoding.h index be852043022ae..16b4c927be84f 100644 --- a/lib/ui/painting/image_encoding.h +++ b/lib/ui/painting/image_encoding.h @@ -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" @@ -25,8 +26,8 @@ Dart_Handle EncodeImage(CanvasImage* canvas_image, int format, Dart_Handle callback_handle); -sk_sp EncodeImage(const sk_sp& raster_image, - ImageByteFormat format); +fml::StatusOr> EncodeImage(const sk_sp& raster_image, + ImageByteFormat format); } // namespace flutter diff --git a/lib/ui/painting/image_encoding_unittests.cc b/lib/ui/painting/image_encoding_unittests.cc index 6b451d4562344..bfd962fec1397 100644 --- a/lib/ui/painting/image_encoding_unittests.cc +++ b/lib/ui/painting/image_encoding_unittests.cc @@ -465,8 +465,8 @@ TEST(ImageEncodingImpellerTest, PngEncoding10XR) { sk_sp image = surface->makeImageSnapshot(); - sk_sp png = EncodeImage(image, ImageByteFormat::kPNG); - EXPECT_TRUE(png); + fml::StatusOr> png = EncodeImage(image, ImageByteFormat::kPNG); + EXPECT_TRUE(png.ok()); } #endif // IMPELLER_SUPPORTS_RENDERING