[RFC] player: handle audio pts more consistently

[Dudemanguy]

See commit message for full details. Supersedes #14205. Hopefully if I understood it right, this should fix all the weird behavior that happens because the audio pts becomes negative.

This fully fixes https://github.com/mpv-player/mpv/issues/10346, and https://github.com/mpv-player/mpv/issues/12247 is still fixed (used a different file that I confirmed exhibited the described issue before some related commits closed it).

[github-actions[bot]]

Download the artifacts for this pull request:

Windows

* [mpv-i686-w64-mingw32](https://nightly.link/mpv-player/mpv/actions/artifacts/1537775501.zip) * [mpv-x86_64-w64-mingw32](https://nightly.link/mpv-player/mpv/actions/artifacts/1537775970.zip) * [mpv-x86_64-windows-msvc](https://nightly.link/mpv-player/mpv/actions/artifacts/1537781299.zip)

macOS

* [mpv-macos-12-intel](https://nightly.link/mpv-player/mpv/actions/artifacts/1537774824.zip) * [mpv-macos-13-intel](https://nightly.link/mpv-player/mpv/actions/artifacts/1537775239.zip) * [mpv-macos-14-arm](https://nightly.link/mpv-player/mpv/actions/artifacts/1537774530.zip)

[ruihe774]

I'd say it is still not correct. It is necessary to track the playback speed of samples when they are enqueued. See my argument in https://github.com/mpv-player/mpv/pull/11955#issuecomment-2024317171

[Dudemanguy]

It is necessary to track the playback speed of samples when they are enqueued. See my argument in

I don't disagree with you, but this is not trying to solve that.

[ruihe774]

It is necessary to track the playback speed of samples when they are enqueued. See my argument in

I don't disagree with you, but this is not trying to solve that.

I'm wondering why not to correct the implementation of calculating playing audio pts instead of patching the handling here.

Adjusting for AO delay is not really relevant to users and can lead to unexpected results. The newly named adjusted_audio_pts function has the speed adjustment logic folded back into it in light of this.

I don't agree with it. I think these "weird behaviors" are caused by the incorrectness of playing audio pts. If we have a correct calculation of playing audio pts there won't be negative numbers. BTW the introducing of adjusted_audio_pts will change the original semantics.

I'm actually working on a refactoring of playing audio pts calculation by using a FIFO list to track audio data enqueuing. (It is not finished yet, though.)

[Dudemanguy]

I'm wondering why not to correct the implementation of calculating playing audio pts instead of patching the handling here.

My interpretation is that using playing audio pts (or adjusted audio pts as I renamed it here) instead of written audio pts in various places is fundamentally not correct. Whether or not the calculation is correct is separate matter.

If we have a correct calculation of playing audio pts there won't be negative numbers.

How so? If the AO delay is greater than the last audio pts, you'll get a negative number. Is that wrong?

[ruihe774]

How so? If the AO delay is greater than the last audio pts, you'll get a negative number. Is that wrong?

Yep, that's my option: using AO delay to calculate playing audio pts is incorrect. Instead, we need to track what audio frame is actually playing by the ao driver. If no audio frame is played, the playing audio pts should be zero instead of negative values.

My interpretation is that using playing audio pts (or adjusted audio pts as I renamed it here) instead of written audio pts in various places is fundamentally not correct. Whether or not the calculation is correct is separate matter.

Why? e.g. shouldn't the progress bar be aligned to the audio pts that is actually playing, instead of the audio pts that is just written?

[Dudemanguy]

Much of the current sync code is written with the expectation of expectation of negative values though. We've tried some naive clamping attempts before in the past with not so great results. Of course if your change handles this and fixes this, that's great. But it might not be easy. In any case, I don't see a conflict. We can improve the current scheme. It's a relatively simple, low risk change (not necessarily in the exact current form but something similar). If someone comes up with something later that makes this obsolete, no biggie.

e.g. shouldn't the progress bar be aligned to the audio pts that is actually playing

Not really because that would include negative numbers. Whatever a user gets as a property should have at least some cosmetics to it.

[ruihe774]

Not really because that would include negative numbers. Whatever a user gets as a property should have at least some cosmetics to it.

e.g. ao_foundation has a delay of more than 1 second:

https://github.com/mpv-player/mpv/blob/a7914086596b785ef81eb1709dd1e44175fb8331/audio/out/ao_avfoundation.m#L310-L311

What's more, there can be considerable delay using ao_pulse and ao_pipewire if connected to a remote device.

I don't think it is acceptable to users if there will be a difference of a few seconds between the progress bar and the actual pts.

[Dudemanguy]

I've changed the approach so that get_playback_time is always nonnegative except for the MP_NOPTS_VALUE case. It seems like that was the intent when it was originally added in a73415584c9dbf920ec14d26d0b0629cae81b3d5. For audio-pts, that property can still return weird numbers but I clarified a bit in the documentation how it is different than time-pos.

[ruihe774]

LGTM now :heart:

[CrendKing]

@Dudemanguy, after this change, time-pos and playback-time have exactly same code. Should we update the doc because currently doc for playback-time still implies they are different.

[Dudemanguy]

Oh yeah, I missed that playback-time is exactly the same now. Maybe we should just deprecate playback-time. Most scripts in the wild I see use time-pos.

[CrendKing]

Thanks. Also, it'd nice if the last "time-pos" in that audio-pts doc be backticked like time-pos.