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
257 stars 45 forks source link

Strategy for MWEngine and config changes #133

Closed scar20 closed 2 years ago

scar20 commented 3 years ago

Short question is where to put MWEngine that will be immune to config changes if for some reasons you can't avoid dealing with them? I noticed that in your MWEngineExample activity, even when you force a rotation of the screen; the engine continue running unaffected. In my version of the same activity (the one for the loop experiment), it stop and if I rotate a second time, it crash. The only difference was that I extended from AppCompatActivity and that needed an extra "android:theme="@style/Theme.AppCompat"" in the manifest. I though that had some side-effects on the behavior for focus (I could rant for days about the "style" thing but i'll refrain..) so I changed back to extending from Activity. Same result. Here a snippet for "run" log when it crash - I can send the long version if needed. D/AAudioStream: setState(s#6) from 11 to 11 E/AAudioStream: setState(6) tried to set to 11 but already CLOSING D/AAudioStream: setState(s#6) from 11 to 12 W/AAudioStream: safeStop() stream not running, state = AAUDIO_STREAM_STATE_CLOSED D/wengine.exampl: PlayerBase::~PlayerBase() V/MWENGINE: AAudio_IO::Error stopping stream. AAUDIO_ERROR_INVALID_STATE D/AAudio: AAudioStream_close(s#6) returned 0 --------- D/AAudio: AAudioStream_close(s#6) called --------------- D/MWENGINE: native audio rendering thread halted A/libc: FORTIFY: pthread_mutex_lock called on a destroyed mutex (0x7cc74300a8) A/libc: Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 6274 (Thread-4), pid 6117 (wengine.example) D/MWENGINE: window focus changed for MWEngineActivity, has focus > true D/MWENGINE: window focus changed for MWEngineActivity, has focus > false V/MWENGINE: STOPPING engine

So is there alternate method for making sure we keep the engine alive in a safe place immune to config changes?

I have take great care to handle them in all of the UI of the Fonofone and I'm ready to connect all the values of the controllers (which are all LiveData from a viewmodel) with MWEngine. Fonofone also have some controllers that open the keyboard without stopping the sound so I foresee a problem there. I though maybe put MWEngine inside the ViewModel but it doest seem that's the right place. A thread? A plain java object inited by the activity with a reference in the viewmodel that will act as the junction box to interconnect the controllers values to the engine? Ok I stop here since that question can branch out too far outside the scope. In resumé I need a rock solid place to put the engine which I can refer to and kill myself when finished.

igorski commented 3 years ago

I had postponed answering this question as there is currently a development going that will be soon merged to the main branch of the repository. This development on one hand addresses issues with the AAudio driver and on the other it addresses the topic of threading.

Basically, the current main branch of the repository lets the Java side handle the threading upon engine start. The native-thread branch does this from the C++ code (with the benefit of also allowing to tune the thread priority). You might benefit from this as the Activity focus changes handled by the OS wouldn't impact the MWEngine thread as before.

As for your question:

In the new API, you can stop() and start() the engine (pause|unpause are deprecated) whenever you see fit on the focus change. Note that when the keyboard opens, the application window curiously loses focus (meaning that the sound would stop if you literally used the same handlers as in the example activity). Maybe you don't want to stop the engine at all (unless a phone call comes in or something), but that depends on your preference / application requirements. There is this helpful diagram on the Activity lifecycle that might help.

scar20 commented 3 years ago

OK So for now my strategy will be not bother and init in the activity and pass around that pointer where needed to create tracks, then do better later on...

igorski commented 3 years ago

Actually re-reading your question I'm wondering if I misinterpreted it.

Is your question about stopping/starting the engine rendering when an application changes focus (which I initially assumed) or is it about where the engine is instantiated and/or where its configuration (channels, processors, events, etc) is maintained throughout the application lifetime (e.g. when changing Activities)

scar20 commented 3 years ago

More of the later. The application is mainly a one activity app with some sub-activity happening. Now I'm a bit rushed to connect the engine with the UI even not in the ideal way. But I want to take a good look at this development branch later on.

Actually, I have all the values of the controllers UI's in a viewmodel and I will need a place to "observe" those values and it can not be the mainActivity since we don't know in advance which one will be connected (that happen dynamically as the user create controllers by dragging them in the workspace from a toolbar) - it is very un-android like. On the other hand, the processes are fixed; each sample have the same processing chain - loop setting, filter, speed, reverb, etc. So on the engine side its easy; array of "tracks" that are all the same.

Its the connecting part that leave me starring at the code and procrastinate... Perhaps I could initialize the engine in the main viewModel, inheriting from AndroidViewModel so I got a pointer to Application and pass that application context to MWEngine. I have a "Track" inner class in the viewmodel that set up the LiveData tracks parameters so I could add the engine code there, ex:

public static class Track { private EngineTrack engineTrack = new EngineTrack(); // to do: add an engineTrack class

    private MutableLiveData<PointF> mPanNormPosition = new MutableLiveData<>();

    public LiveData<PointF> getPanNormPosition() {
        return mPanNormPosition;
    }

    public void setPanNormPosition(PointF p) {
        this.mPanNormPosition.setValue(p);
        enginetrack.panPosition.setValue(p); // or something like that
    }

}

In that way, I dont "observe" the value, I just set it wherever it changing. If I init the engine in the activity, I will need to create a handler class to be able to "observe" change in livedata and connect those change to the engineTrack thing. Bit of thinking out loud here....

Anyway, this can become lengthy - already is - and I'm not asking for you to desing my app... And do not hesitate to tell me if I go too much out of scope. I think I'll go for the viewmodel from AndroidViewModel way since that context wont be destroy with config change.

igorski commented 3 years ago

If you have multiple Activities in your application, the best bet for maintaining the engine code would be using a class with static members. This memory will be persisted across applications as long as they share the same process (and they should as engine threading is handled by the native code (in aforementioned feature branch) / not by the Activity (in current master branch)).

That class would then be the one that initializes the engine, register its events, instruments, processing chains, etc. (and hold those references in static members).

I would suggest indeed using the ViewModel approach as it provides a nice mechanism to keep your UI in sync with your model. However:

Perhaps I could initialize the engine in the main viewModel, inheriting from AndroidViewModel so I got a pointer to Application and pass that application context to MWEngine

I wouldn't suggest doing that for two reasons, on one hand the documentation states "A ViewModel must never reference a view, Lifecycle, or any class that may hold a reference to the activity context.". The other reason is that your ViewModel should be restricted to the actions one can take in the UI of a particular view. If it so happens that those actions will interact with MWEngine, that's fine. But MWEngine will not be triggering any other view related data (or across Activities for that matter).

With the approach of having a class with static members to manage MWEngine, you have decoupled MWEngine from the Activity lifecycle and views. In each Activity that holds a ViewModel your setters and observers would use the static API of your MWEngine class to propagate changes initiated from the view. This does mean that the MWEngine class in itself is not observable and cannot update the view. Which is fine because it should not be its purpose. Think of your ViewModel as a mediator between what the UI is doing and what the engine should know.

This would imply that you would have some extra code for the "same values" (for instance you would be maintaining a "track list" inside the ViewModel that in turn updates the static MWEngine to maintain its list of instruments and events). The benefit though is that you can write this code unique per ViewModel/Activity. Be sure though that when an Activity is created its ViewModel has a method to synchronize itself with the contents of the static MWEngine class members.

I do realize that you probably have an MWEngine.IObserver registered in your main Activity. This observer is now registered upon engine creation, I believe you could benefit from being able to assign a new IObserver instance at runtime (if for instance your activities need to be aware of the sequencer position updating).

scar20 commented 3 years ago

Thanks for the elaborate answer and insight. Here the viewmodel I was refering to is the one used in the "Room with a view" https://developer.android.com/codelabs/android-room-with-a-view#9 example (which I used for the file dialog) in which it have a legit "Application" in its constructor. I need also the "shared viewmodel pattern" therefore this viewmodel is a good candidate since it is "pan-activity" even though you can have shared standard viewmodel. The way I saw it is since this viewmodel have an application, that application can be used in the engine init code in the same way it is used in the room example to init the repository class. From my file dialog part of the app:

public class SoundViewModel extends AndroidViewModel {

private final SoundRepository repository;
private final LiveData<List<SoundItem>> internalSoundsList;
private final LiveData<List<SoundItem>> soundByDirectory;
private final LiveData<List<SoundItem>> userSoundsList;
private final LiveData<List<SoundItem>> favoritesList;
private final MutableLiveData<Long> directoryId = new MutableLiveData<>();
private final LiveData<List<DirectoryItem>> directoryList;

public SoundViewModel(@NonNull Application application) {
    super(application);

    repository = new SoundRepository(application);
    etc.....

My idea was that since the engine and the repository are quite similar in nature - both need to be unique and always available, behind what is happening in the UI - I could use the engine the way repository is used:

private final MWEngine engine; then in the constructor engine = new MWEngine(application.getApplicationContext(), new StateObserverWichIDidntFiguredOutWhereItWillComeFrom())

Just to be clear I don't intend for the engine to refer to views or activity or be observable or update views, it just react to change in its provided value. I can see that I could make a static engine from a singleton pattern - MyEngine.getInstance() - but I need to have instance of tracks from the engine - even though they have the same processing chain, they are separated instance of track and they should be destroyed if there is no sample associated with them as it can happen in this app.

scar20 commented 3 years ago

I also try to see what you have changed in the native-thread branch. I cant see the difference in the java code. Is there a difference in the way it is initialized? In the mainActivity the code look the same in the init part or I missed something. I've seen also that there is already a static MWEngine.getInstance() method, but its not a singleton since in those the constructor is usually private. It is possible that what you suggest is 1- initalize engine in mainActivity - engine could be a subclass in with added method to createTrack() and getTrack(index). 2- In the viewmodel createViewModelTrack method, call (subclassed)MWEngine.getInstance().createTrack() in order to have an associated engineTrack with a viewmodelTrack. 3- set the values in the engineTrack when corresponding LiveData is set in the viewmodelTrack 4- don't bother with config change since they wont stop the engine anymore

I dont know what you mean exactly when you say I believe you could benefit from being able to assign a new IObserver instance at runtime You mean in the constructor? For the sequencer, I actually do not need it, outside the fact that I notice if I do not call _sequencerController = _engine.getSequencerController(), the example do not work even if I dont use that variable afterward - perhaps it could be put in the init method of the engine. Or if I would need it, it will be on a per track basis - here I will have to work out something. The Fonofone have completly independent track, each with a "metronome" object that trigger the sample from 5bpm to 1000bpm. So I only use the sequencer as the ringbuffer provider, not for the metric. OK, out of scope here, will get to that later on.

scar20 commented 3 years ago

For sequencer, I mean the example code wont work if that call to engine.getSequencerController() is not made. The example do work if you make that call, even if you dont use the variable so I guess that call must set something inside that is needed for the engine to run.

igorski commented 3 years ago

notice if I do not call _sequencerController = _engine.getSequencerController(), the example do not work even if I dont use that variable afterward - perhaps it could be put in the init method of the engine

That is odd as getSequencerController() does nothing else other than return a reference to the SequencerController instance that has been created using the engine.createOutput() method. After constructing the MWEngine instance, be sure to call the createOutput() method.

Just to clear up some misunderstandings, my proposal is:

and finally, the points you added:

As for your remark:

Don't bother with config change since they wont stop the engine anymore

The Activity lifecycle should no longer start/stop the engine. Instead if your application is suspended (because the user minimizes it to switch to a different app), you should hook into these focus change events to dispose() / instantiate the engine accordingly (see ExampleActivity, though I am sure you have seen the example and I must admit I'm not entirely aware how to manage this from a multi Activity application. There should be a hook provided by the Android SDK that is unique for the entire application being suspended.)

I would suggest not to enrich MWEngine.java with your custom code as it will make it harder in case you need to update from the MWEngine repository, especially as this file is the basic API exposed to the custom Java app using the engine (for instance you mentioned adding createTrack() in there, see my above suggestion regarding using a singleton manager instead). If you do feel like a method needs to be in there (and has use cases outside of your custom application), we can talk about adding it to the codebase, similar to the native code examples you provided in other issues (for which I'm thankful 👍 ). On a related note: I'm not sure what your update strategy is with regards to keeping in sync with changes made to the MWEngine repository, but the branch I mentioned previously has been merged into the master branch. If you were using AAudio the latest changes provide a massive stability improvement.

Another point of interest: I have updated MWEngine.java. The constructor no longer requires a reference to the ApplicationContext (it was unused after the threading refactor). Additionally, the getInstance() method is now private. If you need to access the active MWEngine instance, you can do so by exposing a method in your custom singleton manager class. The reason the getInstance() method exists is to guard against instantiating multiple engines which would provide a massive toll on the CPU with no practical benefit.

igorski commented 2 years ago

@scar20 can we consider this topic resolved ?

scar20 commented 2 years ago

Yes I got this. I still have others issues somewhat related so I'll try to make them more specific in scope in new threads. Closing this one, Thanks.