-
Notifications
You must be signed in to change notification settings - Fork 6k
Support stencil buffers on OpenGL for Windows and Android #31967
Conversation
|
|
||
| GrBackendRenderTarget render_target(size.width(), // width | ||
| size.height(), // height | ||
| 0, // sample count |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
This pull request is not suitable for automatic merging in its current state.
|
|
The failure here is in a test for the web running framework tests. It cannot possibly be from this change. @yjbanov fyi |
* 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)
|
This has been targetted as the cause of a raster time regressino for customer:money. Going to revert. |
|
@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. |
|
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. |
|
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 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). 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,
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. 😂 |
|
The only benchmarks for memory we have on Android are in the framework repo running applications. |

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.