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

Blur frozen on Android API 31 #77

Closed
jakobulbrich opened this issue Jan 4, 2024 · 17 comments · Fixed by #408
Closed

Blur frozen on Android API 31 #77

jakobulbrich opened this issue Jan 4, 2024 · 17 comments · Fixed by #408
Labels
android wontfix This will not be worked on

Comments

@jakobulbrich
Copy link

On Android 12 (API 31) the blur is frozen while scrolling.
Only when the bottom navigation appears or disappears the blur quickly updates and is then frozen again.

Seems like HazeNode31.onObservedReadsChanged() is not called. Probably a bug in Compose or can we do something about it?

See attached video of the android-jetpack sample on API 31. It is working as expected on API 32.

haze-api31.mp4
@chrisbanes
Copy link
Owner

So it seems that the draw() function isn't being called consistently on API 31, but is on API 32+. I'll raise on the Compose Issue Tracker.

@chrisbanes
Copy link
Owner

https://issuetracker.google.com/issues/318679450

@chrisbanes
Copy link
Owner

I'll have another play tomorrow and see if I can get something working. If not I'll probably have to bump up the min SDK level for the blur impl to 32 for now.

@chrisbanes chrisbanes added the bug Something isn't working label Jan 5, 2024
@gleb-skobinsky
Copy link

I hope I can add some context as to the origins and the reason why this bug happens. It appears that everything is okay on the Compose side. The issue here is with the RenderNode. My hypothesis is that on API 31, it fails to refresh on the JNI side if its position has not updated since the latest draw call. More specifically, in this part of the code:

private fun Effect.updateRenderNodePosition() {
    renderNode.setPosition(0, 0, bounds.width.toInt(), bounds.height.toInt())
    renderNode.translationX = bounds.left
    renderNode.translationY = bounds.top
    renderNodeDirty = false
  }

If we feed different positions at each call, the RenderNode is updated successfully. It is not acceptable to use in the library, of course, but I wrote a little dirty hack to reproduce this behavour: https://github.com/gleb-skobinsky/haze/blob/main/haze/src/androidMain/kotlin/dev/chrisbanes/haze/AndroidHazeNode.kt

2024-02-29.21.47.15.mov

@gleb-skobinsky
Copy link

The video is so short because github does not accept long screen recordings :)

@gleb-skobinsky
Copy link

UPD: I managed to remove the freeze on SDK level 31 without the hacks! Just applied the translation to the setPosition method instead of translation in the record lambda. Check out the source code

@gleb-skobinsky
Copy link

It took hours to debug this...

@chrisbanes
Copy link
Owner

Great detective work @gleb-skobinsky! Send a PR over?

@gleb-skobinsky
Copy link

Yes, I will create a PR today

@chrisbanes chrisbanes added wontfix This will not be worked on and removed bug Something isn't working labels Oct 26, 2024
@chrisbanes
Copy link
Owner

Going to close this, as there's not much we can do. RenderNode received a number of improvements and fixes in Android SDK 32 which we rely on. Therefore we'll need to keep our current behaviour of not using the blur effect on SDK 31.

@chrisbanes chrisbanes closed this as not planned Won't fix, can't repro, duplicate, stale Oct 26, 2024
@Skaldebane
Copy link

Skaldebane commented Nov 11, 2024

Could this be an emulator-specific quirk? I do encounter this bug on an Android 12 emulator, but no physical device I tried thus far exerts this behavior.

I have a Samsung Galaxy A11 running Android 12, and it works fine there.

I also tried some Firebase Remote devices with SDK 31:

  1. Samsung Galaxy A51: Haze works properly, content is updated properly.
  2. Google Pixel 6a: Haze works properly. And this is the closest we can be to AOSP.

There's definitely better ways to get access to more devices, but I guess this paints a good general picture of what's going on. I suspect the emulated graphics in the emulator have something to do with it, but I may be wrong about that.

Though I'd love to see a Xiaomi being tested, since they're notorious for creating their own bugs (let alone fixing AOSP ones), and any other devices.

@chrisbanes chrisbanes reopened this Nov 12, 2024
@chrisbanes
Copy link
Owner

chrisbanes commented Nov 12, 2024

Just like @Skaldebane, I just tried the Haze Samples on a Pixel 6 and Samsung A51 on SDK Level 31 and everything seems to be running fine. I don't have any other SDK 31 devices so this is as much testing as I can do.

I see two paths forward:

  1. Enable blurring on SDK Level 31 (also known as the yolo plan).
  2. Add a flag which allows devs to enableBlurringOnProblematicPlatforms, probably defaulting to false. When true, we enable the effect on SDK 31.

I'm leaning to option 2, but happy to hear suggestions.

Even if this only happens on the SDK 31 emulator, that's still something which devs are going to use.

@Skaldebane
Copy link

Skaldebane commented Nov 12, 2024

Option 2 seems like a safer bet, but would also mean that most developers won't ever opt into it (defaults always win). Not that big of deal given that it's already disabled in current versions, and developers will make sure to have good fallback anyways for older Android versions.

Though if option 1 is the path you end up choosing, it might be a good idea to have the flag, just set to true by default instead.

Of course the equation changes if an efficient blurring method for older versions is found (#140), but then we can use the "compat" version on Android 12 by default.

@chrisbanes
Copy link
Owner

Thinking about it, I think a blurEnabled: () -> Boolean block is the way to go.

It will default to calling a public function, but you can override it however you wish. This is platform-agnostic too, which is a big bonus.

@Skaldebane
Copy link

@chrisbanes That's a great idea!

I assume it's just going to ignore the value on SDK lower than 31 on Android, right?

@chrisbanes
Copy link
Owner

I assume it's just going to ignore the value on SDK lower than 31 on Android, right?

Yes, exactly. It will ignore values where blurring is not feasible.

@chrisbanes
Copy link
Owner

One the PR above lands, you'll be able to do the following and enable blurring everywhere (where it's technically possible):

Modifier.hazeChild(...) {
  blurEnabled = true
}

chrisbanes added a commit that referenced this issue Nov 13, 2024
This allows developers on platforms where it is technically possible to
blur, but Haze defaults not to due to known issues.

Fixes #77
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants