[PATCH v2 0/5] MR5564: mmdevapi: Adjust timing in AudioClient_Initialize, removing related code from drivers.

[Davide Beatrici (＠davidebeatrici)]

Trace output with PulseAudio:

```
0230:trace:mmdevapi:adjust_timing Requested duration 1000000 and period 0
0230:trace:mmdevapi:adjust_timing Device periods: 100000 default and 30000 minimum
0230:trace:mmdevapi:adjust_timing Adjusted duration 1000000 and period 100000
```

--
  v2: winepulse: Remove superfluous timing adjustment.
      wineoss: Remove superfluous timing adjustment.
      winecoreaudio: Remove superfluous timing adjustment.
      winealsa: Remove superfluous timing adjustment.
      mmdevapi: Adjust timing in AudioClient_Initialize.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5564

[PATCH v2 1/5] mmdevapi: Adjust timing in AudioClient_Initialize.

[Davide Beatrici]

From: Davide Beatrici <git(a)davidebeatrici.dev>

---
 dlls/mmdevapi/client.c | 58 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/dlls/mmdevapi/client.c b/dlls/mmdevapi/client.c
index e0c12b8e534..0b849e47e3d 100644
--- a/dlls/mmdevapi/client.c
+++ b/dlls/mmdevapi/client.c
@@ -99,6 +99,61 @@ static inline struct audio_client *impl_from_IAudioStreamVolume(IAudioStreamVolu
     return CONTAINING_RECORD(iface, struct audio_client, IAudioStreamVolume_iface);
 }
 
+static HRESULT adjust_timing(struct audio_client *This,
+                             REFERENCE_TIME *duration, REFERENCE_TIME *period,
+                             const AUDCLNT_SHAREMODE mode, const DWORD flags,
+                             const WAVEFORMATEX *fmt)
+{
+    struct get_device_period_params params;
+    REFERENCE_TIME def_period, min_period;
+
+    TRACE("Requested duration %lu and period %lu\n", (ULONG)*duration, (ULONG)*period);
+
+    params.device     = This->device_name;
+    params.flow       = This->dataflow;
+    params.def_period = &def_period;
+    params.min_period = &min_period;
+
+    wine_unix_call(get_device_period, ¶ms);
+
+    if (FAILED(params.result))
+        return params.result;
+
+    TRACE("Device periods: %lu default and %lu minimum\n", (ULONG)def_period, (ULONG)min_period);
+
+    if (mode == AUDCLNT_SHAREMODE_SHARED) {
+        *period = def_period;
+        if (*duration < 3 * *period)
+            *duration = 3 * *period;
+    } else {
+        const WAVEFORMATEXTENSIBLE *fmtex = (WAVEFORMATEXTENSIBLE *)fmt;
+        if (fmtex->Format.wFormatTag == WAVE_FORMAT_EXTENSIBLE &&
+           (fmtex->dwChannelMask == 0 || fmtex->dwChannelMask & SPEAKER_RESERVED))
+            params.result = AUDCLNT_E_UNSUPPORTED_FORMAT;
+        else {
+            if (*period == 0)
+                *period = def_period;
+            if (*period < min_period || *period > 5000000)
+                params.result = AUDCLNT_E_INVALID_DEVICE_PERIOD;
+            else if (*duration > 20000000) /* The smaller the period, the lower this limit. */
+                params.result = AUDCLNT_E_BUFFER_SIZE_ERROR;
+            else if (flags & AUDCLNT_STREAMFLAGS_EVENTCALLBACK) {
+                if (*duration != *period)
+                    params.result = AUDCLNT_E_BUFDURATION_PERIOD_NOT_EQUAL;
+
+                FIXME("EXCLUSIVE mode with EVENTCALLBACK\n");
+
+                params.result = AUDCLNT_E_DEVICE_IN_USE;
+            } else if (*duration < 8 * *period)
+                *duration = 8 * *period; /* May grow above 2s. */
+        }
+    }
+
+    TRACE("Adjusted duration %lu and period %lu\n", (ULONG)*duration, (ULONG)*period);
+
+    return params.result;
+}
+
 static void dump_fmt(const WAVEFORMATEX *fmt)
 {
     TRACE("wFormatTag: 0x%x (", fmt->wFormatTag);
@@ -502,6 +557,9 @@ static HRESULT WINAPI client_Initialize(IAudioClient3 *iface, AUDCLNT_SHAREMODE
         return E_INVALIDARG;
     }
 
+    if (FAILED(params.result = adjust_timing(This, &duration, &period, mode, flags, fmt)))
+        return params.result;
+
     sessions_lock();
 
     if (This->stream) {
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/5564

[PATCH v2 2/5] winealsa: Remove superfluous timing adjustment.

[Davide Beatrici]

From: Davide Beatrici <git(a)davidebeatrici.dev>

---
 dlls/winealsa.drv/alsa.c | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/dlls/winealsa.drv/alsa.c b/dlls/winealsa.drv/alsa.c
index aa1ad4abe6c..f2691e4cba0 100644
--- a/dlls/winealsa.drv/alsa.c
+++ b/dlls/winealsa.drv/alsa.c
@@ -810,36 +810,6 @@ static NTSTATUS alsa_create_stream(void *args)
 
     params->result = S_OK;
 
-    if (params->share == AUDCLNT_SHAREMODE_SHARED) {
-        params->period = def_period;
-        if (params->duration < 3 * params->period)
-            params->duration = 3 * params->period;
-    } else {
-        if (fmtex->Format.wFormatTag == WAVE_FORMAT_EXTENSIBLE &&
-           (fmtex->dwChannelMask == 0 || fmtex->dwChannelMask & SPEAKER_RESERVED))
-            params->result = AUDCLNT_E_UNSUPPORTED_FORMAT;
-        else {
-            if (!params->period)
-                params->period = def_period;
-            if (params->period < min_period || params->period > 5000000)
-                params->result = AUDCLNT_E_INVALID_DEVICE_PERIOD;
-            else if (params->duration > 20000000) /* The smaller the period, the lower this limit. */
-                params->result = AUDCLNT_E_BUFFER_SIZE_ERROR;
-            else if (params->flags & AUDCLNT_STREAMFLAGS_EVENTCALLBACK) {
-                if (params->duration != params->period)
-                    params->result = AUDCLNT_E_BUFDURATION_PERIOD_NOT_EQUAL;
-
-                FIXME("EXCLUSIVE mode with EVENTCALLBACK\n");
-
-                params->result = AUDCLNT_E_DEVICE_IN_USE;
-            } else if (params->duration < 8 * params->period)
-                params->duration = 8 * params->period; /* May grow above 2s. */
-        }
-    }
-
-    if (FAILED(params->result))
-        return STATUS_SUCCESS;
-
     stream = calloc(1, sizeof(*stream));
     if(!stream){
         params->result = E_OUTOFMEMORY;
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/5564

[PATCH v2 3/5] winecoreaudio: Remove superfluous timing adjustment.

[Davide Beatrici]

From: Davide Beatrici <git(a)davidebeatrici.dev>

---
 dlls/winecoreaudio.drv/coreaudio.c | 31 ------------------------------
 1 file changed, 31 deletions(-)

diff --git a/dlls/winecoreaudio.drv/coreaudio.c b/dlls/winecoreaudio.drv/coreaudio.c
index a79a0d16839..deb9df1b45a 100644
--- a/dlls/winecoreaudio.drv/coreaudio.c
+++ b/dlls/winecoreaudio.drv/coreaudio.c
@@ -725,37 +725,6 @@ static NTSTATUS unix_create_stream(void *args)
 
     params->result = S_OK;
 
-    if (params->share == AUDCLNT_SHAREMODE_SHARED) {
-        params->period = def_period;
-        if (params->duration < 3 * params->period)
-            params->duration = 3 * params->period;
-    } else {
-        const WAVEFORMATEXTENSIBLE *fmtex = (WAVEFORMATEXTENSIBLE *)params->fmt;
-        if (fmtex->Format.wFormatTag == WAVE_FORMAT_EXTENSIBLE &&
-           (fmtex->dwChannelMask == 0 || fmtex->dwChannelMask & SPEAKER_RESERVED))
-            params->result = AUDCLNT_E_UNSUPPORTED_FORMAT;
-        else {
-            if (!params->period)
-                params->period = def_period;
-            if (params->period < min_period || params->period > 5000000)
-                params->result = AUDCLNT_E_INVALID_DEVICE_PERIOD;
-            else if (params->duration > 20000000) /* The smaller the period, the lower this limit. */
-                params->result = AUDCLNT_E_BUFFER_SIZE_ERROR;
-            else if (params->flags & AUDCLNT_STREAMFLAGS_EVENTCALLBACK) {
-                if (params->duration != params->period)
-                    params->result = AUDCLNT_E_BUFDURATION_PERIOD_NOT_EQUAL;
-
-                FIXME("EXCLUSIVE mode with EVENTCALLBACK\n");
-
-                params->result = AUDCLNT_E_DEVICE_IN_USE;
-            } else if (params->duration < 8 * params->period)
-                params->duration = 8 * params->period; /* May grow above 2s. */
-        }
-    }
-
-    if (FAILED(params->result))
-        return STATUS_SUCCESS;
-
     if (!(stream = calloc(1, sizeof(*stream)))) {
         params->result = E_OUTOFMEMORY;
         return STATUS_SUCCESS;
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/5564

[PATCH v2 4/5] wineoss: Remove superfluous timing adjustment.

[Davide Beatrici]

From: Davide Beatrici <git(a)davidebeatrici.dev>

---
 dlls/wineoss.drv/oss.c | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/dlls/wineoss.drv/oss.c b/dlls/wineoss.drv/oss.c
index 89e626f39d6..819e876606c 100644
--- a/dlls/wineoss.drv/oss.c
+++ b/dlls/wineoss.drv/oss.c
@@ -576,36 +576,6 @@ static NTSTATUS oss_create_stream(void *args)
 
     params->result = S_OK;
 
-    if (params->share == AUDCLNT_SHAREMODE_SHARED) {
-        params->period = def_period;
-        if (params->duration < 3 * params->period)
-            params->duration = 3 * params->period;
-    } else {
-        if (fmtex->Format.wFormatTag == WAVE_FORMAT_EXTENSIBLE &&
-           (fmtex->dwChannelMask == 0 || fmtex->dwChannelMask & SPEAKER_RESERVED))
-            params->result = AUDCLNT_E_UNSUPPORTED_FORMAT;
-        else {
-            if (!params->period)
-                params->period = def_period;
-            if (params->period < min_period || params->period > 5000000)
-                params->result = AUDCLNT_E_INVALID_DEVICE_PERIOD;
-            else if (params->duration > 20000000) /* The smaller the period, the lower this limit. */
-                params->result = AUDCLNT_E_BUFFER_SIZE_ERROR;
-            else if (params->flags & AUDCLNT_STREAMFLAGS_EVENTCALLBACK) {
-                if (params->duration != params->period)
-                    params->result = AUDCLNT_E_BUFDURATION_PERIOD_NOT_EQUAL;
-
-                FIXME("EXCLUSIVE mode with EVENTCALLBACK\n");
-
-                params->result = AUDCLNT_E_DEVICE_IN_USE;
-            } else if (params->duration < 8 * params->period)
-                params->duration = 8 * params->period; /* May grow above 2s. */
-        }
-    }
-
-    if (FAILED(params->result))
-        return STATUS_SUCCESS;
-
     stream = calloc(1, sizeof(*stream));
     if(!stream){
         params->result = E_OUTOFMEMORY;
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/5564

[PATCH v2 5/5] winepulse: Remove superfluous timing adjustment.

[Davide Beatrici]

From: Davide Beatrici <git(a)davidebeatrici.dev>

---
 dlls/winepulse.drv/pulse.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/dlls/winepulse.drv/pulse.c b/dlls/winepulse.drv/pulse.c
index 62658fc98e6..dd8d0b4441d 100644
--- a/dlls/winepulse.drv/pulse.c
+++ b/dlls/winepulse.drv/pulse.c
@@ -1116,7 +1116,6 @@ static HRESULT get_device_period_helper(EDataFlow flow, const char *pulse_name,
 static NTSTATUS pulse_create_stream(void *args)
 {
     struct create_stream_params *params = args;
-    REFERENCE_TIME period, duration = params->duration;
     struct pulse_stream *stream;
     unsigned int i, bufsize_bytes;
     HRESULT hr;
@@ -1156,21 +1155,15 @@ static NTSTATUS pulse_create_stream(void *args)
     if (FAILED(hr))
         goto exit;
 
-    period = 0;
-    hr = get_device_period_helper(params->flow, params->device, &period, NULL);
-    if (FAILED(hr))
-        goto exit;
-
-    if (duration < 3 * period)
-        duration = 3 * period;
-
-    stream->def_period = period;
+    stream->def_period = params->period;
 
-    stream->period_bytes = pa_frame_size(&stream->ss) * muldiv(period, stream->ss.rate, 10000000);
+    stream->period_bytes = pa_frame_size(&stream->ss) * muldiv(params->period,
+                                                               stream->ss.rate,
+                                                               10000000);
 
-    stream->bufsize_frames = ceil((duration / 10000000.) * params->fmt->nSamplesPerSec);
+    stream->bufsize_frames = ceil((params->duration / 10000000.) * params->fmt->nSamplesPerSec);
     bufsize_bytes = stream->bufsize_frames * pa_frame_size(&stream->ss);
-    stream->mmdev_period_usec = period / 10;
+    stream->mmdev_period_usec = params->period / 10;
 
     stream->share = params->share;
     stream->flags = params->flags;
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/5564

[PATCH v2 5/5] winepulse: Remove superfluous timing adjustment.

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=145340

Your paranoid android.


=== debian11b (64 bit WoW report) ===

vulkan-1:
vulkan: Timeout

[Davide Beatrici (＠davidebeatrici)]

```c
../dlls/winealsa.drv/alsa.c:88:29: error: 'min_period' defined but not used [-Werror=unused-const-variable=]
   88 | static const REFERENCE_TIME min_period = 50000;
      |                             ^~~~~~~~~~
```

https://gitlab.winehq.org/wine/wine/-/blob/3fa9023bf61e6dd17e997b5ed3ec8c27c...

@huw Should I change this to set the minimum period like the other backends do?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5564#note_69380

[Huw Davies (＠huw)]

Huw Davies (@huw) commented about dlls/winepulse.drv/pulse.c:
> if (FAILED(hr))
>          goto exit;
>
> -    period = 0;
> -    hr = get_device_period_helper(params->flow, params->device, &period, NULL);
> -    if (FAILED(hr))
> -        goto exit;
> -

This looks to me to change the behaviour significantly under winepulse, so it needs more explanation.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5564#note_69862

[Davide Beatrici (＠davidebeatrici)]

On Wed May  8 08:10:01 2024 +0000, Huw Davies wrote:
> This looks to me to change the behaviour significantly under winepulse,
> so it needs more explanation.
> The removed code always forced the default device period, regardless of what the application requested:

```c
static HRESULT get_device_period_helper(EDataFlow flow, const char *pulse_name, REFERENCE_TIME *def, REFERENCE_TIME *min)
{
    struct list *list = (flow == eRender) ? &g_phys_speakers : &g_phys_sources;
    PhysDevice *dev;

    if (!def && !min) {
        return E_POINTER;
    }

    LIST_FOR_EACH_ENTRY(dev, list, PhysDevice, entry) {
        if (strcmp(pulse_name, dev->pulse_name))
            continue;

        if (def)
            *def = dev->def_period;
        if (min)
            *min = dev->min_period;
        return S_OK;
    }

    return E_FAIL;
}
```

The new function `adjust_timing()` does the same thing:

```c
    if (mode == AUDCLNT_SHAREMODE_SHARED) {
        *period = def_period;
        if (*duration < 3 * *period)
            *duration = 3 * *period;
```

We don't care about the `else` branch as exclusive mode is explicitly forbidden with PulseAudio:

```c
    if (params->share == AUDCLNT_SHAREMODE_EXCLUSIVE) {
        params->result = AUDCLNT_E_EXCLUSIVE_MODE_NOT_ALLOWED;
        return STATUS_SUCCESS;
    }
```

In conclusion: with my commit(s) applied all drivers should behave the same as before.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5564#note_69925

[Huw Davies (＠huw)]

On Wed May  8 16:15:09 2024 +0000, Davide Beatrici wrote:
> The removed code always forced the default device period, regardless of
> what the application requested:
> ```c
> static HRESULT get_device_period_helper(EDataFlow flow, const char
> *pulse_name, REFERENCE_TIME *def, REFERENCE_TIME *min)
> {
>     struct list *list = (flow == eRender) ? &g_phys_speakers : &g_phys_sources;
>     PhysDevice *dev;
>     if (!def && !min) {
>         return E_POINTER;
>     }
>     LIST_FOR_EACH_ENTRY(dev, list, PhysDevice, entry) {
>         if (strcmp(pulse_name, dev->pulse_name))
>             continue;
>         if (def)
>             *def = dev->def_period;
>         if (min)
>             *min = dev->min_period;
>         return S_OK;
>     }
>     return E_FAIL;
> }
> ```
> The new function `adjust_timing()` does the same thing:
> ```c
>     if (mode == AUDCLNT_SHAREMODE_SHARED) {
>         *period = def_period;
>         if (*duration < 3 * *period)
>             *duration = 3 * *period;
> ```
> We don't care about the `else` branch as exclusive mode is explicitly
> forbidden with PulseAudio:
> ```c
>     if (params->share == AUDCLNT_SHAREMODE_EXCLUSIVE) {
>         params->result = AUDCLNT_E_EXCLUSIVE_MODE_NOT_ALLOWED;
>         return STATUS_SUCCESS;
>     }
> ```
> In conclusion: with my commit(s) applied all drivers should behave the
> same as before.
> Ah right.  Could you extend the commit message to note that exclusive mode isn't supported with the pulse driver?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5564#note_71622

[Davide Beatrici (＠davidebeatrici)]

On Wed May 29 07:57:15 2024 +0000, Huw Davies wrote:
> Ah right.  Could you extend the commit message to note that exclusive
> mode isn't supported with the pulse driver?
> No problem. Could you also take a look at https://gitlab.winehq.org/wine/wine/-/merge_requests/5564#note_69380, please?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5564#note_71630

[Huw Davies (＠huw)]

On Wed May 29 12:57:17 2024 +0000, Davide Beatrici wrote:
> ```c
> ../dlls/winealsa.drv/alsa.c:88:29: error: 'min_period' defined but not
> used [-Werror=unused-const-variable=]
>    88 | static const REFERENCE_TIME min_period = 50000;
>       |                             ^~~~~~~~~~
> ```
> https://gitlab.winehq.org/wine/wine/-/blob/3fa9023bf61e6dd17e997b5ed3ec8c27c...
> @huw Should I change this to set the minimum period like the other
> backends do?

Let's add a separate commit, at the beginning of this series, to change alsa's `get_device_period()` to return `min_period`.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5564#note_71631