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

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Mar 7, 2023

Removes all usage of FML_OS_PHYSICAL_IOS. Instead, we create a new capabilities check and set this in the metal backend based on the metal GPU family. This allows programmable blending to work on M1 and M2 devices in additon to iOS devices.

The entity_pass logic that relied on FML_OS_PHYSICAL_IOS checks has been refactored to track whether an advanced blend or backdrop filter was used separately. This allows us to determine whether or not to end the pass when we have access to the device capabilities. I could have turned the compile time checks into runtime checks, but then I'd need to plumb device capabilities into the Canvas.

Fixes flutter/flutter#121886
Also part of flutter/flutter#120223 by enabling this on some macOS machines

@jonahwilliams jonahwilliams self-assigned this Mar 7, 2023
@jonahwilliams jonahwilliams changed the title [Impeller] reduce quantity of FML_OS_PHYSICAL_IOS checks [Impeller] Reduce quantity of FML_OS_PHYSICAL_IOS checks. Mar 7, 2023
@jonahwilliams
Copy link
Contributor Author

@jonahwilliams
Copy link
Contributor Author

This is called programmable blending, which looks like it is only supported on "Apple" GPU families

@jonahwilliams
Copy link
Contributor Author

OK, I've got runtime checks working correctly, and confirmed we can use programmable blending on M1 and M2 devices. Yay!

@jonahwilliams jonahwilliams changed the title [Impeller] Reduce quantity of FML_OS_PHYSICAL_IOS checks. [Impeller] Replace FML_OS_PHYSICAL_IOS compile check with runtime capabilties check based on metal GPU family Mar 7, 2023
@jonahwilliams jonahwilliams changed the title [Impeller] Replace FML_OS_PHYSICAL_IOS compile check with runtime capabilties check based on metal GPU family [Impeller] Replace FML_OS_PHYSICAL_IOS compile check with runtime capabilties check based on metal GPU family. Mar 7, 2023
@jonahwilliams jonahwilliams marked this pull request as ready for review March 8, 2023 00:12
@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.

Copy link
Contributor

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM with a minor nit on naming.


bool SupportsTextureToTextureBlits() const;

bool SupportsFramebufferBlending() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

SupportsFramebufferFetch maybe? That we use it for blending perhaps is not related to the feature being supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just saw you comments on what Apple calls it. SGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could call it programmable blending or framebuffer fetch. Your call, it doesn't really matter to me. Though it might be different than programmable blending on opengl/vulkan.

Copy link
Contributor

Choose a reason for hiding this comment

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

My vote if for framebuffer fetch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jonahwilliams
Copy link
Contributor Author

I think the iOS simulator might be lying about its support for this API 😆

@chinmaygarde
Copy link
Contributor

So, its simulating the device in the response for support but not actually supporting it :/

@jonahwilliams
Copy link
Contributor Author

Or I wonder if we're picking up the real macOS metal device but the simulator itself has different shims or validation layers? 🤷‍♂️

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 8, 2023
@auto-submit auto-submit bot merged commit e763087 into flutter:main Mar 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 8, 2023
@jonahwilliams jonahwilliams deleted the special_blend branch March 9, 2023 20:23
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 needs tests

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

[Impeller] FML_OS_PHYSICAL_IOS defines in the entity layer prevent Android optimizations.

2 participants