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

Support per-Period PlaybackParams #3419

Closed
brAzzi64 opened this issue Nov 3, 2017 · 6 comments
Closed

Support per-Period PlaybackParams #3419

brAzzi64 opened this issue Nov 3, 2017 · 6 comments
Assignees
Labels

Comments

@brAzzi64
Copy link

brAzzi64 commented Nov 3, 2017

Issue description

I'm working on an app that plays a list of MP3 sequentially, applying different pitch/shift params to each. I use DynamicConcatenatingMediaSource. Every time I get a onPositionDiscontinuity, I verify that the media has effectively changed, and call setPlaybackParams with the parameters corresponding to that song.

However, right now it takes a while (200-300ms) for the playback params change to hit ExoPlayer's thread (that will eventually .flush() the audio processors), and this is very noticeable to the user, causing a bad experience.

It sounds like in order to get this right, ExoPlayer would need to keep track of the params for each source in the concatenating-source, and apply them to each Window/Period a bit more gracefully.

Do you have any ideas on how to approach a fix for this on app code, or do you think it's something that could be improved on ExoPlayer's side?

Reproduction steps

  1. Seek to almost the end of a song
  2. Let it auto play to the next song, configured with a different pair of pitch/shift
  3. Listen to the audio of the 2nd song modified by the old params for a while, then switching to the new params

Link to test content

This can be reproduced with the media on the ExoPlayer sample app (playlist of Cats -> Dogs), using the mechanism described above.

Version of ExoPlayer being used

ExoPlayer r2.5.1

Device(s) and version(s) of Android being used

Reproduced on a Samsung S8 and Pixel C.

@ojw28
Copy link
Contributor

ojw28 commented Nov 3, 2017

I doubt it's the delay for the playback params to hit the playback thread. It's probably just that at the point where you set the playback params, we've already fed ~200-300ms of audio into the platform AudioTrack, at which point we don't have the ability to modify it. Our support for variable speed/pitch just isn't designed for what you're trying to do.

I think you're correct to say that the player would need to keep track of params for each source for your use case, one way or another, but this is not something we support currently. Marking as an enhancement, but realistically this will be considered low priority because your use case sounds quite niche.

@ojw28 ojw28 changed the title PlaybackParams take 200-300ms to apply when auto-playing next media using DynamicConcatenatingMediaSource Support per-Period PlaybackParams Nov 3, 2017
@brAzzi64
Copy link
Author

brAzzi64 commented Nov 3, 2017

Ok, that makes more sense :)

I definitely understand that this would be low priority for ExoPlayer, but was hoping you had a quick suggestion for an app-space fix.

Currently, I'm already keeping track of the params on a per-song basis; the only thing is new is auto-playing by using dynamic-concatenating-ms and the repeat modes. You can see here.

The 1st approach I'm thinking would be having a quick timer hitting repeatedly (similar to what I use for A-B looping), and when I detect we're "close" to the end of a song, switch the params. The question here would be; how do I estimate those 200-300ms? Is it related to the number of samples we had to specify for buffering on ExoPlayer 1, by any chance?

The 2nd approach would be to skip the auto-play mode altogether, and always use Player.REPEAT_MODE_ONE (path I rather not go down, since the library-side feature works great). Then change the params in-between songs and play whatever comes next if it corresponds.

Any tips would be really appreciated. Thanks!

@ojw28
Copy link
Contributor

ojw28 commented Nov 3, 2017

I can't think of an easy way to do this; I suspect any solution would be quite involved. @andrewlewis may have some ideas.

@andrewlewis
Copy link
Collaborator

At the app level, I wonder if it's possible to get this to work using a custom audio renderer that's based on MediaCodecAudioRenderer:

BaseRenderer.onStreamChanged will be called when the next song starts playing so you could override that and call audioSink.setPlaybackParameters to set the appropriate parameters. You would need a reliable way to find out what window is playing on the playback thread, and a way to map this onto the playback parameters you want to use. This should work perfectly in the sense that the audio samples from the old song will have the old parameters applied and be drained to the underlying AudioTrack, then the new parameters will apply.

One potential difficulty is that you're modifying the playback parameters directly in the renderer (not using the method on the player), so we'd need to make sure the new parameters are picked up by other components. I don't think this is too risky, though, because the player needs to be able to detect that the renderer has rejected a request to set playback parameters (e.g., when transitioning to a source that must be played via passthrough), which happens to make this use case work too.

@brAzzi64
Copy link
Author

brAzzi64 commented Jan 2, 2018

Hi Andrew, sorry I never got to reply, wanted to give an update just for reference.

In the end I had figured out a way to do it with the following (though pretty hacky) approach, which required a change in ExoPlayer: adding an extra AudioTrack::resetAudioProcessors() call in AudioTrack::reset().

  1. Wait for the onPositionDiscontinuity call (with state == READY and playWhenReady == true) and verify that we're effectively changing windows (songs).
  2. Pause playback immediately (setPlayWhenReady -> false).
  3. Set the new playback parameters with Player::setPlaybackParameters(), which will be passed to ExoPlayer's thread and will be set as "draining playback parameters".
  4. Do a Player::seekTo() to the beginning of the new window. This will trigger an onPositionReset() callback in MediaCodecAudioRenderer, which will call AudioTrack::reset(), which will in turn set the "draining playback parameters" as the "current playback params" right away and also do the AudioTrack::resetAudioParameters() call I added, which will effectively get the new parameters to the AudioProcessor.
  5. Resume playback.

This seems to work pretty well in general for me: the beginning of the new song that gets paused right away is rarely noticeable. This definitely "breaks" gapless playback though, but that wasn't critical for my use case. It's not pretty, but it's been working fine for a couple of months now.

Thanks!

Bruno

@andrewlewis
Copy link
Collaborator

Thanks for the update. That sounds reasonable, as long as songs have enough silence at the start so that no audio from the new song is output between the player starting to play the next song and handling onPositionDiscontinuity/pausing the player (if you're not doing gapless playback, this is perhaps fine to assume).

As a general rule, to get this kind of synchronization perfect it is necessary to run operations on the playback thread (like my suggestion above). I'll close this for now as it sounds like you have a working solution, and there's a suggestion for anyone else who might want to do the same thing with gapless playback. Thanks!

@google google locked and limited conversation to collaborators May 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants