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`.
-- v4: winepulse.drv: Implement set_sample_rate. mmdevapi: Implement AudioClockAdjustment_SetSampleRate. mmdevapi: Add stub IAudioClockAdjustment implementation. mmdevapi/tests: Add more IAudioClock tests.
From: Arkadiusz Hiler ahiler@codeweavers.com
--- dlls/mmdevapi/tests/render.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/dlls/mmdevapi/tests/render.c b/dlls/mmdevapi/tests/render.c index b8ef4b5a549..dac047b98ab 100644 --- a/dlls/mmdevapi/tests/render.c +++ b/dlls/mmdevapi/tests/render.c @@ -135,6 +135,7 @@ static void test_audioclient(void) IAudioClient *ac; IAudioClient2 *ac2; IAudioClient3 *ac3; + IAudioClock *acl; IUnknown *unk; HRESULT hr; ULONG ref; @@ -203,6 +204,8 @@ static void test_audioclient(void) ref = IUnknown_Release(unk); ok(ref == 1, "Released count is %lu\n", ref); } + hr = IAudioClient_QueryInterface(ac, &IID_IAudioClock, (void**)&acl); + ok(hr == E_NOINTERFACE, "QueryInterface(IID_IAudioClock) returned %08lx\n", hr);
hr = IAudioClient_GetDevicePeriod(ac, NULL, NULL); ok(hr == E_POINTER, "Invalid GetDevicePeriod call returns %08lx\n", hr); @@ -598,7 +601,7 @@ static void test_formats(AUDCLNT_SHAREMODE mode)
static void test_references(void) { - IAudioClient *ac; + IAudioClient *ac, *ac2; IAudioRenderClient *rc; ISimpleAudioVolume *sav; IAudioStreamVolume *asv; @@ -692,6 +695,9 @@ static void test_references(void) ref = IAudioClock_Release(acl); ok(ref != 0, "AudioClock_Release gave wrong refcount: %lu\n", ref);
+ hr = IAudioClock_QueryInterface(acl, &IID_IAudioClient, (void**)&ac2); + ok(hr == E_NOINTERFACE, "QueryInterface(IID_IAudioClient) returned %08lx\n", hr); + ref = IAudioClient_Release(ac); ok(ref != 0, "Client_Release gave wrong refcount: %lu\n", ref);
From: Andrew Eikum aeikum@codeweavers.com
with tests by Arkadiusz Hiler. --- dlls/mmdevapi/client.c | 61 ++++++++++++++++++++++++++++++++---- dlls/mmdevapi/mmdevdrv.h | 1 + dlls/mmdevapi/tests/render.c | 35 +++++++++++++++++++++ 3 files changed, 91 insertions(+), 6 deletions(-)
diff --git a/dlls/mmdevapi/client.c b/dlls/mmdevapi/client.c index 860c31f4b49..810311dcf48 100644 --- a/dlls/mmdevapi/client.c +++ b/dlls/mmdevapi/client.c @@ -92,6 +92,11 @@ static inline struct audio_client *impl_from_IAudioClock2(IAudioClock2 *iface) return CONTAINING_RECORD(iface, struct audio_client, IAudioClock2_iface); }
+static inline struct audio_client *impl_from_IAudioClockAdjustment(IAudioClockAdjustment *iface) +{ + return CONTAINING_RECORD(iface, struct audio_client, IAudioClockAdjustment_iface); +} + static inline struct audio_client *impl_from_IAudioRenderClient(IAudioRenderClient *iface) { return CONTAINING_RECORD(iface, struct audio_client, IAudioRenderClient_iface); @@ -607,6 +612,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) @@ -617,6 +624,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); @@ -966,6 +975,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; @@ -1273,6 +1285,42 @@ const IAudioClock2Vtbl AudioClock2_Vtbl = clock2_GetDevicePosition };
+static HRESULT WINAPI AudioClockAdjustment_QueryInterface(IAudioClockAdjustment *iface, + REFIID riid, void **ppv) +{ + struct audio_client *This = impl_from_IAudioClockAdjustment(iface); + return IAudioClient3_QueryInterface(&This->IAudioClient3_iface, riid, ppv); +} + +static ULONG WINAPI AudioClockAdjustment_AddRef(IAudioClockAdjustment *iface) +{ + struct audio_client *This = impl_from_IAudioClockAdjustment(iface); + return IAudioClient3_AddRef(&This->IAudioClient3_iface); +} + +static ULONG WINAPI AudioClockAdjustment_Release(IAudioClockAdjustment *iface) +{ + struct audio_client *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); @@ -1552,12 +1600,13 @@ HRESULT AudioClient_Create(GUID *guid, IMMDevice *device, IAudioClient **out)
This->device_name = name;
- This->IAudioCaptureClient_iface.lpVtbl = &AudioCaptureClient_Vtbl; - This->IAudioClient3_iface.lpVtbl = &AudioClient3_Vtbl; - This->IAudioClock_iface.lpVtbl = &AudioClock_Vtbl; - This->IAudioClock2_iface.lpVtbl = &AudioClock2_Vtbl; - This->IAudioRenderClient_iface.lpVtbl = &AudioRenderClient_Vtbl; - This->IAudioStreamVolume_iface.lpVtbl = &AudioStreamVolume_Vtbl; + This->IAudioCaptureClient_iface.lpVtbl = &AudioCaptureClient_Vtbl; + 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;
This->dataflow = dataflow; This->parent = device; diff --git a/dlls/mmdevapi/mmdevdrv.h b/dlls/mmdevapi/mmdevdrv.h index 7683867a9c1..a580043f577 100644 --- a/dlls/mmdevapi/mmdevdrv.h +++ b/dlls/mmdevapi/mmdevdrv.h @@ -62,6 +62,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 dac047b98ab..dfbd3821cdd 100644 --- a/dlls/mmdevapi/tests/render.c +++ b/dlls/mmdevapi/tests/render.c @@ -137,6 +137,7 @@ static void test_audioclient(void) IAudioClient3 *ac3; IAudioClock *acl; IUnknown *unk; + IAudioClockAdjustment *aca; HRESULT hr; ULONG ref; WAVEFORMATEX *pwfx, *pwfx2; @@ -207,6 +208,16 @@ static void test_audioclient(void) hr = IAudioClient_QueryInterface(ac, &IID_IAudioClock, (void**)&acl); ok(hr == E_NOINTERFACE, "QueryInterface(IID_IAudioClock) returned %08lx\n", hr);
+ hr = IAudioClient_QueryInterface(ac, &IID_IAudioClockAdjustment, (void**)&aca); + ok(hr == S_OK, "QueryInterface(IID_IAudioClockAdjustment) returned %08lx\n", hr); + if (aca) + { + hr = IAudioClockAdjustment_QueryInterface(aca, &IID_IAudioClock, (void**)&acl); + ok(hr == E_NOINTERFACE, "QueryInterface(IID_IAudioClock) returned %08lx\n", hr); + ref = IAudioClockAdjustment_Release(aca); + 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);
@@ -603,6 +614,7 @@ static void test_references(void) { IAudioClient *ac, *ac2; IAudioRenderClient *rc; + IAudioClockAdjustment *aca; ISimpleAudioVolume *sav; IAudioStreamVolume *asv; IAudioClock *acl; @@ -643,6 +655,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**)&aca); + 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);
From: Arkadiusz Hiler ahiler@codeweavers.com
--- dlls/mmdevapi/client.c | 16 +++++++++++++--- 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, 29 insertions(+), 3 deletions(-)
diff --git a/dlls/mmdevapi/client.c b/dlls/mmdevapi/client.c index 810311dcf48..8c0fa0a4c39 100644 --- a/dlls/mmdevapi/client.c +++ b/dlls/mmdevapi/client.c @@ -1306,11 +1306,21 @@ static ULONG WINAPI AudioClockAdjustment_Release(IAudioClockAdjustment *iface)
static HRESULT WINAPI AudioClockAdjustment_SetSampleRate(IAudioClockAdjustment *iface, float rate) { - ACImpl *This = impl_from_IAudioClockAdjustment(iface); + struct audio_client *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 4cb4c881bcf..097b129f564 100644 --- a/dlls/mmdevapi/unixlib.h +++ b/dlls/mmdevapi/unixlib.h @@ -225,6 +225,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; @@ -333,6 +340,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 970db59ad12..046b447aafd 100644 --- a/dlls/winealsa.drv/alsa.c +++ b/dlls/winealsa.drv/alsa.c @@ -2503,6 +2503,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, @@ -2960,6 +2961,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 db5e1116bf8..377f3f5f035 100644 --- a/dlls/winecoreaudio.drv/coreaudio.c +++ b/dlls/winecoreaudio.drv/coreaudio.c @@ -1849,6 +1849,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, @@ -2305,6 +2306,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 a017d242a98..cac8e272c8c 100644 --- a/dlls/wineoss.drv/oss.c +++ b/dlls/wineoss.drv/oss.c @@ -1702,6 +1702,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, @@ -2198,6 +2199,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 263d0f1a45a..26b44e79f0c 100644 --- a/dlls/winepulse.drv/pulse.c +++ b/dlls/winepulse.drv/pulse.c @@ -2611,6 +2611,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, @@ -3108,6 +3109,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,
From: Arkadiusz Hiler ahiler@codeweavers.com
--- dlls/winepulse.drv/pulse.c | 65 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 62 insertions(+), 3 deletions(-)
diff --git a/dlls/winepulse.drv/pulse.c b/dlls/winepulse.drv/pulse.c index 26b44e79f0c..d1cb238b7f7 100644 --- a/dlls/winepulse.drv/pulse.c +++ b/dlls/winepulse.drv/pulse.c @@ -69,6 +69,7 @@ struct pulse_stream float vol[PA_CHANNELS_MAX];
REFERENCE_TIME def_period; + REFERENCE_TIME duration;
INT32 locked; BOOL started; @@ -1073,7 +1074,7 @@ static HRESULT pulse_stream_connect(struct pulse_stream *stream, const char *pul pulse_name = NULL; /* use default */
if (stream->dataflow == eRender) - ret = pa_stream_connect_playback(stream->stream, pulse_name, &attr, flags, NULL, NULL); + ret = pa_stream_connect_playback(stream->stream, pulse_name, &attr, flags|PA_STREAM_VARIABLE_RATE, NULL, NULL); else ret = pa_stream_connect_record(stream->stream, pulse_name, &attr, flags); if (ret < 0) { @@ -1158,6 +1159,7 @@ static NTSTATUS pulse_create_stream(void *args) goto exit;
stream->def_period = params->period; + stream->duration = params->duration;
stream->period_bytes = pa_frame_size(&stream->ss) * muldiv(params->period, stream->ss.rate, @@ -2486,6 +2488,63 @@ 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; + int success; + SIZE_T size, new_bufsize_frames; + BYTE *new_buffer = NULL; + pa_sample_spec new_ss; + + pulse_lock(); + if (!pulse_stream_valid(stream)) + hr = AUDCLNT_E_DEVICE_INVALIDATED; + else if (stream->dataflow != eRender) + hr = E_NOTIMPL; + else { + new_ss = stream->ss; + new_ss.rate = params->rate; + new_bufsize_frames = ceil((stream->duration / 10000000.) * new_ss.rate); + size = new_bufsize_frames * 2 * pa_frame_size(&stream->ss); + + if (NtAllocateVirtualMemory(GetCurrentProcess(), (void **)&new_buffer, zero_bits, &size, MEM_COMMIT, PAGE_READWRITE)) + hr = E_OUTOFMEMORY; + else { + if (!wait_pa_operation_complete(pa_stream_update_sample_rate(stream->stream, params->rate, pulse_op_cb, &success))) + success = 0; + + if (!success) { + hr = E_OUTOFMEMORY; + size = 0; + NtFreeVirtualMemory(GetCurrentProcess(), (void **)&new_buffer, &size, MEM_RELEASE); + } else { + if (stream->held_bytes) + wait_pa_operation_complete(pa_stream_flush(stream->stream, pulse_op_cb, &success)); + + stream->clock_lastpos = stream->clock_written = 0; + stream->pa_offs_bytes = stream->lcl_offs_bytes = 0; + stream->held_bytes = stream->pa_held_bytes = 0; + stream->period_bytes = pa_frame_size(&new_ss) * muldiv(stream->mmdev_period_usec, new_ss.rate, 1000000); + stream->real_bufsize_bytes = size; + stream->bufsize_frames = new_bufsize_frames; + stream->ss = new_ss; + + size = 0; + NtFreeVirtualMemory(GetCurrentProcess(), (void **)&stream->local_buffer, &size, MEM_RELEASE); + + silence_buffer(new_ss.format, new_buffer, size); + stream->local_buffer = new_buffer; + } + } + } + pulse_unlock(); + + params->result = hr; + return STATUS_SUCCESS; +} + static NTSTATUS pulse_is_started(void *args) { struct is_started_params *params = args; @@ -2611,7 +2670,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, @@ -3109,7 +3168,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,
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=146879
Your paranoid android.
=== w8 (32 bit report) ===
mmdevapi: render.c:1417: Test failed: GetBuffer large (20671) at iteration 1 render.c:1417: Test failed: GetBuffer large (20671) at iteration 3
=== w8adm (32 bit report) ===
mmdevapi: render.c:1417: Test failed: GetBuffer large (20671) at iteration 8
=== w1064v1507 (32 bit report) ===
mmdevapi: render.c:1417: Test failed: GetBuffer large (20671) at iteration 5 render.c:1417: Test failed: GetBuffer large (20671) at iteration 8
=== w10pro64 (32 bit report) ===
mmdevapi: render.c:1417: Test failed: GetBuffer large (22500) at iteration 2 render.c:1417: Test failed: GetBuffer large (22500) at iteration 4
=== w10pro64 (64 bit report) ===
mmdevapi: render.c:1417: Test failed: GetBuffer large (22500) at iteration 2
=== debian11b (64 bit WoW report) ===
user32: win.c:4037: Test failed: Expected active window 0000000001C50160, got 0000000003E60178. win.c:4038: Test failed: Expected focus window 0000000001C50160, got 0000000003E60178.
-
Andrew Eikum -
Arek Hiler (＠ivyl)
-
Arkadiusz Hiler
-
Marvin