Skip to content
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

Improve blur effect accuracy #2571

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

geomaster
Copy link
Contributor

Analogously to drop shadows in #2548, allow blurs to be applied to composition and image layers using the new OffscreenLayer.

Rename applyShadowToLayers to applyEffectsToLayers and use it for both of these effects. Keep externally-facing applyShadowToLayers and forward to applyEffectsToLayers for backwards compat.

Undo the layer transform when computing the radius of a blur effect, similarly as we do for shadows.

Use fast hardware blur using RenderEffect if possible, and otherwise delegate to a "reasonably fast" box-blur implementation on the CPU.

This is not the final version. Still to do:

  • Try to optimize FastBlur more
  • Check behavior on existing testcases to ensure no regressions
  • Match the scaling constants between implementations to ensure the blur appears the same as in lottie-web, the same with applyEffectsToLayers and without, and the same with hardware vs software implementation of blur.

Copy link

github-actions bot commented Nov 5, 2024

Snapshot Tests
API 23: Report Diff
API 31: Report Diff

Be more careful about the sigma->radius conversion, blur propagation,
and don't forget to undo the pre-existing scale for the RenderEffect
impl. It seems that all implementations of blur we have now - mask
filter, CPU blur, and RenderEffect blur - look mostly the same.
Based on the old box blur algorithm with a running sum, write a new one
that is more readable and more performant, due mostly to avoiding the
use of Bitmap.getPixels() and Bitmap.setPixels().

The blur is still, well, boxy, which we will probably be able to fix by
stacking two invocations of it, or trying some trickery to approximate
the Guassian kernel better but still keeping the linear time complexity.
Copy link

github-actions bot commented Nov 6, 2024

Snapshot Tests
API 23: Report Diff
API 31: Report Diff

Copy link

github-actions bot commented Nov 8, 2024

Snapshot Tests
API 23: Report Diff
API 31: Report Diff

Copy link

Snapshot Tests
API 23: Report Diff
API 31: Report Diff

Copy link
Collaborator

@gpeal gpeal left a comment

Choose a reason for hiding this comment

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

This LGTM! We can merge through the API compat check because I don't have a good solution yet. Let me know if this is ready to merge on your end!

@geomaster
Copy link
Contributor Author

Thanks so much, @gpeal! Looking at some last remaining render diffs - I think I shouldn't have changed this rounding: https://github.com/airbnb/lottie-android/pull/2571/files#diff-f59159579cb60a5c96b48c67ce22f4efb9c861bac8d83d3a9361a63f5797a7a8R398, so will probably revert that back, check with another snapshot run and then I'm completely ok to merge. Thanks again for the quick eyes on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants