[Impeller] force software resize usage for GLES backend.

[jonahwilliams]

Should fix some of the wonderous bugs by avoiding the usage of gl blitframebuffer altogether.

Fixes flutter/flutter#158391

[gaaclarke]

FYI this sidesteps all the texture issues I'm seeing on the Pixel4a. The textures aren't upside down or missing.

The code looks good, I'd like a more apropos condition for the fallback and I'd like a golden image test that goes down this path. If the condition is just checking for opengles we'll get the golden image easily. If it's capabilities based we should mock out the capabilities to force this flow.

[gaaclarke]

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

[jonahwilliams]

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

[gaaclarke]

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

[jonahwilliams]

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

[gaaclarke]

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.

[gaaclarke]

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.

[jonahwilliams]

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.

[gaaclarke]

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.

[jonahwilliams]

Done

[jonahwilliams]

flutter/flutter#158523

[jonahwilliams]

It looks like we'd need to set up the entire engine to actually create an ImageDecodeImpeller instance, hence why all the existing unit tests just call one of the static methods. I'd prefer to have an integration test for this, which would require us to run a devicelab test.

[jonahwilliams]

I can cheat a bit and pass the capabilities into one of the static methods and have it do the determination though.

[gaaclarke]

It looks like we'd need to set up the entire engine to actually create an ImageDecodeImpeller instance, hence why all the existing unit tests just call one of the static methods. I'd prefer to have an integration test for this, which would require us to run a devicelab test.

There are decoder unit tests in ui_unittests, checkout image_decoder_unittests.cc. I had to add some there when I started decoding wide gamut images.

[jonahwilliams]

I did update those tests, the use the static methods of image decode impeller to avoid setting up the full engine. Anyway, I think the test I added which checks the capabilities is sufficient.

[gaaclarke]

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

[gaaclarke]

lgtm, modulo an addition to the TODO just to make sure someone doesn't look on a phone that doesn't have issues and turns it on by mistake.

[gaaclarke]

Suggested change

	// TODO(158388): switch this to true for improved performance
	// on GLES 3.0+ devices.
	// TODO(158388): Switch this to true for improved performance
	// on GLES 3.0+ devices. Note that this wasn't enabled because
	// there were some rendering issues on some devices.

[gaaclarke]

Also, I think this should point to a new issue. Then we can close out the 2 related to the weird blit behavior (since it won't show up anymore after this).

[jonahwilliams]

Filled flutter/flutter#158523

[gaaclarke]

I'm still seeing instances where images aren't loading. I'm not sure if this is the same issue just diminished or if it is a different issue.

[auto-submit]

auto label is removed for flutter/engine/56511, due to - The status or check suite Mac mac_clang_tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.