[Impeller] add run time shaders #168294

Hellomik2002 · 2025-05-04T16:05:48Z

flutter-dashboard · 2025-05-04T16:05:52Z

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

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.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

google-cla · 2025-05-04T16:05:53Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

chinmaygarde · 2025-07-16T18:52:40Z

@Hellomik2002 Can you please address the bots comments about the CLA? I am unable to review the patch unfortunately.

Hellomik2002 · 2025-07-19T11:45:56Z

@Hellomik2002 Can you please address the bots comments about the CLA? I am unable to review the patch unfortunately.

I already signed the contributor license agreement. Can you send link if I forget something.

chinmaygarde · 2025-07-21T18:02:21Z

Interesting. Usually, contributors say "I signed it" and the CLA bot says "all good". But I do see the CLA check being successful in the presubmits. Sorry about that, I'll review it!

Hellomik2002 · 2025-07-23T19:59:22Z

Interesting. Usually, contributors say "I signed it" and the CLA bot says "all good". But I do see the CLA check being successful in the presubmits. Sorry about that, I'll review it!

You are welcome. I will be happy to fix if something needed.

chinmaygarde

Barring some construction issues, I think the big thing missing here is that fact that the shader blobs contain no versioning or validity metadata. We could get into a situation where users persist these blobs but ImpellerC updates its assumptions about layout and such. We cannot guarantee that the old blobs will keep working and users will run into runtime errors.

The same issue is faced by the fragment program API and we need to address this sooner rather than later. Can you file an issue for this? Perhaps we can fix this once and for all in impellerc so both this API and the fragment program API can accept shaders via blobs and we aren't concerned about users accidentally running into runtime crashes and corruption.

chinmaygarde · 2025-07-28T18:12:45Z

engine/src/flutter/lib/gpu/shader.h

  std::unordered_map<std::string, UniformBinding> uniform_structs_;
  std::unordered_map<std::string, TextureBinding> uniform_textures_;
  std::vector<impeller::DescriptorSetLayout> descriptor_set_layouts_;
+  bool is_first_time_;


Initialize scalars in the header in case the ctor misses them (or we add a new ctor).

chinmaygarde · 2025-07-28T18:14:09Z

engine/src/flutter/lib/gpu/shader_library.cc

+    return tonic::ToDart(out_error.value());
+  }
+
+  auto mapping = std::make_unique<fml::NonOwnedMapping>(buffer, bufferLength);


A NonOwnedMapping without a release callback will make the library reference data that can be deleted at any point and will lead to a dangling reference (presumably till the VM collects the bytedata). There must be a copy here.

Hellomik2002 · 2025-07-29T14:23:46Z

Okay At the current moment I need just wait discussion about Shaders Metadata Issue ? And only after that It will be possible add runtime shaders in Flutter ?

chinmaygarde · 2025-07-29T17:15:00Z

Yeah, I am not comfortable getting us into a situation where persisted shaders can cause corruption or crashers in future engines. Is there already an issue filed for it? If not, I can file one and prioritize it.

Hellomik2002 · 2025-07-29T18:52:02Z

I created already #172899 . But yeap it is better you create. I will close my Issue

Hellomik2002 · 2025-09-21T22:01:35Z

@timmaffett I see your code is cleaner and I see that you solved a similar problem. I would be happy get some advise or solutions from you.

Piinks · 2025-11-24T22:48:53Z

Hi @Hellomik2002, thanks for contributing. Is this a change you would like to continue working on?

Hellomik2002 · 2025-11-25T11:23:46Z

Hi @Hellomik2002, thanks for contributing. Is this a change you would like to continue working on?

I need more details what need to be done. I can already upload shaders but also need to add versioning. It means I need write something special in shaderbundle and come up with a format. This topic should be disscussed firstly by flutter team

@chinmaygarde

Fixes #172899 This PR adds a format_version number to the root table of the 3 flat buffer formats that impellerc creates; runtime stages, shader bundles and shader archives. It also adds a 'format_version' key to the root table of the shader bundle json format for completeness (Though it could be argued that the json format does not require this, I feel it may still be useful there). This allows for format compatibility checks with future versions of the table as @chinmaygarde expressed concerns about in #168294 and @jonahwilliams mentions in #166944 . The current code only checks for compatibility with the current version, but this mechanism allows for backwards compatibility in the future and not handling flat buffers with formats with new version number. The big win here is that format_version numbers allow us to abort parsing anything that we may not understand that could possible cause problems, corruptions or crashes. The changes are simple and straightforward and I feel they lay a reasonable foundation for version handling for any future changes. The version numbers themselves are defined within the `runtime_stage_types.fbs`, `shader_bundle.fbs` and `shader_archive.fbs` where I feel they should be as they can directly be incremented if any changes to the flat buffer format are made. Mechanisms for handling of backwards compatibility in the future should be straightforward to make in the corresponding c++ code allowing for the continued support of older (lower) version numbers if so desired. This addresses #172899 and the other concerns mentioned in the issues/pr's mentioned above. I plan on including my tests for exclusion of non-matching versions - Existing test verify that the version checking confirms matching versions. I wanted to get this out there for discussion before spending more time on this in case this approach is rejected outright. I have a companion PR which adds FragmentProgram.fromBytes() to the engine (c++ and canvaskit/wasm). I wanted to separate the impellerc output versioning into it's own (this) PR. ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [X] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [X] I signed the [CLA]. - [X] I listed at least one issue that this PR fixes in the description above. - [X] I updated/added relevant documentation (doc comments with `///`). - [FORTHCOMING] I added new tests to check the change I am making, or this PR is [test-exempt]. - [X] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [X] All existing and new tests are passing. --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: gaaclarke <[email protected]> Co-authored-by: Aaron Clarke <[email protected]>

@chinmaygarde

Fixes flutter#172899 This PR adds a format_version number to the root table of the 3 flat buffer formats that impellerc creates; runtime stages, shader bundles and shader archives. It also adds a 'format_version' key to the root table of the shader bundle json format for completeness (Though it could be argued that the json format does not require this, I feel it may still be useful there). This allows for format compatibility checks with future versions of the table as @chinmaygarde expressed concerns about in flutter#168294 and @jonahwilliams mentions in flutter#166944 . The current code only checks for compatibility with the current version, but this mechanism allows for backwards compatibility in the future and not handling flat buffers with formats with new version number. The big win here is that format_version numbers allow us to abort parsing anything that we may not understand that could possible cause problems, corruptions or crashes. The changes are simple and straightforward and I feel they lay a reasonable foundation for version handling for any future changes. The version numbers themselves are defined within the `runtime_stage_types.fbs`, `shader_bundle.fbs` and `shader_archive.fbs` where I feel they should be as they can directly be incremented if any changes to the flat buffer format are made. Mechanisms for handling of backwards compatibility in the future should be straightforward to make in the corresponding c++ code allowing for the continued support of older (lower) version numbers if so desired. This addresses flutter#172899 and the other concerns mentioned in the issues/pr's mentioned above. I plan on including my tests for exclusion of non-matching versions - Existing test verify that the version checking confirms matching versions. I wanted to get this out there for discussion before spending more time on this in case this approach is rejected outright. I have a companion PR which adds FragmentProgram.fromBytes() to the engine (c++ and canvaskit/wasm). I wanted to separate the impellerc output versioning into it's own (this) PR. ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [X] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [X] I signed the [CLA]. - [X] I listed at least one issue that this PR fixes in the description above. - [X] I updated/added relevant documentation (doc comments with `///`). - [FORTHCOMING] I added new tests to check the change I am making, or this PR is [test-exempt]. - [X] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [X] All existing and new tests are passing. --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: gaaclarke <[email protected]> Co-authored-by: Aaron Clarke <[email protected]>

github-actions bot added the engine flutter/engine related. See also e: labels. label May 4, 2025

Hellomik2002 force-pushed the runtime_shaders_upload branch from c48d01c to 3c47ece Compare May 4, 2025 16:26

This was referenced May 7, 2025

Performance issues greensopinion/flutter-vector-map-tiles#120

Closed

[engine] add upload by bytes #166946

Closed

Hellomik2002 force-pushed the runtime_shaders_upload branch from c497f75 to 918e581 Compare May 8, 2025 16:27

[Impeller] add run time shaders

a35a3c6

Hellomik2002 force-pushed the runtime_shaders_upload branch from 918e581 to a35a3c6 Compare May 12, 2025 11:53

Hellomik2002 mentioned this pull request Jun 2, 2025

Allow for shader loading at runtime #169442

Closed

chinmaygarde reviewed Jul 28, 2025

View reviewed changes

Hellomik2002 mentioned this pull request Jul 29, 2025

Add versioning and validation metadata to Impeller shader blobs to ensure forward compatibility #172899

Closed

timmaffett mentioned this pull request Sep 17, 2025

Adds Impellerc flatbuffer format versioning. #175470

Merged

8 tasks

[Impeller] add run time shaders #168294

Are you sure you want to change the base?

[Impeller] add run time shaders #168294

Conversation

Hellomik2002 commented May 4, 2025

Uh oh!

flutter-dashboard bot commented May 4, 2025

Uh oh!

google-cla bot commented May 4, 2025

Uh oh!

chinmaygarde commented Jul 16, 2025

Uh oh!

Hellomik2002 commented Jul 19, 2025

Uh oh!

chinmaygarde commented Jul 21, 2025

Uh oh!

Hellomik2002 commented Jul 23, 2025

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

Hellomik2002 commented Jul 29, 2025

Uh oh!

chinmaygarde commented Jul 29, 2025

Uh oh!

Hellomik2002 commented Jul 29, 2025

Uh oh!

Hellomik2002 commented Sep 21, 2025

Uh oh!

Piinks commented Nov 24, 2025

Uh oh!

Hellomik2002 commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Hellomik2002 commented Nov 25, 2025 •

edited

Loading