[PATCH 1/2] mf/sar: Query for current padding before requesting sample buffer.
According to both MSDN and our impementation, GetBufferSize returns the size of the buffer, but it doesn't guarantee that all of it is available. In order to know how much of it is available, the caller must also call GetCurrentPadding and subtract that number to the buffer size. Failing to do so might result in GetBuffer returning an error. Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/mf/sar.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/dlls/mf/sar.c b/dlls/mf/sar.c index 77cbfcb21da..eba822ae0fe 100644 --- a/dlls/mf/sar.c +++ b/dlls/mf/sar.c @@ -1751,7 +1751,7 @@ static HRESULT WINAPI audio_renderer_render_callback_GetParameters(IMFAsyncCallb static void audio_renderer_render(struct audio_renderer *renderer, IMFAsyncResult *result) { - unsigned int src_frames, dst_frames, max_frames, src_len; + unsigned int src_frames, dst_frames, max_frames, pad_frames, src_len; struct queued_object *obj, *obj2; BOOL keep_sample = FALSE; IMFMediaBuffer *buffer; @@ -1775,20 +1775,24 @@ static void audio_renderer_render(struct audio_renderer *renderer, IMFAsyncResul { if (SUCCEEDED(IAudioClient_GetBufferSize(renderer->audio_client, &max_frames))) { - src_frames -= obj->u.sample.frame_offset; - dst_frames = min(src_frames, max_frames); - - if (SUCCEEDED(hr = IAudioRenderClient_GetBuffer(renderer->audio_render_client, dst_frames, &dst))) + if (SUCCEEDED(IAudioClient_GetCurrentPadding(renderer->audio_client, &pad_frames))) { - memcpy(dst, src + obj->u.sample.frame_offset * renderer->frame_size, - dst_frames * renderer->frame_size); + max_frames -= pad_frames; + src_frames -= obj->u.sample.frame_offset; + dst_frames = min(src_frames, max_frames); - IAudioRenderClient_ReleaseBuffer(renderer->audio_render_client, dst_frames, 0); + if (SUCCEEDED(hr = IAudioRenderClient_GetBuffer(renderer->audio_render_client, dst_frames, &dst))) + { + memcpy(dst, src + obj->u.sample.frame_offset * renderer->frame_size, + dst_frames * renderer->frame_size); - obj->u.sample.frame_offset += dst_frames; - } + IAudioRenderClient_ReleaseBuffer(renderer->audio_render_client, dst_frames, 0); - keep_sample = FAILED(hr) || src_frames > max_frames; + obj->u.sample.frame_offset += dst_frames; + } + + keep_sample = FAILED(hr) || src_frames > max_frames; + } } } IMFMediaBuffer_Unlock(buffer); -- 2.32.0
The IAudioClient implementation from both Windows and winepulse.drv never sets the event more than once per period, which is usually around 10 ms long. Some codecs produce audio samples shorter than 10 ms, which currently result into the SAR not being able to keep up with the audio client, because the SAR never requests more than a sample per event cycle. This patch causes the event to be set each time the available buffer space is less then half full, and each time a sample is received, so that short samples do not lead to audio client underruns. Of course it would be even better if the codecs didn't generate too short samples, because they uselessly require more CPU cycle, but we should handle them properly anyway. This patch fixes audio stuttering problems in the logo videos of Borderlands 3, Deep Rock Galactic and probably other games. Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/mf/sar.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dlls/mf/sar.c b/dlls/mf/sar.c index eba822ae0fe..1916cbd50c9 100644 --- a/dlls/mf/sar.c +++ b/dlls/mf/sar.c @@ -1360,6 +1360,7 @@ static HRESULT WINAPI audio_renderer_stream_ProcessSample(IMFStreamSink *iface, if (renderer->state == STREAM_STATE_RUNNING) hr = stream_queue_sample(renderer, sample); renderer->flags &= ~SAR_SAMPLE_REQUESTED; + SetEvent(renderer->buffer_ready_event); LeaveCriticalSection(&renderer->cs); return hr; @@ -1757,6 +1758,7 @@ static void audio_renderer_render(struct audio_renderer *renderer, IMFAsyncResul IMFMediaBuffer *buffer; BYTE *dst, *src; HRESULT hr; + BOOL retry_immediately = FALSE; LIST_FOR_EACH_ENTRY_SAFE(obj, obj2, &renderer->queue, struct queued_object, entry) { @@ -1780,6 +1782,7 @@ static void audio_renderer_render(struct audio_renderer *renderer, IMFAsyncResul max_frames -= pad_frames; src_frames -= obj->u.sample.frame_offset; dst_frames = min(src_frames, max_frames); + retry_immediately = max_frames - dst_frames > dst_frames; if (SUCCEEDED(hr = IAudioRenderClient_GetBuffer(renderer->audio_render_client, dst_frames, &dst))) { @@ -1814,6 +1817,8 @@ static void audio_renderer_render(struct audio_renderer *renderer, IMFAsyncResul renderer->flags |= SAR_SAMPLE_REQUESTED; } + if (retry_immediately) + SetEvent(renderer->buffer_ready_event); if (FAILED(hr = MFPutWaitingWorkItem(renderer->buffer_ready_event, 0, result, &renderer->buffer_ready_key))) WARN("Failed to submit wait item, hr %#x.\n", hr); } -- 2.32.0
On 6/21/21 6:52 PM, Giovanni Mascellani wrote:
The IAudioClient implementation from both Windows and winepulse.drv never sets the event more than once per period, which is usually around 10 ms long. Some codecs produce audio samples shorter than 10 ms, which currently result into the SAR not being able to keep up with the audio client, because the SAR never requests more than a sample per event cycle.
This patch causes the event to be set each time the available buffer space is less then half full, and each time a sample is received, so that short samples do not lead to audio client underruns. How SAR handles that on Windows? Could we simply adjust a number of requests that sink sends, based on "audio client buffer size" / "incoming sample buffer size".
I don't think you should touch this event yourself, when it's supposed to be signaled by the audio system.
Of course it would be even better if the codecs didn't generate too short samples, because they uselessly require more CPU cycle, but we should handle them properly anyway.
This patch fixes audio stuttering problems in the logo videos of Borderlands 3, Deep Rock Galactic and probably other games.
Hi, Il 22/06/21 10:35, Nikolay Sivov ha scritto:
How SAR handles that on Windows? Could we simply adjust a number of requests that sink sends, based on "audio client buffer size" / "incoming sample buffer size".
I don't know how Windows' SAR does this. Is there I way I can instantiate Windows' SAR using an IAudioClient provided by me? It seems that it will only take device from the MMDeviceEnumerator provided by Windows itself. The incoming sample buffer size is not known in advance, and it can vary a lot even within the same stream. It can go from 128 to 1024 frames at least. I guess it is a byproduct of how the decoder works and how packets are laid in the encoded file, as well as of the internals of any other transform in the media session. I don't know much about how Vorbis, but I guess we shouldn't depend on any assumption on how large the samples are: if we receive small samples, we just have to ask for more until our playback buffer is full enough.
I don't think you should touch this event yourself, when it's supposed to be signaled by the audio system.
I don't think it is a problem if we touch that event. From my understanding it's not something that IAudioClient is supposed to use internally. It's just a way for IAudioClient to signal that it is ready to process a buffer. If we signal more often than that it's not an issue for the IAudioClient and certainly not for the event itself. I can implement the same change without manually signalling the event, though. The reason I did it this way is that it mostly fit the current SAR architecture, so it doesn't require a lot of changes. BTW, as I write you I realize that my patch is even too much, I am sending an updated version (that still fixes the bug on Borderlands 3, Deep Rock Galactic and Mutant Year Zero). This revision is still event-based, but if you insist on a different solution I'll rework it. Basically the only important point is that when a new sample is received the queue is immediately processed, and not just once per IAudioClient period. Manually signalling the event seems to me the easiest and cleanest way to have that, but if you don't like it I can implement it another way. Thanks for the review, Giovanni.
On 6/22/21 1:12 PM, Giovanni Mascellani wrote:
Hi,
Il 22/06/21 10:35, Nikolay Sivov ha scritto:
How SAR handles that on Windows? Could we simply adjust a number of requests that sink sends, based on "audio client buffer size" / "incoming sample buffer size".
I don't know how Windows' SAR does this. Is there I way I can instantiate Windows' SAR using an IAudioClient provided by me? It seems that it will only take device from the MMDeviceEnumerator provided by Windows itself. Yes, it's probably too hard to experiment with that in a clean way.
The incoming sample buffer size is not known in advance, and it can vary a lot even within the same stream. It can go from 128 to 1024 frames at least. I guess it is a byproduct of how the decoder works and how packets are laid in the encoded file, as well as of the internals of any other transform in the media session. I don't know much about how Vorbis, but I guess we shouldn't depend on any assumption on how large the samples are: if we receive small samples, we just have to ask for more until our playback buffer is full enough. Can we check for "maxframe - pad_frames < accumulated-so-far + some_delta", and request more? Or maybe don't even need to check buffer sizes at all, we initialize with predetermined length, so we can request as much as we need until we have this duration worth of data. I just don't see why we should ignore event-based rendering, after explicitly requesting it from mmdevapi.
I don't think you should touch this event yourself, when it's supposed to be signaled by the audio system.
I don't think it is a problem if we touch that event. From my understanding it's not something that IAudioClient is supposed to use internally. It's just a way for IAudioClient to signal that it is ready to process a buffer. If we signal more often than that it's not an issue for the IAudioClient and certainly not for the event itself.
I can implement the same change without manually signalling the event, though. The reason I did it this way is that it mostly fit the current SAR architecture, so it doesn't require a lot of changes.
I don't think reusing event for that is a good idea, it's not clear to me if we should trigger rendering on ProcessSample() at all, or simply ask for more samples. Doesn't matter if it's more changes at the end, if it will fit better.
BTW, as I write you I realize that my patch is even too much, I am sending an updated version (that still fixes the bug on Borderlands 3, Deep Rock Galactic and Mutant Year Zero). This revision is still event-based, but if you insist on a different solution I'll rework it.
Basically the only important point is that when a new sample is received the queue is immediately processed, and not just once per IAudioClient period. Manually signalling the event seems to me the easiest and cleanest way to have that, but if you don't like it I can implement it another way.
Thanks for the review, Giovanni.
participants (2)
-
Giovanni Mascellani -
Nikolay Sivov