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

[Impeller] Use fast path for CPU generated textures where possible#36466

Merged
auto-submit[bot] merged 8 commits intoflutter:mainfrom
dnfield:cheap_texture
Oct 3, 2022
Merged

[Impeller] Use fast path for CPU generated textures where possible#36466
auto-submit[bot] merged 8 commits intoflutter:mainfrom
dnfield:cheap_texture

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented Sep 28, 2022

Offers a slight improvement on glyph atlas rendering by making the texture upload basically free. Shows improvements locally on our text based benchmarks.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@dnfield
Copy link
Contributor Author

dnfield commented Sep 28, 2022

@jonahwilliams this might be useful for the Gradient textures as well, although those are tiny.

@dnfield dnfield changed the title Use fast path for CPU generated textures where possible [Impeller] Use fast path for CPU generated textures where possible Sep 28, 2022
@jonahwilliams
Copy link
Contributor

They can end up quite big sometimes, if the stops are close together.

@jonahwilliams
Copy link
Contributor

Cheap!

tim-eric-discount

@dnfield
Copy link
Contributor Author

dnfield commented Sep 28, 2022

This needs some work around how the row bytes are handled. Metal expects them to be aligned to a particular value.

@dnfield dnfield marked this pull request as draft September 28, 2022 17:54
@flutter-dashboard
Copy link

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

@dnfield dnfield marked this pull request as ready for review September 28, 2022 21:30
@dnfield
Copy link
Contributor Author

dnfield commented Sep 28, 2022

The tests are passing locally on an M1 mac, going to see if I can try them on an x64...

@dnfield
Copy link
Contributor Author

dnfield commented Sep 28, 2022

Testing that the pixmap dimensions match the texture mipmap size doesn't work across platforms anymore, because we might be allocating rowbytes beyond what the texture dimensions actually are because the platform needs us to do that to keep the alignment requirements for MTLBuffer/MTLTexture working.

@dnfield
Copy link
Contributor Author

dnfield commented Sep 28, 2022

(The check happened to work out fine on arm64 environments but not x64).

@dnfield
Copy link
Contributor Author

dnfield commented Sep 29, 2022

I've refactored this to update the DeviceBuffer API to make the allocation make more sense.

The AsTexture method will now do a texture upload on Vulkan and GLES, but metal has a faster path this way.

GLES could use https://registry.khronos.org/OpenGL/extensions/ARB/ARB_pixel_buffer_object.txt to improve things here, but I'm not sure if we want to pursue that. Vulkan should have something better but that can wait for a subsequent patch (@iskakaushik fyi)

@dnfield dnfield requested a review from iskakaushik September 29, 2022 22:00
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@chinmaygarde
Copy link
Contributor

I had thought of a variant of the AsTexture interface but eventually gave up because it couldn't be implemented across all APIs. The workaround you mention where a texture upload is performed in OpenGL leaves you susepctible to change to the host buffer you make after calling the AsTexture method not actually updating the texture in OpenGL as they would with Metal. I am nervous of having APIs not behaving consistently across all backends. Perhaps AsTexture should just fail on OpenGL and the caller then has to figure out to do the copy. We can have a wrapper method that makes this convenient of course.

@dnfield
Copy link
Contributor Author

dnfield commented Sep 30, 2022

I had thought of a variant of the AsTexture interface but eventually gave up because it couldn't be implemented across all APIs. The workaround you mention where a texture upload is performed in OpenGL leaves you susepctible to change to the host buffer you make after calling the AsTexture method not actually updating the texture in OpenGL as they would with Metal. I am nervous of having APIs not behaving consistently across all backends. Perhaps AsTexture should just fail on OpenGL and the caller then has to figure out to do the copy. We can have a wrapper method that makes this convenient of course.

Maybe something on the context about whether this is supported or not, like offscreen MSAA?

There are pixel buffer object extensions for GL as well but it's not clear to me how available they are (although anything on EGL 3 would have them).

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 3, 2022
@auto-submit auto-submit bot merged commit 0e3ceba into flutter:main Oct 3, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 3, 2022
@dnfield
Copy link
Contributor Author

dnfield commented Oct 3, 2022

That benchmark is unfortunately noisy - locally I had some runs look better and others not. But looking at a different app that just repeatedly pumped that frame, the frame times seemed better on average.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants