igorski / MWEngine

Audio engine and DSP library for Android, written in C++ providing low latency performance within a musical context, while providing a Java/Kotlin API. Supports both OpenSL and AAudio.
MIT License
259 stars 45 forks source link

Audio glitching on a real device. #129

Closed Riobener closed 3 years ago

Riobener commented 3 years ago

Hello again! I keep working with your library and all this time doing this on the android emulator. When I tried to run my app on a real device, it got weird clicks when live playing. Here is example, where i compared your mikrowave app which is using same engine, and my app: https://drive.google.com/file/d/1ah7Udv6W9pfzLj9Sx5ghFOE0ZE0lgMXB/view?usp=sharing My code just using all the same, as mwengine example, just with my simple synth code:

synth = new SynthInstrument();
        synth.getOscillatorProperties(0).setWaveform(0);
        synth.getAudioChannel().setVolume(.7f);

It is working perfeclty almost on all emulators that i tried. My phone is Samsung A71. Sry, if this question already was asked.

Riobener commented 3 years ago

Ok, it was problem with the driver. I forgot to change it. Now it is working fine with OpenSL driver.. Sad, that it is all working bad with AAudio...

But now i have the problem with sine wave. It has some click sound after the next note is starting to play. I tried all wave forms and the problem is only with sine wave and triangle. https://drive.google.com/file/d/1rkxc33CTP7yUD-S4EPBSvPaE3yqGG104/view?usp=sharing

The synth code:

        synth = new SynthInstrument();
        synth.getOscillatorProperties(0).setWaveform(0);
        synth.getAudioChannel().setVolume(.7f);
       //synth.getAdsr().setReleaseTime(0.3f);
        synth.getAdsr().setDecayTime( .5f );
        synth.getAdsr().setSustainLevel( 1f );
        float maxFilterCutoff = ( float ) 48000 / 8;
        float minFilterCutoff = 50.0f;
        _filter = new Filter(
                maxFilterCutoff / 2, ( float ) ( Math.sqrt( 1 ) / 2 ),
                minFilterCutoff, maxFilterCutoff, 2
        );
        synth.getAudioChannel().getProcessingChain().addProcessor( _filter );

The notes binding code:

SynthInstrument instrument = new SynthInstrument();
    Vector<SynthEvent> notes = new Vector<SynthEvent>();
    static int BASE_OCTAVE = 3;
    static List<String> noteNames = Arrays.asList( "C", "C#", "D", "D#", "E", "F", "F#", "G", "G#", "A", "A#", "B" );
for (int i = 0; i < 24; ++i ) {
                        int octave = (int) (BASE_OCTAVE + Math.ceil( i / 12 ));
                        notes.add ( new SynthEvent(( float ) Pitch.note(noteNames.get(i % 12), octave), synth));
                    }

Piano listeners:

@Override
    public void onKeyDown(@NonNull PianoView piano, int key) {
        Log.d(LOG_TAG, "KEY PRESSED IS " + key);
        if(notes.get(key)!=null)
        notes.get(key).play();
    }

    @Override
    public void onKeyUp(@NonNull PianoView piano, int key) {
        Log.d(LOG_TAG, "KEY UPED IS " + key);
        if(notes.get(key)!=null)
        notes.get(key).stop();
    }
igorski commented 3 years ago

For the AAudio issue, could you verify whether the library version in feature branch native-thread solves this issue for your device ? It addresses thread performance tuning specifically for AAudio.

For the pops, sine waves do indeed have this behaviour. What happens is that when the next note plays (for instance when you slide from C to C#), the C# note starts it slope which overlaps with the slope of the previous C note (which is now in its release phase). The cumulative effect of these create a jump in amplitude known as DC offset. This can be filtered in numerous ways, but often the solution can be simpler.

Could you experiment by setting the attack of the synths ADSR to a very small positive value, say 0.01f for the sine wave ?

Riobener commented 3 years ago

For the AAudio issue, could you verify whether the library version in feature branch native-thread solves this issue for your device ? It addresses thread performance tuning specifically for AAudio.

For the pops, sine waves do indeed have this behaviour. What happens is that when the next note plays (for instance when you slide from C to C#), the C# note starts it slope which overlaps with the slope of the previous C note (which is now in its release phase). The cumulative effect of these create a jump in amplitude known as DC offset. This can be filtered in numerous ways, but often the solution can be simpler.

Could you experiment by setting the attack of the synths ADSR to a very small positive value, say 0.01f for the sine wave ?

Well, sadly it didnt fix the AAudio issue. I tried to run example app from native-thread branch, but the result has not changed.

About sine wave, i tried 0.01f and smaller, or higher value, it makes the same effect. Also i tried each combination parameters of ADSR and still not working.

igorski commented 3 years ago

I'll make some time to investigate the sine wave issue, I think it might be a side effect from the "multi timbral keyboard" example I gave you as I think the on/off phases aren't behaving as they should.

For the AAudio issue, I am intrigued as said branch offers improvement on my test devices though I am aware that on the Android eco system there isn't a "one size fits all" solution. Out of interest, what is the exact device you are testing on ? Also, would you consider compiling the code again but with the following change:

If you can comment out line 61 of global.h (which contains _#define PREVENT_CPU_FREQUENCYSCALING) prior to building, does this have a notable difference on your device ?

Riobener commented 3 years ago

I'll make some time to investigate the sine wave issue, I think it might be a side effect from the "multi timbral keyboard" example I gave you as I think the on/off phases aren't behaving as they should.

For the AAudio issue, I am intrigued as said branch offers improvement on my test devices though I am aware that on the Android eco system there isn't a "one size fits all" solution. Out of interest, what is the exact device you are testing on ? Also, would you consider compiling the code again but with the following change:

If you can comment out line 61 of global.h (which contains _#define PREVENT_CPU_FREQUENCYSCALING) prior to building, does this have a notable difference on your device ?

I apologize for keeping you waiting for my answer, I was a little bit busy... Yeah, i tried to comment out line 61, but this, according to my feelings, does not bring any result.

My phone is also - Samsung A71, with Snapdragon 730. I'll attach a link just in case : amazon

I am completely ready to test any performance of your library on my phone in the future. :)

igorski commented 3 years ago

No worries on the wait, I'm already happy you have pointed out this issue and are

completely ready to test any performance of your library on my phone in the future

:smiling_imp:

If I may ask, could you conduct a test where you experience the AAudio glitching (does it by the way happen on occasion or is the glitching / clicking audible throughout ?)

If you could record a .wav of the output and also record the logcat output of all messages starting with AAudio_IO:: that would be a tremendous help. I have an assumption what could be the case and would like to confirm it with the audio file and the logs.

(The sine wave issue I'll treat separately but wil address).

igorski commented 3 years ago

Alright @Riobener, I did some tests and discovered an issue with the AAudio driver that becomes apparent on certain devices. The benefit of AAudio is that it can request a sample amount that corresponds to the amount of available resources the driver deems available for the next timeslice. This means that certain render iterations would render less samples than the initially requested buffer size, which is fine. However when rendering this into the output buffers this would render for the entire requested buffer range, meaning there were drop-ins and outs at the upper end of the actual rendered range.

If you check out branch native-thread once more, can you verify that the AAudio stability is equal to the OpenSL performance ?

Riobener commented 3 years ago

Great job, @igorski! Everything sounds the same now. I understood your explanation, but it's still interesting that the same driver works differently everywhere :) I'll ask just in case, before creating a new topic, is it okay in this section (on the github issues) to discuss questions about the engine's capabilities and the future of engine , or is it more preferable to write only about bugs or improvements? I just have some several questions about the current capabilities of the engine and about the still unrealized.

igorski commented 3 years ago

Good to hear @Riobener !

I'll clean the branch and merge it to master over the coming days as it marks a large stability improvement. I'll keep this issue open to tackle the sine wave popping.

If you have more questions, the GitHub issues are a great spot to post them in, whether they're bug reports or feature requests. I would suggest creating a unique issue per request/bug report as it makes for easier housekeeping and history. :)

Riobener commented 3 years ago

Oh, I'm a bad tester. For AAudio I used to check pulse wave and apparently did not hear that the problem is still exist in sequencer events. The problem completely disappeared from live events.

Good to hear @Riobener !

I'll clean the branch and merge it to master over the coming days as it marks a large stability improvement. I'll keep this issue open to tackle the sine wave popping.

If you have more questions, the GitHub issues are a great spot to post them in, whether they're bug reports or feature requests. I would suggest creating a unique issue per request/bug report as it makes for easier housekeeping and history. :)

igorski commented 3 years ago

🤦‍♂️ Ah, quite right @Riobener ! (I disagree though with your statement as you are an excellent tester as this would have gone by unnoticed !!)

There was a separate buffer management between sequenced and live synth events. This has been equalized. The aforementioned branch has been updated to include this new code.

Additional good news is that the synthesizer instrument now consumes less memory as temporary buffer management is now handled by the instrument and not the individual events.

igorski commented 3 years ago

Note the native-thread branch has been merged into master and been deleted. This because the state of that branch provided a much more performant engine.

Riobener commented 3 years ago

Note the native-thread branch has been merged into master and been deleted. This because the state of that branch provided a much more performant engine.

Good update! All works good. However, after refreshing MWEngine, Swig just wrote that in 177 line of CMake file, swig_add_module is deprecated and that I should instead use swig_add_library, so i just change this: swig_add_module(${target}_wrapped java ${CPP_SRC}/mwengine.i) on this swig_add_library(${target}_wrapped LANGUAGE java SOURCES ${CPP_SRC}/mwengine.i) and build after that was successful, My SWIG version is 4.0.2.

This is a minor error, but I decided to warn you.

igorski commented 3 years ago

Good to hear we finally tackled this issue!

Thanks for pointing out the deprecated function, I have just pushed your change using swig_add_library to the repository.