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

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented Mar 11, 2022

Fixes flutter/flutter#99934

In local testing on a low end and mid-ranged Android device, this doesn't seem to have any profound effect on raster times, but it does seem to give a slight improvement on average and worst case raster times. On the low end device, it causes no memory regression, on the Pixel 4 it causes an extra 10mb to be allocated for graphics.

I could use some help testing this change out on Windows, particularly to see if it has any positive impact for flutter/flutter#97334.

@flutter-dashboard flutter-dashboard bot added embedder Related to the embedder API platform-android labels Mar 11, 2022

GrBackendRenderTarget render_target(size.width(), // width
size.height(), // height
0, // sample count
Copy link
Member

Choose a reason for hiding this comment

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

🤔 How is this working with a sample count of 0? Perhaps Skia clamps it to 1? If you're in the mood for a drive by fix, I'd specify 1 here just so its clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure Skia does some magic to make sure the value turns out right. We're using 0 in a few places - I'd slightly prefer cleaning that up in a separate patch.

There's also a todo in the context_options file that's tied to a skbug that's resolved that we can try to remove.

@dnfield dnfield added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 11, 2022
@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Linux Web Framework tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 11, 2022
@dnfield
Copy link
Contributor Author

dnfield commented Mar 11, 2022

The failure here is in a test for the web running framework tests. It cannot possibly be from this change. @yjbanov fyi

@dnfield dnfield added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 15, 2022
@fluttergithubbot fluttergithubbot merged commit cd7a1e2 into flutter:main Mar 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 16, 2022
zanderso pushed a commit to flutter/flutter that referenced this pull request Mar 16, 2022
* 0199e90 [macOS] fix text selection when there's composing text  (flutter/engine#31936)

* 0ca0ce1 Roll Skia from 9565f4bd902b to a48a3c9417c0 (3 revisions) (flutter/engine#32044)

* a60e8c3 Switch the renderer to impeller based on the presence of a command line flag. (flutter/engine#31959)

* 24051b6 Roll Fuchsia Mac SDK from EOVjR8JSN... to jvlI1s78T... (flutter/engine#32047)

* fef6232 Roll Skia from a48a3c9417c0 to ffb49630eb1a (3 revisions) (flutter/engine#32048)

* 5a80834 [ci.yaml] Migrate remaining targets to cocoon scheduler (flutter/engine#32018)

* cd7a1e2 Support stencil buffers on OpenGL for Windows and Android (flutter/engine#31967)

* dc22c4c Add DlColorSource objects to hold information for SkShaders (flutter/engine#31981)
@dnfield dnfield deleted the stencil branch April 6, 2022 21:24
@dnfield
Copy link
Contributor Author

dnfield commented Apr 6, 2022

This has been targetted as the cause of a raster time regressino for customer:money. Going to revert.

@ajihyf
Copy link
Contributor

ajihyf commented May 22, 2023

@dnfield Hi, sorry to bring up this old issue again. I have tested on several Android phones(including Pixel4 AOSP11 and Pixel4 AOSP13) and it seems that enabling or disabling the stencil buffer does not affect graphics memory. Could you provide the scenario where enabling stencil causes an increase in memory usage? In our scenario, we often use clip for blur mask, disabling stencil will result in software rendering mask in Skia.

@dnfield
Copy link
Contributor Author

dnfield commented May 22, 2023

The benchmark in which it caused a memory increase was for an internal Google customer. I unfortunately cannot share that benchmark publicly.

If you have some benchmarks that show this improves things let's add them to our test suite - we could certainly look at enabling this again, but at the time it didn't show any clear wins (and had at least one clear regression).

FWIW, we are more focused at this point on getting Impeller ready to go which will use the stencil buffer for these kinds of operations.

@ajihyf
Copy link
Contributor

ajihyf commented May 29, 2023

This situation occurs when using complex shapes such as RRect to clip BoxShadow, Skia will use software renderer to render a mask texture and upload it (size equals to w*h, about 300kb for a single button), it seems that the mask is not cached, and the software rendering and texture uploading will be repeated every frame. When the stencil is turned on, the soft rendering disappears, and Skia will use the stencil buffer to assist in the masking operation.

Our scenario is also an internal scenario, but very similar to flutter_inset_box_shadow's' approach(which draws a blurred DRRect with RRect clip). This package's demo app will produce a frame capture in RenderDoc like this (the 553Kb texture is the software rendered mask).
image

In our test, turning on or off the stencil buffer will not affect the Graphics memory. Instead, the Native memory will decrease because the software rendering texture is no longer used. It seems that in Tile-Based Rendering GPU, the stencil value that is not explicitly written back to the framebuffer is a transient value on the on-chip buffer. And almost all mobile GPUs are Tile-Based. The Arm docs says,

The main advantage of tile-based rendering is that a tile is only a small fraction of the total framebuffer. Therefore, it is possible to store the entire working set of color, depth, and stencil on fast on-chip RAM, that is tightly coupled to the GPU shader core.

I've tried to add some dl_benchmarks to show the difference, but it seems that the benchmarks only support Software Renderer on Mac and doesn't work on Android. 😂

@dnfield
Copy link
Contributor Author

dnfield commented May 29, 2023

The only benchmarks for memory we have on Android are in the framework repo running applications.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

embedder Related to the embedder API platform-android waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider adding a stencil buffer for Skia to use on OpenGL backend

4 participants