add FragmentProgram.fromBytes to c++ and web engines. #175479
Conversation
|
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. |
github-actions
bot
added
engine
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces FragmentProgram.fromBytes, a valuable addition that allows loading fragment shaders from byte data, extending beyond the asset bundle. The implementation is consistent across the C++ and web engines, and the refactoring in the C++ code to share logic between fromAsset and fromBytes is a nice improvement. My main feedback is to add a warning to the documentation about the potential for crashes if an incompatible shader version is loaded, as you've noted that versioning is handled in a separate PR.
| /// Creates a fragment program from the provided byte data. | ||
| /// | ||
| /// The byte data must be produced as the output of the `impellerc` | ||
| /// compiler. The constructed object should then be reused via the | ||
| /// [fragmentShader] method to create [Shader] objects that can be used by | ||
| /// [Paint.shader]. |
There was a problem hiding this comment.
Given that loading shaders from bytes can be risky without version checking (as you mentioned in the PR description regarding #175470), it would be beneficial to add a warning to the documentation for fromBytes. This will help prevent developers from accidentally crashing their applications by loading incompatible shader versions.
Here's a suggested addition to the doc comment:
/// Creates a fragment program from the provided byte data.
///
/// The byte data must be produced as the output of the `impellerc`
/// compiler. The constructed object should then be reused via the
/// [fragmentShader] method to create [Shader] objects that can be used by
/// [Paint.shader].
///
/// > [!WARNING]
/// > The format of the byte data is tied to the version of the Flutter engine.
/// > Attempting to load a shader compiled with a different version of `impellerc`
/// > than the one used by the engine may result in a crash or unexpected behavior.There was a problem hiding this comment.
I stated that this PR goes hand in hand with #175470 and with such a requirement a doc comment would not be necessary because this PR should not be merged without #175470 being merged. I separated them as #175470 has other general advantages outside of protecting FragmentProgram (ie protecting ShaderLibrary, et al.)
gaaclarke
changed the title
harryterkelsen
left a comment
harryterkelsen
left a comment
There was a problem hiding this comment.
LGTM for the web bits. Don't have much familiarity with the native engine but the change looks pretty straightforward and correct to me there, too.
3 participants
Fixes #169442
Accomodates #167404 and some of #166944
This PR adds a FragmentProgram.fromBytes() method to allow loading of fragment shaders from outside the original asset bundle. This greatly extends the capabilities of the current restrictions of only allowing fragment shaders to be bundled in the build time asset bundle. (allowing many new capabilities, including items like the protection of shader source code via encryption, downloads of new shaders without reinstallation of program, runtime shader editing, ad infinitum). The referenced issues only partially touch upon why this feature is desperately needed.
This PR goes hand in hand with #175470 which allows versioning of impellerc outputs and protection for the engine from attempting to load incompatible versions of any of the impellerc output flat buffers. PR #175470 should be considered a required component of this PR to protect the engine from unexpected input.
I will incorporate unit tests for these capabilities which exercise the fromBytes() method - I wanted to initiate the conversation for now. I personally feel that the bang for the buck from this PR is extraordinary.
Other issues that this PR resolves or at least provides capabilities for that allow work-arounds:
resolves - #169442 - Allow for shader loading at runtime
partial - #171284 - Add support for loading assets with non-package paths in dart:ui APIs
(for the FragmentProgram.fromAsset() limitations .fromBytes() can be used to load your assets from
any AssetBundle)
Pre-launch Checklist
///).