[Impeller] Migrate all ColorSourceContents to use a shared rendering routine. #50261

bdero · 2024-02-01T23:16:17Z

This is going to be our "in" for performing StC in all the ColorSourceContents when necessary.

No semantic change.

flutter-dashboard · 2024-02-01T23:16:20Z

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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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 · 2024-02-01T23:17:18Z

Pushing up before doing the full refactor to see those goldens. This might end up living in ColorSourceContents instead of its own TU.

bdero · 2024-02-06T22:57:12Z

I refactored this to live in ColorSourceContents. Would like to land this before distributing to the rest of the relevant contents.

johnmccutchan · 2024-02-06T23:00:38Z

test-exempt: refactor

jonahwilliams · 2024-02-06T23:14:49Z

impeller/entity/contents/color_source_contents.h

+  template <typename VertexShaderT>
+  bool DrawPositionsAndUVs(
+      Rect texture_coverage,
+      Matrix effect_transform,


Suggested change

Matrix effect_transform,

const Matrix& effect_transform,

jonahwilliams · 2024-02-06T23:18:38Z

impeller/entity/contents/color_source_contents.h

+      RenderPass& pass,
+      const PipelineBuilderMethod& pipeline_builder,
+      typename VertexShaderT::FrameInfo frame_info,
+      const BindFragmentCallback& bind_pipeline_callback) const {


Alternative suggestion, make color source contents have a virtual BindFragmentCallback method and make this call it. If we have to pass a closure for non trivial cases then its going to end up closing over the entire contents anyway.

Seems like a good approach. Done.

Uhh actually applying this to my follow-up patch is a painful because we have multiple rendering branches for all of the gradients that need different binding behavior.

In all of the use cases we can capture the things we need by reference, and my understanding is that assigning a lambda to a stack std::function shouldn't heap allocate as long as the set of captures is reasonably small.

One sec, gonna check this...

The different gradients are basically different content classes though
if the contents is less verbose with this change it would make more sense to split them in two IMO.

Briefly discussed offline, we can just switch this out later if necessary / ends up being too annoying. Some legit concerns over expanding our use of std::function, like annoying stack traces, compiler differences, etc.

jonahwilliams · 2024-02-06T23:19:13Z

impeller/entity/contents/color_source_contents.h

+  bool DrawPositions(const ContentContext& renderer,
+                     const Entity& entity,
+                     RenderPass& pass,
+                     const PipelineBuilderMethod& pipeline_builder,


Same here, can pipeline_builder be a virtual method on ColorSourceContents?

jonahwilliams · 2024-02-09T03:30:40Z

impeller/entity/contents/color_source_contents.h

+    std::shared_ptr<Pipeline<PipelineDescriptor>> pipeline =
+        pipeline_callback(options);
+    pass.SetPipeline(pipeline);


Either move into SetPipeline or inline the call to avoid the additional shared_ptr release.

Oopsies, done.

jonahwilliams · 2024-02-09T03:31:10Z

impeller/entity/contents/color_source_contents.h

+    auto geometry_result =
+        GetGeometry()->GetPositionBuffer(renderer, entity, pass);
+
+    return DrawGeometry<VertexShaderT>(geometry_result, renderer, entity, pass,


Suggested change

return DrawGeometry<VertexShaderT>(geometry_result, renderer, entity, pass,

return DrawGeometry<VertexShaderT>(std::move(geometry_result), renderer, entity, pass,

geometry_result contains shared_ptrs in the buffer views, should be moved.

jonahwilliams · 2024-02-09T03:31:22Z

impeller/entity/contents/color_source_contents.h

+    auto geometry_result = GetGeometry()->GetPositionUVBuffer(
+        texture_coverage, effect_transform, renderer, entity, pass);
+
+    return DrawGeometry<VertexShaderT>(geometry_result, renderer, entity, pass,


Suggested change

return DrawGeometry<VertexShaderT>(geometry_result, renderer, entity, pass,

return DrawGeometry<VertexShaderT>(std::move(geometry_result), renderer, entity, pass,

Same here

flutter-dashboard · 2024-02-10T00:25:49Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #50261 at sha 9cad64c

bdero · 2024-02-10T02:09:24Z

Okay, let's see how those goldens turn out, but all the color sources are done.

flutter-dashboard · 2024-02-10T02:52:09Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #50261 at sha bd03152

bdero · 2024-02-13T18:52:34Z

Oh, the changed goldens are gone!

This reverts commit e4f5235.

bdero · 2024-02-13T18:56:37Z

I force pushed like a bad boy but let's see what happens.

jonahwilliams

LGTM!

flutter-dashboard · 2024-02-13T19:40:05Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #50261 at sha ccd8718

jonahwilliams · 2024-02-13T20:33:44Z

It looks like this is failing on the tests added for #45308

bdero · 2024-02-13T20:37:41Z

Yup. Only for Vulkan though, which is peculiar.

bdero · 2024-02-13T21:56:42Z

Something funky is going on with this test. No fragment stage uniforms are getting bound for either Vulkan or Metal.

bdero · 2024-02-13T22:05:04Z

nvm, it is binding a struct.

bdero · 2024-02-13T22:46:27Z

This was a silly ordering bug. RuntimeEffectContents forms descriptor sets based on the uniforms while adding the bindings. It's fine to resolve the pipeline after the bind callback.

…rendering routine. (flutter/engine#50261)

…143425) flutter/engine@215d55f...0849250 2024-02-14 30870216+gaaclarke@users.noreply.github.com [Impeller] replaces golden file count with a golden diff file (flutter/engine#50621) 2024-02-14 bdero@google.com [Impeller] Migrate all ColorSourceContents to use a shared rendering routine. (flutter/engine#50261) 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 jsimmons@google.com,rmistry@google.com,zra@google.com 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 the e: impeller label Feb 1, 2024

bdero self-assigned this Feb 1, 2024

bdero requested a review from jonahwilliams February 6, 2024 22:57

jonahwilliams reviewed Feb 6, 2024

View reviewed changes

bdero force-pushed the bdero/colorsourcecontents-utils branch from c297203 to 19b60ea Compare February 8, 2024 02:46

bdero requested a review from jonahwilliams February 8, 2024 03:00

bdero force-pushed the bdero/colorsourcecontents-utils branch from 1861b24 to 9b882af Compare February 8, 2024 22:02

jonahwilliams reviewed Feb 9, 2024

View reviewed changes

bdero changed the title ~~[Impeller] Add shared ColorSourceContents render routine.~~ [Impeller] Migrate ColorSourceContents to use a shared rendering routine. Feb 10, 2024

bdero changed the title ~~[Impeller] Migrate ColorSourceContents to use a shared rendering routine.~~ [Impeller] Migrate all ColorSourceContents to use a shared rendering routine. Feb 10, 2024

flutter-dashboard bot added the will affect goldens label Feb 10, 2024

bdero force-pushed the bdero/colorsourcecontents-utils branch from 9cad64c to 9792f6d Compare February 10, 2024 01:28

bdero requested a review from jonahwilliams February 10, 2024 02:09

bdero force-pushed the bdero/colorsourcecontents-utils branch from bd03152 to fa205f4 Compare February 13, 2024 18:52

bdero added 8 commits February 13, 2024 10:53

[Impeller] Add shared ColorSourceContents render routine.

ef2b22c

Move to ColorSourceContents

2456438

jonah review

a27a5f1

Revert "jonah review"

7d5cd10

This reverts commit e4f5235.

jonah review

7a230e8

renames and comments

fc6be29

Adopt LinearGradientContents

3b0f19f

Migrate TiledTextureContents

5c80aff

bdero added 7 commits February 13, 2024 10:53

Use callback for pipeline

66072d1

Migrate RuntimeEffectContents

5bd771a

dastardly ifdef!

34b2caf

Jonah review

3740ba3

Migrate ConicalGradientContents

7ce4b58

Migrate RadialGradientContents

ffd2196

Migrate SweepGradientContents

ccd8718

bdero force-pushed the bdero/colorsourcecontents-utils branch from fa205f4 to ccd8718 Compare February 13, 2024 18:53

jonahwilliams approved these changes Feb 13, 2024

View reviewed changes

Resolve the pipeline after running the fragment bind callback

7fcc256

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

auto-submit bot merged commit fa2eb89 into flutter:main Feb 14, 2024
29 checks passed

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

Roll Flutter Engine from 215d55f4f82d to 0849250a1419 (2 revisions) flutter/flutter#143425

Merged

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

fa2eb896c [Impeller] Migrate all ColorSourceContents to use a shared …

3697e9b

…rendering routine. (flutter/engine#50261)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] Migrate all ColorSourceContents to use a shared rendering routine. #50261

[Impeller] Migrate all ColorSourceContents to use a shared rendering routine. #50261

bdero commented Feb 1, 2024 •

edited

Loading

flutter-dashboard bot commented Feb 1, 2024

bdero commented Feb 1, 2024

bdero commented Feb 6, 2024

johnmccutchan commented Feb 6, 2024

jonahwilliams Feb 6, 2024

bdero Feb 8, 2024

jonahwilliams Feb 6, 2024

bdero Feb 8, 2024

bdero Feb 8, 2024

jonahwilliams Feb 8, 2024

bdero Feb 8, 2024 •

edited

Loading

jonahwilliams Feb 6, 2024

bdero Feb 8, 2024

jonahwilliams Feb 9, 2024

bdero Feb 9, 2024

jonahwilliams Feb 9, 2024

bdero Feb 9, 2024

jonahwilliams Feb 9, 2024

bdero Feb 9, 2024

flutter-dashboard bot commented Feb 10, 2024

bdero commented Feb 10, 2024

flutter-dashboard bot commented Feb 10, 2024

bdero commented Feb 13, 2024

bdero commented Feb 13, 2024

jonahwilliams left a comment

flutter-dashboard bot commented Feb 13, 2024

jonahwilliams commented Feb 13, 2024

bdero commented Feb 13, 2024

bdero commented Feb 13, 2024

bdero commented Feb 13, 2024

bdero commented Feb 13, 2024

	return DrawGeometry<VertexShaderT>(geometry_result, renderer, entity, pass,
	return DrawGeometry<VertexShaderT>(std::move(geometry_result), renderer, entity, pass,

[Impeller] Migrate all ColorSourceContents to use a shared rendering routine. #50261

[Impeller] Migrate all ColorSourceContents to use a shared rendering routine. #50261

Conversation

bdero commented Feb 1, 2024 • edited Loading

flutter-dashboard bot commented Feb 1, 2024

bdero commented Feb 1, 2024

bdero commented Feb 6, 2024

johnmccutchan commented Feb 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdero Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Feb 10, 2024

bdero commented Feb 10, 2024

flutter-dashboard bot commented Feb 10, 2024

bdero commented Feb 13, 2024

bdero commented Feb 13, 2024

jonahwilliams left a comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Feb 13, 2024

jonahwilliams commented Feb 13, 2024

bdero commented Feb 13, 2024

bdero commented Feb 13, 2024

bdero commented Feb 13, 2024

bdero commented Feb 13, 2024

bdero commented Feb 1, 2024 •

edited

Loading

bdero Feb 8, 2024 •

edited

Loading