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

Allow concurrent buffering and dispatch of input events #76399

Merged
merged 1 commit into from
May 8, 2023

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Apr 24, 2023

Fixes #75893.

Tagged as platform_android, but it really affects any hypothetical platform where Godot's main thread is not the app main/UI thread.

@m4gr3d, @lawnjelly, in the end I've taken a simpler route than double buffering or flushing a copy of the list.

Sadly, I have no time at the moment to test this. Tagging as such.

Counterpart for 3.x is #76400.

UPDATE: Let me give an explanation of what this PR does:

  • _parse_input_event_impl() is the "backend" of input event handling, the function that dispatches the events to the engine and user code. It holds a lock because it can still read/write some common data,
  • On Android we have a special threading situation: Android main/UI thread is not Godot's main thread; Android's GL thread is Godot's main thread. Therefore, input events are received in the Android UI thread and buffered there; later, they are flushed on the GL thread (Godot's main thread). Without this PR, a lock can be held for too long during the flush/dispatch stage, depending on what user code does (like loading a level), preventing the Android UI thread from buffering subsequent input events for loo long (causing the ANR).

@RandomShaper RandomShaper force-pushed the fix_android_input_anr branch 2 times, most recently from 6dca5f5 to 4a6357b Compare May 8, 2023 06:59
@lawnjelly
Copy link
Member

lawnjelly
RandomShaper Ok it seems worth a try, will approve. I'm not totally sure is thread safe but am happy to trust you on that.


RandomShaper
7:41 AM
Thanks!
_parse_input_event_impl() will always be run on Godot's main thread.
I will add an assertion about that so that's clearer for readers of the code and also enforced at the machine level.

lawnjelly
lawnjelly
7:42 AM
Well it can be called directly if no buffering or accumulation I think?
So if any other platforms call from not the main thread in that situation?


m4gr3d
7:43 AM
I'll take a look.
I'm trying to see if I can repro the state that causes the ANR with the current code.
I want to test the PR with that repro project and see if that resolves the issue.

RandomShaper
[7:35 AM](https://chat.godotengine.org/channel/android?msg=NHwixWTAh6A9SRpcz)
@m4gr3d, would you take a look?
👍 1 


RandomShaper
7:44 AM
Yes. And in that case it's still Godot's main thread.

lawnjelly
[7:42 AM](https://chat.godotengine.org/channel/android?msg=rjz4Z6avbfQhYrbuL)
Well it can be called directly if no buffering or accumulation I think?
Any platform having split threads has to enable input buffering (that's an engine internal thing, orthogonal to input accumulation).

lawnjelly
[7:43 AM](https://chat.godotengine.org/channel/android?msg=puvQdeS5vtv7fTLDi)
So if any other platforms call from not the main thread in that situation?

lawnjelly
lawnjelly
7:48 AM
RandomShaper The main danger is that if an incoming event added to the buffer and mucked up the list iteration while flush_buffers() was still running on the main thread.
So am trusting that you have worked out this can't happen.


RandomShaper
7:52 AM
I changed the code that iterates the list, but now you mention it I should at least add a comment about it.

lawnjelly
lawnjelly
7:54 AM
It looks like (in 4.x at least) it just pops the front each time in flush_buffered_events() so it should be ok I think.
It would be very easy to inadvertently break in future though so perhaps a comment would be good to this effect.
Within the flush routine, that the list can be altered during the routine.


RandomShaper
7:59 AM
Done!

@RandomShaper RandomShaper force-pushed the fix_android_input_anr branch from 4a6357b to f369ed9 Compare May 8, 2023 07:15
@akien-mga akien-mga merged commit 49ba2e0 into godotengine:master May 8, 2023
@akien-mga
Copy link
Member

Thanks!

@SirTodd
Copy link

SirTodd commented May 8, 2023

Any chance this could be cherrypicked before 4.1?

@Calinou
Copy link
Member

Calinou commented May 8, 2023

Any chance this could be cherrypicked before 4.1?

This PR already has the cherrypick:4.0 label, but given 4.0.3 RC 1 was already released, I think this will most likely have to wait for 4.0.4.

m4gr3d added a commit to m4gr3d/godot that referenced this pull request May 12, 2023
@akien-mga
Copy link
Member

Cherry-picked for 4.0.3.

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

Successfully merging this pull request may close these issues.

Android [split_config.arm64_v8a.apk!libc++_shared.so] std::__ndk1::recursive_mutex::lock() ANR
6 participants