Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Start stroke tessellation in compute #39543

Merged
merged 13 commits into from
Feb 28, 2023

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Feb 10, 2023

See comment below for update on what this does.

Fixes flutter/flutter#120241

height /= 2;
}
auto size = MTLSizeMake(width, height, 1);
[encoder dispatchThreadgroups:size threadsPerThreadgroup:size];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bug. Previously, only the last command in the pass was being dispatched.

@dnfield dnfield changed the title Polyline generation in compute Start stroke tessellation in compute Feb 24, 2023
@dnfield dnfield changed the title Start stroke tessellation in compute [Impeller] Start stroke tessellation in compute Feb 24, 2023
@dnfield
Copy link
Contributor Author

dnfield commented Feb 24, 2023

Ok. This patch still only does things in tests, but it verifies the following:

  • A compute shader that can generate quadratic bezier curves from cubics.
  • A compute shader that can generate a polyline from quadratic bezier curve definitions
  • A compute shader that can generate stroke vertices for a single contour stroke without any special caps or joins.

And a test that strings them all together.

This feels like enough that I'd like to get it reviewed and tested on CI. Some next steps:

  • Improve the estimation of the sizes of the buffers
  • Actually feed that vertex buffer into a vertex shader to draw the stroke
  • Extend the stroke shader to support multiple contours/caps/joins
  • Prove out using the interface for cubics/quads from our actual path definitions, especailly batching up all the paths for a scene to be used in various passes.

@dnfield dnfield marked this pull request as ready for review February 24, 2023 23:36
@@ -34,7 +34,7 @@ static CompilerBackend CreateMSLCompiler(const spirv_cross::ParsedIR& ir,
// If this version specification changes, the GN rules that process the
// Metal to AIR must be updated as well.
sl_options.msl_version =
spirv_cross::CompilerMSL::Options::make_msl_version(1, 2);
spirv_cross::CompilerMSL::Options::make_msl_version(2, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to make these configurable and add a new target for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh right I have to fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. This is now parameterized here and elsewhere.

Because it's now parameterized, the comment above no longer applies - the MSL version is set once and flows through the GN templates to here and the build_metal_library.py script.

Comment on lines +382 to +384
shaderc_env_version::shaderc_env_version_vulkan_1_1);
spirv_options.SetTargetSpirv(
shaderc_spirv_version::shaderc_spirv_version_1_0);
shaderc_spirv_version::shaderc_spirv_version_1_3);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chinmaygarde or @iskakaushik - is this ok?

I'm not sure why we'd be targetting Vulkan 1.0 here but Vulkan 1.1 above.

I'm also not sure I understand the implications of using SPIR-V version 1.0 here. I need 1.3 for subgroups. I think it's probably ok but don't know where to look it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not ok all the time, I can parameterize this the way I did with the MSL version above.

// but some cases result in no errors or warnings and still have an error
// message. If there's a message we should print it.
if (spv_result_->GetNumErrors() > 0 || spv_result_->GetNumWarnings() > 0 ||
!spv_result_->GetErrorMessage().empty()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the part that fixes flutter/flutter#120241

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 28, 2023
@dnfield
Copy link
Contributor Author

dnfield commented Feb 28, 2023

I think I finally got Windows to build.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller
Projects
No open projects
Archived in project
3 participants