-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Impeller] Implements gaussian blur that scales down before applying the blur #47576
Conversation
0ac7e4b
to
52cce80
Compare
ea53fdd
to
39f83f9
Compare
78cfe70
to
c6ecc90
Compare
@@ -54,7 +55,7 @@ void main() { | |||
// just uses 0.5 as an optimization until the gaussian coefficients are | |||
// calculated and passed in from the cpu. | |||
for (float16_t i = -blur_info.blur_radius; i <= blur_info.blur_radius; | |||
i += 2.0hf) { | |||
i += blur_info.step_size) { |
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.
A 2 step causes waffling that's even more noticeable with the downscale approach.
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.
Interesting! I will dig into this afterwards. Its a nice win if we can get it working but is otherwise completely orthogonal to this change
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.
It's the issue I reported here: flutter/flutter#138259
TEST_P(GaussianBlurFilterContentsTest, CalculateUVsSimple) { | ||
TextureDescriptor desc = { | ||
.format = PixelFormat::kB8G8R8A8UNormInt, | ||
.size = ISize(100, 100), | ||
}; | ||
std::shared_ptr<Texture> texture = MakeTexture(desc); | ||
auto filter_input = FilterInput::Make(texture); | ||
Entity entity; | ||
Quad uvs = GaussianBlurFilterContents::CalculateUVs(filter_input, entity, | ||
ISize(100, 100)); | ||
std::optional<Rect> uvs_bounds = Rect::MakePointBounds(uvs); | ||
EXPECT_TRUE(uvs_bounds.has_value()); | ||
if (uvs_bounds.has_value()) { | ||
EXPECT_TRUE(RectNear(uvs_bounds.value(), Rect::MakeXYWH(0, 0, 1, 1))); | ||
} | ||
} |
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.
If we used mock objects as we had originally I could test the uv values directly when rendering. This is the best I can do with real objects. cc @matanlurey @jonahwilliams
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.
In theory you could instrument the creation of commands and render passes, and then read-back the real UVs. I might implement something like that in the future.
In the meantime I think its fair to say that based on the gaussian structure it would be too difficult to pull out the real UVs (by hooking into the cmd buffer) and that mocks may be appropriate. Factoring out the logic into a testable method is also a completely reasonable means to test IMO.
But @gaaclarke you're the one putting in the work here. If you don't feel like the static methods are sufficient then mock away.
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.
Yea, the issue I ran into is that I kept fixing one test and breaking another and golden image tests cannot be run locally so I have to do manual testing to make sure those 2 tests kept working. Ideally I'd want to scale beyond checking 2 tests too. I'll see if I get around to testing uvs directly via mocks.
@jonahwilliams @bdero are you guys interested in doing a review for this now, not being 100% done? I can keep working on it in this PR. I would like to get the coverage expanding but it will make the review easier to do if that gets split out. I think this is a good flag in the sand. |
Yes, I will happily take a look at this now (well, tomorrow 😄 ) |
So I just tried this out. Wow 🚀 , its crazy fast compared to our current approach. I do see artifacts along the top and bottom edge. I wonder if these are coverage artifacts? I see that you've called that out as a current problem. |
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.
Just some quick notes
false; | ||
#endif | ||
|
||
// TODO(https://github.com/flutter/flutter/issues/131580): Remove once the new |
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.
Can you document (here or in the issue) which cases the blur doesn't handle now?
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 have them listed in the issue above. If we land this early I can copy them over to the issue.
sampler_desc.min_filter = filter; | ||
sampler_desc.mag_filter = filter; | ||
sampler_desc.width_address_mode = address_mode; | ||
sampler_desc.width_address_mode = address_mode; |
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.
you set the width address mode twice here, does this need to be height address mode too? I wonder if this is causing the artifacts.
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 checked and this did not fix the artifacting issue. I suspect this is probably to do with coverage then
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.
Good catch, that definitely would have shown up at some point. The default value is nearest. Maybe that was contributing to the worse waffling we were seeing with the 2 step?
std::shared_ptr<Texture> input_texture, | ||
const SamplerDescriptor& sampler_descriptor, | ||
const GaussianBlurFragmentShader::BlurInfo& blur_info) { | ||
// TODO(gaaclarke): This doesn't render if the blur_info.sigma == 0. |
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.
Where do we check for sigma < kEhClose enough on the old blur? Presumibly we could detect that and just make the filter return the original contents, or "optimize" it out entirely
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.
Good call. I do have a todo to address this. It didn't occur to me yet that the solution was so simple, done.
|
||
/// Calculate how much to scale down the texture depending on the blur radius. | ||
/// This curve was taken from |DirectionalGaussianBlurFilterContents|. | ||
Scalar CalculateScale(Scalar radius) { |
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.
If I wanted to change this scaling, is this where I would do it?
As a follow up we may align to skia's curve, which scales sigma down to 4 proportionally.
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.
Yep, oh interesting. This was me copying the one from the other blur. I think we'll have to revisit this since I think it's a bit too aggressive (because of the artifacts when animating the sigma).
We might want to tweak the scaling algorithm, its a bit jittery when sigma is animated. So we have that lever to change to get animation quality versus performance. I think the static results all look fine with this curve though. Which artifacts? You talking about the cropping of the "halo" around blurred items? Thats the one I know about. |
I see a dark line/flicker across the top and bottom of one of the blurs on wonderous, specifically the one that slides up on the timeline page. |
Oh interesting. I haven't seen that. It probably isn't worth looking into until I've fixed the halo since that's going to change things around the edges. |
Fixed. |
/// Note: This will replace `DirectionalGaussianBlurFilterContents`. | ||
class GaussianBlurFilterContents final : public FilterContents { | ||
public: | ||
GaussianBlurFilterContents(Scalar sigma = 0.0f); |
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.
nit: explicit?
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.
done
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.
LGTM (with nits)!
b9da55a
to
2dc9201
Compare
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.
Awesome progress! Left some comments.
I suspect there is a problem with the way we're scaling the sampling interval because I'm seeing extreme loss of high frequency info for low sigma values. But I haven't stared at it long enough to spot it yet and we can always fix it up later.
.step_size = 1.0, | ||
}); | ||
|
||
// TODO(gaaclarke): Make this pass reuse the texture from pass1. |
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 would we reuse the first pass texture?
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.
We should be able to do:
Downsample(src: input, dst: texture1) -> Horizontal Blur(src: texture1, dst: texture2) -> Vertical Blur(src: texture2, dst: texture1) -> texture1
Right now we are doing:
Downsample(src: input, dst: texture1) -> Horizontal Blur(src: texture, dst: texture2) -> Vertical Blur(src: texture2, dst: texture3) -> texture3
.
I haven't looked into what plumbing we'd need to make this work yet, the ol' ping-pong maneuver. Potentially it will require different synchronization but I suspect the way we are doing passes now it won't be needed. I haven't looked into it yet.
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.
FWIW I've thought about building this into the render target cache itself, but I don't have a good way to enforce that we don't create self dependencies. Perhaps if we deferred actual texture creation (from @chinmaygarde 's suggestion) and just used descriptors, it would be easier to apply these sorts of optimizations generally.
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.
Ah, yeah that makes sense. The first thing that popped into my mind was that you were planning to sample from the first texture for something. 😆
true; | ||
#else | ||
false; | ||
#endif |
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.
Definitely no need to do this now, but it would be nice if this was a temporary runtime flag in Aiks and we just had a new FilterContents::MakeGaussianBlurV2
, that way we could test the blurs in the playgrounds side-by-side in the playgrounds and allow for dirty hacks to flip the flag between the old and new blurs when testing against apps.
impeller/entity/contents/filters/gaussian_blur_filter_contents.cc
Outdated
Show resolved
Hide resolved
@@ -55,6 +56,20 @@ std::shared_ptr<FilterContents> FilterContents::MakeGaussianBlur( | |||
Sigma sigma_y, | |||
BlurStyle blur_style, | |||
Entity::TileMode tile_mode) { | |||
constexpr bool use_new_filter = | |||
#ifdef IMPELLER_ENABLE_NEW_GAUSSIAN_FILTER |
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.
Are you flipping this define on via the build config, or just appending code?
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.
Just editing the code during development. I haven't needed anything else yet.
const Entity& entity, | ||
const Matrix& effect_transform, | ||
const Rect& coverage, | ||
const std::optional<Rect>& coverage_hint) const { |
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 don't see the snapshot's transform being consumed anywhere, which will cause issues with some optimized snapshots.
The "destination rect" of TextureContents
scales the texture when taking a snapshot to avoid an unnecessary render pass. I'd recommend absorbing the scale of the basis vectors in the first downsampling pass, and applying other aspects of the transform to the final returned entity.
So the texture of a snapshot may be in pass space, entity space, or something in-between.
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.
What parameter are you editing to get that? I did see that but I didn't have a case where it manifested. I assumed that was getting absorbed elsewhere so the uv coords weren't effected.
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.
It's the "Path XYWH" at the bottom
@bdero I think that is related to our aggressive scaling function. There is a conversation in this issue a couple places about changing it. |
…138585) flutter/engine@aae07e9...5064aef 2023-11-16 jonahwilliams@google.com [Impeller] Clang tidy even more (flutter/engine#48102) 2023-11-16 30870216+gaaclarke@users.noreply.github.com [Impeller] Implements gaussian blur that scales down before applying the blur (flutter/engine#47576) 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 jonahwilliams@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
A partial implementation of the new gaussian blur effect. This should perform enough of the code to start getting some performance numbers.
Known outstanding problems:
issue: flutter/flutter#131580
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.