PS, this bug is related to #10865 as the fact the renderer does not parrot the speed setting causes repeated MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL events.

If this is the same device/setup as #10865, doesn't this imply that no speed adjustment is possible (neither automatic for live adjustment nor manual)? If so, the speed is correctly reset to 1.0 presumably.

Hey @stevemayhew. We need more information to resolve this issue but there hasn't been an update in 14 weekdays. I'm marking the issue as stale and if there are no new updates in the next 7 days I will close it automatically.

If you have more information that will help us get to the bottom of this, just add a comment!

If this is the same device/setup as #10865, doesn't this imply that no speed adjustment is possible (neither automatic for live adjustment nor manual)? If so, the speed is correctly reset to 1.0 presumably.

@tonihei Sorry for my late reply to this. The issues are different, it is a bit confusing, I'll get to that.

1. Let AudioSink indicate if playback speed changes are supported to avoid unnecessary live speed adjustments #10865 - An enhancement, avoids switching speed between live adj attempt and "normal" speed. There will always be cases this is not possible (Pass-Thru AC-3 for example)
2. User setPlaybackParameters() is overwritten by live adjusment #10882 - this issue, user speed setting is overwritten, this is a bug

It just turns out that having an audio track that does not support speed adjustment is the easiest way to trigger this issue, that is the only real relation.

This issue occurs because two sequences of ExoPlayer internal messages on the playback thread can result in the player overwriting the user speed setting request. The pull request #10883 explains it a little better (I hope), the core sequence is:

1. User thread does a request to set speed to non 1.0x, message is queued to player internal
2. Live adjustment reads current speed as 1.0x and makes and adjustment to != 1.0x
3. Message executes to set speed from user request (MSG_SET_PLAYBACK_PARAMETERS)
4. Msg executes with speed back from renderer (MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL) overrides user speed setting

As I remember, when we discussed this, the design was only the renderer/audio track would retain this internal speed setting. I agree with you, this is the correct way to treat the internal playback speed.

Thanks for having a look, this is definitely a complicated flow.

PS, we think the LivePlaybackSpeedControl is a key feature for hospitality live playback use cases, happy to work on expanding it to more audio sink's

Thanks for the explanations! That was really helpful to understand the issue. One thing I wonder though - what happens to the speed set at step (3) from MSG_SET_PLAYBACK_PARAMETERS? Shouldn't this be processed after step (4) to create yet another MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL with the correct speed? This would of course only happen on a device that genuinely support a speed != 1.0.

what happens to the speed set at step (3) from MSG_SET_PLAYBACK_PARAMETERS?

Yes, the MSG_SET_PLAYBACK_PARAMETERS processing does exactly the right things:

1. Informs the current MediaClock with MediaClock.setPlaybackParameters()
2. correctly sets the user visible PlaybackParameter in PlaybackInfo (including the requested speed)
3. add a PlaybackInfoUpdate so the event is posted to PlayerListener (so the user thread view of PlaybackInfo is
   It is correctly matching the player thread

Shouldn't this be processed after step (4) to create yet another MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL with the correct speed? This would of course only happen on a device that genuinely support a speed != 1.0.

Yes, but in our use case, as we disable the audio renderer when setting the fast playback speed (for obvious reasons). So the syncClocks() call does not tigger the onPlaybackParametersChanged() message, I'd need to look back over my notes to make sure I have the sequence correct.

One thing, in your change #d640ceda the update is deferred by introducing the MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL whereas before it was in the same handler message (doSomeWork() likely) that saw the clock sync caused parameter change.

Stepping up a bit, there quite a few versions of PlaybackParameters that need to track each other in some fashion that is key to understanding the use cases correctly for each of these data sources:

1. StandaloneMediaClock.playbackParameters — always tracks change from Live Playback Speed Control and the MSG_SET_PLAYBACK_PARAMETERS
2. ExoPlayerImplInternal.playbackInfo.playbackParameters — should reflect only user settings? Or follow MediaClock and renderer?. It seems from the check before applying live adjustment (playbackInfo.playbackParameters.speed == 1f, more clearly playbackInfo.playbackParameters.speed == PlaybackParameters.DEFAULT.speed), it should only follow user settings
3. ExoPlayerImpl.playbackInfo.playbackParameters — clearly this should reflect the user setting, as it is already updated before the player thread copy is updated (so only one onPlaybackParametersChanged() event fires)
4. Renderer MediaClock — In the case where speed change is not supported this appears to state at 1.0 no matter what we set, obviously it is completely dependent on how the render implements MediaClock

As I understand the design only 1 and 4 should ever contain speeds set by the LiveSpeedControl, if so that was the essence of the change in the pull request.

I think your summary sounds correct and you are essentially saying everything works as intended (?), maybe except for the case where you disable the audio renderer (which I didn't fully understand yet).

As I understand the design only 1 and 4 should ever contain speeds set by the LiveSpeedControl, if so that was the essence of the change in the pull request.

The first part of this statement is definitely true, but the PR didn't work still because it prevented updating playbackInfo if the speed change is not set by LiveSpeedControl.

As I understand the design only 1 and 4 should ever contain speeds set by the LiveSpeedControl, if so that was the essence of the change in the pull request.

The first part of this statement is definitely true, but the PR didn't work still because it prevented updating playbackInfo if the speed change is not set by LiveSpeedControl.

I'm confused, all the test cases work and we are definitely setting the playback speed from the Player.setPlaybackParamerters(). with the pull request code and it works. Maybe I'm confused about the function of the internal messages. I saw these two:

1. MSG_SET_PLAYBACK_PARAMETERS — this is signaled (and only) when the API changes speed, it sets the speed in playbackInfo and fires the event to update the user mode version (in EPI).
2. MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL — This is signaled by the MediaClock when it finds it need to sync standalone and renderer clock source speeds if they are different.

The bug happens because of a switch from audio renderer (that has been speed adjusted by live adjust) to standalone clock. This causes the MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL which overwrites the user requested speed.

I'm not sure all the use cases for this second message, but it appears it was done so that speed changes for slow motion could be propagated to the video renderer and the track selector, but it also sets the user speed setting. It seems to me only the first message (MSG_SET_PLAYBACK_PARAMETERS) should change the user selected speed.

The case you are probably missing is a device or AudioProcessor setup that can't support a requested playback speed. In this case, there will

1. MSG_SET_PLAYBACK_PARAMETERS to set the user-requested speed (let's say 16.0)
2. A delay before the audio sink actually attempts to apply these parameters because it happens mid-playback and we need to wait for some pending buffers to flush out.
3. The audio sink realizing that it can only support up to 8x and changing the speed back to 8.0.
4. This triggers a MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL at the next clock sync.
5. When we handle this message, we really want to update the user-visible speed in PlaybackInfo to ensure the change is known to the user.

Step 3 depends on various factors, but in the end the AudioSink can freely change the requested speed. So ExoPlayer needs to propagate the speed change from MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL back to playbackInfo.

For the live speed adjustment case, we know that the user-requested speed must be 1.0 (otherwise the live adjustment isn't turned on). And the only realistic reason why the audio sink will change the requested minimal adjustment is to set it back to 1.0. As this is the same value as before, we assume that no user-visible update will happen via this path. This isn't fully guaranteed actually because the audio sink could decide to change our speed arbitarily, but this is just very unlikely in practice.

Sorry it took a while to respond @tonihei. Yes, the use case you described is broken by the pull request so will need to find another way to fix the issue. The fix works for us because we never set high speed playback with audio (audio over 16x is useful possibly for ADA support, but nothing we would use).

The issue still exists that the live playback speed control adjustment value is leaked into the user visible PlaybackInfo overriding the value set from the UI to a not quite 1.0 speed (like 1.03) that comes form the live speed control.

I'll spend some time today to try and capture the exact sequence of messages / live adjustments that cause this to happen then we can work out a better fix then the pull request.

The issue still exists that the live playback speed control adjustment value is leaked into the user visible PlaybackInfo overriding the value set from the UI to a not quite 1.0 speed (like 1.03) that comes form the live speed control.

Based on the discussion above, I think all cases should already work as expected:

Case 1: No speed adjustment is possible:

• All live speed adjustments and user-requested speed != 1.0 will result in a MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL with speed=1.0.
• The user-visible speed in PlaybackInfo will be reset to 1.0 and won't change again.

Case 2: All speed adjustments are possible:

• The player will stop sending new live speed changed to the media clock as soon as the user-requested speed is set (in MSG_SET_PLAYBACK_PARAMETERS).
• No MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL will ever happen because the speed changes are fully supported.

Case 3: Only the live-speed adjustment speeds are possible, the user-requested speed needs to be corrected:

• Similar to case 2, but with a single MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL for the updated user-requested speed.

Case 4: Only the user-requested speed is possible, but not the live-speed adjustment speed:

• This case will probably not get a MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL either (after MSG_SET_PLAYBACK_PARAMETERS) because the pending user-requested speed is supported and no change is observed after this point.

Ah, thanks @tonihei excellent enumeration of the 4 cases. So, I think what is happening is an unexpected consequence of case 1 or case 2.

I put some numbers on the sequence in the original bug description, you can see the logging associated with the in the description that shows the sequence of messages in the branch. Again, the Application Thread

1. disables audio renderer (MSG_TRACK_SELECTION_INVALIDATED)
2. set play when ready true (MSG_SET_PLAY_WHEN_READY)
3. sets speed != 1.0, e.g. 8.0 (MSG_SET_PLAYBACK_PARAMETERS)

You can see the sequences in 2b. and 3. in the updated main bug description setup the errant MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL. The fact that the change from this callback is queued to the player handler message queue sets up the possibility of state changes that invalidate the message.

I'm very clear why my pull request is invalid, Just to riff on some ideas for MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL , this is obviously a very complicated sequence:

1. handling of the MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL in step 5 when the renderer clock is no longer active seems wrong.
2. Why not handle the PlaybackParametersListener.onPlaybackParametersChanged() synchronously (that eliminates all possible sequencing errors)
3. Avoid attempting to live adjust speed when MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL is pending (seems a bit like a hack, adding more conditions to the already complex check:

   // Adjust live playback speed to new position.
    if (playbackInfo.playWhenReady
        && playbackInfo.playbackState == Player.STATE_READY
        && shouldUseLivePlaybackSpeedControl(playbackInfo.timeline, playbackInfo.periodId)
        && playbackInfo.playbackParameters.speed == 1f) 

Again, thanks for your patience on this use case.

It is on my plate to proposing a proper API for ROLE_TRICK_PLAY track usage (I'm thinking of a PlaybackParameters.setTrickplayMode(boolean)). Hopefully that can bring these use cases more front and center.

Ah! Thanks for the updated logs. I can see the problem now, which only happens if we switch from renderer to stand-alone clock while MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL is still pending. The pending change back to 1.0 is stale at this point because it refers to the renderer clock, whereas the stand-alone clock is of course perfectly happy to play any speed. Let me think about this a bit to see what we can do.

this is obviously a very complicated sequence

I agree. The main complication is that setting a speed on the renderer clock is not checked immediately for support, but only slightly later when it's actually applied. That's why we blindly assume it works and then use the additional MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL callback to signal when something changed.

1. handling of the MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL in step 5 when the renderer clock is no longer active seems wrong.

Yes, see beginning of my reply where I came to the same conclusion. This event must not be handled, not triggered, or handled before the switch to the stand-alone clock.

2. Why not handle the PlaybackParametersListener.onPlaybackParametersChanged() synchronously (that eliminates all possible sequencing errors)

Might be a possibility, but needs to be checked carefully. We always send incoming tasks as a new message (even on the same thread) to ensure the order of event execution is always the same as the order in which the events are triggered. This is more of a general rule that avoids that we even have to think about re-entrance scenarios.

Avoid attempting to live adjust speed when MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL is pending (seems a bit like a hack, adding more conditions to the already complex check:

Sounds wrong, yes. It would make more sense to implement go/exoghi/10865 to avoid setting the speed at all if it's not supported.

It is on my plate to proposing a proper API for ROLE_TRICK_PLAY track usage (I'm thinking of a PlaybackParameters.setTrickplayMode(boolean))

Not really part of this bug, but I'm curious why you'd want to put it in there? The PlaybackParameters are really more of AudioOutputConfigurationParameters at the moment and trick-play doesn't seem to fit logically. If it's just about having an API to configure the player to change it's internal behavior for trick-play, then we probably need a separate ExoPlayer for this.

I'll will upload a fix for this (with a test case). The solution I went for is to remove pending internal speed updates when a new speed is set. This makes sense because any newly set speed makes the previous pending message stale.