[PATCH 0/7] MR695: winegstreamer: Initial fixes and refactor of the WM async reader.
This includes some fixes for obvious mistakes as well as initial cleanup of the WM async reader, in preparation for implementing proper threading support. I'm planning on doing some more refactoring first, ultimately implementing the async reader on top of the sync reader, and removing the need for a private interface. I've checked that it can indeed work on Windows too, and it'll make the code simpler overall. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/695
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wm_asyncreader.c | 97 +++++++++++++++-------------- 1 file changed, 49 insertions(+), 48 deletions(-) diff --git a/dlls/winegstreamer/wm_asyncreader.c b/dlls/winegstreamer/wm_asyncreader.c index be9f881b9c2..92c6ba88392 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -50,9 +50,10 @@ struct async_reader void *context; LARGE_INTEGER clock_frequency; - HANDLE stream_thread; - CRITICAL_SECTION stream_cs; - CONDITION_VARIABLE stream_cv; + + HANDLE callback_thread; + CRITICAL_SECTION callback_cs; + CONDITION_VARIABLE callback_cv; bool running; struct list async_ops; @@ -69,7 +70,7 @@ static REFERENCE_TIME get_current_time(const struct async_reader *reader) return (time.QuadPart * 1000) / reader->clock_frequency.QuadPart * 10000; } -static void stream_thread_running(struct async_reader *reader, IWMReaderCallbackAdvanced *callback_advanced) +static void callback_thread_run(struct async_reader *reader, IWMReaderCallbackAdvanced *callback_advanced) { IWMReaderCallback *callback = reader->callback; REFERENCE_TIME start_time; @@ -85,9 +86,9 @@ static void stream_thread_running(struct async_reader *reader, IWMReaderCallback while (reader->running && list_empty(&reader->async_ops)) { - LeaveCriticalSection(&reader->stream_cs); + LeaveCriticalSection(&reader->callback_cs); hr = wm_reader_get_stream_sample(&reader->reader, callback_advanced, 0, &sample, &pts, &duration, &flags, &stream_number); - EnterCriticalSection(&reader->stream_cs); + EnterCriticalSection(&reader->callback_cs); if (hr != S_OK) break; @@ -99,13 +100,13 @@ static void stream_thread_running(struct async_reader *reader, IWMReaderCallback if (pts > user_time && callback_advanced) { - LeaveCriticalSection(&reader->stream_cs); + LeaveCriticalSection(&reader->callback_cs); IWMReaderCallbackAdvanced_OnTime(callback_advanced, user_time, reader->context); - EnterCriticalSection(&reader->stream_cs); + EnterCriticalSection(&reader->callback_cs); } while (pts > reader->user_time && reader->running && list_empty(&reader->async_ops)) - SleepConditionVariableCS(&reader->stream_cv, &reader->stream_cs, INFINITE); + SleepConditionVariableCS(&reader->callback_cv, &reader->callback_cs, INFINITE); } else { @@ -116,21 +117,21 @@ static void stream_thread_running(struct async_reader *reader, IWMReaderCallback if (pts <= current_time - start_time) break; - SleepConditionVariableCS(&reader->stream_cv, &reader->stream_cs, + SleepConditionVariableCS(&reader->callback_cv, &reader->callback_cs, (pts - (current_time - start_time)) / 10000); } } if (reader->running && list_empty(&reader->async_ops)) { - LeaveCriticalSection(&reader->stream_cs); + LeaveCriticalSection(&reader->callback_cs); if (stream->read_compressed) hr = IWMReaderCallbackAdvanced_OnStreamSample(callback_advanced, stream_number, pts, duration, flags, sample, reader->context); else hr = IWMReaderCallback_OnSample(callback, stream_number - 1, pts, duration, flags, sample, reader->context); - EnterCriticalSection(&reader->stream_cs); + EnterCriticalSection(&reader->callback_cs); TRACE("Callback returned %#lx.\n", hr); } @@ -143,7 +144,7 @@ static void stream_thread_running(struct async_reader *reader, IWMReaderCallback BOOL user_clock = reader->user_clock; QWORD user_time = reader->user_time; - LeaveCriticalSection(&reader->stream_cs); + LeaveCriticalSection(&reader->callback_cs); IWMReaderCallback_OnStatus(callback, WMT_END_OF_STREAMING, S_OK, WMT_TYPE_DWORD, (BYTE *)&zero, reader->context); @@ -159,7 +160,7 @@ static void stream_thread_running(struct async_reader *reader, IWMReaderCallback user_time, reader->context); } - EnterCriticalSection(&reader->stream_cs); + EnterCriticalSection(&reader->callback_cs); TRACE("Reached end of stream; exiting.\n"); } @@ -169,7 +170,7 @@ static void stream_thread_running(struct async_reader *reader, IWMReaderCallback } } -static DWORD WINAPI stream_thread(void *arg) +static DWORD WINAPI async_reader_callback_thread(void *arg) { struct async_reader *reader = arg; IWMReaderCallback *callback = reader->callback; @@ -186,7 +187,7 @@ static DWORD WINAPI stream_thread(void *arg) IWMReaderCallback_OnStatus(reader->callback, WMT_OPENED, S_OK, WMT_TYPE_DWORD, (BYTE *)&zero, reader->context); - EnterCriticalSection(&reader->stream_cs); + EnterCriticalSection(&reader->callback_cs); while (reader->running) { @@ -202,28 +203,28 @@ static DWORD WINAPI stream_thread(void *arg) { case ASYNC_OP_START: { - LeaveCriticalSection(&reader->stream_cs); + LeaveCriticalSection(&reader->callback_cs); IWMReaderCallback_OnStatus(reader->callback, WMT_STARTED, hr, WMT_TYPE_DWORD, (BYTE *)&zero, reader->context); - EnterCriticalSection(&reader->stream_cs); + EnterCriticalSection(&reader->callback_cs); if (SUCCEEDED(hr)) - stream_thread_running(reader, callback_advanced); + callback_thread_run(reader, callback_advanced); break; } case ASYNC_OP_STOP: - LeaveCriticalSection(&reader->stream_cs); + LeaveCriticalSection(&reader->callback_cs); IWMReaderCallback_OnStatus(reader->callback, WMT_STOPPED, hr, WMT_TYPE_DWORD, (BYTE *)&zero, reader->context); - EnterCriticalSection(&reader->stream_cs); + EnterCriticalSection(&reader->callback_cs); break; case ASYNC_OP_CLOSE: - LeaveCriticalSection(&reader->stream_cs); + LeaveCriticalSection(&reader->callback_cs); IWMReaderCallback_OnStatus(reader->callback, WMT_CLOSED, hr, WMT_TYPE_DWORD, (BYTE *)&zero, reader->context); - EnterCriticalSection(&reader->stream_cs); + EnterCriticalSection(&reader->callback_cs); if (SUCCEEDED(hr)) reader->running = false; @@ -234,13 +235,13 @@ static DWORD WINAPI stream_thread(void *arg) } if (reader->running && list_empty(&reader->async_ops)) - SleepConditionVariableCS(&reader->stream_cv, &reader->stream_cs, INFINITE); + SleepConditionVariableCS(&reader->callback_cv, &reader->callback_cs, INFINITE); } if (callback_advanced) IWMReaderCallbackAdvanced_Release(callback_advanced); - LeaveCriticalSection(&reader->stream_cs); + LeaveCriticalSection(&reader->callback_cs); TRACE("Reader is stopping; exiting.\n"); return 0; @@ -252,7 +253,7 @@ static HRESULT open_stream(struct async_reader *reader, IWMReaderCallback *callb reader->context = context; reader->running = true; - if (!(reader->stream_thread = CreateThread(NULL, 0, stream_thread, reader, 0, NULL))) + if (!(reader->callback_thread = CreateThread(NULL, 0, async_reader_callback_thread, reader, 0, NULL))) { IWMReaderCallback_Release(reader->callback); reader->running = false; @@ -266,11 +267,11 @@ static HRESULT open_stream(struct async_reader *reader, IWMReaderCallback *callb static void close_stream(struct async_reader *reader) { - if (reader->stream_thread) + if (reader->callback_thread) { - WaitForSingleObject(reader->stream_thread, INFINITE); - CloseHandle(reader->stream_thread); - reader->stream_thread = NULL; + WaitForSingleObject(reader->callback_thread, INFINITE); + CloseHandle(reader->callback_thread); + reader->callback_thread = NULL; } } @@ -278,7 +279,7 @@ static HRESULT async_reader_queue_op(struct async_reader *reader, enum async_op_ { struct async_op *op; - EnterCriticalSection(&reader->stream_cs); + EnterCriticalSection(&reader->callback_cs); if (!(op = calloc(1, sizeof(*op)))) return E_OUTOFMEMORY; @@ -287,8 +288,8 @@ static HRESULT async_reader_queue_op(struct async_reader *reader, enum async_op_ list_add_tail(&reader->async_ops, &op->entry); - LeaveCriticalSection(&reader->stream_cs); - WakeConditionVariable(&reader->stream_cv); + LeaveCriticalSection(&reader->callback_cs); + WakeConditionVariable(&reader->callback_cv); return S_OK; } @@ -430,7 +431,7 @@ static HRESULT WINAPI WMReader_Start(IWMReader *iface, EnterCriticalSection(&reader->reader.cs); - if (!reader->stream_thread) + if (!reader->callback_thread) hr = NS_E_INVALID_REQUEST; else { @@ -452,7 +453,7 @@ static HRESULT WINAPI WMReader_Stop(IWMReader *iface) EnterCriticalSection(&reader->reader.cs); - if (!reader->stream_thread) + if (!reader->callback_thread) hr = E_UNEXPECTED; else hr = async_reader_queue_op(reader, ASYNC_OP_STOP, NULL); @@ -522,9 +523,9 @@ static HRESULT WINAPI WMReaderAdvanced_SetUserProvidedClock(IWMReaderAdvanced6 * TRACE("reader %p, user_clock %d.\n", reader, user_clock); - EnterCriticalSection(&reader->stream_cs); + EnterCriticalSection(&reader->callback_cs); reader->user_clock = !!user_clock; - LeaveCriticalSection(&reader->stream_cs); + LeaveCriticalSection(&reader->callback_cs); return S_OK; } @@ -541,19 +542,19 @@ static HRESULT WINAPI WMReaderAdvanced_DeliverTime(IWMReaderAdvanced6 *iface, QW TRACE("reader %p, time %s.\n", reader, debugstr_time(time)); - EnterCriticalSection(&reader->stream_cs); + EnterCriticalSection(&reader->callback_cs); if (!reader->user_clock) { - LeaveCriticalSection(&reader->stream_cs); + LeaveCriticalSection(&reader->callback_cs); WARN("Not using a user-provided clock; returning E_UNEXPECTED.\n"); return E_UNEXPECTED; } reader->user_time = time; - LeaveCriticalSection(&reader->stream_cs); - WakeConditionVariable(&reader->stream_cv); + LeaveCriticalSection(&reader->callback_cs); + WakeConditionVariable(&reader->callback_cv); return S_OK; } @@ -1674,15 +1675,15 @@ static void async_reader_destroy(struct wm_reader *iface) TRACE("reader %p.\n", reader); - EnterCriticalSection(&reader->stream_cs); + EnterCriticalSection(&reader->callback_cs); reader->running = false; - LeaveCriticalSection(&reader->stream_cs); - WakeConditionVariable(&reader->stream_cv); + LeaveCriticalSection(&reader->callback_cs); + WakeConditionVariable(&reader->callback_cv); close_stream(reader); - reader->stream_cs.DebugInfo->Spare[0] = 0; - DeleteCriticalSection(&reader->stream_cs); + reader->callback_cs.DebugInfo->Spare[0] = 0; + DeleteCriticalSection(&reader->callback_cs); wm_reader_close(&reader->reader); @@ -1718,8 +1719,8 @@ HRESULT WINAPI winegstreamer_create_wm_async_reader(IWMReader **reader) object->IWMReaderStreamClock_iface.lpVtbl = &WMReaderStreamClockVtbl; object->IWMReaderTypeNegotiation_iface.lpVtbl = &WMReaderTypeNegotiationVtbl; - InitializeCriticalSection(&object->stream_cs); - object->stream_cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": async_reader.stream_cs"); + InitializeCriticalSection(&object->callback_cs); + object->callback_cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": async_reader.callback_cs"); QueryPerformanceFrequency(&object->clock_frequency); list_init(&object->async_ops); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/695
On 8/22/22 12:26, Rémi Bernon wrote:
- HANDLE stream_thread; - CRITICAL_SECTION stream_cs; - CONDITION_VARIABLE stream_cv; + + HANDLE callback_thread; + CRITICAL_SECTION callback_cs; + CONDITION_VARIABLE callback_cv;
What's the point of this change? I don't see how this is an improvement.
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wm_asyncreader.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/dlls/winegstreamer/wm_asyncreader.c b/dlls/winegstreamer/wm_asyncreader.c index 92c6ba88392..6f4aa303d82 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -267,12 +267,20 @@ static HRESULT open_stream(struct async_reader *reader, IWMReaderCallback *callb static void close_stream(struct async_reader *reader) { + struct async_op *op, *next; + if (reader->callback_thread) { WaitForSingleObject(reader->callback_thread, INFINITE); CloseHandle(reader->callback_thread); reader->callback_thread = NULL; } + + LIST_FOR_EACH_ENTRY_SAFE(op, next, &reader->async_ops, struct async_op, entry) + { + list_remove(&op->entry); + free(op); + } } static HRESULT async_reader_queue_op(struct async_reader *reader, enum async_op_type type, void *context) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/695
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wm_reader.c | 35 +++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/dlls/winegstreamer/wm_reader.c b/dlls/winegstreamer/wm_reader.c index 6c15413bba6..156f9d79446 100644 --- a/dlls/winegstreamer/wm_reader.c +++ b/dlls/winegstreamer/wm_reader.c @@ -1764,6 +1764,7 @@ HRESULT wm_reader_set_output_props(struct wm_reader *reader, DWORD output, struct output_props *props = unsafe_impl_from_IWMOutputMediaProps(props_iface); struct wg_format format, pref_format; struct wm_stream *stream; + HRESULT hr = S_OK; int i; strmbase_dump_media_type(&props->mt); @@ -1786,18 +1787,15 @@ HRESULT wm_reader_set_output_props(struct wm_reader *reader, DWORD output, if (pref_format.major_type != format.major_type) { /* R.U.S.E sets the type of the wrong stream, apparently by accident. */ - LeaveCriticalSection(&reader->cs); - WARN("Major types don't match; returning NS_E_INCOMPATIBLE_FORMAT.\n"); - return NS_E_INCOMPATIBLE_FORMAT; + hr = NS_E_INCOMPATIBLE_FORMAT; } - - switch (pref_format.major_type) + else switch (pref_format.major_type) { case WG_MAJOR_TYPE_AUDIO: if (format.u.audio.format == WG_AUDIO_FORMAT_UNKNOWN) - return NS_E_AUDIO_CODEC_NOT_INSTALLED; - if (format.u.audio.channels > pref_format.u.audio.channels) - return NS_E_AUDIO_CODEC_NOT_INSTALLED; + hr = NS_E_AUDIO_CODEC_NOT_INSTALLED; + else if (format.u.audio.channels > pref_format.u.audio.channels) + hr = NS_E_AUDIO_CODEC_NOT_INSTALLED; break; case WG_MAJOR_TYPE_VIDEO: @@ -1805,16 +1803,23 @@ HRESULT wm_reader_set_output_props(struct wm_reader *reader, DWORD output, if (format.u.video.format == video_formats[i]) break; if (i == ARRAY_SIZE(video_formats)) - return NS_E_INVALID_OUTPUT_FORMAT; - - if (pref_format.u.video.width != format.u.video.width) - return NS_E_INVALID_OUTPUT_FORMAT; - if (pref_format.u.video.height != format.u.video.height) - return NS_E_INVALID_OUTPUT_FORMAT; + hr = NS_E_INVALID_OUTPUT_FORMAT; + else if (pref_format.u.video.width != format.u.video.width) + hr = NS_E_INVALID_OUTPUT_FORMAT; + else if (pref_format.u.video.height != format.u.video.height) + hr = NS_E_INVALID_OUTPUT_FORMAT; break; default: - return NS_E_INCOMPATIBLE_FORMAT; + hr = NS_E_INCOMPATIBLE_FORMAT; + break; + } + + if (FAILED(hr)) + { + WARN("Unsupported media type, returning %#lx.\n", hr); + LeaveCriticalSection(&reader->cs); + return hr; } stream->format = format; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/695
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wm_asyncreader.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dlls/winegstreamer/wm_asyncreader.c b/dlls/winegstreamer/wm_asyncreader.c index 6f4aa303d82..222ed84bd95 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -290,7 +290,10 @@ static HRESULT async_reader_queue_op(struct async_reader *reader, enum async_op_ EnterCriticalSection(&reader->callback_cs); if (!(op = calloc(1, sizeof(*op)))) + { + LeaveCriticalSection(&reader->callback_cs); return E_OUTOFMEMORY; + } op->type = type; op->new_context = context; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/695
On 8/22/22 12:26, Rémi Bernon wrote:
diff --git a/dlls/winegstreamer/wm_asyncreader.c b/dlls/winegstreamer/wm_asyncreader.c index 6f4aa303d82..222ed84bd95 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -290,7 +290,10 @@ static HRESULT async_reader_queue_op(struct async_reader *reader, enum async_op_ EnterCriticalSection(&reader->callback_cs);
if (!(op = calloc(1, sizeof(*op)))) + { + LeaveCriticalSection(&reader->callback_cs); return E_OUTOFMEMORY; + } op->type = type; op->new_context = context;
I think it'd make more sense to limit the scope of the mutex to the list_add_tail() call.
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wm_asyncreader.c | 54 ++++++++++++++--------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/dlls/winegstreamer/wm_asyncreader.c b/dlls/winegstreamer/wm_asyncreader.c index 222ed84bd95..4d774cc2c62 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -247,25 +247,29 @@ static DWORD WINAPI async_reader_callback_thread(void *arg) return 0; } -static HRESULT open_stream(struct async_reader *reader, IWMReaderCallback *callback, void *context) +static HRESULT async_reader_queue_op(struct async_reader *reader, enum async_op_type type, void *context) { - IWMReaderCallback_AddRef((reader->callback = callback)); - reader->context = context; + struct async_op *op; - reader->running = true; - if (!(reader->callback_thread = CreateThread(NULL, 0, async_reader_callback_thread, reader, 0, NULL))) + EnterCriticalSection(&reader->callback_cs); + + if (!(op = calloc(1, sizeof(*op)))) { - IWMReaderCallback_Release(reader->callback); - reader->running = false; - reader->callback = NULL; - reader->context = NULL; + LeaveCriticalSection(&reader->callback_cs); return E_OUTOFMEMORY; } + op->type = type; + op->new_context = context; + + list_add_tail(&reader->async_ops, &op->entry); + + LeaveCriticalSection(&reader->callback_cs); + WakeConditionVariable(&reader->callback_cv); return S_OK; } -static void close_stream(struct async_reader *reader) +static void async_reader_close(struct async_reader *reader) { struct async_op *op, *next; @@ -283,24 +287,20 @@ static void close_stream(struct async_reader *reader) } } -static HRESULT async_reader_queue_op(struct async_reader *reader, enum async_op_type type, void *context) +static HRESULT async_reader_open(struct async_reader *reader, IWMReaderCallback *callback, void *context) { - struct async_op *op; - - EnterCriticalSection(&reader->callback_cs); + IWMReaderCallback_AddRef((reader->callback = callback)); + reader->context = context; - if (!(op = calloc(1, sizeof(*op)))) + reader->running = true; + if (!(reader->callback_thread = CreateThread(NULL, 0, async_reader_callback_thread, reader, 0, NULL))) { - LeaveCriticalSection(&reader->callback_cs); + IWMReaderCallback_Release(reader->callback); + reader->running = false; + reader->callback = NULL; + reader->context = NULL; return E_OUTOFMEMORY; } - op->type = type; - op->new_context = context; - - list_add_tail(&reader->async_ops, &op->entry); - - LeaveCriticalSection(&reader->callback_cs); - WakeConditionVariable(&reader->callback_cv); return S_OK; } @@ -350,7 +350,7 @@ static HRESULT WINAPI WMReader_Open(IWMReader *iface, const WCHAR *url, } if (SUCCEEDED(hr = wm_reader_open_file(&reader->reader, url)) - && FAILED(hr = open_stream(reader, callback, context))) + && FAILED(hr = async_reader_open(reader, callback, context))) wm_reader_close(&reader->reader); LeaveCriticalSection(&reader->reader.cs); @@ -367,7 +367,7 @@ static HRESULT WINAPI WMReader_Close(IWMReader *iface) EnterCriticalSection(&reader->reader.cs); async_reader_queue_op(reader, ASYNC_OP_CLOSE, NULL); - close_stream(reader); + async_reader_close(reader); hr = wm_reader_close(&reader->reader); if (reader->callback) @@ -830,7 +830,7 @@ static HRESULT WINAPI WMReaderAdvanced2_OpenStream(IWMReaderAdvanced6 *iface, } if (SUCCEEDED(hr = wm_reader_open_stream(&reader->reader, stream)) - && FAILED(hr = open_stream(reader, callback, context))) + && FAILED(hr = async_reader_open(reader, callback, context))) wm_reader_close(&reader->reader); LeaveCriticalSection(&reader->reader.cs); @@ -1691,7 +1691,7 @@ static void async_reader_destroy(struct wm_reader *iface) LeaveCriticalSection(&reader->callback_cs); WakeConditionVariable(&reader->callback_cv); - close_stream(reader); + async_reader_close(reader); reader->callback_cs.DebugInfo->Spare[0] = 0; DeleteCriticalSection(&reader->callback_cs); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/695
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wm_asyncreader.c | 52 +++++++++++++++++------------ 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/dlls/winegstreamer/wm_asyncreader.c b/dlls/winegstreamer/wm_asyncreader.c index 4d774cc2c62..bbc9130ddc2 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -269,9 +269,24 @@ static HRESULT async_reader_queue_op(struct async_reader *reader, enum async_op_ return S_OK; } -static void async_reader_close(struct async_reader *reader) +static void async_reader_close(struct async_reader *reader, bool force) { struct async_op *op, *next; + HRESULT hr; + + if (!force && FAILED(hr = async_reader_queue_op(reader, ASYNC_OP_CLOSE, NULL))) + { + ERR("Failed to queue close op, hr %#lx\n", hr); + force = true; + } + + if (force) + { + EnterCriticalSection(&reader->callback_cs); + reader->running = false; + LeaveCriticalSection(&reader->callback_cs); + WakeConditionVariable(&reader->callback_cv); + } if (reader->callback_thread) { @@ -285,24 +300,29 @@ static void async_reader_close(struct async_reader *reader) list_remove(&op->entry); free(op); } + + if (reader->callback) + IWMReaderCallback_Release(reader->callback); + reader->callback = NULL; + reader->context = NULL; } static HRESULT async_reader_open(struct async_reader *reader, IWMReaderCallback *callback, void *context) { + HRESULT hr = E_OUTOFMEMORY; + IWMReaderCallback_AddRef((reader->callback = callback)); reader->context = context; reader->running = true; if (!(reader->callback_thread = CreateThread(NULL, 0, async_reader_callback_thread, reader, 0, NULL))) - { - IWMReaderCallback_Release(reader->callback); - reader->running = false; - reader->callback = NULL; - reader->context = NULL; - return E_OUTOFMEMORY; - } + goto error; return S_OK; + +error: + async_reader_close(reader, true); + return hr; } static struct async_reader *impl_from_IWMReader(IWMReader *iface) @@ -366,13 +386,9 @@ static HRESULT WINAPI WMReader_Close(IWMReader *iface) EnterCriticalSection(&reader->reader.cs); - async_reader_queue_op(reader, ASYNC_OP_CLOSE, NULL); - async_reader_close(reader); + async_reader_close(reader, false); hr = wm_reader_close(&reader->reader); - if (reader->callback) - IWMReaderCallback_Release(reader->callback); - reader->callback = NULL; LeaveCriticalSection(&reader->reader.cs); @@ -1686,21 +1702,13 @@ static void async_reader_destroy(struct wm_reader *iface) TRACE("reader %p.\n", reader); - EnterCriticalSection(&reader->callback_cs); - reader->running = false; - LeaveCriticalSection(&reader->callback_cs); - WakeConditionVariable(&reader->callback_cv); - - async_reader_close(reader); + async_reader_close(reader, true); reader->callback_cs.DebugInfo->Spare[0] = 0; DeleteCriticalSection(&reader->callback_cs); wm_reader_close(&reader->reader); - if (reader->callback) - IWMReaderCallback_Release(reader->callback); - wm_reader_cleanup(&reader->reader); free(reader); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/695
On 8/22/22 12:27, Rémi Bernon wrote:
From: Rémi Bernon <rbernon(a)codeweavers.com>
--- dlls/winegstreamer/wm_asyncreader.c | 52 +++++++++++++++++------------ 1 file changed, 30 insertions(+), 22 deletions(-)
I'm having a hard time seeing this as an improvement.
From: R��mi Bernon <rbernon(a)codeweavers.com> And release it on Close. --- dlls/winegstreamer/wm_asyncreader.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/dlls/winegstreamer/wm_asyncreader.c b/dlls/winegstreamer/wm_asyncreader.c index bbc9130ddc2..18bcc0c6bc9 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -46,6 +46,7 @@ struct async_reader IWMReaderTypeNegotiation IWMReaderTypeNegotiation_iface; IReferenceClock IReferenceClock_iface; + IWMReaderCallbackAdvanced *callback_advanced; IWMReaderCallback *callback; void *context; @@ -70,8 +71,9 @@ static REFERENCE_TIME get_current_time(const struct async_reader *reader) return (time.QuadPart * 1000) / reader->clock_frequency.QuadPart * 10000; } -static void callback_thread_run(struct async_reader *reader, IWMReaderCallbackAdvanced *callback_advanced) +static void callback_thread_run(struct async_reader *reader) { + IWMReaderCallbackAdvanced *callback_advanced = reader->callback_advanced; IWMReaderCallback *callback = reader->callback; REFERENCE_TIME start_time; struct wm_stream *stream; @@ -173,17 +175,10 @@ static void callback_thread_run(struct async_reader *reader, IWMReaderCallbackAd static DWORD WINAPI async_reader_callback_thread(void *arg) { struct async_reader *reader = arg; - IWMReaderCallback *callback = reader->callback; - IWMReaderCallbackAdvanced *callback_advanced; static const DWORD zero; struct list *entry; HRESULT hr = S_OK; - if (FAILED(hr = IWMReaderCallback_QueryInterface(callback, - &IID_IWMReaderCallbackAdvanced, (void **)&callback_advanced))) - callback_advanced = NULL; - TRACE("Querying for IWMReaderCallbackAdvanced returned %#lx.\n", hr); - IWMReaderCallback_OnStatus(reader->callback, WMT_OPENED, S_OK, WMT_TYPE_DWORD, (BYTE *)&zero, reader->context); @@ -209,7 +204,7 @@ static DWORD WINAPI async_reader_callback_thread(void *arg) EnterCriticalSection(&reader->callback_cs); if (SUCCEEDED(hr)) - callback_thread_run(reader, callback_advanced); + callback_thread_run(reader); break; } @@ -238,9 +233,6 @@ static DWORD WINAPI async_reader_callback_thread(void *arg) SleepConditionVariableCS(&reader->callback_cv, &reader->callback_cs, INFINITE); } - if (callback_advanced) - IWMReaderCallbackAdvanced_Release(callback_advanced); - LeaveCriticalSection(&reader->callback_cs); TRACE("Reader is stopping; exiting.\n"); @@ -301,6 +293,10 @@ static void async_reader_close(struct async_reader *reader, bool force) free(op); } + if (reader->callback_advanced) + IWMReaderCallbackAdvanced_Release(reader->callback_advanced); + reader->callback_advanced = NULL; + if (reader->callback) IWMReaderCallback_Release(reader->callback); reader->callback = NULL; @@ -314,6 +310,13 @@ static HRESULT async_reader_open(struct async_reader *reader, IWMReaderCallback IWMReaderCallback_AddRef((reader->callback = callback)); reader->context = context; + if (FAILED(hr = IWMReaderCallback_QueryInterface(callback, &IID_IWMReaderCallbackAdvanced, + (void **)&reader->callback_advanced))) + { + WARN("Failed to retrieve IWMReaderCallbackAdvanced interface, hr %#lx\n", hr); + reader->callback_advanced = NULL; + } + reader->running = true; if (!(reader->callback_thread = CreateThread(NULL, 0, async_reader_callback_thread, reader, 0, NULL))) goto error; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/695
On 8/22/22 12:27, Rémi Bernon wrote:
From: Rémi Bernon <rbernon(a)codeweavers.com>
And release it on Close. --- dlls/winegstreamer/wm_asyncreader.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-)
Why do we need this?
On 8/22/22 12:26, Rémi Bernon (@rbernon) wrote:
This includes some fixes for obvious mistakes as well as initial cleanup of the WM async reader, in preparation for implementing proper threading support.
The cleanup is certainly appreciated, but what's broken about our current threading?
I'm planning on doing some more refactoring first, ultimately implementing the async reader on top of the sync reader, and removing the need for a private interface. I've checked that it can indeed work on Windows too, and it'll make the code simpler overall.
What about the code is actually made simpler? One of the reasons I tend to avoid implementing one externally visible API on top of another is that it tends to make logs harder to read (mostly due to the extra clutter). That's not to say I'm sure it's not worth it in this case, but it's not immediately clear to me that the code would get a lot simpler.
This includes some fixes for obvious mistakes as well as initial cleanup of the WM async reader, in preparation for implementing proper threading support.
The cleanup is certainly appreciated, but what's broken about our current threading?
The qasf (https://gitlab.winehq.org/wine/wine/-/merge_requests/686) and wmvcore (https://gitlab.winehq.org/wine/wine/-/merge_requests/685) tests show that there's actually at least one thread per stream, and likely two (one waiting / and eventually delivering samples, and one allocating samples), in addition to the callback thread.
I'm planning on doing some more refactoring first, ultimately implementing the async reader on top of the sync reader, and removing the need for a private interface. I've checked that it can indeed work on Windows too, and it'll make the code simpler overall.
What about the code is actually made simpler?
It will move all the async reader specific behavior to the async reader side, making it possible to move it out of winegstreamer and look at it alone. Current wm_reader is a mix of different use cases, depending on whether it's used from the async or sync side, and it complicates the code. Refactoring will also remove the internal struct wm_reader object, merging it with the sync reader, as well as the internal helpers which are just there to factor the implementation between the sync and async reader equivalents. Most async reader functions can be implemented as a direct forward to the sync reader functions and currently some sync reader methods areimplemented and not the async ones, and reversely, depending on what was needed.
One of the reasons I tend to avoid implementing one externally visible API on top of another is that it tends to make logs harder to read (mostly due to the extra clutter). That's not to say I'm sure it's not worth it in this case, but it's not immediately clear to me that the code would get a lot simpler.
I get that, but for most functions I don't think there's any need to log the async reader side as they are just proxying the call to the sync reader. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/695#note_6690
On 8/22/22 18:03, Rémi Bernon (@rbernon) wrote:
This includes some fixes for obvious mistakes as well as initial cleanup of the WM async reader, in preparation for implementing proper threading support.
The cleanup is certainly appreciated, but what's broken about our current threading?
The qasf (https://gitlab.winehq.org/wine/wine/-/merge_requests/686) and wmvcore (https://gitlab.winehq.org/wine/wine/-/merge_requests/685) tests show that there's actually at least one thread per stream, and likely two (one waiting / and eventually delivering samples, and one allocating samples), in addition to the callback thread.
Okay, thanks for the clarification. In the future, if submitting patch sets as if they are independent, it would help to explain all of the reasoning and context in each independent patch series. I had not even looked at patch set 685 yet, and 686 did not by itself imply anything about wmvcore behaviour. (It may be worth pointing out that 15 patches is a lot to review at once, even if split across multiple series, and that splitting them like this may not be as productive as intended.)
I'm planning on doing some more refactoring first, ultimately implementing the async reader on top of the sync reader, and removing the need for a private interface. I've checked that it can indeed work on Windows too, and it'll make the code simpler overall.
What about the code is actually made simpler?
It will move all the async reader specific behavior to the async reader side, making it possible to move it out of winegstreamer and look at it alone.
Current wm_reader is a mix of different use cases, depending on whether it's used from the async or sync side, and it complicates the code.
I'm not sure I understand. As far as I can see the only currently async-specific code in wm_reader.c is the allocate-for-output support, and that will have to be changed anyway if we implement IWMSyncReader2::SetAllocateFor*(). Whether the async reader lives in wmvcore or winegstreamer is not something I'm inclined to see as important.
Refactoring will also remove the internal struct wm_reader object, merging it with the sync reader, as well as the internal helpers which are just there to factor the implementation between the sync and async reader equivalents. Most async reader functions can be implemented as a direct forward to the sync reader functions and currently some sync reader methods areimplemented and not the async ones, and reversely, depending on what was needed.
Sure. I'm not sure I see it as worthwhile yet, but maybe seeing the code will change my mind. I don't hate the idea at least.
One of the reasons I tend to avoid implementing one externally visible API on top of another is that it tends to make logs harder to read (mostly due to the extra clutter). That's not to say I'm sure it's not worth it in this case, but it's not immediately clear to me that the code would get a lot simpler.
I get that, but for most functions I don't think there's any need to log the async reader side as they are just proxying the call to the sync reader.
I really don't like that idea. I find it extremely unhelpful not to trace the outermost API entry point for a given call, and I've been annoyed at the lack of such traces in logs many times.
I'm having a hard time seeing this as an improvement.
Gitlab doesn't show any context here as the comments aren't attached to commits, so I'm guessing from the shortstat that it's about releasing the callback on async_reader_close, and I don't see how it's not an improvement. The callback is acquired in async_reader_open, and at least for the sake of symmetry async_reader_close should undo everything the former did. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/695#note_6693
On 8/22/22 18:05, Rémi Bernon (@rbernon) wrote:
I'm having a hard time seeing this as an improvement.
Gitlab doesn't show any context here as the comments aren't attached to commits, so I'm guessing from the shortstat that it's about releasing the callback on async_reader_close, and I don't see how it's not an improvement.
The callback is acquired in async_reader_open, and at least for the sake of symmetry async_reader_close should undo everything the former did.
Sorry about that, I'll make it clearer which patch I'm responding to in the future. Reading the patch a little more carefully, the callback and context part is fine and makes sense. What I'm less convinced about, and what the patch subject doesn't actually mention, is that it also moves the async_reader_queue_op() and "reader->running = false" calls from two different paths to that function, which doesn't immediately make much sense to me.
Why do we need this?
Because we'll need it in more threads, which could probably query it themselves, though I'm not sure why this is wrong. We'll later need to create an IWMReaderAllocatorEx from the callback here anyway. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/695#note_6694
On 8/22/22 18:07, Rémi Bernon (@rbernon) wrote:
Why do we need this?
Because we'll need it in more threads, which could probably query it themselves, though I'm not sure why this is wrong. We'll later need to create an IWMReaderAllocatorEx from the callback here anyway.
Thanks, makes sense.
On 8/22/22 12:26, R��mi Bernon wrote:
- HANDLE stream_thread; - CRITICAL_SECTION stream_cs; - CONDITION_VARIABLE stream_cv; + + HANDLE callback_thread; + CRITICAL_SECTION callback_cs; + CONDITION_VARIABLE callback_cv;
What's the point of this change? I don't see how this is an improvement.
Like said above, the threading is wrong and we'll need more threads. The current "stream" thread, is only supposed to call the callbacks, so it's going to be named "callback_thread". And it will have nothing to do with streams. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/695#note_6695
On 8/22/22 18:08, Rémi Bernon (@rbernon) wrote:
On 8/22/22 12:26, Rémi Bernon wrote:
- HANDLE stream_thread; - CRITICAL_SECTION stream_cs; - CONDITION_VARIABLE stream_cv; + + HANDLE callback_thread; + CRITICAL_SECTION callback_cs; + CONDITION_VARIABLE callback_cv;
What's the point of this change? I don't see how this is an improvement.
Like said above, the threading is wrong and we'll need more threads. The current "stream" thread, is only supposed to call the callbacks, so it's going to be named "callback_thread". And it will have nothing to do with streams.
Thanks, makes sense.
On Mon Aug 22 22:20:50 2022 +0000, **** wrote:
Zebediah Figura (she/her) replied on the mailing list: ``` On 8/22/22 12:26, R��mi Bernon wrote:
diff --git a/dlls/winegstreamer/wm_asyncreader.c b/dlls/winegstreamer/wm_asyncreader.c index 6f4aa303d82..222ed84bd95 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -290,7 +290,10 @@ static HRESULT async_reader_queue_op(struct async_reader *reader, enum async_op_ EnterCriticalSection(&reader->callback_cs);
if (!(op = calloc(1, sizeof(*op)))) + { + LeaveCriticalSection(&reader->callback_cs); return E_OUTOFMEMORY; + } op->type = type; op->new_context = context;
I think it'd make more sense to limit the scope of the mutex to the list_add_tail() call. ``` Indeed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/695#note_6696
On 8/22/22 18:03, R��mi Bernon (@rbernon) wrote:
This includes some fixes for obvious mistakes as well as initial cleanup of the WM async reader, in preparation for implementing proper threading support.
The cleanup is certainly appreciated, but what's broken about our current threading?
The qasf (https://gitlab.winehq.org/wine/wine/-/merge_requests/686) and wmvcore (https://gitlab.winehq.org/wine/wine/-/merge_requests/685) tests show that there's actually at least one thread per stream, and likely two (one waiting / and eventually delivering samples, and one allocating samples), in addition to the callback thread.
Okay, thanks for the clarification. In the future, if submitting patch sets as if they are independent, it would help to explain all of the reasoning and context in each independent patch series. I had not even looked at patch set 685 yet, and 686 did not by itself imply anything about wmvcore behaviour.
Well, I didn't think that these fixes and cleanups, although motivated in part by the threading issues, were really dependent.
I'm planning on doing some more refactoring first, ultimately implementing the async reader on top of the sync reader, and removing the need for a private interface. I've checked that it can indeed work on Windows too, and it'll make the code simpler overall.
What about the code is actually made simpler?
It will move all the async reader specific behavior to the async reader side, making it possible to move it out of winegstreamer and look at it alone.
Current wm_reader is a mix of different use cases, depending on whether it's used from the async or sync side, and it complicates the code.
I'm not sure I understand. As far as I can see the only currently async-specific code in wm_reader.c is the allocate-for-output support, and that will have to be changed anyway if we implement IWMSyncReader2::SetAllocateFor*().
Whether the async reader lives in wmvcore or winegstreamer is not something I'm inclined to see as important.
The async reader has access to any state kept in the wm_reader, it's hard to tell across three different sources what is used and where. Yes, there's actually not so much async reader specifics on wm_reader side, and that's probably a good reason to split it as it means the private interface isn't really necessary.
Refactoring will also remove the internal struct wm_reader object, merging it with the sync reader, as well as the internal helpers which are just there to factor the implementation between the sync and async reader equivalents. Most async reader functions can be implemented as a direct forward to the sync reader functions and currently some sync reader methods areimplemented and not the async ones, and reversely, depending on what was needed.
Sure. I'm not sure I see it as worthwhile yet, but maybe seeing the code will change my mind. I don't hate the idea at least.
I pushed a branch with the changes I intend to do there: https://gitlab.winehq.org/rbernon/wine/-/commits/wip/wmvcore/v1
One of the reasons I tend to avoid implementing one externally visible API on top of another is that it tends to make logs harder to read (mostly due to the extra clutter). That's not to say I'm sure it's not worth it in this case, but it's not immediately clear to me that the code would get a lot simpler.
I get that, but for most functions I don't think there's any need to log the async reader side as they are just proxying the call to the sync reader.
I really don't like that idea. I find it extremely unhelpful not to trace the outermost API entry point for a given call, and I've been annoyed at the lack of such traces in logs many times.
I guess we can keep the async reader side logging as well, I don't mind personally and I don't think it's so much clutter. Note that you're pointing the issue with having too few logging, and at the same time worried that using the sync reader would cause clutter. I think that in general there's never enough logging, and the logs are huge anyway. So I don't think implementing things on top of each other is an issue, we do that all the time. Also currently, none of the wm_reader_* entry points are really tracing anything. It's probably okay-ish as it's almost single threaded, but adding more threads will just make debugging the async reader very difficult. Implementing it on top of the sync reader will solve that as every call to the sync reader will be automatically traced. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/695#note_6842
On 8/24/22 12:50, Rémi Bernon (@rbernon) wrote:
On 8/22/22 18:03, Rémi Bernon (@rbernon) wrote:
This includes some fixes for obvious mistakes as well as initial cleanup of the WM async reader, in preparation for implementing proper threading support.
The cleanup is certainly appreciated, but what's broken about our current threading?
The qasf (https://gitlab.winehq.org/wine/wine/-/merge_requests/686) and wmvcore (https://gitlab.winehq.org/wine/wine/-/merge_requests/685) tests show that there's actually at least one thread per stream, and likely two (one waiting / and eventually delivering samples, and one allocating samples), in addition to the callback thread.
Okay, thanks for the clarification. In the future, if submitting patch sets as if they are independent, it would help to explain all of the reasoning and context in each independent patch series. I had not even looked at patch set 685 yet, and 686 did not by itself imply anything about wmvcore behaviour.
Well, I didn't think that these fixes and cleanups, although motivated in part by the threading issues, were really dependent.
Not dependent in terms of code, no, but in reasoning some of these patches don't make sense without that context.
I'm planning on doing some more refactoring first, ultimately implementing the async reader on top of the sync reader, and removing the need for a private interface. I've checked that it can indeed work on Windows too, and it'll make the code simpler overall.
What about the code is actually made simpler?
It will move all the async reader specific behavior to the async reader side, making it possible to move it out of winegstreamer and look at it alone.
Current wm_reader is a mix of different use cases, depending on whether it's used from the async or sync side, and it complicates the code.
I'm not sure I understand. As far as I can see the only currently async-specific code in wm_reader.c is the allocate-for-output support, and that will have to be changed anyway if we implement IWMSyncReader2::SetAllocateFor*().
Whether the async reader lives in wmvcore or winegstreamer is not something I'm inclined to see as important.
The async reader has access to any state kept in the wm_reader, it's hard to tell across three different sources what is used and where.
Yes, there's actually not so much async reader specifics on wm_reader side, and that's probably a good reason to split it as it means the private interface isn't really necessary.
Sure, that's a good reason to switch.
Refactoring will also remove the internal struct wm_reader object, merging it with the sync reader, as well as the internal helpers which are just there to factor the implementation between the sync and async reader equivalents. Most async reader functions can be implemented as a direct forward to the sync reader functions and currently some sync reader methods areimplemented and not the async ones, and reversely, depending on what was needed.
Sure. I'm not sure I see it as worthwhile yet, but maybe seeing the code will change my mind. I don't hate the idea at least.
I pushed a branch with the changes I intend to do there: https://gitlab.winehq.org/rbernon/wine/-/commits/wip/wmvcore/v1
I had a brief look and I think I'm convinced enough that this is a good idea.
One of the reasons I tend to avoid implementing one externally visible API on top of another is that it tends to make logs harder to read (mostly due to the extra clutter). That's not to say I'm sure it's not worth it in this case, but it's not immediately clear to me that the code would get a lot simpler.
I get that, but for most functions I don't think there's any need to log the async reader side as they are just proxying the call to the sync reader.
I really don't like that idea. I find it extremely unhelpful not to trace the outermost API entry point for a given call, and I've been annoyed at the lack of such traces in logs many times.
I guess we can keep the async reader side logging as well, I don't mind personally and I don't think it's so much clutter.
Note that you're pointing the issue with having too few logging, and at the same time worried that using the sync reader would cause clutter. I think that in general there's never enough logging, and the logs are huge anyway. So I don't think implementing things on top of each other is an issue, we do that all the time.
Also currently, none of the wm_reader_* entry points are really tracing anything. It's probably okay-ish as it's almost single threaded, but adding more threads will just make debugging the async reader very difficult. Implementing it on top of the sync reader will solve that as every call to the sync reader will be automatically traced.
To be clear I think that if we want to implement the async reader directly on top of the sync reader, I'd rather trace both entry points (and have that extra clutter) than just one of them. My preference in general, though, is that multiple entry points which do the same thing be backed by a single helper which is not traced. It's not a strong preference, though. At this point my hesitation to refactor wmvcore is motivated more by inertia than anything.
participants (2)
-
Rémi Bernon -
Zebediah Figura (she/her)