[PATCH 0/4] MR9441: quartz/dsoundrender: A few bits around EOS handling.
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/quartz/dsoundrender.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dlls/quartz/dsoundrender.c b/dlls/quartz/dsoundrender.c index 59609a3c849..3a562669efe 100644 --- a/dlls/quartz/dsoundrender.c +++ b/dlls/quartz/dsoundrender.c @@ -156,7 +156,7 @@ static void update_positions(struct dsound_render *filter, DWORD *seqwritepos, D } static HRESULT get_write_pos(struct dsound_render *filter, - DWORD *ret_writepos, REFERENCE_TIME write_at, DWORD *pfree) + DWORD *ret_writepos, REFERENCE_TIME write_at, DWORD *pfree, REFERENCE_TIME max_fill) { DWORD writepos, min_writepos, playpos; REFERENCE_TIME max_lag = 50 * 10000; @@ -210,7 +210,7 @@ static HRESULT get_write_pos(struct dsound_render *filter, WARN("Delta too big %s/%s, too far ahead.\n", debugstr_time(delta_t), debugstr_time(max_lag)); aheadbytes = pos_from_time(filter, delta_t); WARN("Advancing %lu bytes.\n", aheadbytes); - if (delta_t >= DSoundRenderer_Max_Fill) + if (delta_t >= max_fill) return S_FALSE; *ret_writepos = (min_writepos + aheadbytes) % filter->buf_size; } @@ -219,7 +219,7 @@ end: *pfree = playpos - *ret_writepos; else *pfree = filter->buf_size + playpos - *ret_writepos; - if (time_from_pos(filter, filter->buf_size - *pfree) >= DSoundRenderer_Max_Fill) + if (time_from_pos(filter, filter->buf_size - *pfree) >= max_fill) { TRACE("Blocked: too full %s / %s\n", debugstr_time(time_from_pos(filter, filter->buf_size - *pfree)), debugstr_time(DSoundRenderer_Max_Fill)); @@ -239,7 +239,7 @@ static HRESULT send_sample_data(struct dsound_render *filter, BYTE *buf1, *buf2; if (filter->filter.state == State_Running) - hr = get_write_pos(filter, &writepos, tStart, &free); + hr = get_write_pos(filter, &writepos, tStart, &free, data ? DSoundRenderer_Max_Fill : MAXLONGLONG); else hr = S_FALSE; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9441
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/quartz/tests/dsoundrender.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/dlls/quartz/tests/dsoundrender.c b/dlls/quartz/tests/dsoundrender.c index 322c1a37ce8..f3ae13231db 100644 --- a/dlls/quartz/tests/dsoundrender.c +++ b/dlls/quartz/tests/dsoundrender.c @@ -1120,10 +1120,19 @@ static void test_eos(IPin *pin, IMemInputPin *input, IMediaSeeking *seeking, IMe hr = IMediaControl_Pause(control); ok(hr == S_FALSE, "Got hr %#lx.\n", hr); + ret = check_ec_complete(eventsrc, 0); + ok(!ret, "Got unexpected EC_COMPLETE.\n"); + hr = send_frame(input); ok(hr == S_OK, "Got hr %#lx.\n", hr); + ret = check_ec_complete(eventsrc, 0); + ok(!ret, "Got unexpected EC_COMPLETE.\n"); + hr = IMediaControl_GetState(control, 1000, &state); ok(hr == S_OK, "Got hr %#lx.\n", hr); + ret = check_ec_complete(eventsrc, 0); + ok(!ret, "Got unexpected EC_COMPLETE.\n"); + hr = IMediaControl_Run(control); ok(hr == S_OK, "Got hr %#lx.\n", hr); ret = check_ec_complete(eventsrc, 0); @@ -1150,25 +1159,38 @@ static void test_eos(IPin *pin, IMemInputPin *input, IMediaSeeking *seeking, IMe ok(hr == S_FALSE, "Got hr %#lx.\n", hr); hr = send_frame(input); ok(hr == S_OK, "Got hr %#lx.\n", hr); + ret = check_ec_complete(eventsrc, 0); + ok(!ret, "Got unexpected EC_COMPLETE.\n"); hr = IPin_BeginFlush(pin); ok(hr == S_OK, "Got hr %#lx.\n", hr); + ret = check_ec_complete(eventsrc, 0); + ok(!ret, "Got unexpected EC_COMPLETE.\n"); hr = IPin_EndOfStream(pin); todo_wine ok(hr == S_FALSE, "Got hr %#lx.\n", hr); + ret = check_ec_complete(eventsrc, 100); + todo_wine ok(!ret, "Got unexpected EC_COMPLETE.\n"); hr = IPin_EndFlush(pin); ok(hr == S_OK, "Got hr %#lx.\n", hr); + ret = check_ec_complete(eventsrc, 0); + ok(!ret, "Got unexpected EC_COMPLETE.\n"); hr = IMediaControl_Stop(control); ok(hr == S_OK, "Got hr %#lx.\n", hr); ret = check_ec_complete(eventsrc, 0); - todo_wine ok(!ret, "Got unexpected EC_COMPLETE.\n"); + ok(!ret, "Got unexpected EC_COMPLETE.\n"); /* Test sending EOS and then flushing or stopping. */ hr = IMediaControl_Pause(control); ok(hr == S_FALSE, "Got hr %#lx.\n", hr); + ret = check_ec_complete(eventsrc, 0); + ok(!ret, "Got unexpected EC_COMPLETE.\n"); + hr = send_frame(input); ok(hr == S_OK, "Got hr %#lx.\n", hr); + ret = check_ec_complete(eventsrc, 0); + ok(!ret, "Got unexpected EC_COMPLETE.\n"); hr = IMediaControl_GetState(control, 1000, &state); ok(hr == S_OK, "Got hr %#lx.\n", hr); hr = IMediaControl_Run(control); @@ -1183,11 +1205,18 @@ static void test_eos(IPin *pin, IMemInputPin *input, IMediaSeeking *seeking, IMe hr = IPin_BeginFlush(pin); ok(hr == S_OK, "Got hr %#lx.\n", hr); + ret = check_ec_complete(eventsrc, 0); + ok(!ret, "Got unexpected EC_COMPLETE.\n"); hr = IPin_EndFlush(pin); ok(hr == S_OK, "Got hr %#lx.\n", hr); + ret = check_ec_complete(eventsrc, 0); + ok(!ret, "Got unexpected EC_COMPLETE.\n"); hr = send_frame(input); ok(hr == S_OK, "Got hr %#lx.\n", hr); + ret = check_ec_complete(eventsrc, 0); + ok(!ret, "Got unexpected EC_COMPLETE.\n"); + hr = IPin_EndOfStream(pin); ok(hr == S_OK, "Got hr %#lx.\n", hr); ret = check_ec_complete(eventsrc, 0); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9441
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/quartz/dsoundrender.c | 47 ++++++++++++++++++++------------ dlls/quartz/tests/dsoundrender.c | 8 +++--- 2 files changed, 33 insertions(+), 22 deletions(-) diff --git a/dlls/quartz/dsoundrender.c b/dlls/quartz/dsoundrender.c index 3a562669efe..42d5f5cdd25 100644 --- a/dlls/quartz/dsoundrender.c +++ b/dlls/quartz/dsoundrender.c @@ -64,7 +64,7 @@ struct dsound_render * to immediately unblock the streaming thread. */ HANDLE flush_event; REFERENCE_TIME stream_start; - BOOL eos; + BOOL eos, eos_complete_pending; IDirectSound8 *dsound; LPDIRECTSOUNDBUFFER dsbuffer; @@ -349,6 +349,22 @@ static HRESULT render_sample(struct dsound_render *filter, IMediaSample *pSample return send_sample_data(filter, start, pbSrcStream, cbSrcStream); } +static void complete_eos(struct dsound_render *filter) +{ + IFilterGraph *graph = filter->filter.graph; + IMediaEventSink *event_sink; + + if (filter->filter.state == State_Running && graph + && SUCCEEDED(IFilterGraph_QueryInterface(graph, + &IID_IMediaEventSink, (void **)&event_sink))) + { + IMediaEventSink_Notify(event_sink, EC_COMPLETE, S_OK, + (LONG_PTR)&filter->filter.IBaseFilter_iface); + IMediaEventSink_Release(event_sink); + } + SetEvent(filter->state_event); +} + static DWORD WINAPI render_thread_run(void *arg) { struct dsound_render *filter = arg; @@ -361,14 +377,18 @@ static DWORD WINAPI render_thread_run(void *arg) { IMediaSample *sample; - if (filter->eos) + if (filter->eos && (filter->filter.state != State_Running || !filter->queued_sample_count)) { + BOOL complete_pending; + + complete_pending = filter->eos_complete_pending; + filter->eos_complete_pending = FALSE; LeaveCriticalSection(&filter->render_cs); - TRACE("Got EOS.\n"); /* Clear the buffer. */ send_sample_data(filter, -1, NULL, filter->buf_size); - - TRACE("Render thread exiting.\n"); + if (complete_pending) + complete_eos(filter); + TRACE("Render thread exiting on EOS.\n"); return 0; } @@ -510,24 +530,15 @@ static void dsound_render_sink_disconnect(struct strmbase_sink *iface) static HRESULT dsound_render_sink_eos(struct strmbase_sink *iface) { struct dsound_render *filter = impl_from_strmbase_pin(&iface->pin); - IFilterGraph *graph = filter->filter.graph; - IMediaEventSink *event_sink; + BOOL complete_pending; EnterCriticalSection(&filter->render_cs); filter->eos = TRUE; + filter->eos_complete_pending = complete_pending = (filter->filter.state == State_Running); LeaveCriticalSection(&filter->render_cs); WakeConditionVariable(&filter->render_cv); - - if (filter->filter.state == State_Running && graph - && SUCCEEDED(IFilterGraph_QueryInterface(graph, - &IID_IMediaEventSink, (void **)&event_sink))) - { - IMediaEventSink_Notify(event_sink, EC_COMPLETE, S_OK, - (LONG_PTR)&filter->filter.IBaseFilter_iface); - IMediaEventSink_Release(event_sink); - } - SetEvent(filter->state_event); - + if (!complete_pending) + complete_eos(filter); return S_OK; } diff --git a/dlls/quartz/tests/dsoundrender.c b/dlls/quartz/tests/dsoundrender.c index f3ae13231db..6486a152f32 100644 --- a/dlls/quartz/tests/dsoundrender.c +++ b/dlls/quartz/tests/dsoundrender.c @@ -1141,9 +1141,9 @@ static void test_eos(IPin *pin, IMemInputPin *input, IMediaSeeking *seeking, IMe hr = IPin_EndOfStream(pin); ok(hr == S_OK, "Got hr %#lx.\n", hr); ret = check_ec_complete(eventsrc, 0); - todo_wine ok(!ret, "Got unexpected EC_COMPLETE.\n"); + ok(!ret, "Got unexpected EC_COMPLETE.\n"); ret = check_ec_complete(eventsrc, 2000); - todo_wine ok(ret == 1, "Expected EC_COMPLETE.\n"); + ok(ret == 1, "Expected EC_COMPLETE.\n"); hr = IMediaSeeking_GetCurrentPosition(seeking, &time); ok(hr == 0xdeadbeef, "Got hr %#lx.\n", hr); @@ -1201,7 +1201,7 @@ static void test_eos(IPin *pin, IMemInputPin *input, IMediaSeeking *seeking, IMe hr = IPin_EndOfStream(pin); ok(hr == S_OK, "Got hr %#lx.\n", hr); ret = check_ec_complete(eventsrc, 0); - todo_wine ok(!ret, "Got unexpected EC_COMPLETE.\n"); + ok(!ret, "Got unexpected EC_COMPLETE.\n"); hr = IPin_BeginFlush(pin); ok(hr == S_OK, "Got hr %#lx.\n", hr); @@ -1210,7 +1210,7 @@ static void test_eos(IPin *pin, IMemInputPin *input, IMediaSeeking *seeking, IMe hr = IPin_EndFlush(pin); ok(hr == S_OK, "Got hr %#lx.\n", hr); ret = check_ec_complete(eventsrc, 0); - ok(!ret, "Got unexpected EC_COMPLETE.\n"); + todo_wine ok(!ret, "Got unexpected EC_COMPLETE.\n"); hr = send_frame(input); ok(hr == S_OK, "Got hr %#lx.\n", hr); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9441
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/quartz/dsoundrender.c | 3 ++- dlls/quartz/tests/dsoundrender.c | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/dlls/quartz/dsoundrender.c b/dlls/quartz/dsoundrender.c index 42d5f5cdd25..9720fd974a4 100644 --- a/dlls/quartz/dsoundrender.c +++ b/dlls/quartz/dsoundrender.c @@ -381,7 +381,7 @@ static DWORD WINAPI render_thread_run(void *arg) { BOOL complete_pending; - complete_pending = filter->eos_complete_pending; + complete_pending = WaitForSingleObject(filter->flush_event, 0) && filter->eos_complete_pending; filter->eos_complete_pending = FALSE; LeaveCriticalSection(&filter->render_cs); /* Clear the buffer. */ @@ -555,6 +555,7 @@ static HRESULT dsound_render_sink_end_flush(struct strmbase_sink *iface) struct dsound_render *filter = impl_from_strmbase_pin(&iface->pin); EnterCriticalSection(&filter->filter.stream_cs); + filter->eos_complete_pending = FALSE; if (filter->eos && filter->filter.state != State_Stopped) { WaitForSingleObject(filter->render_thread, INFINITE); diff --git a/dlls/quartz/tests/dsoundrender.c b/dlls/quartz/tests/dsoundrender.c index 6486a152f32..b036050ff69 100644 --- a/dlls/quartz/tests/dsoundrender.c +++ b/dlls/quartz/tests/dsoundrender.c @@ -1168,8 +1168,8 @@ static void test_eos(IPin *pin, IMemInputPin *input, IMediaSeeking *seeking, IMe ok(!ret, "Got unexpected EC_COMPLETE.\n"); hr = IPin_EndOfStream(pin); todo_wine ok(hr == S_FALSE, "Got hr %#lx.\n", hr); - ret = check_ec_complete(eventsrc, 100); - todo_wine ok(!ret, "Got unexpected EC_COMPLETE.\n"); + ret = check_ec_complete(eventsrc, 0); + ok(!ret, "Got unexpected EC_COMPLETE.\n"); hr = IPin_EndFlush(pin); ok(hr == S_OK, "Got hr %#lx.\n", hr); ret = check_ec_complete(eventsrc, 0); @@ -1210,7 +1210,7 @@ static void test_eos(IPin *pin, IMemInputPin *input, IMediaSeeking *seeking, IMe hr = IPin_EndFlush(pin); ok(hr == S_OK, "Got hr %#lx.\n", hr); ret = check_ec_complete(eventsrc, 0); - todo_wine ok(!ret, "Got unexpected EC_COMPLETE.\n"); + ok(!ret, "Got unexpected EC_COMPLETE.\n"); hr = send_frame(input); ok(hr == S_OK, "Got hr %#lx.\n", hr); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9441
That (specifically, "Send all queued samples to dsound before issuing EC_COMPLETE.") fixes Rigid Force Alpha ending voice lines early (about ~0.2s early but that can be clearly heard during initial cutscene). Each voice line is an mp3 file played through direct show with dsound renderer. What happens is that upon receiving EOS 'render_thread_run' may still have queued buffers, as well as the data to be played in the dsound buffer (~150ms). When the game receives EC_COMPLETE event it immediately stops the graph (while rendering thread is also not going to play queued samples anyway). So the right thing to do seems to wait for the currently in-flight sound to be played. A pre-requisite problem I found on the way is what is addressed in the first patch ("Do not limit maximum fill when clearing the buffer with zeroes."). The overall dsound buffer is 1sec long. Regardless of my changes 'send_sample_data(filter, -1, NULL, filter->buf_size);' in render_thread_run() is currently waiting about 1s always. That's because of maximum fill check in get_write_pos() which, once 150ms of zeroes are filled into the buffer, will wait until those are played before moving further. While what we probably want to do here (especially with my patch addressing EC_COMPLETE event timing) is to only wait for the previously submitted data not yet played. That is achieved with the first patch, it will now wait only for the amount of data previously present in the buffer. The second tests patch (Check for EC_COMPLETE more often in test_eos()) helps to interpret test results, currently 'todo_wine' appear after later calls while EC_COMPLETE was sent from earlier calls. The added todo with "Send all queued samples to dsound before issuing EC_COMPLETE." is not really added, that is essentially pre-existing extra EC_COMPLETE (removed todo before that) which is arriving later after the patch. All the remaining EC_COMPLETE-related todos are fixed in the last patch (which is a separate aspect of handling flush, which is not exactly related to the other parts). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9441#note_121761
So the right thing to do seems to wait for the currently in-flight sound to be played. A pre-requisite problem I found on the way is what is addressed in the first patch ("Do not limit maximum fill when clearing the buffer with zeroes."). The overall dsound buffer is 1sec long. Regardless of my changes 'send_sample_data(filter, -1, NULL, filter-\>buf_size);' in render_thread_run() is currently waiting about 1s always. That's because of maximum fill check in get_write_pos() which, once 150ms of zeroes are filled into the buffer, will wait until those are played before moving further. While what we probably want to do here (especially with my patch addressing EC_COMPLETE event timing) is to only wait for the previously submitted data not yet played. That is achieved with the first patch, it will now wait only for the amount of data previously present in the buffer.
I'm not thinking through things as clearly as I'd like (and dsoundrender really demands it), but as long as we're not overwriting old data (and I think we aren't) then I don't think there's a reason to ever have that limit? I think we can just remove it entirely.
The second tests patch (Check for EC_COMPLETE more often in test_eos()) helps to interpret test results, currently 'todo_wine' appear after later calls while EC_COMPLETE was sent from earlier calls. The added todo with "Send all queued samples to dsound before issuing EC_COMPLETE." is not really added, that is essentially pre-existing extra EC_COMPLETE (removed todo before that) which is arriving later after the patch. All the remaining EC_COMPLETE-related todos are fixed in the last patch (which is a separate aspect of handling flush, which is not exactly related to the other parts).
Fine, although some of those really are redundant, but eh, it's not like they're doing any harm. 3/4: ``` @@ -361,14 +377,18 @@ static DWORD WINAPI render_thread_run(void *arg) { IMediaSample *sample; - if (filter->eos) + if (filter->eos && (filter->filter.state != State_Running || !filter->queued_sample_count)) { + BOOL complete_pending; + + complete_pending = filter->eos_complete_pending; + filter->eos_complete_pending = FALSE; LeaveCriticalSection(&filter->render_cs); - TRACE("Got EOS.\n"); /* Clear the buffer. */ send_sample_data(filter, -1, NULL, filter->buf_size); - - TRACE("Render thread exiting.\n"); + if (complete_pending) + complete_eos(filter); + TRACE("Render thread exiting on EOS.\n"); return 0; } ``` So this is actually kind of two changes: (1) Don't clear the buffer until we've played all the queued samples. That's fine, except I don't think we want the exception for paused state? (2) Delay complete_eos(). This is... yeah, as you mentioned, the right time, at least after 1/4. It's not the most declarative way to do it—we really should be waiting for the right playpos—but I'll accept it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9441#note_122578
but as long as we're not overwriting old data (and I think we aren't) then I don't think there's a reason to ever have that limit? I think we can just remove it entirely.
This way, without changing some other things, we will be queuing up to 1sec of data. I am not sure if that will work very well with flush or setting stream position, as it will have to play the buffer through until it can do anything else (that's probably even regardless of my changes). Maybe we should then first reduce the buffer size to something like 40ms from 1sec? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9441#note_122609
On Mon Nov 17 20:41:22 2025 +0000, Paul Gofman wrote:
but as long as we're not overwriting old data (and I think we aren't) then I don't think there's a reason to ever have that limit? I think we can just remove it entirely. This way, without changing some other things, we will be queuing up to 1sec of data. I am not sure if that will work very well with flush or setting stream position, as it will have to play the buffer through until it can do anything else (that's probably even regardless of my changes). Maybe we should then first reduce the buffer size to something like 40ms from 1sec? Why would it be a problem with flushing? When we get a flush we immediately wipe the buffer, including data that is queued but hasn't been played yet. We don't wait for it, and shouldn't; the point of a flush is to discard everything as fast as possible.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9441#note_122612
On Mon Nov 17 22:00:23 2025 +0000, Elizabeth Figura wrote:
Why would it be a problem with flushing? When we get a flush we immediately wipe the buffer, including data that is queued but hasn't been played yet. We don't wait for it, and shouldn't; the point of a flush is to discard everything as fast as possible. The only place where send_sample_data() explicitly waits now in the running state is when get_write_pos() returns S_FALSE and that only happens due to DSoundRenderer_Max_Fill check. Then, that wait is 10ms wait of flush_event. Removing DSoundRenderer_Max_Fill will make setting that event a dead code (there is some handling in my later patch but that is a different aspect). It seems to me doing exactly that, just dropping that limit, makes send_sample_data() busy-loop waiting for output dsound buffer to get some space when full and this process is not aborted with flush_event? Unless I am missing something we can't just drop that logic without replacing it with something else entirely.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9441#note_122671
On Tue Nov 18 00:26:48 2025 +0000, Paul Gofman wrote:
The only place where send_sample_data() explicitly waits now in the running state is when get_write_pos() returns S_FALSE and that only happens due to DSoundRenderer_Max_Fill check. Then, that wait is 10ms wait of flush_event. Removing DSoundRenderer_Max_Fill will make setting that event a dead code (there is some handling in my later patch but that is a different aspect). It seems to me doing exactly that, just dropping that limit, makes send_sample_data() busy-loop waiting for output dsound buffer to get some space when full and this process is not aborted with flush_event? Unless I am missing something we can't just drop that logic without replacing it with something else entirely. I think this will probably work if send_sample_data(), with dropped DSoundRenderer_Max_Fill will be waiting for 10ms on flush_event when either 'free' or locked buffer size is 0 (instead of depending on get_write_pos status which can be made void in this case).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9441#note_122678
I think this will probably work if send_sample_data(), with dropped DSoundRenderer_Max_Fill will be waiting for 10ms on flush_event when either 'free' or locked buffer size is 0 (instead of depending on get_write_pos status which can be made void in this case).
Right, yeah. Sorry I didn't see that immediately. I think that's also a problem with 1/4 here anyway. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9441#note_122688
On Tue Nov 18 03:21:07 2025 +0000, Elizabeth Figura wrote:
I think this will probably work if send_sample_data(), with dropped DSoundRenderer_Max_Fill will be waiting for 10ms on flush_event when either 'free' or locked buffer size is 0 (instead of depending on get_write_pos status which can be made void in this case). Right, yeah. Sorry I didn't see that immediately. I think that's also a problem with 1/4 here anyway. Yeah thanks, I will update this way tomorrow.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9441#note_122692
participants (3)
-
Elizabeth Figura (＠zfigura) -
Paul Gofman
-
Paul Gofman (＠gofman)