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

[Impeller] Avoid encoding metal commands while the GPU is unavailable when decoding images. #42349

Merged
auto-submit merged 6 commits into from
May 26, 2023
Merged

[Impeller] Avoid encoding metal commands while the GPU is unavailable when decoding images. #42349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b14952e
[Impeller] Avoid encoding metal commands while the GPU is unavailable…
dnfield May 26, 2023
86e0411
Aaron review
dnfield May 26, 2023
1cbd4cf
test
dnfield May 26, 2023
4ca1ed1
Update lib/ui/painting/image_decoder_impeller.cc
dnfield May 26, 2023
020469e
Update lib/ui/painting/image_decoder_impeller.cc
dnfield May 26, 2023
3f456de
bdero/gaaclarke review
dnfield May 26, 2023
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
3 changes: 3 additions & 0 deletions impeller/renderer/backend/metal/render_pass_mtl.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,10 +385,13 @@ static bool Bind(PassBindingsCache& pass,
}

if (texture.NeedsMipmapGeneration()) {
// TODO(127697): generate mips when the GPU is available on iOS.
#if !FML_OS_IOS
VALIDATION_LOG
<< "Texture at binding index " << bind_index
<< " has a mip count > 1, but the mipmap has not been generated.";
return false;
#endif // !FML_OS_IOS
}

return pass.SetTexture(stage, bind_index,
Expand Down
6 changes: 4 additions & 2 deletions lib/ui/painting/image_decoder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,16 @@ std::unique_ptr<ImageDecoder> ImageDecoder::Make(
const Settings& settings,
const TaskRunners& runners,
std::shared_ptr<fml::ConcurrentTaskRunner> concurrent_task_runner,
fml::WeakPtr<IOManager> io_manager) {
fml::WeakPtr<IOManager> io_manager,
const std::shared_ptr<fml::SyncSwitch>& gpu_disabled_switch) {
#if IMPELLER_SUPPORTS_RENDERING
if (settings.enable_impeller) {
return std::make_unique<ImageDecoderImpeller>(
runners, //
std::move(concurrent_task_runner), //
std::move(io_manager), //
settings.enable_wide_gamut);
settings.enable_wide_gamut, //
gpu_disabled_switch);
}
#endif // IMPELLER_SUPPORTS_RENDERING
return std::make_unique<ImageDecoderSkia>(
Expand Down
3 changes: 2 additions & 1 deletion lib/ui/painting/image_decoder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ class ImageDecoder {
const Settings& settings,
const TaskRunners& runners,
std::shared_ptr<fml::ConcurrentTaskRunner> concurrent_task_runner,
fml::WeakPtr<IOManager> io_manager);
fml::WeakPtr<IOManager> io_manager,
const std::shared_ptr<fml::SyncSwitch>& gpu_disabled_switch);

virtual ~ImageDecoder();

Expand Down
133 changes: 85 additions & 48 deletions lib/ui/painting/image_decoder_impeller.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,11 @@ ImageDecoderImpeller::ImageDecoderImpeller(
const TaskRunners& runners,
std::shared_ptr<fml::ConcurrentTaskRunner> concurrent_task_runner,
const fml::WeakPtr<IOManager>& io_manager,
bool supports_wide_gamut)
bool supports_wide_gamut,
const std::shared_ptr<fml::SyncSwitch>& gpu_disabled_switch)
: ImageDecoder(runners, std::move(concurrent_task_runner), io_manager),
supports_wide_gamut_(supports_wide_gamut) {
supports_wide_gamut_(supports_wide_gamut),
gpu_disabled_switch_(gpu_disabled_switch) {
std::promise<std::shared_ptr<impeller::Context>> context_promise;
context_ = context_promise.get_future();
runners_.GetIOTaskRunner()->PostTask(fml::MakeCopyable(
Expand Down Expand Up @@ -246,18 +248,11 @@ DecompressResult ImageDecoderImpeller::DecompressTexture(
.image_info = scaled_bitmap->info()};
}

std::pair<sk_sp<DlImage>, std::string>
ImageDecoderImpeller::UploadTextureToPrivate(
/// Only call this method if the GPU is available.
static std::pair<sk_sp<DlImage>, std::string> UnsafeUploadTextureToPrivate(
const std::shared_ptr<impeller::Context>& context,
const std::shared_ptr<impeller::DeviceBuffer>& buffer,
const SkImageInfo& image_info) {
TRACE_EVENT0("impeller", __FUNCTION__);
if (!context) {
return std::make_pair(nullptr, "No Impeller context is available");
}
if (!buffer) {
return std::make_pair(nullptr, "No Impeller device buffer is available");
}
const auto pixel_format =
impeller::skia_conversions::ToPixelFormat(image_info.colorType());
if (!pixel_format) {
Expand Down Expand Up @@ -318,10 +313,40 @@ ImageDecoderImpeller::UploadTextureToPrivate(
impeller::DlImageImpeller::Make(std::move(dest_texture)), std::string());
}

std::pair<sk_sp<DlImage>, std::string>
ImageDecoderImpeller::UploadTextureToPrivate(
const std::shared_ptr<impeller::Context>& context,
const std::shared_ptr<impeller::DeviceBuffer>& buffer,
const SkImageInfo& image_info,
std::shared_ptr<SkBitmap> bitmap,
const std::shared_ptr<fml::SyncSwitch>& gpu_disabled_switch) {
TRACE_EVENT0("impeller", __FUNCTION__);
if (!context) {
return std::make_pair(nullptr, "No Impeller context is available");
}
if (!buffer) {
return std::make_pair(nullptr, "No Impeller device buffer is available");
}

std::pair<sk_sp<DlImage>, std::string> result;
gpu_disabled_switch->Execute(
fml::SyncSwitch::Handlers()
.SetIfFalse([&result, context, buffer, image_info] {
result = UnsafeUploadTextureToPrivate(context, buffer, image_info);
})
.SetIfTrue([&result, context, bitmap, gpu_disabled_switch] {
// create_mips is false because we already know the GPU is disabled.
Copy link
Member

@gaaclarke gaaclarke May 26, 2023

Choose a reason for hiding this comment

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

Can't this result in suboptimal rendering based on when the gpu is disabled? Are we going to get bug reports about blurry images that we can't source back to coming from backgrounded apps?

Copy link
Contributor Author

@dnfield dnfield May 26, 2023

Choose a reason for hiding this comment

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

It can result in suboptimal rendering. We could choose to build mips when the GPU is available again, but that's going to be a more invasive/risky change. I'd like this to be cherry pickable.

result = UploadTextureToShared(context, bitmap, gpu_disabled_switch,
/*create_mips=*/false);
}));
return result;
}

std::pair<sk_sp<DlImage>, std::string>
ImageDecoderImpeller::UploadTextureToShared(
const std::shared_ptr<impeller::Context>& context,
std::shared_ptr<SkBitmap> bitmap,
const std::shared_ptr<fml::SyncSwitch>& gpu_disabled_switch,
bool create_mips) {
TRACE_EVENT0("impeller", __FUNCTION__);
if (!context) {
Expand Down Expand Up @@ -370,32 +395,40 @@ ImageDecoderImpeller::UploadTextureToShared(
texture->SetLabel(impeller::SPrintF("ui.Image(%p)", texture.get()).c_str());

if (texture_descriptor.mip_count > 1u && create_mips) {
auto command_buffer = context->CreateCommandBuffer();
if (!command_buffer) {
std::string decode_error(
"Could not create command buffer for mipmap generation.");
FML_DLOG(ERROR) << decode_error;
return std::make_pair(nullptr, decode_error);
std::optional<std::string> decode_error;

// The only platform that needs mipmapping unconditionally is GL.
// GL based platforms never disable GPU access.
// This is only really needed for iOS.
gpu_disabled_switch->Execute(fml::SyncSwitch::Handlers().SetIfFalse(
Copy link
Member

@gaaclarke gaaclarke May 26, 2023

Choose a reason for hiding this comment

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

Should the true branch be setting decode_error?

Copy link
Contributor Author

@dnfield dnfield May 26, 2023

Choose a reason for hiding this comment

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

No - added a comment. We only need to do this unconditionally for GL,and we'll always unconditionally do it for GL.

Copy link
Member

@gaaclarke gaaclarke May 26, 2023

Choose a reason for hiding this comment

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

We ripped out opengl for iOS right?

Copy link
Contributor Author

@dnfield dnfield May 26, 2023

Choose a reason for hiding this comment

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

Correct

[context, &texture, &decode_error] {
auto command_buffer = context->CreateCommandBuffer();
if (!command_buffer) {
decode_error =
"Could not create command buffer for mipmap generation.";
return;
}
command_buffer->SetLabel("Mipmap Command Buffer");

auto blit_pass = command_buffer->CreateBlitPass();
if (!blit_pass) {
decode_error = "Could not create blit pass for mipmap generation.";
return;
}
blit_pass->SetLabel("Mipmap Blit Pass");
blit_pass->GenerateMipmap(texture);
Copy link
Member

@bdero bdero May 26, 2023

Choose a reason for hiding this comment

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

The texture.NeedsMipmapGeneration() check also needs to be removed from render_pass_mtl.mm.

Copy link
Contributor Author

@dnfield dnfield May 26, 2023

Choose a reason for hiding this comment

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

Should we change that to a DLOG on iOS?

Copy link
Contributor Author

@dnfield dnfield May 26, 2023

Choose a reason for hiding this comment

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

I ifdef'd it out for IOS.

Copy link
Member

@bdero bdero May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think we need to rethink these safety checks in general and have the HAL be less opinionated about it. While it's unsafe for GLES and probably worth tracking in some way, fully mipmapped textures can be uploaded and/or single mip levels can be replaced individually (albeit, Impeller doesn't support this yet). One example is storing radiance maps for image based lighting.


blit_pass->EncodeCommands(context->GetResourceAllocator());
if (!command_buffer->SubmitCommands()) {
decode_error = "Failed to submit blit pass command buffer.";
return;
}
command_buffer->WaitUntilScheduled();
}));
Copy link
Member

@bdero bdero May 26, 2023

Choose a reason for hiding this comment

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

This isn't something we should solve now, but we'll eventually need a solution to defer generating mipmaps. Not generating mipmaps will cause min filtered images to continue appearing more blurry than Skia once we start properly setting the mip bias.

Copy link
Contributor Author

@dnfield dnfield May 26, 2023

Choose a reason for hiding this comment

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

Filed flutter/flutter#127697

if (decode_error.has_value()) {
FML_DLOG(ERROR) << decode_error.value();
return std::make_pair(nullptr, decode_error.value());
}
command_buffer->SetLabel("Mipmap Command Buffer");

auto blit_pass = command_buffer->CreateBlitPass();
if (!blit_pass) {
std::string decode_error(
"Could not create blit pass for mipmap generation.");
FML_DLOG(ERROR) << decode_error;
return std::make_pair(nullptr, decode_error);
}
blit_pass->SetLabel("Mipmap Blit Pass");
blit_pass->GenerateMipmap(texture);

blit_pass->EncodeCommands(context->GetResourceAllocator());
if (!command_buffer->SubmitCommands()) {
std::string decode_error("Failed to submit blit pass command buffer.");
FML_DLOG(ERROR) << decode_error;
return std::make_pair(nullptr, decode_error);
}
command_buffer->WaitUntilScheduled();
}

return std::make_pair(impeller::DlImageImpeller::Make(std::move(texture)),
Expand Down Expand Up @@ -429,8 +462,8 @@ void ImageDecoderImpeller::Decode(fml::RefPtr<ImageDescriptor> descriptor,
target_size = SkISize::Make(target_width, target_height), //
io_runner = runners_.GetIOTaskRunner(), //
result,
supports_wide_gamut = supports_wide_gamut_ //
]() {
supports_wide_gamut = supports_wide_gamut_, //
gpu_disabled_switch = gpu_disabled_switch_]() {
if (!context) {
result(nullptr, "No Impeller context is available");
return;
Expand All @@ -446,24 +479,28 @@ void ImageDecoderImpeller::Decode(fml::RefPtr<ImageDescriptor> descriptor,
result(nullptr, bitmap_result.decode_error);
return;
}
auto upload_texture_and_invoke_result = [result, context,
bitmap_result]() {
// TODO(jonahwilliams): remove ifdef once blit from buffer to
// texture is implemented on other platforms.
auto upload_texture_and_invoke_result = [result, context, bitmap_result,
gpu_disabled_switch]() {
// TODO(jonahwilliams): remove ifdef once blit from buffer
// to texture is implemented on other platforms.
sk_sp<DlImage> image;
std::string decode_error;

#ifdef FML_OS_IOS
std::tie(image, decode_error) = UploadTextureToPrivate(
context, bitmap_result.device_buffer, bitmap_result.image_info);
context, bitmap_result.device_buffer, bitmap_result.image_info,
bitmap_result.sk_bitmap, gpu_disabled_switch);
#else
std::tie(image, decode_error) =
UploadTextureToShared(context, bitmap_result.sk_bitmap);
#endif
UploadTextureToShared(context, bitmap_result.sk_bitmap,
gpu_disabled_switch, /*create_mips=*/true);
#endif // FML_OS_IOS
result(image, decode_error);
};
// TODO(jonahwilliams): https://github.com/flutter/flutter/issues/123058
// Technically we don't need to post tasks to the io runner, but without
// this forced serialization we can end up overloading the GPU and/or
// TODO(jonahwilliams):
// https://github.com/flutter/flutter/issues/123058 Technically we
// don't need to post tasks to the io runner, but without this
// forced serialization we can end up overloading the GPU and/or
// competing with raster workloads.
io_runner->PostTask(upload_texture_and_invoke_result);
});
Expand Down
13 changes: 11 additions & 2 deletions lib/ui/painting/image_decoder_impeller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ class ImageDecoderImpeller final : public ImageDecoder {
const TaskRunners& runners,
std::shared_ptr<fml::ConcurrentTaskRunner> concurrent_task_runner,
const fml::WeakPtr<IOManager>& io_manager,
bool supports_wide_gamut);
bool supports_wide_gamut,
const std::shared_ptr<fml::SyncSwitch>& gpu_disabled_switch);

~ImageDecoderImpeller() override;

Expand All @@ -72,27 +73,35 @@ class ImageDecoderImpeller final : public ImageDecoder {
/// @param context The Impeller graphics context.
/// @param buffer A host buffer containing the image to be uploaded.
/// @param image_info Format information about the particular image.
/// @param bitmap A bitmap containg the image to be uploaded.
/// @param gpu_disabled_switch Whether the GPU is available command encoding.
/// @return A DlImage.
static std::pair<sk_sp<DlImage>, std::string> UploadTextureToPrivate(
const std::shared_ptr<impeller::Context>& context,
const std::shared_ptr<impeller::DeviceBuffer>& buffer,
const SkImageInfo& image_info);
const SkImageInfo& image_info,
std::shared_ptr<SkBitmap> bitmap,
const std::shared_ptr<fml::SyncSwitch>& gpu_disabled_switch);

/// @brief Create a host visible texture from the provided bitmap.
/// @param context The Impeller graphics context.
/// @param bitmap A bitmap containg the image to be uploaded.
/// @param create_mips Whether mipmaps should be generated for the given
/// image.
/// @param gpu_disabled_switch Whether the GPU is available for mipmap
/// creation.
/// @return A DlImage.
static std::pair<sk_sp<DlImage>, std::string> UploadTextureToShared(
const std::shared_ptr<impeller::Context>& context,
std::shared_ptr<SkBitmap> bitmap,
const std::shared_ptr<fml::SyncSwitch>& gpu_disabled_switch,
bool create_mips = true);

private:
using FutureContext = std::shared_future<std::shared_ptr<impeller::Context>>;
FutureContext context_;
const bool supports_wide_gamut_;
std::shared_ptr<fml::SyncSwitch> gpu_disabled_switch_;

FML_DISALLOW_COPY_AND_ASSIGN(ImageDecoderImpeller);
};
Expand Down
58 changes: 46 additions & 12 deletions lib/ui/painting/image_decoder_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,12 @@ class TestImpellerContext : public impeller::Context {
}

std::shared_ptr<CommandBuffer> CreateCommandBuffer() const override {
command_buffer_count_ += 1;
return nullptr;
}

mutable size_t command_buffer_count_ = 0;

private:
std::shared_ptr<const Capabilities> capabilities_;
};
Expand Down Expand Up @@ -280,7 +283,8 @@ TEST_F(ImageDecoderFixtureTest, CanCreateImageDecoder) {
TestIOManager manager(runners.GetIOTaskRunner());
Settings settings;
auto decoder = ImageDecoder::Make(settings, runners, loop->GetTaskRunner(),
manager.GetWeakIOManager());
manager.GetWeakIOManager(),
std::make_shared<fml::SyncSwitch>());
ASSERT_NE(decoder, nullptr);
});
}
Expand Down Expand Up @@ -331,7 +335,8 @@ TEST_F(ImageDecoderFixtureTest, InvalidImageResultsError) {
TestIOManager manager(runners.GetIOTaskRunner());
Settings settings;
auto decoder = ImageDecoder::Make(settings, runners, loop->GetTaskRunner(),
manager.GetWeakIOManager());
manager.GetWeakIOManager(),
std::make_shared<fml::SyncSwitch>());

auto data = OpenFixtureAsSkData("ThisDoesNotExist.jpg");
ASSERT_FALSE(data);
Expand Down Expand Up @@ -370,9 +375,9 @@ TEST_F(ImageDecoderFixtureTest, ValidImageResultsInSuccess) {
};
auto decode_image = [&]() {
Settings settings;
std::unique_ptr<ImageDecoder> image_decoder =
ImageDecoder::Make(settings, runners, loop->GetTaskRunner(),
io_manager->GetWeakIOManager());
std::unique_ptr<ImageDecoder> image_decoder = ImageDecoder::Make(
settings, runners, loop->GetTaskRunner(),
io_manager->GetWeakIOManager(), std::make_shared<fml::SyncSwitch>());

auto data = OpenFixtureAsSkData("DashInNooglerHat.jpg");

Expand Down Expand Up @@ -426,6 +431,34 @@ float HalfToFloat(uint16_t half) {
}
} // namespace

TEST_F(ImageDecoderFixtureTest, ImpellerUploadToSharedNoGpu) {
#if !IMPELLER_SUPPORTS_RENDERING
GTEST_SKIP() << "Impeller only test.";
#endif // IMPELLER_SUPPORTS_RENDERING

auto no_gpu_access_context =
std::make_shared<impeller::TestImpellerContext>();
auto gpu_disabled_switch = std::make_shared<fml::SyncSwitch>(true);

auto info = SkImageInfo::Make(10, 10, SkColorType::kRGBA_8888_SkColorType,
SkAlphaType::kPremul_SkAlphaType);
auto bitmap = std::make_shared<SkBitmap>();
bitmap->allocPixels(info, 10 * 4);
impeller::DeviceBufferDescriptor desc;
desc.size = bitmap->computeByteSize();
auto buffer = std::make_shared<impeller::TestImpellerDeviceBuffer>(desc);

auto result = ImageDecoderImpeller::UploadTextureToPrivate(
no_gpu_access_context, buffer, info, bitmap, gpu_disabled_switch);
ASSERT_EQ(no_gpu_access_context->command_buffer_count_, 0ul);
ASSERT_EQ(result.second, "");

result = ImageDecoderImpeller::UploadTextureToShared(
no_gpu_access_context, bitmap, gpu_disabled_switch, true);
ASSERT_EQ(no_gpu_access_context->command_buffer_count_, 0ul);
ASSERT_EQ(result.second, "");
}

TEST_F(ImageDecoderFixtureTest, ImpellerNullColorspace) {
auto info = SkImageInfo::Make(10, 10, SkColorType::kRGBA_8888_SkColorType,
SkAlphaType::kPremul_SkAlphaType);
Expand Down Expand Up @@ -642,9 +675,9 @@ TEST_F(ImageDecoderFixtureTest, ExifDataIsRespectedOnDecode) {
SkISize decoded_size = SkISize::MakeEmpty();
auto decode_image = [&]() {
Settings settings;
std::unique_ptr<ImageDecoder> image_decoder =
ImageDecoder::Make(settings, runners, loop->GetTaskRunner(),
io_manager->GetWeakIOManager());
std::unique_ptr<ImageDecoder> image_decoder = ImageDecoder::Make(
settings, runners, loop->GetTaskRunner(),
io_manager->GetWeakIOManager(), std::make_shared<fml::SyncSwitch>());

auto data = OpenFixtureAsSkData("Horizontal.jpg");

Expand Down Expand Up @@ -703,9 +736,9 @@ TEST_F(ImageDecoderFixtureTest, CanDecodeWithoutAGPUContext) {

auto decode_image = [&]() {
Settings settings;
std::unique_ptr<ImageDecoder> image_decoder =
ImageDecoder::Make(settings, runners, loop->GetTaskRunner(),
io_manager->GetWeakIOManager());
std::unique_ptr<ImageDecoder> image_decoder = ImageDecoder::Make(
settings, runners, loop->GetTaskRunner(),
io_manager->GetWeakIOManager(), std::make_shared<fml::SyncSwitch>());

auto data = OpenFixtureAsSkData("DashInNooglerHat.jpg");

Expand Down Expand Up @@ -771,7 +804,8 @@ TEST_F(ImageDecoderFixtureTest, CanDecodeWithResizes) {
PostTaskSync(runners.GetUITaskRunner(), [&]() {
Settings settings;
image_decoder = ImageDecoder::Make(settings, runners, loop->GetTaskRunner(),
io_manager->GetWeakIOManager());
io_manager->GetWeakIOManager(),
std::make_shared<fml::SyncSwitch>());
});

// Setup a generic decoding utility that gives us the final decoded size.
Expand Down
Loading