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

Let AudioSink indicate if playback speed changes are supported to avoid unnecessary live speed adjustments #10865

Open
1 task
psharma676 opened this issue Dec 13, 2022 · 7 comments
Assignees

Comments

@psharma676
Copy link

ExoPlayer Version

2.15.0

Devices that reproduce the issue

Tivo Streaming device running Android 10
Nvidia Sheild running Android 11

Devices that do not reproduce the issue

No response

Reproducible in the demo app?

Yes

Reproduction steps

  1. Apply attached patch "patch-r2.15.0.txt" to branch at tag r2.15.0. This will generate logs referenced in "Actual result" section below, or you can use your own logging.
  2. Play a working (AAC audio) stream and compare with Non-Working (AC3 stream).

Expected result

Playback speed for AC3 streams should adjust to push liveOffsetUs up to idealTargetLiveOffsetUs.

Actual result

Note in the logs:
For AAC: liveOffsetUs → currentTargetLiveOffsetUs and adjustedPlaybackSpeed → 1.0 eventually
For AC3: liveOffsetUs does not move towards currentTargetLiveOffsetUs and adjustedPlaybackSpeed stays at 1.3

In both cases above currentTargetLiveOffsetUs →idealTargetLiveOffsetUs, which should be expected behavior. So AAC will eventually end up at the playback position of idealTargetLiveOffsetUs, but not AC3.

The same behavior is seen in r.2.18.X as well as androidx/media (some adjustments are required to trigger/force Playback Speed Control because of commit, #9329)

Media

patch-r2.15.0.txt
Media streaming information will be sent separately via email.

Bug Report

@tonihei
Copy link
Collaborator

tonihei commented Dec 14, 2022

liveOffsetUs does not move towards currentTargetLiveOffsetUs and adjustedPlaybackSpeed stays at 1.3

This sounds odd and logically impossible :) Could you also share the logs you obtained with the attached patch? I'm curious to see how the values change (or not change) and I don't have an NVidea Shield available right now.

@tonihei tonihei self-assigned this Dec 14, 2022
@psharma676
Copy link
Author

psharma676 commented Dec 14, 2022

Issue-10865-Dec-14-22-log.zip

  1. I have used a slightly different patch than previously uploaded, which I think would be more useful. I have included it in Issue-10865-Dec-14-22-log.zip along with logs for an AAC run as well as an AC3 run (the files are named appropriately).
  2. The logs are filtered, please let me know if you'd like completely unfiltered logs.
  3. From the logs at least one conclusion can be made that mediaClock.setPlaybackParameters(playbackInfo.playbackParameters.withSpeed(adjustedSpeed)) is likely not working as expected for AC3, or is being overwritten.

@stevemayhew
Copy link
Contributor

BTW @tonihei I noticed the Live Offset feature is turned off by default with a comment it is not so useful for non-LL streaming. There is one place it is very useful, the hospitality use case (many TV's in a bar, health club, etc, all watching the same event)

@psharma676
Copy link
Author

Issue-10865-Dec-16-22-log.zip
I have another data point that I feel could be useful. Current logs were generated by the changes identical to the ones in the previous comment PLUS a single line change to DefaultAudioSink.java. It is useful to compare these logs with the ones posted in the previous comment by me.

With this change you will notice:

  1. mediaClock.setPlaybackParameters(speed) is being preserved between calls
  2. liveOffsetUs starts to track towards idealTargetLiveOffsetUs.

Even though the liveOffsetUs starts to track there are other side effects like terrible lip sync that shows up after a few minutes. The change to DefaultAudioSink.java is just to help understand the behavior of this issue - its not a proposal of a real change :-) . I'm curious to hear back what you think!

@tonihei
Copy link
Collaborator

tonihei commented Dec 21, 2022

Thanks for the all the logs and description. It looks like DefaultAudioSink just doesn't support changing the playback speed for this combination of device and format (maybe it's using tunneling or some other passthrough mode?). This is basically what you've overwritten in your second patch.

Then I think this is really just working as intended because the speed can't be adjusted in the output path. We don't know that in advance at the moment because the clock (=DefaultAudioSink by default) can be injected by apps and there is a delay before we even find out. Is this behavior causing any problems during playback besides not having live offset adjustment? I think at the time of writing the code we considered this issue and said that it will needlessly try to set the adjusted speed, but since it's a no-op, it also doesn't really matter.

We could improve the code and resolve the confusion you had by adding a method to MediaClock that lets callers check if playback speed adjustments are supported in the current configuration. This then would allow the player to NOT call the live playback speed adjustment if the speed can't be adjusted anyway.

stevemayhew added a commit to TiVo/ExoPlayer that referenced this issue Dec 22, 2022
There are multiple sequences of handler messages which can cause a user
request to update playback speed to be overwritten by the `LivePlaybackSpeedControl` value.

All the sequeunces share a common feature that:

1. a queued `MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL` executes after the `MSG_SET_PLAYBACK_PARAMETERS`
2. The `PlaybackParameters` in this internal change are not the intended speed.

The easiest way to reproduce this is with AC-3 pass-thru audio, in this case the audio renderer does not properly
support speed change, see bug:
  google#10865

According to this commit,

 [Add back support for setting audio pitch](google@74c493f51e) [andrewlewis]

This callback to report the internal speed change is needed for the renderers to run slow motion properly.

In any event, there is no reason to propegate this internal speed change to the user speed setting (`ExoPlayerImplInternal.playbackParameters`).
The change simply fixes `MSG_PLAYBACK_PARAMETERS_CHANGED_INTERNAL` not to update the user setting.
@psharma676
Copy link
Author

Is this behavior causing any problems during playback besides not having live offset adjustment?

Should have a relation to #10882

(maybe it's using tunneling or some other passthrough mode?)

I am using stock ExoPlayer@r2.15.0 - the only changes I've made is what I also attached in the patch. So, tunneling is off. The only dominant condition check in DefaultAudioSink.shouldApplyAudioProcessorPlaybackParameters() is MimeTypes.AUDIO_RAW.equals(configuration.inputFormat.sampleMimeType) which will always evaluate to false for AC3 and the other checks don't matter.

So, what I understand is that, as long as the input stream is AC3, we will not be able to adjust playback position by speed adjusting, and that is expected behavior. In that case, and other cases too, it may make sense to add an API that lets callers check if playback speed adjustments are supported in the current configuration. Please let me know if I should keep this issue around for the API addition or can be closed.

@tonihei
Copy link
Collaborator

tonihei commented Jan 5, 2023

I'll mark it as an enhancement to avoid the unnecessary constant updates that don't work anyway.

@tonihei tonihei changed the title Playback speed does not adjust when audio is AC3 Let AudioSink indicate if playback speed changes are supported to avoid unnecessary live speed adjustments Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants