Add setImageSampler (for replacing setSampler) #37821

bdero · 2022-11-21T23:07:11Z

setImageSampler guarantees we don't need an additional texture/render pass per-sampler. It's possible for users to emulate the functionality of setSampler in-shader.

The intention is to do the following before the stable branch cut:

Land this and roll into the framework.
Migrate stuff in the framework from setSampler to setImageSampler.
Remove setSampler.

I filed flutter/flutter#115794 along the way.

flutter-dashboard · 2022-11-21T23:07:14Z

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.

bdero · 2022-11-21T23:10:04Z

I'm working on testing this against the example apps we have floating around.

zanderso

lgtm w/ nit.

zanderso · 2022-11-21T23:12:26Z

lib/ui/painting.dart

@@ -4314,13 +4314,27 @@ class FragmentShader extends Shader {
    _floats[index] = value;
  }

+  /// Sets the sampler uniform at [index] to [image].
+  ///
+  /// The index provided to setSampler is the index of the sampler uniform defined


Suggested change

/// The index provided to setSampler is the index of the sampler uniform defined

/// The index provided to setImageSampler is the index of the sampler uniform defined

jonahwilliams · 2022-11-21T23:40:22Z

lib/ui/painting.dart

  /// Sets the sampler uniform at [index] to [sampler].
  ///
  /// The index provided to setSampler is the index of the sampler uniform defined
  /// in the fragment program, excluding all non-sampler uniforms.
  ///
  /// All the sampler uniforms that a shader expects must be provided or the
  /// results will be undefined.
+  ///
+  /// Use of this method may result in poor performance with the Impeller backend.


I would be careful about describing something as having poor performance with no refernce point. I've seen users go to extensive lengths to avoid invoking apis that have warnings like this, sometimes exceeding the original performance cost.

SGTM, removed this.

jonahwilliams

LGTM!

bdero · 2022-11-22T01:29:20Z

Added implementation for canvaskit

bdero · 2022-11-22T02:50:26Z

Smoke tested this on iOS and fixed an issue where the engine handle wasn't being passed in and it's working as expected now. Attempting to smoke test canvaskit.

jonahwilliams · 2022-11-22T02:52:07Z

I wouldn't worry about canvaskit right now, the required Skia API isn't available in most engines and I'll probably disable shader compilation for the next stable

bdero · 2022-11-22T02:55:27Z

Alrighty, I'll go ahead and merge after this CI run then.

auto-submit · 2022-11-22T03:36:22Z

auto label is removed for flutter/engine, pr: 37821, due to - The status or check suite Mac Unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

…ine#37821)

…115842) * eba7ecddc Add setImageSampler (for replacing setSampler) (flutter/engine#37821) * 15dce5e11 Roll Skia from 57b4252cf211 to 550fd51bd254 (3 revisions) (flutter/engine#37830) * 9b07044e7 Revert "[impeller] Remove declare_undefined_values (#37829)" (flutter/engine#37831) * 1a6180869 Roll Dart SDK from 6b8e98070f26 to 27c45cd51796 (1 revision) (flutter/engine#37833) * 8bc415423 Roll Skia from 550fd51bd254 to 94c63addc990 (1 revision) (flutter/engine#37834) * 7b4291548 Roll Skia from 94c63addc990 to 0dec6d1823b3 (4 revisions) (flutter/engine#37836) * a7fa7c6ec Roll Fuchsia Linux SDK from xBfEjlXUsix6Wka-i... to 5Xd8MJTM-pWPWCgUa... (flutter/engine#37840) * b6e33b527 Roll Fuchsia Mac SDK from oaqX39ssn3PUxE9it... to mwKdD1jX_KVP1U76z... (flutter/engine#37844) * 37e2aaa90 Roll Skia from 0dec6d1823b3 to 3b2d9e4bf668 (1 revision) (flutter/engine#37845)

…lutter#115842) * eba7ecddc Add setImageSampler (for replacing setSampler) (flutter/engine#37821) * 15dce5e11 Roll Skia from 57b4252cf211 to 550fd51bd254 (3 revisions) (flutter/engine#37830) * 9b07044e7 Revert "[impeller] Remove declare_undefined_values (flutter#37829)" (flutter/engine#37831) * 1a6180869 Roll Dart SDK from 6b8e98070f26 to 27c45cd51796 (1 revision) (flutter/engine#37833) * 8bc415423 Roll Skia from 550fd51bd254 to 94c63addc990 (1 revision) (flutter/engine#37834) * 7b4291548 Roll Skia from 94c63addc990 to 0dec6d1823b3 (4 revisions) (flutter/engine#37836) * a7fa7c6ec Roll Fuchsia Linux SDK from xBfEjlXUsix6Wka-i... to 5Xd8MJTM-pWPWCgUa... (flutter/engine#37840) * b6e33b527 Roll Fuchsia Mac SDK from oaqX39ssn3PUxE9it... to mwKdD1jX_KVP1U76z... (flutter/engine#37844) * 37e2aaa90 Roll Skia from 0dec6d1823b3 to 3b2d9e4bf668 (1 revision) (flutter/engine#37845)

bdero added 2 commits November 21, 2022 14:47

Add setImageSampler (for replacing setSampler)

f1daacf

Includes

ecb29be

bdero added the e: impeller label Nov 21, 2022

bdero requested review from zanderso and jonahwilliams November 21, 2022 23:07

bdero self-assigned this Nov 21, 2022

flutter-dashboard bot added the needs tests label Nov 21, 2022

Remove deprecation tag, just in case there are roll problems

4190467

zanderso approved these changes Nov 21, 2022

View reviewed changes

jonahwilliams reviewed Nov 21, 2022

View reviewed changes

bdero added 2 commits November 21, 2022 15:48

Remove comment

5a5021f

Fix comment

6e9c9a0

jonahwilliams approved these changes Nov 22, 2022

View reviewed changes

Add setImageSampler for canvaskit

e43e5f5

bdero added 5 commits November 21, 2022 17:34

Remove arg

f56bfed

Fix conversion

0a353bb

Float64List

0109a0d

final...

0d00c73

Pass along the engine handle

da6f1cb

bdero added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2022

bdero added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2022

auto-submit bot merged commit eba7ecd into flutter:main Nov 22, 2022

bdero mentioned this pull request Nov 22, 2022

Remove setSampler from FragmentShader #37839

Merged

engine-flutter-autoroll mentioned this pull request Nov 22, 2022

Roll Flutter Engine from c3645c3b8947 to 37e2aaa901e8 (9 revisions) flutter/flutter#115842

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 22, 2022

eba7ecddc Add setImageSampler (for replacing setSampler) (flutter/eng…

c8ec06b

…ine#37821)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setImageSampler (for replacing setSampler) #37821

Add setImageSampler (for replacing setSampler) #37821

bdero commented Nov 21, 2022

flutter-dashboard bot commented Nov 21, 2022

bdero commented Nov 21, 2022

zanderso left a comment

zanderso Nov 21, 2022

bdero Nov 21, 2022

jonahwilliams Nov 21, 2022

bdero Nov 21, 2022

jonahwilliams left a comment

bdero commented Nov 22, 2022

bdero commented Nov 22, 2022

jonahwilliams commented Nov 22, 2022

bdero commented Nov 22, 2022

auto-submit bot commented Nov 22, 2022

	/// The index provided to setSampler is the index of the sampler uniform defined
	/// The index provided to setImageSampler is the index of the sampler uniform defined

Add setImageSampler (for replacing setSampler) #37821

Add setImageSampler (for replacing setSampler) #37821

Conversation

bdero commented Nov 21, 2022

flutter-dashboard bot commented Nov 21, 2022

bdero commented Nov 21, 2022

zanderso left a comment

Choose a reason for hiding this comment

zanderso Nov 21, 2022

Choose a reason for hiding this comment

bdero Nov 21, 2022

Choose a reason for hiding this comment

jonahwilliams Nov 21, 2022

Choose a reason for hiding this comment

bdero Nov 21, 2022

Choose a reason for hiding this comment

jonahwilliams left a comment

Choose a reason for hiding this comment

bdero commented Nov 22, 2022

bdero commented Nov 22, 2022

jonahwilliams commented Nov 22, 2022

bdero commented Nov 22, 2022

auto-submit bot commented Nov 22, 2022