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

feat: Add AudioOptions class #978

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: Add AudioOptions class #978

wants to merge 6 commits into from

Conversation

karasusan
Copy link
Collaborator

@karasusan karasusan commented Sep 21, 2023

This PR adds the AudioOptions class to set audio options to tracks.
The options are defined by the AudioOptions class.

class AudioOptions
{
        public bool? echoCancellation;
        public bool? autoGainControl;
        public bool? noiseSuppression;
        public bool? highpassFilter;
}

This PR makes AudioStreamTrack constructor changing.

  • AudioStreamTrack(AudioOptions options = null);
  • AudioStreamTrack(AudioSource source, AudioOptions options = null)
  • AudioStreamTrack(AudioListener listener, AudioOptions options = null)

@karasusan karasusan changed the title add audiooption add audio option Sep 21, 2023
@karasusan karasusan changed the title add audio option feat: Add audio option Sep 21, 2023
@karasusan karasusan marked this pull request as ready for review September 25, 2023 06:49
@karasusan karasusan changed the title feat: Add audio option feat: Add AudioOptions class Sep 25, 2023
@flamacore
Copy link

flamacore commented Oct 1, 2023

So hey, I've followed the issues that are mentioned in this PR and I also would like to give a try to the audio options.

Sadly I have problems with getting this specific pr to build as I'm not very well versed with CMake and the like. It throws an error about not finding presets and shuts down.

Anyhow, is there any progress on making this a reality? Any sort of easier way I can add this and experiment with it in my own project yet?

@karasusan
Copy link
Collaborator Author

karasusan commented Oct 3, 2023

@flamacore
Hi, if you can help us to test the audio option, I'll be glad to take you a package.

Here you are.
https://drive.google.com/file/d/1Qf4ZLTqNxT-hoLfgJj3JZIpFu3hEPLNI/view?usp=drive_link

Please install the tgz file following this manual.
https://docs.unity3d.com/Manual/upm-ui-tarball.html

@flamacore
Copy link

@karasusan would be delighted to. Thanks a lot for sharing the package. I'll give it a check soon and will inform if it works or not.

@MaximKurbanov
Copy link
Contributor

I tested and issue #964 remains.
I tried this with android-android and windows-android:

  #...
    {
  #...
        var track = new AudioStreamTrack(sendAudioSource, GetAudioOptions());
        mediaStream.AddTrack(track);
    }

    private AudioOptions GetAudioOptions()
    {
        return new AudioOptions()
        {
            echoCancellation = true,
            noiseSuppression = true
        };
    }

@flamacore
Copy link

flamacore commented Oct 5, 2023

Can confirm the echo issue persists. Not sure if it's related to us enabling/disabling the options or if the internal echo cancellation works as intended. EDIT: I tried it multiple ways:

  • iOS - iOS
  • iOS - Android
  • iOS - Windows
  • Android Windows

@karasusan
Copy link
Collaborator Author

@MaximKurbanov
@flamacore

Thank you for checking. I'd like to clarify which this issue is a specific by mobile platforms or not.
For example, windows and windows, or windows or macOS.

It looks there are different implementations for mobile and desktop platforms.
https://stackoverflow.com/questions/62479789/webrtc-android-echo-cancellation

@MaximKurbanov
Copy link
Contributor

@karasusan
I tested Windows-Mac, AEC does not work.

@karasusan
Copy link
Collaborator Author

@MaximKurbanov
I understood the cause of this issue.
Sorry but we need more time to fix it.

I can't share you many information about that, but at least I can say that the "audio processing" in native code is not processed.

The "audio processing" in webrtc is implemented here.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/modules/audio_processing/audio_processing_impl.cc

@pixlhero
Copy link

pixlhero commented Dec 12, 2023

@karasusan
I tested the package from Oct 6th on Mac-Editor <-> iPad.
It also didn't cancel the echo.

This is a show-stopper for us. So we really, really need this fix in the next release (pre-8). 🙏

Also building the plugin doesn't work for us. So if 0272460 would fix anything (we're targeting iOS) it would be great if you could provide us with a .tgz build.

がんばってよ!

@pixlhero
Copy link

pixlhero commented Dec 13, 2023

@karasusan I tested the package from Oct 6th on Mac-Editor <-> iPad. It also didn't cancel the echo.

This is a show-stopper for us. So we really, really need this fix in the next release (pre-8). 🙏

Also building the plugin doesn't work for us. So if 0272460 would fix anything (we're targeting iOS) it would be great if you could provide us with a .tgz build.

がんばってよ!

We found a workaround on iOS involving AVAudioSession. So for our project this issue is resolved.
https://gist.github.com/pixlhero/3d370dcdbcfaefa4df12138161307da4

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.

4 participants