-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Impeller] Add blit pass #34901
[Impeller] Add blit pass #34901
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nits about the API but otherwise looks great!
impeller/playground/playground.cc
Outdated
@@ -334,7 +334,8 @@ std::optional<DecompressedImage> Playground::LoadFixtureImageRGBA( | |||
} | |||
|
|||
std::shared_ptr<Texture> Playground::CreateTextureForFixture( | |||
const char* fixture_name) const { | |||
const char* fixture_name, | |||
std::optional<size_t> mip_count) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this just be enable_mipmapping
? Specifying the mip counts explicitly is perhaps a too expressive for what we need in the playgrounds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
namespace impeller { | ||
|
||
struct BlitCommand { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just use inheritance instead please. This is going to get out of hand once we add variants for texture-buffer copies, indirect buffers, etc..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will also make the blit-pass deal with just one AddCommand variant. We can figure out what each command does in its its subclass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my aggressive preference for contiguous memory perhaps getting the best of me. I'll change this to be a class hierarchy and make backend implementations for each command.
Note that this just the struct for encoding commands. Does the BlitPass API itself look OK to you, or would you rather we expose the BlitCommand hierarchy as part of the API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The BlitPass itself looks great to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to a class hierarchy.
|
||
if (source->GetTextureDescriptor().sample_count != | ||
destination->GetTextureDescriptor().sample_count) { | ||
VALIDATION_LOG << SPrintF( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you miss a return here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, nice catch.
Just an initial review, but IMO, using inheritance will make it easier to build fallbacks for ES2 as well and the blit-pass as a render mess can be contained to a single spot. |
Mipmaps are working on Metal/GLES and texture copying works on Metal. Working on the GLES texture copy. |
For now, yes. I think we need the ES2 fallback anyway though. Towards that end, once @iskakaushik has the Vulkan stuff semi functional, we might want to take a call on whether we want to use ES3 features at all. Instead, focusing all our energy on Vulkan. |
I think the presubmits should be fixed by a rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! One really minor comment issue. But otherwise LGTM.
|
||
namespace impeller { | ||
|
||
/// Mixin for dispatching metal commands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dubious comment. Perhaps just copy-pasta?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
if (!gl.BlitFramebuffer.IsAvailable()) { | ||
// TODO(bdero): Emulate the blit using a raster draw call here. | ||
FML_LOG(ERROR) << "Texture blit fallback not implemented yet for GLES2."; | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, I suppose we can't really use CopyTextureToTexture for real until we have this fallback in place anyhow, so might as well fail loudly by failing the whole frame. Done.
Resolves flutter/flutter#105148.
Add blit pass with texture->texture blit and mipmap generation commands. Implemented for Metal/GLES and stubbed for Vulkan.
For the initial GLES implementation of the texture blit, I used
glBlitFramebuffer
, which is a GLES3 proc. For this reason, I switched to the GLES3 header and added it as an optional proc in the resolver. I still need to add a fallback draw call for the ~7% of Android devices that don't support GLES3 -- I plan to add this in a later PR.Mipmap playground:
Screen.Recording.2022-07-27.at.4.44.09.AM.mov
impeller_unittests --gtest_filter="Play/RendererTest.CanGenerateMipmaps/*" --timeout=0