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

Replace the current audio driver with Oboe on Android #86776

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kus04e4ek
Copy link
Contributor

@kus04e4ek kus04e4ek commented Jan 4, 2024

Closes: godot-proposals#2358

Sample project 5

TODO:

Known Issues:

  • OpenSL not working. Tested on the virtual machine.
  • Bad audio quality on machines with support only for in16_t format. Couldn't reproduce it on my phone. (My phone supports int32_t format, I just didn't know). Tested on the virtual machine.
  • Audio not working on the physical device. An issue with my phone.
  • Microphone not working.
  • Connecting a new default device will be ignored if done outside the app.
  • Stream closes outside the app and doesn't reopen if other stream started by the other app.

See the comment for new TODO/TODO after merge list.

@AThousandShips
Copy link
Member

You will also need to add the library to the list in COPYRIGHT.txt, instructions are available there

std::shared_ptr<InputErrorCallback> input_error_callback;

std::shared_ptr<oboe::AudioStreamDataCallback> audio_output_callback;
std::shared_ptr<oboe::AudioStreamDataCallback> audio_input_callback;
Copy link
Member

Choose a reason for hiding this comment

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

We don't use the standard library normally, see here, would need to be discussed here depending on the library using it, no other engine component uses this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I know. Oboe uses shared_ptr and regular pointers, but methods accepting regular pointers are marked as deprecated, when I think about it again, it probably will be fine to use regular pointers, it just might create problems when closing the app (Through is this important?)

This comment was marked as outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked when shred_ptr was added, it was in the google/oboe#821 to fix an issue with the data race, without shared_ptr the data race will still occur, so I think shared_ptr should stay

@kus04e4ek
Copy link
Contributor Author

  • Audio not working on the physical device.

Seems to be a problem with my device. Opened an issue on oboe repository.

doc/classes/AudioServer.xml Outdated Show resolved Hide resolved
}

jclass cCharSequence = env->GetObjectClass(source);
jmethodID toString = env->GetMethodID(cCharSequence, "toString", "()Ljava/lang/String;");
Copy link
Contributor Author

@kus04e4ek kus04e4ek Mar 9, 2024

Choose a reason for hiding this comment

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

Camel Case to be consistent with "jni_utils.cpp", e.g.

jmethodID getName = env->GetMethodID(cclass, "getName", "()Ljava/lang/String;");
jstring clsName = (jstring)env->CallObjectMethod(cls, getName);

But that code is pretty old, maybe that code should be more consistent and not the other way around.

@@ -336,12 +337,14 @@ class Godot(private val context: Context) : SensorEventListener {
}

if (nativeLayerInitializeCompleted && !nativeLayerSetupCompleted) {
nativeLayerSetupCompleted = GodotLib.setup(commandLine.toTypedArray(), tts)
var audioManager: AudioManager = context.getSystemService(Context.AUDIO_SERVICE) as AudioManager
nativeLayerSetupCompleted = GodotLib.setup(commandLine.toTypedArray(), tts, audioManager)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

context.getSystemService(Context.AUDIO_SERVICE) can be called on the C++ side, but the code would be harder to understand and longer, this is just here for readability. Will change if needed

@kus04e4ek kus04e4ek force-pushed the master branch 2 times, most recently from 82f0379 to 30de669 Compare March 9, 2024 15:56
@kus04e4ek
Copy link
Contributor Author

kus04e4ek commented Mar 9, 2024

I think the PR is ready. There are just a few issues that are worth addressing:

  • Not all audio device types are supported when using set_device. I think it's fine to leave it like this for now? I would love to do a follow-up PR latter, but I could always finish it in this PR, if that's important. I'm just not sure yet what would be the right approach for every type. Some types might start working only when specific conditions are met, so marked those as not working for now.
  • Not all audio device types are tested when using set_device. I just don't have them. So, if anyone wants to help me with that, then I would be happy to accept your help. Currently, tested are: built-in_earpiece (not working working), built-in_speaker (working), bluetooth_sco (working), bluetooth_a2dp (working), built-in_microphone (working), fm_tuner (working? Tested only on the emulator, and there was just a high-pitch sound, I guess it should be like that?), telephony (not working), remote_submix (not working), echo_reference (didn't test, but it's not expected to work, it's reserved for system apps only).
  • SCO support is experimental? Right now, when setting the device on one of the streams, the device on the other would be set too. I did it that way, because turning on SCO for one stream would turn on SCO for the other. Also, when turning on SCO, the default device used by Android changes, so did it for the consistency on what the default device is. I don't think that any other Audio Driver changes devices for both streams on one set_device call, so I would appreciate some feedback. Also, setting SCO just doesn't work seemingly at random times? I have suspicions, but still not sure. See this comment about SCO.

Issues above should all be fine when not using set_device, I assume that the default device picked by Android always works, so maybe it would be better to disable it for now?

And some theoretically possible feature, that I could do in a follow-up, is very limited set_device for OpenSL.

Also, #88628 could be implemented for Android too. For the same reasons as the ones why it is implemented for macOS.

EDIT:
If you're interested in testing types, then all of them can be found here: https://developer.android.com/reference/android/media/AudioDeviceInfo#TYPE_AUX_LINE
Also, when testing SCO your device might require "MODIFY_AUDIO_SETTINGS" permission, this can be given at the export preset.

@kus04e4ek kus04e4ek marked this pull request as ready for review March 9, 2024 16:42
@kus04e4ek kus04e4ek requested review from a team as code owners March 9, 2024 16:42
@@ -653,7 +653,7 @@
<param index="0" name="name" type="String" />
<description>
Requests permission from the OS for the given [param name]. Returns [code]true[/code] if the permission has been successfully granted.
[b]Note:[/b] This method is currently only implemented on Android, to specifically request permission for [code]"RECORD_AUDIO"[/code] by [code]AudioDriverOpenSL[/code].
[b]Note:[/b] This method is currently only implemented on Android and is used by [code]AudioDriverOboe[/code].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that the specific permissions and what they are for should be put somewhere else? It would feel better if they were in AudioServer docs somewhere, where the user would be expected to look first for any audio specific questions and issues. This PR also adds one more permission "MODIFY_AUDIO_SETTINGS" for SCO support on older devices, but I'm out of ideas where to put it.

@@ -63,6 +63,7 @@ public final class PermissionsUtil {
public static final int REQUEST_RECORD_AUDIO_PERMISSION = 1;
public static final int REQUEST_CAMERA_PERMISSION = 2;
public static final int REQUEST_VIBRATE_PERMISSION = 3;
public static final int REQUEST_MODIFY_AUDIO_SETTINGS_PERMISSION = 4;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that it's unused, but added for consistency

@@ -62,6 +62,7 @@ class AudioDriver {
unsigned int input_size = 0;

void audio_server_process(int p_frames, int32_t *p_buffer, bool p_update_mix_time = true);
void audio_server_init_channels_and_buffers();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exposed it, since Oboe doesn't recommend to allocate memory in callbacks, which would happen through audio_server_process if device changed

## oboe

- Upstream: https://github.com/google/oboe
- Version: 1.8.2 (16d72c89be9eb8a7d617a7be531a31dba3db74f1, 2024)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Put version 1.8.2, although it's not released yet, just to have this fix: google/oboe#1968. Without this fix, on some devices, the audio would be silent or with a lot of glitches since the code checking for it was never called

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed version from "1.8.2" to "git", but the point still stands

@loliaudio
Copy link

EDIT: If you're interested in testing types, then all of them can be found here: https://developer.android.com/reference/android/media/AudioDeviceInfo#TYPE_AUX_LINE Also, when testing SCO your device might require "MODIFY_AUDIO_SETTINGS" permission, this can be given at the export preset.

I would like to help testing, but I have no knowledge abou C++.
I have some of the devices listed.
Is there a way of doing it?

@kus04e4ek
Copy link
Contributor Author

kus04e4ek commented Mar 15, 2024

I would like to help testing, but I have no knowledge abou C++.
I have some of the devices listed.
Is there a way of doing it?

Thanks for replying! Here are instructions on how to build this PR and how to export with it:
First of all you need to install git and SCons.
Then follow the instructions here (everything before "Providing launcher icons" is required) and here.
Clone this PR's github repository: git clone https://github.com/kus04e4ek/godot.git.
I build with these options: scons tests=false deprecated=no minizip=no vulkan=no modules_enabled_by_default=no module_text_server_fb_enabled=yes module_gdscript_enabled=yes module_freetype_enabled=yes module_minimp3_enabled=yes platform=android generate_apk=yes target=template_debug arch=<insert your architecture>
Follow the instructions here (everything before "Providing launcher icons" is required)
Godot 4.3 dev 4 is compatible with the export template, so you can just use it as an editor.
Setup the export preset for android by overriding Custom Template/Debug, choosing the correct architecture and turning on Modify Audio Settings and Record Audio permissions. (To get your architecture you can use AIDA64)
EDIT: Was incorrect in assuming that you would have to build Godot, you can just download artifacts by clicking Details next to 🔗 GHA / 🤖 Android / Template (target=template_release) (pull_request), then choose Upload artifact and download archive from the Artifact download URL link.

@kus04e4ek
Copy link
Contributor Author

Right now, when setting the device on one of the streams, the device on the other would be set too. I did it that way, because turning on SCO for one stream would turn on SCO for the other. Also, when turning on SCO, the default device used by Android changes, so did it for the consistency on what the default device is.

I misinterpreted Android docs, now output and input streams do not depend on each other. This introduced more bugs when setting output device, while input stream uses SCO. Have no ideas what the issue is, but will work on it. Default device still changes, but I don't think that's a big issue.

Setting SCO just doesn't work seemingly at random times?

Fixed it, but input stream that uses SCO is very unstable due to #86428.

@bestvcboy
Copy link

May I ask which version this is likely to be supported on?

@kus04e4ek
Copy link
Contributor Author

kus04e4ek commented Jun 6, 2024

May I ask which version this is likely to be supported on?

Can't really guarantee anything here, I'm not part of Godot's team, I just work on it when I can (right now I don't have time). There are still some issues and I want to refactor the code (I'll mark it as a draft, kinda forgot to do it before😅). I am also more focused on #90013 for now, which would simplify the code here and I think it should be merged first. Not many users request Oboe and there's a lot of code to go through when reviewing, so this PR is probably not a priority for members. As far as I'm aware there's no consensus on using Oboe in the first place (I plan to write a comment on Oboe's proposal page that would go through what are the advantages and disadvantages of using Oboe) (should probably mention that no one on the team seems to be against using Oboe though). I think there's currently only one member of the audio team and that's reduz, who also has other stuff to do, so the reviewing process for this PR may take a long time. Because of all of this, I don't think it should be expected to be part of Godot any time soon if it will be.
EDIT: I also forgot to mention that I want to split this PR into non-Oboe specific ones, that could be applied to OpenSL.

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.

Replace the current audio driver with Oboe on Android
5 participants