[PATCH 0/5] MR803: winegstreamer: Refactor wm async reader clock.
From: R��mi Bernon <rbernon(a)codeweavers.com> And unconditionally assign new context for ASYNC_OP_START. --- dlls/winegstreamer/wm_asyncreader.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/dlls/winegstreamer/wm_asyncreader.c b/dlls/winegstreamer/wm_asyncreader.c index f3e2209881a..a14beb37951 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -30,7 +30,7 @@ struct async_op ASYNC_OP_STOP, ASYNC_OP_CLOSE, } type; - void *new_context; + void *data; struct list entry; }; @@ -191,13 +191,12 @@ static DWORD WINAPI async_reader_callback_thread(void *arg) struct async_op *op = LIST_ENTRY(entry, struct async_op, entry); list_remove(&op->entry); - if (op->new_context) - reader->context = op->new_context; hr = list_empty(&reader->async_ops) ? S_OK : E_ABORT; switch (op->type) { case ASYNC_OP_START: { + reader->context = op->data; LeaveCriticalSection(&reader->callback_cs); IWMReaderCallback_OnStatus(reader->callback, WMT_STARTED, hr, WMT_TYPE_DWORD, (BYTE *)&zero, reader->context); @@ -291,14 +290,14 @@ error: return hr; } -static HRESULT async_reader_queue_op(struct async_reader *reader, enum async_op_type type, void *context) +static HRESULT async_reader_queue_op(struct async_reader *reader, enum async_op_type type, void *data) { struct async_op *op; if (!(op = calloc(1, sizeof(*op)))) return E_OUTOFMEMORY; op->type = type; - op->new_context = context; + op->data = data; EnterCriticalSection(&reader->callback_cs); list_add_tail(&reader->async_ops, &op->entry); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/803
On 9/8/22 01:56, Rémi Bernon wrote:
@@ -191,13 +191,12 @@ static DWORD WINAPI async_reader_callback_thread(void *arg) struct async_op *op = LIST_ENTRY(entry, struct async_op, entry); list_remove(&op->entry);
- if (op->new_context) - reader->context = op->new_context; hr = list_empty(&reader->async_ops) ? S_OK : E_ABORT; switch (op->type) { case ASYNC_OP_START: { + reader->context = op->data; LeaveCriticalSection(&reader->callback_cs); IWMReaderCallback_OnStatus(reader->callback, WMT_STARTED, hr, WMT_TYPE_DWORD, (BYTE *)&zero, reader->context);
Note that this is a change in behaviour; now if someone calls IWMReader::Start() with a NULL context that'll override the previous one. That's what I would expect, but it'd be nice to have a test. It'd also be nice to avoid including functional changes in patches whose subject line describes just a refactoring.
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wm_asyncreader.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/dlls/winegstreamer/wm_asyncreader.c b/dlls/winegstreamer/wm_asyncreader.c index a14beb37951..fa56eac737e 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -22,6 +22,13 @@ WINE_DEFAULT_DEBUG_CHANNEL(wmvcore); +struct async_start_info +{ + QWORD start; + QWORD duration; + void *context; +}; + struct async_op { enum async_op_type @@ -196,7 +203,13 @@ static DWORD WINAPI async_reader_callback_thread(void *arg) { case ASYNC_OP_START: { - reader->context = op->data; + struct async_start_info *info = op->data; + + reader->context = info->context; + if (SUCCEEDED(hr)) + wm_reader_seek(&reader->reader, info->start, info->duration); + free(info); + LeaveCriticalSection(&reader->callback_cs); IWMReaderCallback_OnStatus(reader->callback, WMT_STARTED, hr, WMT_TYPE_DWORD, (BYTE *)&zero, reader->context); @@ -432,6 +445,7 @@ static HRESULT WINAPI WMReader_Start(IWMReader *iface, QWORD start, QWORD duration, float rate, void *context) { struct async_reader *reader = impl_from_IWMReader(iface); + struct async_start_info *info; HRESULT hr; TRACE("reader %p, start %s, duration %s, rate %.8e, context %p.\n", @@ -440,18 +454,23 @@ static HRESULT WINAPI WMReader_Start(IWMReader *iface, if (rate != 1.0f) FIXME("Ignoring rate %.8e.\n", rate); + if (!(info = calloc(1, sizeof(*info)))) + return E_OUTOFMEMORY; + info->start = start; + info->duration = duration; + info->context = context; + EnterCriticalSection(&reader->reader.cs); if (!reader->callback_thread) hr = NS_E_INVALID_REQUEST; else - { - wm_reader_seek(&reader->reader, start, duration); - hr = async_reader_queue_op(reader, ASYNC_OP_START, context); - } + hr = async_reader_queue_op(reader, ASYNC_OP_START, info); LeaveCriticalSection(&reader->reader.cs); + if (FAILED(hr)) + free(info); return hr; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/803
On 9/8/22 01:56, Rémi Bernon wrote:
@@ -440,18 +454,23 @@ static HRESULT WINAPI WMReader_Start(IWMReader *iface, if (rate != 1.0f) FIXME("Ignoring rate %.8e.\n", rate);
+ if (!(info = calloc(1, sizeof(*info)))) + return E_OUTOFMEMORY; + info->start = start; + info->duration = duration; + info->context = context; + EnterCriticalSection(&reader->reader.cs);
if (!reader->callback_thread) hr = NS_E_INVALID_REQUEST; else - { - wm_reader_seek(&reader->reader, start, duration); - hr = async_reader_queue_op(reader, ASYNC_OP_START, context); - } + hr = async_reader_queue_op(reader, ASYNC_OP_START, info);
LeaveCriticalSection(&reader->reader.cs);
+ if (FAILED(hr)) + free(info); return hr; }
Can we just embed this directly into the async_op structure (perhaps with a union), and avoid the extra allocation?
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wm_asyncreader.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/dlls/winegstreamer/wm_asyncreader.c b/dlls/winegstreamer/wm_asyncreader.c index fa56eac737e..6fb37264e3d 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -57,6 +57,7 @@ struct async_reader IWMReaderCallback *callback; void *context; + REFERENCE_TIME clock_start; LARGE_INTEGER clock_frequency; HANDLE callback_thread; @@ -70,19 +71,17 @@ struct async_reader QWORD user_time; }; -static REFERENCE_TIME get_current_time(const struct async_reader *reader) +static REFERENCE_TIME async_reader_get_time(const struct async_reader *reader, bool absolute) { LARGE_INTEGER time; - QueryPerformanceCounter(&time); - return (time.QuadPart * 1000) / reader->clock_frequency.QuadPart * 10000; + return (time.QuadPart * 1000) / reader->clock_frequency.QuadPart * 10000 - (absolute ? 0 : reader->clock_start); } 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; static const DWORD zero; QWORD pts, duration; @@ -91,8 +90,6 @@ static void callback_thread_run(struct async_reader *reader) HRESULT hr = S_OK; DWORD flags; - start_time = get_current_time(reader); - while (reader->running && list_empty(&reader->async_ops)) { LeaveCriticalSection(&reader->callback_cs); @@ -121,13 +118,13 @@ static void callback_thread_run(struct async_reader *reader) { while (reader->running && list_empty(&reader->async_ops)) { - REFERENCE_TIME current_time = get_current_time(reader); + REFERENCE_TIME current_time = async_reader_get_time(reader, false); - if (pts <= current_time - start_time) + if (pts <= current_time) break; SleepConditionVariableCS(&reader->callback_cv, &reader->callback_cs, - (pts - (current_time - start_time)) / 10000); + (pts - current_time) / 10000); } } @@ -207,7 +204,10 @@ static DWORD WINAPI async_reader_callback_thread(void *arg) reader->context = info->context; if (SUCCEEDED(hr)) + { wm_reader_seek(&reader->reader, info->start, info->duration); + reader->clock_start = async_reader_get_time(reader, true); + } free(info); LeaveCriticalSection(&reader->callback_cs); @@ -221,6 +221,9 @@ static DWORD WINAPI async_reader_callback_thread(void *arg) } case ASYNC_OP_STOP: + if (SUCCEEDED(hr)) + reader->clock_start = 0; + LeaveCriticalSection(&reader->callback_cs); IWMReaderCallback_OnStatus(reader->callback, WMT_STOPPED, hr, WMT_TYPE_DWORD, (BYTE *)&zero, reader->context); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/803
On 9/8/22 01:56, Rémi Bernon wrote:
-static REFERENCE_TIME get_current_time(const struct async_reader *reader) +static REFERENCE_TIME async_reader_get_time(const struct async_reader *reader, bool absolute) { LARGE_INTEGER time; - QueryPerformanceCounter(&time); - return (time.QuadPart * 1000) / reader->clock_frequency.QuadPart * 10000; + return (time.QuadPart * 1000) / reader->clock_frequency.QuadPart * 10000 - (absolute ? 0 : reader->clock_start); }
I have a hard time seeing this as an improvement over just subtracting reader->clock_start where necessary.
@@ -221,6 +221,9 @@ static DWORD WINAPI async_reader_callback_thread(void *arg) }
case ASYNC_OP_STOP: + if (SUCCEEDED(hr)) + reader->clock_start = 0; + LeaveCriticalSection(&reader->callback_cs); IWMReaderCallback_OnStatus(reader->callback, WMT_STOPPED, hr, WMT_TYPE_DWORD, (BYTE *)&zero, reader->context);
Why do we need this?
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wm_asyncreader.c | 67 ++++++++++++++++------------- 1 file changed, 37 insertions(+), 30 deletions(-) diff --git a/dlls/winegstreamer/wm_asyncreader.c b/dlls/winegstreamer/wm_asyncreader.c index 6fb37264e3d..6a310c35b79 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -78,6 +78,41 @@ static REFERENCE_TIME async_reader_get_time(const struct async_reader *reader, b return (time.QuadPart * 1000) / reader->clock_frequency.QuadPart * 10000 - (absolute ? 0 : reader->clock_start); } +static bool async_reader_needs_wait(struct async_reader *reader, QWORD pts, DWORD *timeout) +{ + if (!reader->user_clock) + { + REFERENCE_TIME current_time = async_reader_get_time(reader, false); + *timeout = (pts - current_time) / 10000; + return pts > current_time; + } + + *timeout = INFINITE; + return pts > reader->user_time; +} + +static bool async_reader_wait_pts(struct async_reader *reader, QWORD pts) +{ + IWMReaderCallbackAdvanced *callback_advanced = reader->callback_advanced; + DWORD timeout; + bool ret; + + TRACE("reader %p, pts %I64d.\n", reader, pts); + + if (callback_advanced && reader->user_clock && pts > reader->user_time) + { + QWORD user_time = reader->user_time; + LeaveCriticalSection(&reader->callback_cs); + IWMReaderCallbackAdvanced_OnTime(callback_advanced, user_time, reader->context); + EnterCriticalSection(&reader->callback_cs); + } + + while ((ret = reader->running && list_empty(&reader->async_ops)) && async_reader_needs_wait(reader, pts, &timeout)) + SleepConditionVariableCS(&reader->callback_cv, &reader->callback_cs, timeout); + + return ret; +} + static void callback_thread_run(struct async_reader *reader) { IWMReaderCallbackAdvanced *callback_advanced = reader->callback_advanced; @@ -98,38 +133,10 @@ static void callback_thread_run(struct async_reader *reader) if (hr != S_OK) break; - stream = wm_reader_get_stream_by_stream_number(&reader->reader, stream_number); - - if (reader->user_clock) + if (async_reader_wait_pts(reader, pts)) { - QWORD user_time = reader->user_time; + stream = wm_reader_get_stream_by_stream_number(&reader->reader, stream_number); - if (pts > user_time && callback_advanced) - { - LeaveCriticalSection(&reader->callback_cs); - IWMReaderCallbackAdvanced_OnTime(callback_advanced, user_time, reader->context); - EnterCriticalSection(&reader->callback_cs); - } - - while (pts > reader->user_time && reader->running && list_empty(&reader->async_ops)) - SleepConditionVariableCS(&reader->callback_cv, &reader->callback_cs, INFINITE); - } - else - { - while (reader->running && list_empty(&reader->async_ops)) - { - REFERENCE_TIME current_time = async_reader_get_time(reader, false); - - if (pts <= current_time) - break; - - SleepConditionVariableCS(&reader->callback_cv, &reader->callback_cs, - (pts - current_time) / 10000); - } - } - - if (reader->running && list_empty(&reader->async_ops)) - { LeaveCriticalSection(&reader->callback_cs); if (stream->read_compressed) hr = IWMReaderCallbackAdvanced_OnStreamSample(callback_advanced, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/803
On 9/8/22 01:56, Rémi Bernon wrote:
+static bool async_reader_needs_wait(struct async_reader *reader, QWORD pts, DWORD *timeout) +{ + if (!reader->user_clock) + { + REFERENCE_TIME current_time = async_reader_get_time(reader, false); + *timeout = (pts - current_time) / 10000; + return pts > current_time; + } + + *timeout = INFINITE; + return pts > reader->user_time; +} +
This changes behaviour, which isn't obvious from the subject. In general please try to avoid moving code and changing it in the same patch, or if that's necessary please make it clearer in the patch subject what's being done.
+static bool async_reader_wait_pts(struct async_reader *reader, QWORD pts) +{ + IWMReaderCallbackAdvanced *callback_advanced = reader->callback_advanced; + DWORD timeout; + bool ret; + + TRACE("reader %p, pts %I64d.\n", reader, pts); + + if (callback_advanced && reader->user_clock && pts > reader->user_time) + { + QWORD user_time = reader->user_time; + LeaveCriticalSection(&reader->callback_cs); + IWMReaderCallbackAdvanced_OnTime(callback_advanced, user_time, reader->context); + EnterCriticalSection(&reader->callback_cs); + } + + while ((ret = reader->running && list_empty(&reader->async_ops)) && async_reader_needs_wait(reader, pts, &timeout)) + SleepConditionVariableCS(&reader->callback_cv, &reader->callback_cs, timeout);
I can't help but feel like this callback is returning the same information in two different ways. I.e. instead of returning a bool, it could just return the timeout, and that could be zero if we are already ready to present. I'd appreciate parentheses around "reader->running && list_empty(&reader->async_ops)".
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wm_asyncreader.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/winegstreamer/wm_asyncreader.c b/dlls/winegstreamer/wm_asyncreader.c index 6a310c35b79..b2f3056c95b 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -566,6 +566,7 @@ static HRESULT WINAPI WMReaderAdvanced_SetUserProvidedClock(IWMReaderAdvanced6 * EnterCriticalSection(&reader->callback_cs); reader->user_clock = !!user_clock; LeaveCriticalSection(&reader->callback_cs); + WakeConditionVariable(&reader->callback_cv); return S_OK; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/803
participants (2)
-
Rémi Bernon -
Zebediah Figura