Fix AudioStreamGenerator stopping playback after a buffer underrun

[ellenhp]

Fixes #65155

Tweaked repro project: generator_repro.zip

The rationale here is that the number of samples mixed by a playback doesn't give us enough information to make a decision about whether to continue with that playback. I tested this with an ogg to make sure that it doesn't break playback of other streams, but it wouldn't hurt to test with other formats.

[bluenote10]

The simplest thing ended up working. @bluenote10 could you test #73162?

Yes that was also the first thing I tried, and based on some preliminary experiments I'd also conclude that it works. I'll do some more testing and if I find something fishy I'll report back.

Thank you very much for helping out so quickly!

[bluenote10]

Perhaps a minor observation: So far the else branch of that condition had the invariant mixed_frames == buffer_size which is now no longer the case. Do we have to change line 374 from playback->lookahead[i] = buf[buffer_size + i]; to playback->lookahead[i] = buf[mixed_frames + i]; to make sure that the lookahead buffer is copied from where the playback stopped outputting samples, instead of from the absolute end of the buffer?

[ellenhp]

That's a good catch. The lookahead buffer is used for fade-out if the playback is stopped on the boundary between two 512-sample mixing windows, so it would make sense to me to fill it with silence in the case of a buffer underrun. Filling it with the last few frames of mixed audio would mean that if it's ever used for fade-out, there will be audio played immediately after the underrun's silence, then faded out to zero, which wouldn't make much sense.

[reduz]

I feel this is the wrong solution, and it should be fixed on the side of the generator code.

[reduz]

From what i understand, generator never ends unless you actually stop it, so if there is an underrun and a pop resulting because of it, it should be user fault IMO, but it should not return less samples.

[ellenhp]

Right now if there's ever a single underrun, the playback is removed and no amount of filling the buffer will ever restart playback. That's what I'm trying to fix with this change. I don't understand how we could implement this in the generator without significant changes to the audio server.

[reduz]

@ellenhp IMO AudioStreamGeneratorPlayback::_mix_internal should always return p_frames, and only return less if the user explicitly states that no further audio will be sent (maybe adding some function end_stream())

[ellenhp]

Mentioned in RocketChat too but I don't have time for a playback API change right now so this will have to wait unless somebody else wants to take it up.

[bluenote10]

@ellenhp IMO AudioStreamGeneratorPlayback::_mix_internal should always return p_frames, and only return less if the user explicitly states that no further audio will be sent (maybe adding some function end_stream())

Perhaps even take it a step further and require all implementors of AudioStreamPlayback::mix to always return exactly the number of requested frames? After all, it would be easy for all implementors to satisfy this requirement by producing just zeros if they run out of actual data (which could be either at the end-of-stream for finite length generators or the dreaded buffer underrund for infinite generators). Would this simplify the logic, because the audio server doesn't have to care about the number of samples, and can simply use is_playing() for removing playbacks from the list?

[ellenhp]

Doing that would definitely work and has benefits like reduz outlined in the comments of #71780.

[reduz]

@bluenote10 @ellenhp That's how it worked originally, so I suppose we could just revert that change from #51296, but at this point I feel its something that would require some significant refactoring in the audio code.

The main problem is that, if the audio stream ends by itself, it should not really require ramp down. Ramp down only makes sense if you suddendly stop the audio, but currently there is no way to discern this, so IMO we will need to really think through how everything works after 4.0 is out and do the fixes later.

[ellenhp]

@bluenote10 managed to bisect this and found it was caused by #55846. I think it's unrelated to #51296.

[bluenote10]

@bluenote10 managed to bisect this and found it was caused by #55846. I think it's unrelated to #51296.

@ellenhp Yes, fully in line with what my bisecting revealed -- that's why I had pinged you on #55846 initially 😉 The exact commits I had looked at were:

[...] the commit that broke the audio stream generator is afd2bba, i.e., this PR #55846. The commit immediately before (3017530) still works fine.

If I understand it correctly: #51296 prepared the logic in the audio server to terminate streams if mixed_frames != buffer_size, but at that point in time, the mix function still returned the full p_frames, i.e., mixed_frames always equaled buffer_size. Later #55846 changed that, and the combination of both leads to the issue.

[bluenote10]

IMO we will need to really think through how everything works after 4.0 is out and do the fixes later.

@reduz I'm just wondering if this PR wouldn't still make sense as a hotfix for 4.0. After all, audio stream generation is more or less completely unusable on 4.0 currently, if the streams get terminated immediately in the majority of cases. The hotfix seems to make them fully usable again, and unless we see a specific concern why the approach may actually break something else, it would make sense to include it in the release. We could nonetheless look for a more appropriate long term solution later after 4.0 is released.

[seocwen]

I think at minimum reverting to a state where AudioStreamGenerator actually functions meets users basics expectations even if, architecturally, a better solution exists on the horizon. To be sure, people do use these resources and duct tape is much appreciated over breakage. It also means less people looking for help with something effectively maintained in a broken state, which has other effects on the community.