[Impeller] Add StC color source/clip behavior + constexpr flag for enabling StC. #50817

bdero · 2024-02-21T08:40:47Z

Included in this PR is:

Behavior for clips/restores when StC is enabled.
Behavior for color source drawing when StC is enabled.
New filled path tessellator behavior.
A constexpr flag for enabling StC.

This patch shouldn't actually change any behavior when StC is off.

jonahwilliams · 2024-02-21T16:48:20Z

impeller/entity/contents/color_source_contents.h

+      if (is_stencil_then_cover) {
+        pass.SetStencilReference(0);
+
+        /// Stencil preparation draw.


What is the stencil preparation?

Oh i c, I think I'm just having a hard time following this code now. This is essentially taking whatever geometry we got and configuring how to treat it, in this case it goes to the stencil buffer. Then we add the cover + reset afterwards. makes sense!

jonahwilliams

LGTM!

chinmaygarde

This is awesome. Mostly nits. I was going to suggest factoring some of this out to reduce cyclomatic complexity since it is getting tough to follow. But I suspect once this is well baked, we won't have the giant constexpr check around kEnableStencilThenCover.

chinmaygarde · 2024-02-21T19:29:16Z

impeller/entity/contents/content_context.h

+  /// 2. Switches the generic tessellation fallback to use stencil-then-cover.
+  ///    See also: https://github.com/flutter/flutter/issues/123671
+  ///
+  // TODO(bdero): Remove this setting once StC is fully de-risked


I don't see an enumeration of anticipated risks. We should jot them down so we aren't surprised. Perhaps discuss during the weekly and I can prep. a list.

I don't think we need to do that in the source code, we can handle that in the issue tracker

Totally, just pointing out that we don't have that written down anywhere.

Just to clarify my thinking here around this flag:

I think "fully de-risked" wasn't quite the right terminology for me to use here. Overall this is a big change to the perf characteristics of paths and clips, and the biggest risk in my eyes is users encountering slow (but reasonable) usage patterns that I haven't come across/considered.

After the flag flip, overall risk will rapidly decay through:

Seeing benchmark & golden results

The team churning on tasks at ToT

The engine rolling into g3

Smoke testing (Wondrous, Gallery)

Profiling the trouble cases

I anticipate the flag's lifespan to be a few weeks. Maybe slightly longer if nontrivial problems pop up and the flag needs to get flipped back and forth a few times.

This flag impacts the config of most RenderPasses and draw calls, so if at some point we decide to keep it around for several months (which I doubt will happen), we should set up golden variations for both modes to avoid bitrot.

chinmaygarde · 2024-02-21T19:31:44Z

impeller/entity/entity.cc


 float Entity::GetShaderClipDepth() const {
-  return new_clip_depth_ * kDepthEpsilon;
+  return std::clamp(new_clip_depth_ * kDepthEpsilon, 0.0f, 1.0f);


Is this just defensive coding or did you actually run into this?

Oh, I see the max-uint setting below. Nevermind.

Yeah not sure about keeping the max int pattern... thinking about replacing with a SetMaxDepth or adding a kMaxDepth sentinel to make the code easier to read.

chinmaygarde · 2024-02-21T19:46:12Z

impeller/entity/contents/content_context.h

    /// stencil buffer after a stencil-then-cover operation.
-    kSetToRef,
-    /// Draw the stencil for the nonzero fill path rule.
    kNonZeroWrite,


For clarity, I suggest the following convention. Make the StencilMode specify the phase in play clearly. Something like k<Phase><Option> perhaps. In my head, with stencil-then-cover, kEvenOddWrite would be kStencilEvenOddFill. You used the term "preparation" above. If you prefer that (I'm warming up to it too), kPreparationEvenOddFill. I found kEvenOffWrite too terse to follow initially. Ditto for the "cover" phase.

You can then document the two phases in detail.

chinmaygarde · 2024-02-21T19:48:06Z

impeller/entity/contents/content_context.cc

      case StencilMode::kCoverCompare:
        front_stencil.stencil_compare = CompareFunction::kNotEqual;
-        front_stencil.depth_stencil_pass = StencilOperation::kKeep;
+        front_stencil.depth_stencil_pass =


I'd add a comment here that says the reference value ought to be zero.

Ditto below.

Added some comments here. This is also documented in the docstrings, but tried to make it a little more clear.

chinmaygarde · 2024-02-21T19:50:15Z

impeller/entity/contents/content_context.h

+  ///
+  // TODO(bdero): Remove this setting once StC is fully de-risked
+  //              https://github.com/flutter/flutter/issues/123671
+  static constexpr bool kEnableStencilThenCover = false;


I like the if constexpr!

Woo! I'll probably use this pattern for other huge feature migrations in the future. ContentContext turned out to be a good place for it because everything in the universe depends on it. ;)

…lag for enabling StC. (flutter/engine#50817)

…143933) flutter/engine@7777480...9250bfd 2024-02-22 [email protected] Roll Skia from 0b55d78d32f9 to b9c16065b76d (1 revision) (flutter/engine#50861) 2024-02-22 [email protected] Roll Skia from cbdbcf9bebe6 to 0b55d78d32f9 (1 revision) (flutter/engine#50860) 2024-02-22 [email protected] Roll Fuchsia Linux SDK from dLhvv964txwnSlvNw... to j9cJ94K-T1i3u5xGh... (flutter/engine#50858) 2024-02-22 [email protected] Roll Skia from ec119f44e6b9 to cbdbcf9bebe6 (2 revisions) (flutter/engine#50857) 2024-02-22 [email protected] Roll Skia from 4aa97aca087c to ec119f44e6b9 (1 revision) (flutter/engine#50849) 2024-02-22 [email protected] [Impeller] Add StC color source/clip behavior + constexpr flag for enabling StC. (flutter/engine#50817) 2024-02-22 [email protected] Roll Skia from 3fd1a5a7c7c1 to 4aa97aca087c (2 revisions) (flutter/engine#50848) 2024-02-22 [email protected] Minor non-semantic refactors of `ExternalTexturesFlutterActivity` (flutter/engine#50845) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from dLhvv964txwn to j9cJ94K-T1i3 If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

bdero added 2 commits February 21, 2024 00:34

[Impeller] Add constexpr flag for enabling stencil-then-cover.

87110a1

Add path for StC in filled paths

b3934ac

github-actions bot added the e: impeller label Feb 21, 2024

bdero requested review from chinmaygarde and jonahwilliams February 21, 2024 09:13

Add intersect/difference clip StC behavior

a04527b

bdero changed the title ~~[Impeller] Add constexpr flag for enabling stencil-then-cover.~~ [Impeller] Add StC clip behavior + constexpr flag for enabling StC. Feb 21, 2024

Fix lint

07d6e72

bdero force-pushed the bdero/stc-switch branch from 12a3ee0 to 07d6e72 Compare February 21, 2024 11:27

Add StC color source behavior

42d2888

bdero changed the title ~~[Impeller] Add StC clip behavior + constexpr flag for enabling StC.~~ [Impeller] Add StC color source/clip behavior + constexpr flag for enabling StC. Feb 21, 2024

bdero added 5 commits February 21, 2024 03:55

Set convexity in test path

cb98fdb

Clear depth

e80cadc

Correct compare

e153670

Set depth for backdrop filters and MSAA backdrop restores

34f653d

Default stencil mode

90d5e29

bdero force-pushed the bdero/stc-switch branch from c7c1687 to 90d5e29 Compare February 21, 2024 13:45

jonahwilliams reviewed Feb 21, 2024

View reviewed changes

jonahwilliams approved these changes Feb 21, 2024

View reviewed changes

chinmaygarde approved these changes Feb 21, 2024

View reviewed changes

chinmay review + docstring cleanup

5845b69

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

auto-submit bot merged commit 2e09c41 into flutter:main Feb 22, 2024

engine-flutter-autoroll mentioned this pull request Feb 22, 2024

Roll Flutter Engine from 7777480eb94d to 9250bfdb38c5 (8 revisions) flutter/flutter#143933

Merged

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

2e09c418a [Impeller] Add StC color source/clip behavior + constexpr f…

4d4be9e

…lag for enabling StC. (flutter/engine#50817)

[Impeller] Add StC color source/clip behavior + constexpr flag for enabling StC. #50817

[Impeller] Add StC color source/clip behavior + constexpr flag for enabling StC. #50817

Uh oh!

Conversation

bdero commented Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdero Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bdero commented Feb 21, 2024 •

edited

Loading

bdero Feb 22, 2024 •

edited

Loading