Make AudioStreamPlayer's get_playback_position more accurate

[Mickeon]

Closes #71122

This PR introduces a bit of a "workaround" to the inaccuracy of AudioStreamPlayer's get_playback_position. As inspired by a comment, we can subtract the time to the next mix from the AudioServer to get more precise results.
The results are fairly noticeable. When testing, the same result rarely appears twice across multiple processed frames (unless the stream is paused of course).
However, this may come at the cost of performance (if you actually used this previously unreliable method).
I am also a tad concerned about thread conflicts of some kind? Please take a look.

Also affects AudioStreamPlayer2D and AudioStreamPlayer3D.

[ellenhp]

Thanks for the PR, I like this idea. Question: would it make more sense to subtract the time until the next mix? That's actually just a question for now, I'm not saying to go do that quite yet. My intuition says that if we just started a playback and handed 10ms of audio off to the operating system 1ms ago, it's probably 1ms into playing that audio, not 11ms. So the current position of the playback (10ms) minus 9ms until the next mix would get us the right result there. But we definitely don't ever want to return a negative result, which becomes possible once we start to do subtraction.

[Mickeon]

Thank you very much! Your assumption was very much correct. I have tested this for a few minutes of constantly reopening the same scene. With a subtraction, values have never gone below 0, even with varying pitch_scale. That tells me it is even more accurate.

I have actually already modified the PR accordingly, but who knows. There may be an obscure situation where the returned value somehow goes below 0, and that may need to be accounted for.

EDIT: Negative numbers have LITERALLY occurred after this comment, even though nothing has changed. Classic!

[ellenhp]

A comment, and one other question: How does this interact with paused playbacks? If a playback is paused I don't think we should be looking at time until/since a mix.

[ellenhp]

I think if we're subtracting this should be the time until the next mix. If we subtract the time since the last mix, the playback position will go down over the course of the mix, then jump up, go down again etc.

[Mickeon]

This is true, but I tested again, and unfortunately negative numbers seem to happen. So to keep it safe it would probably be best adding instead of subtracting, unless the consensus is against it, and clamping the number is the right approach.

[ellenhp]

I would expect negative numbers if you were to call this after calling play() and prior to the first mix completing so that's not too surprising. The bigger issue I see though is the pausing problem, and that the easiest way to solve the issue with pausing (an if statement) could cause the playback positions to not be monotonic.

[Mickeon]

easiest way to solve the issue with pausing (an if statement) could cause the playback positions to not be monotonic.

I have vigorously tested the simple fix for stream_paused. That seems to work surprisingly well.

However, a bit of an issue.
There are occasions with this PR where the next playback position value is, in fact, smaller than the previous value. But the difference between the two can be so negligible, it may be a floating-point rounding error.

[MJacred]

I agree that subtracting get_time_to_next_mix sounds the most sense reasonable.

Apart from that, I'm wondering if we should involve AudioServer::get_output_latency()? There's a difference between technical "mixing" playback and audible playback.
Possibly the best option would be to just document this in the func get_playback_position to avoid extra computation cost? And anybody who's interested can post-process it themselves.

Sadly, I can't comment on pausing. I only know that there's a fade out and I'm uncertain how exactly that affects _last_mix_time…

[MJacred]

@Mickeon: get_time_to_next_mix is sometimes negative, please see #49403. Maybe we really must apply latency…

[Mickeon]

Maybe we really must apply latency…

That sounds good on paper. At least, according to #49403 comments, that looks to be the solution.
But that's something outside of this PR. How to handle it here? Should get_time_to_next_mix() return a negative value for the playback_position formula? Or should it be clamped or otherwise modified, like MAX(AudioServer::get_singleton()->get_time_to_next_mix() * pitch_scale, 0.0)?

[MJacred]

Hm… I stand with my first comment: put latency info into the method docs. So in this case: "MAX" it as you suggest

[Mickeon]

How does this interact with paused playbacks? If a playback is paused I don't think we should be looking at time until/since a mix.

Yes. In this PR, when the stream is paused the returned value "fluctuates" because it assumes the AudioStreamPlayer is always playing. Fixing this nicely may require storing AudioServer.get_time_since_last_mix every time the stream is paused, and accounting for it when calling get_playback_position.
I'm not sure if that's worth it, though, because it would inevitably return a value smaller than the previous when unpaused. Although, it does sound like expected behavior...

[RedMser]

Since you're editing the existing method, instead of adding a new one, this PR should be marked as breaking compat.

[Mickeon]

I suppose it could for now, but stuff is still up the air. It could even be an optional parameter in get_playback_position to make it more accurate. Regardless, it seems like most users are unable to make use of the returned value, because of how inconsistently it refreshes.

[ellenhp]

Since you're editing the existing method, instead of adding a new one, this PR should be marked as breaking compat.

This depends on whether we can make the new implementation monotonic in all cases IMO.

If not, maybe this method should be added as a new one like get_playback_position_interpolated?

[Mickeon]

Updated the PR following the above comments.

• Use AudioServer's get_time_to_next_mix instead of get_time_since_last_mix & subtract;
  • This seems to be the recommended solution.
    Unfortunately, it comes at the cost of sometimes returning negative numbers:
    AudioServer.get_time_to_next_mix() can return negative values #49403
    To compensate for this, the best I can do here is clamping this value above 0.
• Do not consider mix time when the stream is paused;
• Add a note about latency in the documentation.

With these changes, and despite this, The method in this PR is unfortunately still not monotonic; That is, the next returned value is not always guaranteed to be greater than the previous, by a pretty small margin:

This could have something to do with the aforementioned bug, or something else beyond the scope of the current PR. I am not knowledgeable enough to say.

---

Thank you everyone very much for the very swift feedback!

[ellenhp]

Another thing to keep in mind is that we need to test with pitch_scale and the wav ping-pong loop mode to make sure it behaves as expected.

[MJacred]

not consider mix time when the stream is paused

Actually, while the stream is paused I'd propose to return AudioServer::get_singleton()->get_playback_position.
Because that value should be accurate during pause.

And to the docs, we could add that get_time_since_last_mix returns the pause duration, when paused.

This came to me after thinking about it away from computer, so it would be nice to double-check that!

P.S.: I also suggest changing float get_playback_position(Ref<AudioStreamPlayback> p_playback); in audio_server.h to return double, because that's what all the audio stream playbacks return…

P.P.S.: I really like the proposal with get_playback_position_interpolated

[Mickeon]

Actually, while the stream is paused I'd propose to return AudioServer::get_singleton()->get_playback_position.

It already does this in this PR. No "interpolation" is performed when paused.

P.S.: I also suggest changing float get_playback_position(Ref<AudioStreamPlayback> p_playback); in audio_server.h to return double, because that's what all the audio stream playbacks return…

I actively avoided to change the AudioServer API. That method may return float because it's lighter for the internal API to manage, but AudioStreamPlayback is an exposed class. I... am not sure.

[MJacred]

It already does this in this PR. No "interpolation" is performed when paused.

Indeed. My bad, I misread that

And regarding that float to double: Actually, it happens from time to time that float was used instead of double (which seems accidental), like with loop offset, which is fixed in another PR. But sure, someone from the team can decide