From 5118dcf8a527ddefabc87905ccc9fd2aec777965 Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Fri, 8 Jan 2021 20:54:02 -0800 Subject: [PATCH] During image decoding, avoid using smart pointers for DartWrappables that cross thread-boundaries. (flutter/engine#23503) --- .../flutter/lib/ui/painting/image_decoder.cc | 75 ++++++++++++------- .../flutter/lib/ui/painting/image_decoder.h | 2 +- .../ui/painting/image_decoder_unittests.cc | 12 +-- 3 files changed, 53 insertions(+), 36 deletions(-) diff --git a/engine/src/flutter/lib/ui/painting/image_decoder.cc b/engine/src/flutter/lib/ui/painting/image_decoder.cc index e679e4d296b..3d497f9aea8 100644 --- a/engine/src/flutter/lib/ui/painting/image_decoder.cc +++ b/engine/src/flutter/lib/ui/painting/image_decoder.cc @@ -75,7 +75,7 @@ static sk_sp ResizeRasterImage(sk_sp image, } static sk_sp ImageFromDecompressedData( - fml::RefPtr descriptor, + ImageDescriptor* descriptor, uint32_t target_width, uint32_t target_height, const fml::tracing::TraceFlow& flow) { @@ -98,7 +98,7 @@ static sk_sp ImageFromDecompressedData( SkISize::Make(target_width, target_height), flow); } -sk_sp ImageFromCompressedData(fml::RefPtr descriptor, +sk_sp ImageFromCompressedData(ImageDescriptor* descriptor, uint32_t target_width, uint32_t target_height, const fml::tracing::TraceFlow& flow) { @@ -216,38 +216,55 @@ static SkiaGPUObject UploadRasterImage( return result; } -void ImageDecoder::Decode(fml::RefPtr descriptor, +void ImageDecoder::Decode(fml::RefPtr descriptor_ref_ptr, uint32_t target_width, uint32_t target_height, const ImageResult& callback) { TRACE_EVENT0("flutter", __FUNCTION__); fml::tracing::TraceFlow flow(__FUNCTION__); + // ImageDescriptors have Dart peers that must be collected on the UI thread. + // However, closures in MakeCopyable below capture the descriptor. The + // captures of copyable closures may be collected on any of the thread + // participating in task execution. + // + // To avoid this issue, we resort to manually reference counting the + // descriptor. Since all task flows invoke the `result` callback, the raw + // descriptor is retained in the beginning and released in the `result` + // callback. + // + // `ImageDecoder::Decode` itself is invoked on the UI thread, so the + // collection of the smart pointer from which we obtained the raw descriptor + // is fine in this scope. + auto raw_descriptor = descriptor_ref_ptr.get(); + raw_descriptor->AddRef(); + FML_DCHECK(callback); FML_DCHECK(runners_.GetUITaskRunner()->RunsTasksOnCurrentThread()); // Always service the callback (and cleanup the descriptor) on the UI thread. - auto result = [callback, descriptor, ui_runner = runners_.GetUITaskRunner()]( - SkiaGPUObject image, - fml::tracing::TraceFlow flow) { - ui_runner->PostTask( - fml::MakeCopyable([callback, descriptor, image = std::move(image), - flow = std::move(flow)]() mutable { - // We are going to terminate the trace flow here. Flows cannot - // terminate without a base trace. Add one explicitly. - TRACE_EVENT0("flutter", "ImageDecodeCallback"); - flow.End(); - callback(std::move(image)); - })); - }; + auto result = + [callback, raw_descriptor, ui_runner = runners_.GetUITaskRunner()]( + SkiaGPUObject image, fml::tracing::TraceFlow flow) { + ui_runner->PostTask(fml::MakeCopyable( + [callback, raw_descriptor, image = std::move(image), + flow = std::move(flow)]() mutable { + // We are going to terminate the trace flow here. Flows cannot + // terminate without a base trace. Add one explicitly. + TRACE_EVENT0("flutter", "ImageDecodeCallback"); + flow.End(); + callback(std::move(image)); + raw_descriptor->Release(); + })); + }; - if (!descriptor->data() || descriptor->data()->size() == 0) { + if (!raw_descriptor->data() || raw_descriptor->data()->size() == 0) { result({}, std::move(flow)); return; } concurrent_task_runner_->PostTask( - fml::MakeCopyable([descriptor, // + fml::MakeCopyable([raw_descriptor, // io_manager = io_manager_, // io_runner = runners_.GetIOTaskRunner(), // result, // @@ -258,16 +275,15 @@ void ImageDecoder::Decode(fml::RefPtr descriptor, // Step 1: Decompress the image. // On Worker. - auto decompressed = - descriptor->is_compressed() - ? ImageFromCompressedData(std::move(descriptor), // - target_width, // - target_height, // - flow) - : ImageFromDecompressedData(std::move(descriptor), // - target_width, // - target_height, // - flow); + auto decompressed = raw_descriptor->is_compressed() + ? ImageFromCompressedData(raw_descriptor, // + target_width, // + target_height, // + flow) + : ImageFromDecompressedData(raw_descriptor, // + target_width, // + target_height, // + flow); if (!decompressed) { FML_LOG(ERROR) << "Could not decompress image."; @@ -283,7 +299,8 @@ void ImageDecoder::Decode(fml::RefPtr descriptor, std::move(flow)]() mutable { if (!io_manager) { FML_LOG(ERROR) << "Could not acquire IO manager."; - return result({}, std::move(flow)); + result({}, std::move(flow)); + return; } // If the IO manager does not have a resource context, the caller diff --git a/engine/src/flutter/lib/ui/painting/image_decoder.h b/engine/src/flutter/lib/ui/painting/image_decoder.h index dbcf4701d04..e5f2fba6df1 100644 --- a/engine/src/flutter/lib/ui/painting/image_decoder.h +++ b/engine/src/flutter/lib/ui/painting/image_decoder.h @@ -61,7 +61,7 @@ class ImageDecoder { FML_DISALLOW_COPY_AND_ASSIGN(ImageDecoder); }; -sk_sp ImageFromCompressedData(fml::RefPtr descriptor, +sk_sp ImageFromCompressedData(ImageDescriptor* descriptor, uint32_t target_width, uint32_t target_height, const fml::tracing::TraceFlow& flow); diff --git a/engine/src/flutter/lib/ui/painting/image_decoder_unittests.cc b/engine/src/flutter/lib/ui/painting/image_decoder_unittests.cc index 0a6dfbaed4d..2e37af243f2 100644 --- a/engine/src/flutter/lib/ui/painting/image_decoder_unittests.cc +++ b/engine/src/flutter/lib/ui/painting/image_decoder_unittests.cc @@ -556,10 +556,10 @@ TEST(ImageDecoderTest, VerifySimpleDecoding) { auto descriptor = fml::MakeRefCounted(std::move(data), std::move(codec)); - ASSERT_EQ( - ImageFromCompressedData(descriptor, 6, 2, fml::tracing::TraceFlow("")) - ->dimensions(), - SkISize::Make(6, 2)); + ASSERT_EQ(ImageFromCompressedData(descriptor.get(), 6, 2, + fml::tracing::TraceFlow("")) + ->dimensions(), + SkISize::Make(6, 2)); } TEST(ImageDecoderTest, VerifySubpixelDecodingPreservesExifOrientation) { @@ -574,8 +574,8 @@ TEST(ImageDecoderTest, VerifySubpixelDecodingPreservesExifOrientation) { ASSERT_EQ(SkISize::Make(600, 200), image->dimensions()); auto decode = [descriptor](uint32_t target_width, uint32_t target_height) { - return ImageFromCompressedData(descriptor, target_width, target_height, - fml::tracing::TraceFlow("")); + return ImageFromCompressedData(descriptor.get(), target_width, + target_height, fml::tracing::TraceFlow("")); }; auto expected_data = OpenFixtureAsSkData("Horizontal.png");