Audio playback refactor

[seadowg]

Closes #2521

There's already been some good work and discussion around how to fix some of the bugs and quirks around audio playback in the app (#2522, #2863 and #2817 for example). For people not familiar with the bugs: there are multiple problems with play/stop buttons interacting with one another. After having a look at the problem, it made sense to give it another stab now that we have some new tooling maturing in the Android Jetpack world.

After digging into the current architecture it made sense to me that a good way forward would be to separate out audio playback functionality entirely from the question world so that it could be tested and implemented in isolation. To help myself out I ended up drawing a picture of this in Miro:

This draft starts with an initial pass at that that uses an AudioPlayerViewModel to coordinate the MediaPlayer and expose state and actions to an AudioButton view that is only responsible for rendering whether it is in the "playing" state or not and for emitting (via a listener) "playClicked" and "stopClicked" events.

I worked through this with @lognaturel and one thing we noticed was how many different behaviors of the app that audio playback is part of. Although this isn't finished yet it made sense to get a draft PR up early to get feedback and also have people point out features we might be missing. I'm going to enumerate them below (and tick off what is working in this reimplementation) - shout out if anything is missing:

• Question audio can be played/stopped
• Select option audio can be played/stopped
• Playing one audio button on a screen stops any other playing one
• Question option text is highlight when audio is being played
• Select option text is highlighted when audio is being played
• MediaPlayer objects are released when Activity is destroyed
• MediaPlayer objects are released when Activity is paused
• Moving between form screens stops playback
• Audio can be autoplayed for questions that are being forward swiped into
• Playing an audio button should stop an AudioWidget (and vice versa)
• Audio in an audio widget can be played/paused
• Audio in an audio widget can be seeked with buttons
• Audio in an audio widget can be seeked with seek bar
• Pausing/resuming/rotating Activity doesn't cause any problems (currently playing audio etc)

Just to note: this PR does not fix the problems outlined in #2861 - autoplay for select options is not functioning correctly. We'll follow up with a fix for that once this is in.

What has been done to verify that this works as intended?

Used several different forms involving AudioWidgets, audio in question media and in select options etc etc.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

There might be some accidental changes to behavior given how little of this functionality was tested or documented. It would be to double check that media in forms generally still works as expected (I've listed the behaviours I could find above) and that we don't see any (negative) performance changes compared to the current version of the app. This is a big set of changes so I wouldn't be surprised if find problems - I'd imagine with something I didn't realize the app could do.

Do we need any specific form for testing your changes? If so, please attach one.

Good to use as many media forms as possible but a good one to start with is the "MediaForm" linked in #2521.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

I think it would be good to add documentation for the audio functionality. I'll make sure to file an an issue once we're happy with this!

Before submitting this PR, please make sure you have:

• run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
• verified that any code or assets from external sources are properly credited in comments and/or in the about file.
• verified that any new UI elements use theme colors. UI Components Style guidelines

[seadowg]

Interesting note from something I've realised: the way that swiping through pages/groups works in Collect means that it's hard for us to use the FormEntryActivity as our "lifecycle" for architecture components. The lifecycle of most views is really dictated by their parent ODKView as they are created and destroyed as we move through each group of questions. This suggests that we should really be using a Fragement to own each group (which would also let us use components like the ViewPager. That's certainly a change for another time though so for now we may just have to give the ODKView its own Lifecycle object - this will be a little hacky but doable I think.

EDIT: A way nicer way to handle this is to just have the FormEntryActivity notify the AudioPlayerViewModel to stop the MediaPlayer we can use the fact that our ViewModel is cached at the Activity level to make this happen easily.

[seadowg]

Our "nicer way" in my last comment only solves part of the problem as we still need a valid lifecycle for LiveData observations. I'm going to spike/investigate 3 paths forward and report back with interestings when I'm done:

• Give the ODKView it's own "lifecycle" using the lifecycle tools Jetpack gives us.
• Remove lifecycle from observations and manage deregistering ourselves
• Make the ODKView a Fragment so it has lifecycle we can use

[seadowg]

Ok here's my findings from the spikes:

1. Give the ODKView it's own "lifecycle" using the Lifecycle tools Jetpack gives us.

In the FormEntryActivity I ended up using an ObserverToken (an object I made that implements LifecycleOwner). Here's the definition of ObserverToken and how it's used in the FormEntryActivity:

// ObserverToken

public class ObserverToken implements LifecycleOwner {

    private LifecycleRegistry lifecycleRegistry = new LifecycleRegistry(this);

    public ObserverToken() {
        lifecycleRegistry.handleLifecycleEvent(ON_RESUME);
    }

    public void release() {
        lifecycleRegistry.handleLifecycleEvent(ON_DESTROY);
    }

    @NonNull
    @Override
    public Lifecycle getLifecycle() {
        return lifecycleRegistry;
    }
}


// FormEntryActivity

private ObserverToken odkViewObserverToken = new ObserverToken();

public ObserverToken getODKViewObserverToken() {
    return odkViewObserverToken;
}

private void releaseOdkView() {
    odkViewObserverToken.release();
    odkViewObserverToken = new ObserverToken();
    
    if (odkView != null) {
        odkView.releaseWidgetResources();
        odkView = null;
    }
}

You can see here that we end up with the ObserverToken standing in as "lifecycle" for the ODKView. Then we can use it in views like the MediaLayout:

LifecycleOwner lifecycleOwner = activity.getODKViewObserverToken();
liveData.observe(lifeCycleOwner, () -> ...);

2. Remove lifecycle from observations and manage deregistering ourselves.

Here I ended up with a LiveDataObserver object that we can instantiate for classes that observer LiveData but have an "out of sync" lifecycle (like the views under ODKView). Here's the definition of LiveDataObserver and how it's used:

// LiveDataObserver

public class LiveDataObserver {

    private final List<Pair<LiveData, Observer>> observations = new ArrayList<>();

    public <T> void observe(LiveData<T> liveData, Observer<T> observer) {
        liveData.observeForever(observer);
        observations.add(new Pair<>(liveData, observer));
    }

    public void release() {
        for (Pair<LiveData, Observer> observation : observations) {
            LiveData liveData = observation.first;
            Observer observer = observation.second;

            liveData.removeObserver(observer);
        }

        observations.clear();
    }
}

/// MediaLayout (and anywhere we observer LiveData)

private LiveDataObserver liveDataObserver = new LiveDataObserver();

liveDataObserver.observe(liveData, () -> ...);

public void release() {
    liveDataObserver.release();
}

Here you can see that we need to be able to call a method on any view that uses LiveData
to release the observers. This is doable though as we can start at ODKView.releaseWidgetResources() and work our way down the view tree from there.

3. Make the ODKView a Fragment so it has lifecycle we can use.

It may not surprising to hear this but: this seems like it's not going to be easy. Most likely we'd need to go for a full ViewPager/Fragment switch over. This would involve moving widget rendering code into a Fragment. I spent a bit of time on playing around with this but pulled out when it looked like it was going to end up being the rest of my week.

Recommendation

Option 2 leaves us with the ability to use LiveData later but removes a lot of the benefits of it (the lifecycle aware parts) while Option 1 lets us use LiveData properly while introducing some weirdness to the FormEntryActivity. To me it feels like Option 2 is essentially us adapting new code for old problems while Option 1 is the reverse of that. In my opinion, it makes sense to go with Option 1 as long as we're looking to move the FormEntryActivity to a ViewPager setup (it seems like there may be some UX advantages to this as well as just simpler code). This would mean that our slightly weird ObserverToken solution would just be temporary and would mean that the new Audio feature code wouldn't have to change when the Fragment is introduced.

We could of course drop LiveData entirely but, to me, it does feel like this part of the app heavily benefits from a reactive framework of some kind. Rx is of course the standard alternative but then we're back in the world of lifecycle management anyway.

It'd be great to get your thoughts on this @lognaturel and @grzesiek2010!

[lognaturel]

Thanks for the clear presentation of your spike results, @seadowg. I'm convinced that if we're going to go in on LiveData, the general approach of representing the creation and destruction of the questions views as lifecycles seems preferable to using observeForever. Also agree that also doing the refactor to Fragments here will make this too big and that it would be ideal to do it in stages.

Forgive me if this should be obvious, but I'm not clear on why you need to add the ObserverToken concept. Couldn't ODKView implement LifecycleOwner, handle ON_RESUME in its constructor and handle ON_DESTROY in releaseWidgetResources (which should then be renamed)? What am I missing?

[seadowg]

@lognaturel Yeah that would totally work. My thinking when I was spiking things out was just keeping things as contained as possible so it would be easier to present them back - that's how I ended up with the token rather than it in the view code. Moving forward though it might be a nice way to actually implement it as it keeps all the Jetpack lifecycle code out of the view itself which makes it feel a little more "bolt on" (and hopefully "bolt off"). What do you think?

[lognaturel]

keeping things as contained as possible

Ah yes, that did make for a really nice summary. 👍 I'm generally very much for compartmentalization but in this case, I wonder whether having the ODKView have the lifecycle field would make things easier to understand and closer to the end goal of having a Fragment be the container instead.

I don't feel super strongly about it but I am liking what I see in 6f1044a. It feels a little funky to have something that's a LifecycleOwner be called odkViewLifecycle but I think it makes enough sense.

[seadowg]

@lognaturel yeah I think I agree. On second thoughts I think this might also fit in to the FormEntryActivity in a neater way as well as we won't have a second field to manage. I'll have a go at making the ODKView implement LifecycleOwner and see how it looks!

[seadowg]

@lognaturel ach I'd forgotten (weekend brain 🤷‍♂️) that it's awkward to have the lifecycle on the ODKView itself as the widgets are created as part of the ODKView construction. This means the ODKView itself hasn't been assigned to the FormEntryActivity field. We could rework this all but I suggest we leave it for the moment as that would be better incorporated as a step on the road to "Fragmentizing" the ODKView.

[lognaturel]

@seadowg and I have talked through this a couple of times and I'm on board with the current direction. The big thing I'd like to see in a cleanup pass is more narration (is anyone surprised). For example, capturing some of the back and forth we had above about where the lifecycle used for the ODKView should go would be valuable. I think ViewHelper and ScreenContext are particular candidates for some high-level description of why things are as they are now and where they might go in an ideal future.

I'm going to be out for the next two weeks. @grzesiek2010, please take over review! Don't hesitate to get on a call to discuss. I think it'd be good for this to go into QA as soon as possible because the big risk is that there's functionality missing. @seadowg says he'll likely have something ready for that mid-week. 🙏

[seadowg]

@mmarciniak90 sorry I forgot to readd the "needs-testing" label to this. Will do.

[mmarciniak90]

@seadowg, the first thing that I noticed is that audio is not always stopped when I swipe to the next page. It works for Birds form but not for Media Form.

[seadowg]

@mmarciniak90 ah I think I see what might be wrong. Is the proble happening on the last screen of the form every time?

[seadowg]

@mmarciniak90 I've pushed a fix that should hopefully resolve that problem

[mmarciniak90]

@seadowg sorry that I'm adding issues partially. I'm still working on testing this PR.

• Crash occurs when audio file is missing and the user clicks on Play button. Problem is not visible when a video file is missed. Toast with message File ... is missing should be visible.

org.odk.collect.android E/AndroidRuntime: FATAL EXCEPTION: main Process: org.odk.collect.android, PID: 23123 java.lang.RuntimeException at org.odk.collect.android.audio.AudioPlayerViewModel.loadClip(AudioPlayerViewModel.java:176) at org.odk.collect.android.audio.AudioPlayerViewModel.play(AudioPlayerViewModel.java:37) at org.odk.collect.android.audio.AudioHelper$AudioButtonListener.onPlayClicked(AudioHelper.java:155) at org.odk.collect.android.audio.AudioButton.onClick(AudioButton.java:82) at android.view.View.performClick(View.java:6213) at android.view.View$PerformClick.run(View.java:23645) at android.os.Handler.handleCallback(Handler.java:751) at android.os.Handler.dispatchMessage(Handler.java:95) at android.os.Looper.loop(Looper.java:154) at android.app.ActivityThread.main(ActivityThread.java:6692) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1468) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1358)

• test_audio form: autoplay works when the form is loaded and when the user jumps to a question. It does not work when the user swipes back to question with autoplay audio

I also noticed behavior which could be improved but they exist on v1.23.3 and they are related only to Android 8.1 and 9.0

• rotating the device does not stop playing an audio file
• test_audio form: video file is played, again and again, it does not return to form

[seadowg]

@mmarciniak90 no that's great. Good to get these things early! I'll have a look at the crash.

test_audio form: autoplay works when the form is loaded and when the user jumps to a question. It does not work when the user swipes back to question with autoplay audio

That was the old behaviour (I thought anyway) so I left it intact. The behaviour makes sense to me but maybe we play around with it in future to see if it's what people want?

rotating the device does not stop playing an audio file

Ah yeah that will work now. Do we think it should stop on rotate?

test_audio form: video file is played, again and again, it does not return to form

I saw this as well. It seemed on my devices (and emulator) Android was set to "loop video" (it's in the options menu of the video) by default.

[mmarciniak90]

@seadowg

1. The behaviour makes sense to me but maybe we play around with it in future to see if it's what people want?

Sounds good

2. I see some inconsistency here because on Android 8.1 and 9 audio is played even if device orientation is changed but on all lower Android versions, audio is stopped.

3. You have right! There was Loop video option selected. But still, it does not return to form, to the question. Lower Android versions are moving the user to form when the video ends.

As I said, the last two points are visible in v1.23.3.

[seadowg]

I see some inconsistency here because on Android 8.1 and 9 audio is played even if device orientation is changed but on all lower Android versions, audio is stopped.

You have right! There was Loop video option selected. But still, it does not return to form, to the question. Lower Android versions are moving the user to form when the video ends.

Both of these feel like they're ok but I'll take a look into why they happen to make sure neither are suggesting some underlying problem.

[seadowg]

@lognaturel would be interesting to get your opinion of the style of error handling (using LiveData) in 418e43f

[lognaturel]

These kinds of errors could happen multiple times while a specific question/field list is being displayed so modeling them as events that are streamed seems fine to me. Any alternative I can think of would require the view model or audio helper to have some knowledge of where errors should be displayed. I can't think of an alternative using LiveData. What kind of objections were you thinking I might have? Did you consider alternatives?

Where I haven't been able to find an analogous structure I liked is when the info that needs to be displayed is only displayed exactly once. For example, that's the case with the background location tracking warning toast and that's why I ultimately abandoned using a LiveData-based solution there.

[seadowg]

@lognaturel the only alternative I considered was having play throw an Exception. The disadvantage was having to show toasts from multiple places - wherever play is called. I like that this solution let us set up our error handling in one place and then let everything else forget about it.

when the info that needs to be displayed is only displayed exactly once

Ah that's interesting. In my head you'd do this by only "emitting" the error once from a ViewModel as it feels like the fact that the error has happened before becomes applicaiton state rather than view state.

[seadowg]

@mmarciniak90 I spent some time look into the orientation changes causing the audio to stop. I found the same thing that on API 26+ (8.0) the audio doesn't stop when the phone is rotated but on older devices it does.

TLDR: this is because on devices before API 26 the app does the same thing when it rotates as when we change screens or switch apps and we stop audio at those points.

Full(er) explanation: This seems to be because of a behaviour change in Android but is also to do with some potential weirdness on our side. It looks like we're using a flag in our AndroidManifest.xml on a lot of our Activities that stops them from being recreated when we go through an orientation change (configChanges=orientation). On devices before API 26 however it looks like the FormEntryActivity still goes through onPause, onDestroy etc. Because our new audio implementation stops playback and releases the MediaPlayer when it's host Activity is paused the playback stops for these API levels. API 26+ devices behave as you'd expect: the Activity doesn't pause on rotate and the playback doesn't stop. If I remove the configChanges flag from the FormEntryActivity then the behaviour is consistent across API levels (the playback always stops).

Frankly this was really confusing and it took me a while to realize those flags were on the manifest. I couldn't find anything on the behaviour of configChanges being altered at API 26 but it does seem that Google aren't encouraging using the flag. It looks like @shobhitagarwal1612 posted this as an issue back in 2017 with #799. I think we should leave this as is for this PR but I would be tempted to investigate removing the configChanges flag in future to avoid these kind of inconsistencies.

[mmarciniak90]

@seadowg Thanks for the so precise analyze. This knowledge will be helpful for testing other PRs too.

Tested with success

Verified on Android: 4.2, 4.4, 5.1, 6.0, 7.0, 8.1, 9.0

Verified cases:

• light/dark mode
• delete media - File ... is missing is visible.
• a few questions on one page - play/pause media from different questions
• highlighting question or answer when media is played
• minimize - media is stopped
• swipe to last view - media is stopped
• rotating when media is playing; Android 8.1 and 9.0 - media is continued; lower versions - audio is stopped

@opendatakit-bot unlabel "needs testing"
@opendatakit-bot label "behavior verified"

[seadowg]

@lognaturel @grzesiek2010 feel happy with this merging in?

[lognaturel]

Thrilled! 🎉