Android: Ensure cleanup of all subobjects in the OpenSL audio driver #85955
Conversation
|
Can you please open an issue report for this, or link to one if one exists, to help track the issue (so it doesn't get lost if this PR is closed for some reason, or a different solution is required) |
drivers/unix/os_unix.h
Outdated
| @@ -48,6 +48,7 @@ class OS_Unix : public OS { | |||
|
|
|||
| public: | |||
| OS_Unix(); | |||
| virtual ~OS_Unix(); | |||
There was a problem hiding this comment.
Is this needed? OS already has a virtual destructor
There was a problem hiding this comment.
Perhaps not, though I recall that when I was breakpointing through this, the stack traces looked a bit different - and clearer in my opinion - when I explicitly declared this intermediately inherited virtual destructor.
| } | ||
|
|
||
| void AudioDriverOpenSL::set_pause(bool p_pause) { | ||
| pause = p_pause; | ||
|
|
||
| if (active) { | ||
| if (active && playItf) { |
There was a problem hiding this comment.
There are other contexts where the different pointers are used without a check, should they not have them as well?
Also active is not set to true until after it has initialized this pointer
There was a problem hiding this comment.
I added this check primarily to ward against possible data race conditions of accessing the playItf while the driver is being cleaned up more than out of any concern for the state of a partial initialization success/failure where some members are populated before the active flag is toggled on. My familiarity with the extent of set_pause's usage is limited, but if there is some notion of a guarantee that it won't be called in parallel with driver destruction, then I can take this out as redundant. Otherwise, I can instead add extra checks elsewhere to the cases you described.
There was a problem hiding this comment.
Went ahead and added a bit of extra carefulness here...
11ff925
There was a problem hiding this comment.
I added this check primarily to ward against possible data race conditions of accessing the
playItfwhile the driver is being cleaned up.
There can still be a rare data race condition when this line executes between these lines:
godot/platform/android/audio_driver_opensl.cpp
Lines 280 to 281 in c197418
Because of that
playItf is not nullptr here:But will be
nullptr here:IMO doesn't have to be addressed in this PR
|
See also: |
This comment was marked as outdated.
This comment was marked as outdated.
|
@Alex2782 the callback nullification seems redundant since I do not believe those function pointers will be invoked inside the underlying OpenSL engine whenever the corresponding interface reference's state is " |
|
@AThousandShips |
|
@Alex2782 After some testing, I have found that the buffer array deletion and buffer interface callback nullification causes a crash upon app closure: 
|
|
ok I'll try it tomorrow too: official audio demos I didn't notice any crashes in my test project. I just found it strange that something was active even though I wasn't playing any sound. |
|
Tested with spectrum demo. No crashes or OpenSL error messages. 👍 BACK button, with APP-LIST button + wipe away (remove) the app At first everything is paused, stopped and only the OpenGL Logcat |
There was a problem hiding this comment.
The changes look good!
Can you address the comments from @AThousandShips and squash your commits once the PR is ready.
|
@m4gr3d I believe that I have already addressed the comments by @AThousandShips thus far to date, and I will handle the commit squash shortly here. |
PierceLBrooks
force-pushed
the
plb/fix-opensl-audio-driver-closure
branch
from
71f3566
to
c197418
Compare
| @@ -330,3 +365,7 @@ void AudioDriverOpenSL::set_pause(bool p_pause) { | |||
|
|
|||
| AudioDriverOpenSL::AudioDriverOpenSL() { | |||
| } | |||
|
|
|||
| AudioDriverOpenSL::~AudioDriverOpenSL() { | |||
| end(); | |||
There was a problem hiding this comment.
It's unnecessary, don't think the destructor of AudioDriverOpenSL can be called without finish if everything initialized properly. Here is the order of calls as far as I'm aware: _terminate() -> Main::cleanup() -> Main::cleanup() (copied to include another link) -> audio_server->finish() -> AudioServer::finish() -> AudioDriver::finish() -> delete os_android -> ~OS_Android() -> ~AudioDriverOpenSL(). If it crashes, no destructors are called. If AudioServer was not created, AudioDriverOpenSL::init, AudioDriverOpenSL::start and AudioDriverOpenSL::input_start can't be called
There was a problem hiding this comment.
I don't see any harm in leaving this here since if end() has already been called, the second call will be a no-op.
There was a problem hiding this comment.
There's no harm, but it's not needed too. It also creates inconsistency with other AudioDrivers as no other AudioDriver defines a destructor, there's already a function finish for this. It's more of a style change than anything
There was a problem hiding this comment.
I did a quick test and the fix still works as expected with the destructor removed so it can be removed for consistency.
There was a problem hiding this comment.
I pushed an updated removing that destructor.
I also removed the explicit ~OS_Unix destructor and virtual qualifier for ~OS_Android which were discussed above. They're not wrong, but also don't seem needed for that fix, and would introduce an inconsistency between OS_Android and other OS implementations.
If we want to make sure all classes that inherit one with a virtual destructor also make it explicit in their declaration, that should likely be a separate style fix (as I understand it it shouldn't impact the behavior: https://stackoverflow.com/questions/71249522/virtual-destructor-needed-for-class-which-is-both-derived-and-base).
Still would be worth testing the latest commit again to make sure.
There was a problem hiding this comment.
Checked the latest push and libOpenSLES: Object::Destroy errors are still gone
|
@akien-mga #85979 is more visible after #94661 (the Android OS thinks the app has crashed even though it's still running) so we should include this PR in 4.3. I had approved this PR a while ago, so it's good to go from my point of view. |
akien-mga
changed the title
akien-mga
force-pushed
the
plb/fix-opensl-audio-driver-closure
branch
from
c197418
to
7b99033
Compare
akien-mga
force-pushed
the
plb/fix-opensl-audio-driver-closure
branch
from
7b99033
to
e3482a9
Compare
|
Thanks! And congrats for your first merged Godot contribution 🎉 |
Depending on different Android devices, without these changes, a crash or a hang may occur when the Godot engine instance is being torn down as the OpenSL audio playback subsystem gets closed. The most easily expressible side effect of such an occurrence appears as follows ( taken from ADB LogCat ):
Bugsquad edit: