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

[Impeller] combine text shaders. #52392

Merged
auto-submit merged 9 commits into from
Apr 25, 2024
Merged

[Impeller] combine text shaders. #52392

auto-submit merged 9 commits into from
Apr 25, 2024

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Apr 25, 2024 •
edited
Loading

Combine both text shaders into one. One more down!

flutter/flutter#143540

@jonahwilliams
Copy link
Contributor Author

jonahwilliams commented Apr 25, 2024

Version 1:

Changes found in shader flutter/impeller/entity/gles/glyph_atlas.frag.gles on core Mali-G78:
has_uniform_computation:
  False <- before
  True <- after
longest_path_cycles in variant Main
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  varying     texture     bound       
before  0.031250    0.031250    0.031250    0.000000    0.000000    0.500000    0.250000    varying
after   0.109375    0.031250    0.109375    0.000000    0.000000    0.250000    0.250000    varying,texture

shortest_path_cycles in variant Main
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  varying     texture     bound       
before  0.031250    0.031250    0.000000    0.000000    0.000000    0.500000    0.250000    varying
after   0.062500    0.031250    0.062500    0.000000    0.000000    0.250000    0.250000    varying,texture

total_cycles in variant Main
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  varying     texture     bound       
before  0.031250    0.031250    0.031250    0.000000    0.000000    0.500000    0.250000    varying
after   0.125000    0.062500    0.125000    0.000000    0.000000    0.250000    0.250000    varying,texture

In variant Main:
  uniform_registers_used: 4 <- before
  uniform_registers_used: 6 <- after

@jonahwilliams jonahwilliams marked this pull request as ready for review April 25, 2024 19:28
jonahwilliams
jonahwilliams commented Apr 25, 2024
View reviewed changes
impeller/entity/shaders/glyph_atlas.frag
if (use_alpha_color_channel == 1.0) {
frag_color = value.aaaa * v_text_color;

if (frag_info.is_color_glyph == 1.0) {
Copy link
Contributor Author

@jonahwilliams jonahwilliams Apr 25, 2024

Choose a reason for hiding this comment

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

I looked at using mix(...) but malioc said it was more expensive.

Copy link
Contributor

@chinmaygarde chinmaygarde Apr 25, 2024

Choose a reason for hiding this comment

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

lol, that was my go-to trick for reducing branches. Glad to know thats obsolete.

jonahwilliams
jonahwilliams commented Apr 25, 2024
View reviewed changes
impeller/entity/contents/text_contents.cc

for (const Point& point : unit_points) {
vtx.unit_position = point;
std::memcpy(vtx_contents++, &vtx, sizeof(VS::PerVertexData));
Copy link
Contributor Author

@jonahwilliams jonahwilliams Apr 25, 2024

Choose a reason for hiding this comment

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

I don't know why I wrote this with memcpy

Copy link
Contributor

@chinmaygarde chinmaygarde Apr 25, 2024

Choose a reason for hiding this comment

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

I do it when I'm not sure about data alignment. But thats not a concern here.

chinmaygarde
chinmaygarde approved these changes Apr 25, 2024
View reviewed changes
Copy link
Contributor

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

I usually don't pay too much attention to uniform computation. Its the register spills.

impeller/entity/contents/text_contents.cc

for (const Point& point : unit_points) {
vtx.unit_position = point;
std::memcpy(vtx_contents++, &vtx, sizeof(VS::PerVertexData));
Copy link
Contributor

@chinmaygarde chinmaygarde Apr 25, 2024

Choose a reason for hiding this comment

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

I do it when I'm not sure about data alignment. But thats not a concern here.

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 25, 2024
@auto-submit auto-submit bot merged commit a09295f into flutter:main Apr 25, 2024
@jonahwilliams jonahwilliams deleted the combine_text_shaders branch April 25, 2024 20:18
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 25, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 25, 2024
@engine-flutter-autoroll
a09295f [Impeller] combine text shaders. (flutter/engine#52392)
8f51fb8
jmagman pushed a commit to flutter/flutter that referenced this pull request Apr 25, 2024
@engine-flutter-autoroll
Roll Flutter Engine from 3768ca0 to a09295f (1 revision) (#147386)
014cf33 
flutter/engine@3768ca0...a09295f

2024-04-25 jonahwilliams@google.com [Impeller] combine text shaders.
(flutter/engine#52392)

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 jimgraham@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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@chinmaygarde chinmaygarde chinmaygarde approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jonahwilliams @chinmaygarde