[PATCH v2 0/3] MR5585: Arek Hiler's avatar mmdevapi: Implement AudioClockAdjustment_SetSampleRate.
This is based on work done by Andrew Eikum. It has been in some form in Proton for the last 4 years. If server's sampling rate is not 48kHz **DOOM Eternal** will try to set it 48kHz for the streams using the implemented interface. There's a whole class of audio devices that use 44.1kHz sampling rate and at least PulseAudio / PipeWire tends to inherit the value from the hardware to avoid resampling. The value can also be overridden by the user via the audio server's config files. In such cases, if the interface is not present or stubbed, this results in **audio underruns and noticeable crackling**. It's easy to test with pipewire-pulse: ``` $ cat /etc/pipewire/pipewire.conf.d/sample-rate.conf context.properties = { default.clock.rate = 41100 } ``` With PulseAudio this should be doable via setting `default-sample-rate = 41100` in `/etc/pulse/daemon.conf`. -- v2: winepulse.drv: Implement set_sample_rate. mmdevapi: Implement AudioClockAdjustment_SetSampleRate. mmdevapi: Add stub IAudioClockAdjustment implementation. https://gitlab.winehq.org/wine/wine/-/merge_requests/5585
From: Andrew Eikum <aeikum(a)codeweavers.com> --- dlls/mmdevapi/client.c | 49 ++++++++++++++++++++++++++++++++++++ dlls/mmdevapi/mmdevdrv.h | 1 + dlls/mmdevapi/tests/render.c | 33 ++++++++++++++++++++++++ 3 files changed, 83 insertions(+) diff --git a/dlls/mmdevapi/client.c b/dlls/mmdevapi/client.c index e0c12b8e534..39027bb25a9 100644 --- a/dlls/mmdevapi/client.c +++ b/dlls/mmdevapi/client.c @@ -89,6 +89,11 @@ static inline struct audio_client *impl_from_IAudioClock2(IAudioClock2 *iface) return CONTAINING_RECORD(iface, struct audio_client, IAudioClock2_iface); } +static inline ACImpl *impl_from_IAudioClockAdjustment(IAudioClockAdjustment *iface) +{ + return CONTAINING_RECORD(iface, ACImpl, IAudioClockAdjustment_iface); +} + static inline struct audio_client *impl_from_IAudioRenderClient(IAudioRenderClient *iface) { return CONTAINING_RECORD(iface, struct audio_client, IAudioRenderClient_iface); @@ -407,6 +412,8 @@ const IAudioCaptureClientVtbl AudioCaptureClient_Vtbl = static HRESULT WINAPI client_QueryInterface(IAudioClient3 *iface, REFIID riid, void **ppv) { + struct audio_client *This = impl_from_IAudioClient3(iface); + TRACE("(%p)->(%s, %p)\n", iface, debugstr_guid(riid), ppv); if (!ppv) @@ -417,6 +424,8 @@ static HRESULT WINAPI client_QueryInterface(IAudioClient3 *iface, REFIID riid, v IsEqualIID(riid, &IID_IAudioClient2) || IsEqualIID(riid, &IID_IAudioClient3)) *ppv = iface; + else if (IsEqualIID(riid, &IID_IAudioClockAdjustment)) + *ppv = &This->IAudioClockAdjustment_iface; else if(IsEqualIID(riid, &IID_IMarshal)) { struct audio_client *This = impl_from_IAudioClient3(iface); return IUnknown_QueryInterface(This->marshal, riid, ppv); @@ -855,6 +864,9 @@ static HRESULT WINAPI client_GetService(IAudioClient3 *iface, REFIID riid, void if (!new_session) IUnknown_AddRef((IUnknown *)*ppv); + } else if (IsEqualIID(riid, &IID_IAudioClockAdjustment)) { + hr = E_INVALIDARG; + goto exit; } else { FIXME("stub %s\n", debugstr_guid(riid)); hr = E_NOINTERFACE; @@ -1144,6 +1156,42 @@ const IAudioClock2Vtbl AudioClock2_Vtbl = clock2_GetDevicePosition }; +static HRESULT WINAPI AudioClockAdjustment_QueryInterface(IAudioClockAdjustment *iface, + REFIID riid, void **ppv) +{ + ACImpl *This = impl_from_IAudioClockAdjustment(iface); + return IAudioClock_QueryInterface(&This->IAudioClock_iface, riid, ppv); +} + +static ULONG WINAPI AudioClockAdjustment_AddRef(IAudioClockAdjustment *iface) +{ + ACImpl *This = impl_from_IAudioClockAdjustment(iface); + return IAudioClient3_AddRef(&This->IAudioClient3_iface); +} + +static ULONG WINAPI AudioClockAdjustment_Release(IAudioClockAdjustment *iface) +{ + ACImpl *This = impl_from_IAudioClockAdjustment(iface); + return IAudioClient3_Release(&This->IAudioClient3_iface); +} + +static HRESULT WINAPI AudioClockAdjustment_SetSampleRate(IAudioClockAdjustment *iface, float rate) +{ + ACImpl *This = impl_from_IAudioClockAdjustment(iface); + + FIXME("(%p)->(%f) stub\n", This, rate); + + return E_NOTIMPL; +} + +const IAudioClockAdjustmentVtbl AudioClockAdjustment_Vtbl = +{ + AudioClockAdjustment_QueryInterface, + AudioClockAdjustment_AddRef, + AudioClockAdjustment_Release, + AudioClockAdjustment_SetSampleRate +}; + static HRESULT WINAPI render_QueryInterface(IAudioRenderClient *iface, REFIID riid, void **ppv) { struct audio_client *This = impl_from_IAudioRenderClient(iface); @@ -1430,6 +1478,7 @@ HRESULT AudioClient_Create(GUID *guid, IMMDevice *device, IAudioClient **out) This->IAudioClient3_iface.lpVtbl = &AudioClient3_Vtbl; This->IAudioClock_iface.lpVtbl = &AudioClock_Vtbl; This->IAudioClock2_iface.lpVtbl = &AudioClock2_Vtbl; + This->IAudioClockAdjustment_iface.lpVtbl = &AudioClockAdjustment_Vtbl; This->IAudioRenderClient_iface.lpVtbl = &AudioRenderClient_Vtbl; This->IAudioStreamVolume_iface.lpVtbl = &AudioStreamVolume_Vtbl; diff --git a/dlls/mmdevapi/mmdevdrv.h b/dlls/mmdevapi/mmdevdrv.h index df21859cbad..004de87f11c 100644 --- a/dlls/mmdevapi/mmdevdrv.h +++ b/dlls/mmdevapi/mmdevdrv.h @@ -60,6 +60,7 @@ struct audio_client { IAudioCaptureClient IAudioCaptureClient_iface; IAudioClock IAudioClock_iface; IAudioClock2 IAudioClock2_iface; + IAudioClockAdjustment IAudioClockAdjustment_iface; IAudioStreamVolume IAudioStreamVolume_iface; LONG ref; diff --git a/dlls/mmdevapi/tests/render.c b/dlls/mmdevapi/tests/render.c index a7da68ec3f1..b0252bedebd 100644 --- a/dlls/mmdevapi/tests/render.c +++ b/dlls/mmdevapi/tests/render.c @@ -136,6 +136,7 @@ static void test_audioclient(void) IAudioClient2 *ac2; IAudioClient3 *ac3; IUnknown *unk; + IAudioClockAdjustment *clk; HRESULT hr; ULONG ref; WAVEFORMATEX *pwfx, *pwfx2; @@ -204,6 +205,14 @@ static void test_audioclient(void) ok(ref == 1, "Released count is %lu\n", ref); } + hr = IAudioClient_QueryInterface(ac, &IID_IAudioClockAdjustment, (void**)&clk); + ok(hr == S_OK, "QueryInterface(IID_IAudioClockAdjustment) returned %08lx\n", hr); + if (clk) + { + ref = IAudioClockAdjustment_Release(clk); + ok(ref == 1, "Released count is %lu\n", ref); + } + hr = IAudioClient_GetDevicePeriod(ac, NULL, NULL); ok(hr == E_POINTER, "Invalid GetDevicePeriod call returns %08lx\n", hr); @@ -600,6 +609,7 @@ static void test_references(void) { IAudioClient *ac; IAudioRenderClient *rc; + IAudioClockAdjustment *clk; ISimpleAudioVolume *sav; IAudioStreamVolume *asv; IAudioClock *acl; @@ -640,6 +650,29 @@ static void test_references(void) ref = IAudioRenderClient_Release(rc); ok(ref == 0, "RenderClient_Release gave wrong refcount: %lu\n", ref); + /* IAudioClockAdjustment */ + hr = IMMDevice_Activate(dev, &IID_IAudioClient, CLSCTX_INPROC_SERVER, + NULL, (void**)&ac); + ok(hr == S_OK, "Activation failed with %08lx\n", hr); + if(hr != S_OK) + return; + + hr = IAudioClient_GetMixFormat(ac, &pwfx); + ok(hr == S_OK, "GetMixFormat failed: %08lx\n", hr); + + hr = IAudioClient_Initialize(ac, AUDCLNT_SHAREMODE_SHARED, 0, 5000000, + 0, pwfx, NULL); + ok(hr == S_OK, "Initialize failed: %08lx\n", hr); + + CoTaskMemFree(pwfx); + + /* contrary to what MSDN says this interface should be obtained via QueryInterface() */ + hr = IAudioClient_GetService(ac, &IID_IAudioClockAdjustment, (void**)&clk); + ok(hr == E_INVALIDARG, "IAudioClient_GetService(IID_IAudioClockAdjustment) returned %08lx\n", hr); + + ref = IAudioClient_Release(ac); + ok(ref == 0, "Client_Release gave wrong refcount: %lu\n", ref); + /* ISimpleAudioVolume */ hr = IMMDevice_Activate(dev, &IID_IAudioClient, CLSCTX_INPROC_SERVER, NULL, (void**)&ac); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5585
From: Arkadiusz Hiler <ahiler(a)codeweavers.com> --- dlls/mmdevapi/client.c | 14 ++++++++++++-- dlls/mmdevapi/unixlib.h | 8 ++++++++ dlls/winealsa.drv/alsa.c | 2 ++ dlls/winecoreaudio.drv/coreaudio.c | 2 ++ dlls/wineoss.drv/oss.c | 2 ++ dlls/winepulse.drv/pulse.c | 2 ++ 6 files changed, 28 insertions(+), 2 deletions(-) diff --git a/dlls/mmdevapi/client.c b/dlls/mmdevapi/client.c index 39027bb25a9..1aaac681d96 100644 --- a/dlls/mmdevapi/client.c +++ b/dlls/mmdevapi/client.c @@ -1178,10 +1178,20 @@ static ULONG WINAPI AudioClockAdjustment_Release(IAudioClockAdjustment *iface) static HRESULT WINAPI AudioClockAdjustment_SetSampleRate(IAudioClockAdjustment *iface, float rate) { ACImpl *This = impl_from_IAudioClockAdjustment(iface); + struct set_sample_rate_params params; - FIXME("(%p)->(%f) stub\n", This, rate); + TRACE("(%p)->(%f)\n", This, rate); - return E_NOTIMPL; + if (!This->stream) + return AUDCLNT_E_NOT_INITIALIZED; + + params.stream = This->stream; + params.rate = rate; + params.result = E_NOTIMPL; + + wine_unix_call(set_sample_rate, ¶ms); + + return params.result; } const IAudioClockAdjustmentVtbl AudioClockAdjustment_Vtbl = diff --git a/dlls/mmdevapi/unixlib.h b/dlls/mmdevapi/unixlib.h index d83ed918a51..3bc9d1269dd 100644 --- a/dlls/mmdevapi/unixlib.h +++ b/dlls/mmdevapi/unixlib.h @@ -216,6 +216,13 @@ struct set_event_handle_params HRESULT result; }; +struct set_sample_rate_params +{ + stream_handle stream; + float rate; + HRESULT result; +}; + struct test_connect_params { const WCHAR *name; @@ -323,6 +330,7 @@ enum unix_funcs get_position, set_volumes, set_event_handle, + set_sample_rate, test_connect, is_started, get_prop_value, diff --git a/dlls/winealsa.drv/alsa.c b/dlls/winealsa.drv/alsa.c index aa1ad4abe6c..919bf2b198a 100644 --- a/dlls/winealsa.drv/alsa.c +++ b/dlls/winealsa.drv/alsa.c @@ -2532,6 +2532,7 @@ const unixlib_entry_t __wine_unix_call_funcs[] = alsa_set_volumes, alsa_set_event_handle, alsa_not_implemented, + alsa_not_implemented, alsa_is_started, alsa_get_prop_value, alsa_not_implemented, @@ -2988,6 +2989,7 @@ const unixlib_entry_t __wine_unix_call_wow64_funcs[] = alsa_wow64_set_volumes, alsa_wow64_set_event_handle, alsa_not_implemented, + alsa_not_implemented, alsa_is_started, alsa_wow64_get_prop_value, alsa_not_implemented, diff --git a/dlls/winecoreaudio.drv/coreaudio.c b/dlls/winecoreaudio.drv/coreaudio.c index a79a0d16839..f7daef1ed27 100644 --- a/dlls/winecoreaudio.drv/coreaudio.c +++ b/dlls/winecoreaudio.drv/coreaudio.c @@ -1879,6 +1879,7 @@ const unixlib_entry_t __wine_unix_call_funcs[] = unix_set_volumes, unix_set_event_handle, unix_not_implemented, + unix_not_implemented, unix_is_started, unix_get_prop_value, unix_midi_init, @@ -2334,6 +2335,7 @@ const unixlib_entry_t __wine_unix_call_wow64_funcs[] = unix_wow64_set_volumes, unix_wow64_set_event_handle, unix_not_implemented, + unix_not_implemented, unix_is_started, unix_wow64_get_prop_value, unix_wow64_midi_init, diff --git a/dlls/wineoss.drv/oss.c b/dlls/wineoss.drv/oss.c index 89e626f39d6..f7396847577 100644 --- a/dlls/wineoss.drv/oss.c +++ b/dlls/wineoss.drv/oss.c @@ -1731,6 +1731,7 @@ const unixlib_entry_t __wine_unix_call_funcs[] = oss_get_position, oss_set_volumes, oss_set_event_handle, + oss_not_implemented, oss_test_connect, oss_is_started, oss_get_prop_value, @@ -2226,6 +2227,7 @@ const unixlib_entry_t __wine_unix_call_wow64_funcs[] = oss_wow64_get_position, oss_wow64_set_volumes, oss_wow64_set_event_handle, + oss_not_implemented, oss_wow64_test_connect, oss_is_started, oss_wow64_get_prop_value, diff --git a/dlls/winepulse.drv/pulse.c b/dlls/winepulse.drv/pulse.c index 62658fc98e6..328b84c7ca4 100644 --- a/dlls/winepulse.drv/pulse.c +++ b/dlls/winepulse.drv/pulse.c @@ -2562,6 +2562,7 @@ const unixlib_entry_t __wine_unix_call_funcs[] = pulse_get_position, pulse_set_volumes, pulse_set_event_handle, + pulse_not_implemented, pulse_test_connect, pulse_is_started, pulse_get_prop_value, @@ -3033,6 +3034,7 @@ const unixlib_entry_t __wine_unix_call_wow64_funcs[] = pulse_wow64_get_position, pulse_wow64_set_volumes, pulse_wow64_set_event_handle, + pulse_not_implemented, pulse_wow64_test_connect, pulse_is_started, pulse_wow64_get_prop_value, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5585
From: Arkadiusz Hiler <ahiler(a)codeweavers.com> --- dlls/winepulse.drv/pulse.c | 41 +++++++++++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/dlls/winepulse.drv/pulse.c b/dlls/winepulse.drv/pulse.c index 328b84c7ca4..6dfa203000f 100644 --- a/dlls/winepulse.drv/pulse.c +++ b/dlls/winepulse.drv/pulse.c @@ -723,7 +723,7 @@ static void pulse_probe_settings(int render, const char *pulse_name, WAVEFORMATE ret = -1; else if (render) ret = pa_stream_connect_playback(stream, pulse_name, &attr, - PA_STREAM_START_CORKED|PA_STREAM_FIX_RATE|PA_STREAM_FIX_CHANNELS|PA_STREAM_EARLY_REQUESTS, NULL, NULL); + PA_STREAM_START_CORKED|PA_STREAM_FIX_RATE|PA_STREAM_FIX_CHANNELS|PA_STREAM_EARLY_REQUESTS|PA_STREAM_VARIABLE_RATE, NULL, NULL); else ret = pa_stream_connect_record(stream, pulse_name, &attr, PA_STREAM_START_CORKED|PA_STREAM_FIX_RATE|PA_STREAM_FIX_CHANNELS|PA_STREAM_EARLY_REQUESTS); if (ret >= 0) { @@ -2438,6 +2438,41 @@ static NTSTATUS pulse_set_event_handle(void *args) return STATUS_SUCCESS; } +static NTSTATUS pulse_set_sample_rate(void *args) +{ + struct set_sample_rate_params *params = args; + struct pulse_stream *stream = handle_get_stream(params->stream); + HRESULT hr = S_OK; + pa_operation *o; + int success; + + pulse_lock(); + if (!pulse_stream_valid(stream)) + hr = AUDCLNT_E_DEVICE_INVALIDATED; + else + { + if (!(o = pa_stream_update_sample_rate(stream->stream, params->rate, pulse_op_cb, &success))) + success = 0; + else + { + while (pa_operation_get_state(o) == PA_OPERATION_RUNNING) + pulse_cond_wait(); + pa_operation_unref(o); + } + + if (!success) hr = E_FAIL; + else + { + stream->ss.rate = params->rate; + stream->period_bytes = pa_frame_size(&stream->ss) * muldiv(stream->mmdev_period_usec, stream->ss.rate, 1000000); + } + } + pulse_unlock(); + + params->result = hr; + return STATUS_SUCCESS; +} + static NTSTATUS pulse_is_started(void *args) { struct is_started_params *params = args; @@ -2562,7 +2597,7 @@ const unixlib_entry_t __wine_unix_call_funcs[] = pulse_get_position, pulse_set_volumes, pulse_set_event_handle, - pulse_not_implemented, + pulse_set_sample_rate, pulse_test_connect, pulse_is_started, pulse_get_prop_value, @@ -3034,7 +3069,7 @@ const unixlib_entry_t __wine_unix_call_wow64_funcs[] = pulse_wow64_get_position, pulse_wow64_set_volumes, pulse_wow64_set_event_handle, - pulse_not_implemented, + pulse_set_sample_rate, pulse_wow64_test_connect, pulse_is_started, pulse_wow64_get_prop_value, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5585
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=145448 Your paranoid android. === w8adm (32 bit report) === mmdevapi: render.c:1409: Test failed: GetBuffer large (20671) at iteration 7 === w1064v1507 (32 bit report) === mmdevapi: render.c:1409: Test failed: GetBuffer large (20671) at iteration 2 render.c:1409: Test failed: GetBuffer large (20671) at iteration 4 === w1064v1507 (64 bit report) === mmdevapi: render.c:1409: Test failed: GetBuffer large (20671) at iteration 4 render.c:1409: Test failed: GetBuffer large (20671) at iteration 6 === w1064_adm (64 bit report) === mmdevapi: render.c:1409: Test failed: GetBuffer large (22500) at iteration 7 render.c:1409: Test failed: GetBuffer large (22500) at iteration 7
This merge request was approved by Arek Hiler. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5585
There are some existing tests that seem to be flaky: ``` on test-win10-21h2-32: render.c:1409: Test failed: GetBuffer large (22500) at iteration 4 render.c:1409: Test failed: GetBuffer large (22500) at iteration 6 render.c:1409: Test failed: GetBuffer large (22500) at iteration 7 2744:render: 2330 tests executed (0 marked as todo, 3 as flaky, 3 failures), 2 skipped. ``` I've also stumbled upon those while testbotting my changes. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5585#note_69919
Huw Davies (@huw) commented about dlls/mmdevapi/client.c:
clock2_GetDevicePosition };
+static HRESULT WINAPI AudioClockAdjustment_QueryInterface(IAudioClockAdjustment *iface, + REFIID riid, void **ppv) +{ + ACImpl *This = impl_from_IAudioClockAdjustment(iface); + return IAudioClock_QueryInterface(&This->IAudioClock_iface, riid, ppv); +} +
This looks a bit odd, given that `IAudioClockAdjustment` can be QI'ed from `IAudioClient`. We need more tests: e.g. can you QI `IAudioClient` from `IAudioClockAdjustment`? Does QI'ing `IUnknown` from `IAudioClockAdjustment` give the same result as QI'ing `IUnknown` from `IAudioClient` (as it should if it's the same object). I'm wondering whether our current `IAudioClock` implementation is correct in this regard too. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5585#note_70614
On Thu May 16 08:50:10 2024 +0000, Huw Davies wrote:
This looks a bit odd, given that `IAudioClockAdjustment` can be QI'ed from `IAudioClient`. We need more tests: e.g. can you QI `IAudioClient` from `IAudioClockAdjustment`? Does QI'ing `IUnknown` from `IAudioClockAdjustment` give the same result as QI'ing `IUnknown` from `IAudioClient` (as it should if it's the same object). I'm wondering whether our current `IAudioClock` implementation is correct in this regard too. `IAudioClock` is a separate thing because we get it via `GetService()`. It's impossible to get it via `QueryInterface()` from `AudioClient`. Some more test + change to `ACA_QueryInterface()` to just use `AudioClient3`'s version incoming :-)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5585#note_70645
participants (5)
-
Andrew Eikum -
Arek Hiler (＠ivyl)
-
Arkadiusz Hiler
-
Huw Davies (＠huw)
-
Marvin