Add setReservedOffset to crop the blurred area for mostly-static BlurViews - #270
Add setReservedOffset to crop the blurred area for mostly-static BlurViews#270plneple wants to merge 3 commits into
Conversation
fc3698c to
fbb8ffb
Compare
|
Hey, thanks for the PR.
Do you have evidence that this happens without the fix, and doesn't happen with the fix?
Do you have evidence that the blur processes a larger region without your fix? So it doesn't really matter that the RenderNode has a larger size than the target View as long as |
|
Hey @Dimezis, I'll get back to you on your questions, but before I dig in — could you clarify whether your skepticism is about the implementation itself or the wording of the PR description? And what would convince you? Benchmarks, code paths, showcases, etc.? So far the only verification I have is anecdotal, from integrating this into a |
|
@plneple As the TODO in code suggested, I also thought that this larger RenderNode might result in having to blur the larger area, but after digging a bit deeper, I'm not sure it's really the case. If you can prove in some way (benchmark and/or code path) that this smaller RenderNode is more performant, I'd happily accept the PR as overall it sounds like a good idea. |
|
@Dimezis First of all, you are correct - blur filter has nothing to do with the implementation I present. Blur does only process the displayed region and samples only the pixels required to draw it. However, there are still some gains for using smaller RenderNode. In conclusion, it seems like the main win is avoiding unnecessary rasterization, not applying the blur to a smaller area. Also, upon inspecting the hwui / skia source code I also realized I was wrong in "conservatively" estimating the blur offset, so I have fixed that in my latest commit. P.S. It turns out I am completely clueless how to set up a benchmark for this, but I can setup a toy example if you would like where you can see performance before / after the change on your own device. |
|
Thanks, I'll try to measure the memory footprint, and will also double check if there's a way to have a BlurView-sized RenderNode while somehow maintaining the animation capabilities |
Hi 👋
Currently the blur
RenderNodealways covers the entireBlurTarget, so any change in the background causes the blur effect to be re-evaluated over the whole target for everyBlurView. That's great for aBlurViewthat animates across the screen, but it's wasted work in what I expect is the most common case — aBlurViewthat is static or barely moves.This PR adds an opt-in
setReservedOffset(Integer). When set, the snapshot node is cropped to theBlurView's bounds plus the blur's required padding (conservatively estimated from the blur radius) plus the developer-provided offset, so the blur only processes that region. If the view moves outside the reserved region, the snapshot is re-recorded with a fresh crop — so the offset is the slack you give it before re-recording kicks in. WhensetReservedOffsetis never called (or called withnull), the existing full-target behavior is preserved. The change only affects the API 31+RenderNodepath.This also addresses the existing TODO in
hardwarePathabout keeping the node closer to theBlurView's size instead of the target's.Tradeoff: a
BlurViewthat continuously animates beyond its reserved offset will sometimes re-record the snapshot, so the old default remains better for that case.I am happy to change anything about the design if it helps land the improvement.