androidx / media

Jetpack Media3 support libraries for media use cases, including ExoPlayer, an extensible media player for Android
https://developer.android.com/media/media3
Apache License 2.0
1.65k stars 390 forks source link

Add AudioTrackProvider to allow access to AudioTrack instance #1737

Open Tolriq opened 1 month ago

Tolriq commented 1 month ago

Opening this as a question, but maybe it's a bug.

To implements fades properly with VolumeShaper there's a need to access the AudioTrack. While it's not an issue to expose it in a fork and use the events onAudioTrackInitialized (Well if we ignore the https://github.com/androidx/media/issues/1728 issue), the order of the events between audio track initialization and the playing state are not consistent.

The onIsPlayingChanged or onPlaybackStateChanged can be called before the AudioTrack is actually initialized (So obviously it's not actually already playing).

This makes things a little more complicated to manage all the cases, specially when audio track can be reused or recreated during media change and with the delayed release there's even cases when potentially the previous audiotrack is not released and the app could try to apply the volumeshaper to the wrong one.

Is there something I'm missing or a better event / way to handle those cases ?

Tolriq commented 4 weeks ago

Also pinging @tonihei since he did the rework on the audiotrack async init/release and the recent changes to volume ramp kinda match the needs here.

tonihei commented 4 weeks ago

The onIsPlayingChanged or onPlaybackStateChanged can be called before the AudioTrack is actually initialized (So obviously it's not actually already playing).

Just for reference, we also have an internal bug for this: b/165787154. We haven't changed any logic for this though as it seems likely a relatively risky change, because parts of our code or code in apps using ExoPlayer could accidentally rely on this behavior, and we haven't seen a major problem with the current behavior (besides the potential misleading listener order).

For figuring out when audio started playing exactly, we have onAudioPositionAdvancing. This method will only be called after playback actually started though once we know the exact time audio started advancing. I assume this is not what you are looking for here?

order of the events between audio track initialization and the playing state are not consistent

This part is actually expected. The audio track initialization is a fairly low-level analytics callback that is sent directly from the component where it happened. The playing state is part of the core Player.Listener class and has more guarantees about ordering and consistency with the rest of the player state.

Is there something I'm missing or a better event / way to handle those cases ?

I haven't actively used VolumeShaper before. What kind of events would be needed to make the integration work nicely from your perspective?

Tolriq commented 4 weeks ago

@tonihei VolumeShaper by itself is pretty easy to use, the only actual issue is getting access to the proper AudioTrack to be sure to apply the correct effect to the correct place. Because we create the shaper from the AudioTrack via audioTrack.createVolumeShaper. So we can actually ignore that and just think about getting access to the proper AudioTrack at the proper moment (and know when it changes).

The first necessary step would be to have an access to the AudioTrack to create the shaper (Easy in a fork).

Then depending on the needs (fade in, fade out, fade between) we need to have a signal to properly apply the shaper (or create and apply) on the proper AudioTrack.

From an ForwardingAudioSink I can overwrite play and pause to handle the fade in, fade out. But there's already the case where the AudioTrack is not always present when play is called so we need to then delay the init when receiving onAudioTrackInitialized (With the https://github.com/androidx/media/issues/1728 issue that makes it a little harder than necessary).

While this is not really hard this works.

Now comes the fade between and the difficulty.

For the fade out, we send a message with the timestamp, then apply the shaper in theory there's always the proper AudioTrack, and if not then nothing is probably playing or a more important issue.

Then fade in and the difficulty: The AudioTrack can be reused or another one created and the previous one destroyed.

Issue is that we can't know before hand if it will be reused or not, and when there's another one created, the previous one is destroyed asynchronously.

So since the events may not arrive in the correct order, we may think we already are playing on the new AudioTrack but we are not yet and since the previous one is not yet released we can think we access the proper one and can't rely on the fact that it would be null for example.

So TL;DR, the need is to reliably know when we can access the proper AudioTrack in play and during track change with or without new AudioTrack creation.

Tolriq commented 4 weeks ago

@tonihei

So looking a little more in DefaultAudioSink and all the different cases, I think the need can be quite simply filled by adding an onAudioTrackConfigured event to AudioSink.Listener.

Then in handleBuffer add a var audioTrackWasConfigured. Set it to true if pendingConfiguration!=null and after the try catch for the !isAudioTrackInitialized().

Then call the event if audioTrackWasConfigured is true.

This ensure that we are always called with a valid audioTrack ready, before any played data for that audioTrack and handle reusing, first audioTrack, and new audiotrack on media change.

Edit: So no it does not work for audioTrack reuse the event is sent too early and the track is still playing the previous item.

tonihei commented 3 weeks ago

So looking a little more in DefaultAudioSink and all the different cases, I think the need can be quite simply filled by adding an onAudioTrackConfigured event to AudioSink.Listener.

We could add that is it's useful, but given your "edit" below I'm not sure if it covers what you need still? Please confirm if it's useful to solve this issue and we can add the callback.

Edit: So no it does not work for audioTrack reuse the event is sent too early and the track is still playing the previous item.

When we reuse the AudioTrack, it will be one continuous playback with the actual transition happening seamlessly some time after sending down the audio samples into the pipeline. The only way to know when the transition point is reached is by listening to the playback position I think. If your goal is to apply something at the transition point, I wonder if it would be better to use the audio processors in ExoPlayer rather than VolumeShaper?

the only actual issue is getting access to the proper AudioTrack

We have a pending internal change for an AudioTrackProvider that lets you access the actual AudioTrack instance if needed at the time of creation (and make additional configurations that wouldn't be possible without this access). Not sure how quickly we get around to submit this, but I assume this removes the need to fork this class? @tianyif (FYI)

Tolriq commented 3 weeks ago

The problem of audio processors is that they work in the past most of the time. So you can't apply a fade out on pressing the pause button since the processor have already processed the data and we can't properly rollback in time or I'm missing something obvious that does not implies delaying the audio output to the duration of the fade out?

Every solution I've searched ended up to VolumeShaper for the fade out on pause or seek. So dealing with half the solution via VolumeShapers and the rest via AudioProcessor would be a lot more work, specially if we want to achieve the same volume curves. (If we could do cross fade via AudioProcessor then it would be different and worth it).

We could add that is it's useful, but given your "edit" below I'm not sure if it covers what you need still? Please confirm if it's useful to solve this issue and we can add the callback.

I'm not sure about it, one benefit I see is that this consolidate the events in the AudioSink.Listener and so remove the need to override the AudioSink configure. But currently we'd still need to override the pause to actually block the sink from playing again before the effect is finished. (Again unless I'm missing something to achieve that differently, needed for fade out/in during seek for example)

But I assume this removes the need to fork this class

In theory, if I figure out all the quirks of the event ordering then yes, currently due to the audioTrack delayed release, I have rare cases where the shaper still exist on the previous AudioTrack for a couple ms and when applying fails and so I catch a recreate a shaper at that moment. I guess with a simple missing synchronisation somewhere I can ensure that I have a var with the up to date AudioTrack at that moment and so there should not be an issue

tonihei commented 3 weeks ago

fade out on pause or seek

Not sure what the exact goal is and if you are aware already: the audio framework also applies some automatic fade out when pausing and resuming audio tracks. And we recently made sure the same logic is definitely used for seeks as well.

I'm not sure about it

Sounds like we leave everything as it is for now then if the same logic can already be achieved by overriding ForwardingAudioSink.configure? Please let me know if a new callback definitely makes a difference and we can add it still.

Tolriq commented 3 weeks ago

Not sure what the exact goal is and if you are aware already: the audio framework also applies some automatic fade out when pausing and resuming audio tracks. And we recently made sure the same logic is definitely used for seeks as well.

Users want something way longer than 20 ms :( Also when seeking and skipping next, they want longer fade out and longer fade in to avoid huge audio differences.

Those fade in/out (And cross fade but that's another story) are by far the most requested feature for my app. And I see frequent references here too.

Sounds like we leave everything as it is for now then if the same logic can already be achieved by overriding ForwardingAudioSink.configure? Please let me know if a new callback definitely makes a difference and we can add it still.

No need for now as there's still a need for a ForwardingAudioSink for the pause anyway, so I'll keep overriding the configure too to know about track reuse.


Can we keep this issue as a reference for AudioTrackProvider implementation?

And thanks for the answers, I'm back to try to mix all the events to cover all cases.

Tolriq commented 3 weeks ago

@tonihei sorry to bother but one last question about audiotrack reuse and having a event on media change to react.

From test I see that onPositionDiscontinuity and onMediaItemTransition can be delayed a lot (I have cases where reading exoplayer currentposition in the callbacks is already 350ms in :()

Is there a better event from ExoPlayer or the sink or somewhere else to have something called more closely to the actual switch ?

tonihei commented 3 weeks ago

From test I see that onPositionDiscontinuity and onMediaItemTransition can be delayed a lot (I have cases where reading exoplayer currentposition in the callbacks is already 350ms in :()

If you need to know about these events on the app's main thread, there is probably not much we can do as the delay is caused by other tasks running on the main thread already. ExoPlayer triggers these events on the internal playback thread in the same moment the position moves past the transition point (i.e. ExoPlayer.getCurrentPosition <= 10 at that exact moment). The rest is a simple Android Handler message sent to the main thread that informs all listeners inline. If you need to react at the exact same moment, you probably need to use the ForwardingAudioSink you already have and do something based on the getCurrentPositionUs method call.

Tolriq commented 3 weeks ago

I have a dedicated thread for ExoPlayer and not using main but still explains the delay.

Thanks I'll try to understand better how that function works. During my tests the value just kept increasing even on media change.