[PATCH 1/3] winepulse.drv: Stop writing the same data again on underflow.

[Rémi Bernon]

Data in [lcl_offs_bytes;lcl_offs_bytes+held_bytes] is the data we've
received from the client and that we consider being used by pulse,
regardless of how we write it to it.

A subset of this is [pa_offs_bytes;pa_offs_bytes+pa_held_bytes], which
is the data we have received from the client but not yet written to
pulse.

If pulsed has underflown it means that it has used all the data we've
written already, and resetting pa_offs_bytes and pa_held_bytes like here
will only make us write the same data again (in addition to silence) to
mend the underflow, ending up with potentially duplicate audio frames
being played.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52225
Signed-off-by: Rémi Bernon 
---
 dlls/winepulse.drv/pulse.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/dlls/winepulse.drv/pulse.c b/dlls/winepulse.drv/pulse.c
index bce70ac358c..c329e8c423c 100644
--- a/dlls/winepulse.drv/pulse.c
+++ b/dlls/winepulse.drv/pulse.c
@@ -260,9 +260,6 @@ static void pulse_underflow_callback(pa_stream *s, void *userdata)
     struct pulse_stream *stream = userdata;
     WARN("%p: Underflow\n", userdata);
     stream->just_underran = TRUE;
-    /* re-sync */
-    stream->pa_offs_bytes = stream->lcl_offs_bytes;
-    stream->pa_held_bytes = stream->held_bytes;
 }

static void pulse_started_callback(pa_stream *s, void *userdata)

[PATCH 2/3] winepulse.drv: Keep buffer size separate from allocated size.

[Rémi Bernon]

So that it rounds to integral audio frame count.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52225
Signed-off-by: Rémi Bernon 
---
 dlls/winepulse.drv/pulse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dlls/winepulse.drv/pulse.c b/dlls/winepulse.drv/pulse.c
index c329e8c423c..fb8133e9e45 100644
--- a/dlls/winepulse.drv/pulse.c
+++ b/dlls/winepulse.drv/pulse.c
@@ -876,7 +876,7 @@ static NTSTATUS pulse_create_stream(void *args)
             stream->alloc_size = stream->real_bufsize_bytes =
                 stream->bufsize_frames * 2 * pa_frame_size(&stream->ss);
             if (NtAllocateVirtualMemory(GetCurrentProcess(), (void **)&stream->local_buffer,
-                                        0, &stream->real_bufsize_bytes, MEM_COMMIT, PAGE_READWRITE))
+                                        0, &stream->alloc_size, MEM_COMMIT, PAGE_READWRITE))
                 hr = E_OUTOFMEMORY;
         } else {
             UINT32 i, capture_packets;

[PATCH 2/3] winepulse.drv: Keep buffer size separate from allocated size.

[Andrew Eikum]

Signed-off-by: Andrew Eikum

On Thu, Dec 16, 2021 at 06:20:25PM +0100, Rémi Bernon wrote:
> So that it rounds to integral audio frame count.
>
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52225
> Signed-off-by: Rémi Bernon
>
> dlls/winepulse.drv/pulse.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/dlls/winepulse.drv/pulse.c b/dlls/winepulse.drv/pulse.c
> index c329e8c423c..fb8133e9e45 100644
> --- a/dlls/winepulse.drv/pulse.c
> +++ b/dlls/winepulse.drv/pulse.c
> @@ -876,7 +876,7 @@ static NTSTATUS pulse_create_stream(void *args)
>              stream->alloc_size = stream->real_bufsize_bytes =
>                  stream->bufsize_frames * 2 * pa_frame_size(&stream->ss);
>              if (NtAllocateVirtualMemory(GetCurrentProcess(), (void **)&stream->local_buffer,
> -
>                                    0, &stream->real_bufsize_bytes, MEM_COMMIT, PAGE_READWRITE))
> -
>                                    0, &stream->alloc_size, MEM_COMMIT, PAGE_READWRITE))
>            hr = E_OUTOFMEMORY;
>    } else {
>        UINT32 i, capture_packets;
>
> -- 
> 2.34.0

[PATCH 3/3] winepulse.drv: Write data to pulse in pulse_release_render_buffer.

[Rémi Bernon]

Instead of waiting for the timer loop, which sometimes causes pulse
buffer underflows.

This greatly reduces the amount of underflows in Prince of Persia: The
Forgotten Sands, as well as in Forza Horizon 4 introduction and menu
audio, which are suffering from audio clicks since PE xaudio conversion.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52225
Signed-off-by: Rémi Bernon 
---
 dlls/winepulse.drv/pulse.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/dlls/winepulse.drv/pulse.c b/dlls/winepulse.drv/pulse.c
index fb8133e9e45..bd886e0d879 100644
--- a/dlls/winepulse.drv/pulse.c
+++ b/dlls/winepulse.drv/pulse.c
@@ -1675,6 +1675,9 @@ static NTSTATUS pulse_release_render_buffer(void *args)
     stream->clock_written += written_bytes;
     stream->locked = 0;

+    /* push as much data as we can to pulseaudio too */
+    pulse_write(stream);
+
     TRACE("Released %u, held %lu\n", params->written_frames, stream->held_bytes / pa_frame_size(&stream->ss));

pulse_unlock();

[PATCH 3/3] winepulse.drv: Write data to pulse in pulse_release_render_buffer.

[Andrew Eikum]

Signed-off-by: Andrew Eikum

On Thu, Dec 16, 2021 at 06:20:26PM +0100, Rémi Bernon wrote:
> Instead of waiting for the timer loop, which sometimes causes pulse
> buffer underflows.
>
> This greatly reduces the amount of underflows in Prince of Persia: The
> Forgotten Sands, as well as in Forza Horizon 4 introduction and menu
> audio, which are suffering from audio clicks since PE xaudio conversion.
>
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52225
> Signed-off-by: Rémi Bernon
>
> dlls/winepulse.drv/pulse.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/dlls/winepulse.drv/pulse.c b/dlls/winepulse.drv/pulse.c
> index fb8133e9e45..bd886e0d879 100644
> --- a/dlls/winepulse.drv/pulse.c
> +++ b/dlls/winepulse.drv/pulse.c
> @@ -1675,6 +1675,9 @@ static NTSTATUS pulse_release_render_buffer(void *args)
>      stream->clock_written += written_bytes;
>      stream->locked = 0;
> -
>
> /* push as much data as we can to pulseaudio too */
> -
>
> pulse_write(stream);
> -
>
> TRACE("Released %u, held %lu\n", params->written_frames, stream->held_bytes / pa_frame_size(&stream->ss));
>
> pulse_unlock();
>
> -- 
> 2.34.0

[Rémi Bernon]

On 12/16/21 18:20, Rémi Bernon wrote:
> Data in [lcl_offs_bytes;lcl_offs_bytes+held_bytes] is the data we've
> received from the client and that we consider being used by pulse,
> regardless of how we write it to it.
>
> A subset of this is [pa_offs_bytes;pa_offs_bytes+pa_held_bytes], which
> is the data we have received from the client but not yet written to
> pulse.
>
> If pulsed has underflown it means that it has used all the data we've
> written already, and resetting pa_offs_bytes and pa_held_bytes like here
> will only make us write the same data again (in addition to silence) to
> mend the underflow, ending up with potentially duplicate audio frames
> being played.
>
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52225
> Signed-off-by: Rémi Bernon
>
> dlls/winepulse.drv/pulse.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/dlls/winepulse.drv/pulse.c b/dlls/winepulse.drv/pulse.c
> index bce70ac358c..c329e8c423c 100644
> --- a/dlls/winepulse.drv/pulse.c
> +++ b/dlls/winepulse.drv/pulse.c
> @@ -260,9 +260,6 @@ static void pulse_underflow_callback(pa_stream *s, void *userdata)
>       struct pulse_stream *stream = userdata;
>       WARN("%p: Underflow\n", userdata);
>       stream->just_underran = TRUE;
> -
>
> /* re-sync */
> -
>
> stream->pa_offs_bytes = stream->lcl_offs_bytes;
> -
>
> stream->pa_held_bytes = stream->held_bytes;
> }
>
> static void pulse_started_callback(pa_stream *s, void *userdata)

Regarding this, although I couldn't confirm with Still Life 2, I suspect 
it may be the cause of duplicate audio frames, as illustrated in the 
attached picture.

For this picture, I captured pulse output (with Prince of Persia: 
Forgotten Sands), with Wine 7.0-rc1 (so without this patch), but with 
some local changes to add a positive spike before pushing new data, and 
a negative spike after pushing the data, right after we detected an 
underflow.

On the picture, the output audio frames are in blue. The silence is 
likely the underflow, and I suspect pulse to be keeping a bit of data 
internally, which would explain the frames after it, and before the 
first positive spike.

I then added on top, in red, a copy of the portion between the spikes, 
shifted in time to better show that it matches audio frames that have 
already been provided to pulse, and played (and split, before the 
silence, and after). Only the part between the positive blue spike and 
the negative red spike are new audio frames that were not yet sent to pulse.

[Andrew Eikum]

Signed-off-by: Andrew Eikum

On Thu, Dec 16, 2021 at 06:20:24PM +0100, Rémi Bernon wrote:
> Data in [lcl_offs_bytes;lcl_offs_bytes+held_bytes] is the data we've
> received from the client and that we consider being used by pulse,
> regardless of how we write it to it.
>
> A subset of this is [pa_offs_bytes;pa_offs_bytes+pa_held_bytes], which
> is the data we have received from the client but not yet written to
> pulse.
>
> If pulsed has underflown it means that it has used all the data we've
> written already, and resetting pa_offs_bytes and pa_held_bytes like here
> will only make us write the same data again (in addition to silence) to
> mend the underflow, ending up with potentially duplicate audio frames
> being played.
>
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52225
> Signed-off-by: Rémi Bernon
>
> dlls/winepulse.drv/pulse.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/dlls/winepulse.drv/pulse.c b/dlls/winepulse.drv/pulse.c
> index bce70ac358c..c329e8c423c 100644
> --- a/dlls/winepulse.drv/pulse.c
> +++ b/dlls/winepulse.drv/pulse.c
> @@ -260,9 +260,6 @@ static void pulse_underflow_callback(pa_stream *s, void *userdata)
>      struct pulse_stream *stream = userdata;
>      WARN("%p: Underflow\n", userdata);
>      stream->just_underran = TRUE;
> - /* re-sync */
> - stream->pa_offs_bytes = stream->lcl_offs_bytes;
> - stream->pa_held_bytes = stream->held_bytes;
>
> }
> static void pulse_started_callback(pa_stream *s, void *userdata)
>
> 2.34.0