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

[Impeller] force software resize usage for GLES backend. #56511

Merged
auto-submit merged 11 commits into from
Nov 14, 2024
Merged

[Impeller] force software resize usage for GLES backend. #56511

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9187475
force gles to use software resize.
Nov 11, 2024
9874f49
invert condition.
Nov 11, 2024
1ccba5c
Merge branch 'main' of github.com:flutter/engine into use_software_re…
Nov 11, 2024
abde368
Fix unittests.
Nov 11, 2024
aec6be9
update unittests.
Nov 12, 2024
d11c8de
check caps.
Nov 12, 2024
acf8b3f
++
Nov 12, 2024
e27de09
fix caps.
Nov 12, 2024
83280c8
update doc comment.
Nov 12, 2024
4b2f2f5
++
Nov 12, 2024
cbd7afa
Merge branch 'main' into use_software_resize
Nov 13, 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
8 changes: 6 additions & 2 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 @@ -114,6 +114,7 @@ DecompressResult ImageDecoderImpeller::DecompressTexture(
SkISize target_size,
impeller::ISize max_texture_size,
bool supports_wide_gamut,
bool force_cpu_resize,
const std::shared_ptr<impeller::Allocator>& allocator) {
TRACE_EVENT0("impeller", __FUNCTION__);
if (!descriptor) {
Expand Down Expand Up @@ -238,7 +239,7 @@ DecompressResult ImageDecoderImpeller::DecompressTexture(
: std::optional<SkImageInfo>(image_info.makeDimensions(target_size));

if (source_size.width() > max_texture_size.width ||
source_size.height() > max_texture_size.height) {
source_size.height() > max_texture_size.height || force_cpu_resize) {
//----------------------------------------------------------------------------
/// 2. If the decoded image isn't the requested target size and the src size
/// exceeds the device max texture size, perform a slow CPU reisze.
Expand Down Expand Up @@ -529,7 +530,10 @@ void ImageDecoderImpeller::Decode(fml::RefPtr<ImageDescriptor> descriptor,
// Always decompress on the concurrent runner.
auto bitmap_result = DecompressTexture(
raw_descriptor, target_size, max_size_supported,
supports_wide_gamut, context->GetResourceAllocator());
/*supports_wide_gamut=*/supports_wide_gamut,
/*force_cpu_resize=*/
!context->GetCapabilities()->SupportsTextureToTextureBlits(),
Copy link
Member

@gaaclarke gaaclarke Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can we make this more apropos? Can it just check if the backend is opengles?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Nov 12, 2024

Choose a reason for hiding this comment

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

I think this is the right check. if we fix the texture blit issues then this lets us conditionally enable it for devices that support blit framebuffer

Copy link
Member

@gaaclarke gaaclarke Nov 12, 2024

Choose a reason for hiding this comment

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

SupportsTextureToTextureBlits() appears to be true only for the Metal backend. We wouldn't want this for Vulkan, no?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Nov 12, 2024

Choose a reason for hiding this comment

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

https://github.com/flutter/engine/blob/main/impeller/renderer/backend/vulkan/capabilities_vk.cc#L623-L625

Copy link
Member

@gaaclarke gaaclarke Nov 12, 2024

Choose a reason for hiding this comment

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

Okay, so this check is essentially a check for opengles. Why is this a different check than gl.BlitFramebuffer.IsAvailable()? Seems like they should be the same.

Copy link
Member

@gaaclarke gaaclarke Nov 12, 2024

Choose a reason for hiding this comment

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

Right, I mean why wasn't CapabilitiesGLES like this:

bool CapabilitiesGLES::SupportsTextureToTextureBlits() const {
  return gl.BlitFramebuffer.IsAvailable();
}

Or a version check for version 3? I think this code looks good considering everything, and since the Capabilities is just hardcoded to false we should be getting test coverage (assuming there is a golden test for this). But if the reason CapabilitiesGLES::SupportsTextureToTextureBlits() is false is because of these weird issues we should explain it in a comment in CapabilitiesGLES::SupportsTextureToTextureBlits(). I'm happy to write it, I'm just trying to understand why it was hardcoded to false before we identified these issues to make sure I understand everything.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Nov 12, 2024

Choose a reason for hiding this comment

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

Right, I mean why wasn't CapabilitiesGLES like this:

Because blit framebuffer is still misconfigured for the reisze path. Once that is fixed we can update the capabilities gles to do that.

Copy link
Member

@gaaclarke gaaclarke Nov 12, 2024

Choose a reason for hiding this comment

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

Okay, sounds good. Can we add a TODO to capabilities_gles.cc then? We can mark this PR as fixing the thing I've been looking into (and the upside-down textures I think). The TODO can point to an issue that is a performance improvement, not a bug.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Nov 12, 2024

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

@jonahwilliams jonahwilliams Nov 12, 2024

Choose a reason for hiding this comment

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

flutter/flutter#158523

context->GetResourceAllocator());
if (!bitmap_result.device_buffer) {
result(nullptr, bitmap_result.decode_error);
return;
Expand Down
1 change: 1 addition & 0 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 @@ -68,6 +68,7 @@ class ImageDecoderImpeller final : public ImageDecoder {
SkISize target_size,
impeller::ISize max_texture_size,
bool supports_wide_gamut,
bool force_cpu_resize,
const std::shared_ptr<impeller::Allocator>& allocator);

/// @brief Create a device private texture from the provided host buffer.
Expand Down
10 changes: 5 additions & 5 deletions lib/ui/painting/image_decoder_no_gl_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ TEST(ImageDecoderNoGLTest, ImpellerWideGamutDisplayP3) {
std::optional<DecompressResult> wide_result =
ImageDecoderImpeller::DecompressTexture(
descriptor.get(), SkISize::Make(100, 100), {100, 100},
/*supports_wide_gamut=*/true, allocator);
/*supports_wide_gamut=*/true, /*force_cpu_resize=*/false, allocator);
ASSERT_TRUE(wide_result.has_value());
ASSERT_EQ(wide_result->image_info.colorType(), kRGBA_F16_SkColorType);
ASSERT_TRUE(wide_result->image_info.colorSpace()->isSRGB());
Expand All @@ -124,7 +124,7 @@ TEST(ImageDecoderNoGLTest, ImpellerWideGamutDisplayP3) {
std::optional<DecompressResult> narrow_result =
ImageDecoderImpeller::DecompressTexture(
descriptor.get(), SkISize::Make(100, 100), {100, 100},
/*supports_wide_gamut=*/false, allocator);
/*supports_wide_gamut=*/false, /*force_cpu_resize=*/false, allocator);

ASSERT_TRUE(narrow_result.has_value());
ASSERT_EQ(narrow_result->image_info.colorType(), kRGBA_8888_SkColorType);
Expand Down Expand Up @@ -157,7 +157,7 @@ TEST(ImageDecoderNoGLTest, ImpellerWideGamutIndexedPng) {
std::optional<DecompressResult> wide_result =
ImageDecoderImpeller::DecompressTexture(
descriptor.get(), SkISize::Make(100, 100), {100, 100},
/*supports_wide_gamut=*/true, allocator);
/*supports_wide_gamut=*/true, /*force_cpu_resize=*/false, allocator);
ASSERT_EQ(wide_result->image_info.colorType(), kBGR_101010x_XR_SkColorType);
ASSERT_TRUE(wide_result->image_info.colorSpace()->isSRGB());

Expand All @@ -180,7 +180,7 @@ TEST(ImageDecoderNoGLTest, ImpellerWideGamutIndexedPng) {
std::optional<DecompressResult> narrow_result =
ImageDecoderImpeller::DecompressTexture(
descriptor.get(), SkISize::Make(100, 100), {100, 100},
/*supports_wide_gamut=*/false, allocator);
/*supports_wide_gamut=*/false, /*force_cpu_resize=*/false, allocator);

ASSERT_TRUE(narrow_result.has_value());
ASSERT_EQ(narrow_result->image_info.colorType(), kRGBA_8888_SkColorType);
Expand Down Expand Up @@ -210,7 +210,7 @@ TEST(ImageDecoderNoGLTest, ImepllerUnmultipliedAlphaPng) {
std::optional<DecompressResult> result =
ImageDecoderImpeller::DecompressTexture(
descriptor.get(), SkISize::Make(11, 11), {11, 11},
/*supports_wide_gamut=*/true, allocator);
/*supports_wide_gamut=*/true, /*force_cpu_resize=*/false, allocator);
ASSERT_EQ(result->image_info.colorType(), kRGBA_8888_SkColorType);

const SkPixmap& pixmap = result->sk_bitmap->pixmap();
Expand Down
25 changes: 17 additions & 8 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 @@ -446,7 +446,8 @@ TEST_F(ImageDecoderFixtureTest, ImpellerNullColorspace) {
std::optional<DecompressResult> decompressed =
ImageDecoderImpeller::DecompressTexture(
descriptor.get(), SkISize::Make(100, 100), {100, 100},
/*supports_wide_gamut=*/true, allocator);
/*supports_wide_gamut=*/true,
/*force_cpu_resize=*/false, allocator);
Copy link
Member

@gaaclarke gaaclarke Nov 12, 2024

Choose a reason for hiding this comment

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

Heads up, these weren't migrated to the new signature that takes in Capabilties.

ASSERT_TRUE(decompressed.has_value());
ASSERT_EQ(decompressed->image_info.colorType(), kRGBA_8888_SkColorType);
ASSERT_EQ(decompressed->image_info.colorSpace(), nullptr);
Expand All @@ -473,7 +474,8 @@ TEST_F(ImageDecoderFixtureTest, ImpellerPixelConversion32F) {
std::optional<DecompressResult> decompressed =
ImageDecoderImpeller::DecompressTexture(
descriptor.get(), SkISize::Make(100, 100), {100, 100},
/*supports_wide_gamut=*/true, allocator);
/*supports_wide_gamut=*/true,
/*force_cpu_resize=*/false, allocator);

ASSERT_TRUE(decompressed.has_value());
ASSERT_EQ(decompressed->image_info.colorType(), kRGBA_F16_SkColorType);
Expand Down Expand Up @@ -501,7 +503,7 @@ TEST_F(ImageDecoderFixtureTest, ImpellerWideGamutDisplayP3Opaque) {
std::optional<DecompressResult> wide_result =
ImageDecoderImpeller::DecompressTexture(
descriptor.get(), SkISize::Make(100, 100), {100, 100},
/*supports_wide_gamut=*/true, allocator);
/*supports_wide_gamut=*/true, /*force_cpu_resize=*/false, allocator);

ASSERT_TRUE(wide_result.has_value());
ASSERT_EQ(wide_result->image_info.colorType(), kBGR_101010x_XR_SkColorType);
Expand All @@ -526,7 +528,7 @@ TEST_F(ImageDecoderFixtureTest, ImpellerWideGamutDisplayP3Opaque) {
std::optional<DecompressResult> narrow_result =
ImageDecoderImpeller::DecompressTexture(
descriptor.get(), SkISize::Make(100, 100), {100, 100},
/*supports_wide_gamut=*/false, allocator);
/*supports_wide_gamut=*/false, /*force_cpu_resize=*/false, allocator);

ASSERT_TRUE(narrow_result.has_value());
ASSERT_EQ(narrow_result->image_info.colorType(), kRGBA_8888_SkColorType);
Expand All @@ -553,7 +555,7 @@ TEST_F(ImageDecoderFixtureTest, ImpellerNonWideGamut) {
std::optional<DecompressResult> result =
ImageDecoderImpeller::DecompressTexture(
descriptor.get(), SkISize::Make(600, 200), {600, 200},
/*supports_wide_gamut=*/true, allocator);
/*supports_wide_gamut=*/true, /*force_cpu_resize=*/false, allocator);

ASSERT_TRUE(result.has_value());
ASSERT_EQ(result->image_info.colorType(), kRGBA_8888_SkColorType);
Expand Down Expand Up @@ -808,25 +810,32 @@ TEST(ImageDecoderTest, VerifySimpleDecoding) {
std::make_shared<impeller::TestImpellerAllocator>();
auto result_1 = ImageDecoderImpeller::DecompressTexture(
descriptor.get(), SkISize::Make(6, 2), {1000, 1000},
/*supports_wide_gamut=*/false, allocator);
/*supports_wide_gamut=*/false, /*force_cpu_resize=*/false, allocator);
EXPECT_EQ(result_1.sk_bitmap->width(), 75);
EXPECT_EQ(result_1.sk_bitmap->height(), 25);

// Bitmap sizes reflect the scaled size if the source size is larger than
// max texture size even if destination size isn't max texture size.
auto result_2 = ImageDecoderImpeller::DecompressTexture(
descriptor.get(), SkISize::Make(6, 2), {10, 10},
/*supports_wide_gamut=*/false, allocator);
/*supports_wide_gamut=*/false, /*force_cpu_resize=*/false, allocator);
EXPECT_EQ(result_2.sk_bitmap->width(), 6);
EXPECT_EQ(result_2.sk_bitmap->height(), 2);

// If the destination size is larger than the max texture size the image
// is scaled down.
auto result_3 = ImageDecoderImpeller::DecompressTexture(
descriptor.get(), SkISize::Make(60, 20), {10, 10},
/*supports_wide_gamut=*/false, allocator);
/*supports_wide_gamut=*/false, /*force_cpu_resize=*/false, allocator);
EXPECT_EQ(result_3.sk_bitmap->width(), 10);
EXPECT_EQ(result_3.sk_bitmap->height(), 10);

// CPU resize is forced.
auto result_4 = ImageDecoderImpeller::DecompressTexture(
descriptor.get(), SkISize::Make(6, 2), {1000, 1000},
/*supports_wide_gamut=*/false, /*force_cpu_resize=*/true, allocator);
EXPECT_EQ(result_4.sk_bitmap->width(), 6);
EXPECT_EQ(result_4.sk_bitmap->height(), 2);
#endif // IMPELLER_SUPPORTS_RENDERING
}

Expand Down