From: Rémi Bernon rbernon@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 b4750592778..b882d205220 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -560,6 +560,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; }
From: Rémi Bernon rbernon@codeweavers.com
--- dlls/winegstreamer/wm_asyncreader.c | 66 +++++++++++++++++++---------- 1 file changed, 44 insertions(+), 22 deletions(-)
diff --git a/dlls/winegstreamer/wm_asyncreader.c b/dlls/winegstreamer/wm_asyncreader.c index b882d205220..ed19955a531 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -44,6 +44,14 @@ struct async_op struct list entry; };
+struct sample +{ + INSSBuffer *buffer; + QWORD pts, duration; + DWORD flags; + WORD stream; +}; + struct async_reader { struct wm_reader reader; @@ -121,42 +129,56 @@ static bool async_reader_wait_pts(struct async_reader *reader, QWORD pts) return false; }
+static void async_reader_deliver_sample(struct async_reader *reader, struct sample *sample) +{ + IWMReaderCallbackAdvanced *callback_advanced = reader->callback_advanced; + IWMReaderCallback *callback = reader->callback; + struct wm_stream *stream; + BOOL read_compressed; + HRESULT hr; + + TRACE("reader %p, stream %u, pts %I64d, duration %I64d, flags %#lx, buffer %p.\n", + reader, sample->stream, sample->pts, sample->duration, sample->flags, sample->buffer); + + stream = wm_reader_get_stream_by_stream_number(&reader->reader, sample->stream); + read_compressed = stream->read_compressed; + + LeaveCriticalSection(&reader->callback_cs); + if (read_compressed) + hr = IWMReaderCallbackAdvanced_OnStreamSample(callback_advanced, sample->stream, + sample->pts, sample->duration, sample->flags, sample->buffer, reader->context); + else + hr = IWMReaderCallback_OnSample(callback, sample->stream - 1, sample->pts, sample->duration, + sample->flags, sample->buffer, reader->context); + EnterCriticalSection(&reader->callback_cs); + + TRACE("Callback returned %#lx.\n", hr); + + INSSBuffer_Release(sample->buffer); +} + static void callback_thread_run(struct async_reader *reader) { IWMReaderCallbackAdvanced *callback_advanced = reader->callback_advanced; IWMReaderCallback *callback = reader->callback; static const DWORD zero; - QWORD pts, duration; - WORD stream_number; - INSSBuffer *sample; HRESULT hr = S_OK; - DWORD flags;
while (reader->running && list_empty(&reader->async_ops)) { + struct sample sample; + LeaveCriticalSection(&reader->callback_cs); - hr = wm_reader_get_stream_sample(&reader->reader, callback_advanced, 0, &sample, &pts, &duration, &flags, &stream_number); + hr = wm_reader_get_stream_sample(&reader->reader, callback_advanced, 0, &sample.buffer, + &sample.pts, &sample.duration, &sample.flags, &sample.stream); EnterCriticalSection(&reader->callback_cs); if (hr != S_OK) break;
- if (async_reader_wait_pts(reader, pts)) - { - struct wm_stream *stream = wm_reader_get_stream_by_stream_number(&reader->reader, stream_number); - - 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->callback_cs); - - TRACE("Callback returned %#lx.\n", hr); - } - - INSSBuffer_Release(sample); + if (async_reader_wait_pts(reader, sample.pts)) + async_reader_deliver_sample(reader, &sample); + else + INSSBuffer_Release(sample.buffer); }
if (hr == NS_E_NO_MORE_SAMPLES)
On 9/14/22 05:01, Rémi Bernon wrote:
+static void async_reader_deliver_sample(struct async_reader *reader, struct sample *sample) +{
- IWMReaderCallbackAdvanced *callback_advanced = reader->callback_advanced;
- IWMReaderCallback *callback = reader->callback;
- struct wm_stream *stream;
- BOOL read_compressed;
- HRESULT hr;
- TRACE("reader %p, stream %u, pts %I64d, duration %I64d, flags %#lx, buffer %p.\n",
reader, sample->stream, sample->pts, sample->duration, sample->flags, sample->buffer);
Not a big deal, but we can use debugstr_time() for PTS and duration, which is a bit easier to read.
- stream = wm_reader_get_stream_by_stream_number(&reader->reader, sample->stream);
- read_compressed = stream->read_compressed;
- LeaveCriticalSection(&reader->callback_cs);
- if (read_compressed)
hr = IWMReaderCallbackAdvanced_OnStreamSample(callback_advanced, sample->stream,sample->pts, sample->duration, sample->flags, sample->buffer, reader->context);- else
hr = IWMReaderCallback_OnSample(callback, sample->stream - 1, sample->pts, sample->duration,sample->flags, sample->buffer, reader->context);- EnterCriticalSection(&reader->callback_cs);
- TRACE("Callback returned %#lx.\n", hr);
- INSSBuffer_Release(sample->buffer);
I find it generally more idiomatic when input-only parameters aren't freed by default; this would also obviate that "else" below.
(Resending because the gitlab bridge broke. Sorry for the delay.)
On 9/14/22 05:01, Rémi Bernon wrote:
+static void async_reader_deliver_sample(struct async_reader *reader, struct sample *sample) +{
- IWMReaderCallbackAdvanced *callback_advanced = reader->callback_advanced;
- IWMReaderCallback *callback = reader->callback;
- struct wm_stream *stream;
- BOOL read_compressed;
- HRESULT hr;
- TRACE("reader %p, stream %u, pts %I64d, duration %I64d, flags %#lx, buffer %p.\n",
reader, sample->stream, sample->pts, sample->duration, sample->flags, sample->buffer);
Not a big deal, but we can use debugstr_time() for PTS and duration, which is a bit easier to read.
- stream = wm_reader_get_stream_by_stream_number(&reader->reader, sample->stream);
- read_compressed = stream->read_compressed;
- LeaveCriticalSection(&reader->callback_cs);
- if (read_compressed)
hr = IWMReaderCallbackAdvanced_OnStreamSample(callback_advanced, sample->stream,sample->pts, sample->duration, sample->flags, sample->buffer, reader->context);- else
hr = IWMReaderCallback_OnSample(callback, sample->stream - 1, sample->pts, sample->duration,sample->flags, sample->buffer, reader->context);- EnterCriticalSection(&reader->callback_cs);
- TRACE("Callback returned %#lx.\n", hr);
- INSSBuffer_Release(sample->buffer);
I find it generally more idiomatic when input-only parameters aren't freed by default; this would also obviate that "else" below.
From: Rémi Bernon rbernon@codeweavers.com
--- dlls/winegstreamer/wm_asyncreader.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/dlls/winegstreamer/wm_asyncreader.c b/dlls/winegstreamer/wm_asyncreader.c index ed19955a531..61ca6ce9681 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -64,6 +64,8 @@ struct async_reader IWMReaderTypeNegotiation IWMReaderTypeNegotiation_iface; IReferenceClock IReferenceClock_iface;
+ CRITICAL_SECTION cs; + IWMReaderCallbackAdvanced *callback_advanced; IWMReaderCallback *callback; void *context; @@ -389,11 +391,13 @@ static HRESULT WINAPI WMReader_Open(IWMReader *iface, const WCHAR *url, TRACE("reader %p, url %s, callback %p, context %p.\n", reader, debugstr_w(url), callback, context);
+ EnterCriticalSection(&reader->cs); EnterCriticalSection(&reader->reader.cs);
if (reader->reader.wg_parser) { LeaveCriticalSection(&reader->reader.cs); + LeaveCriticalSection(&reader->cs); WARN("Stream is already open; returning E_UNEXPECTED.\n"); return E_UNEXPECTED; } @@ -403,6 +407,7 @@ static HRESULT WINAPI WMReader_Open(IWMReader *iface, const WCHAR *url, wm_reader_close(&reader->reader);
LeaveCriticalSection(&reader->reader.cs); + LeaveCriticalSection(&reader->cs); return hr; }
@@ -413,7 +418,7 @@ static HRESULT WINAPI WMReader_Close(IWMReader *iface)
TRACE("reader %p.\n", reader);
- EnterCriticalSection(&reader->reader.cs); + EnterCriticalSection(&reader->cs);
if (SUCCEEDED(hr = async_reader_queue_op(reader, ASYNC_OP_CLOSE, NULL))) { @@ -421,7 +426,7 @@ static HRESULT WINAPI WMReader_Close(IWMReader *iface) hr = wm_reader_close(&reader->reader); }
- LeaveCriticalSection(&reader->reader.cs); + LeaveCriticalSection(&reader->cs);
return hr; } @@ -488,14 +493,14 @@ static HRESULT WINAPI WMReader_Start(IWMReader *iface, if (rate != 1.0f) FIXME("Ignoring rate %.8e.\n", rate);
- EnterCriticalSection(&reader->reader.cs); + EnterCriticalSection(&reader->cs);
if (!reader->callback_thread) hr = NS_E_INVALID_REQUEST; else hr = async_reader_queue_op(reader, ASYNC_OP_START, &data);
- LeaveCriticalSection(&reader->reader.cs); + LeaveCriticalSection(&reader->cs);
return hr; } @@ -507,14 +512,14 @@ static HRESULT WINAPI WMReader_Stop(IWMReader *iface)
TRACE("reader %p.\n", reader);
- EnterCriticalSection(&reader->reader.cs); + EnterCriticalSection(&reader->cs);
if (!reader->callback_thread) hr = E_UNEXPECTED; else hr = async_reader_queue_op(reader, ASYNC_OP_STOP, NULL);
- LeaveCriticalSection(&reader->reader.cs); + LeaveCriticalSection(&reader->cs);
return hr; } @@ -866,11 +871,13 @@ static HRESULT WINAPI WMReaderAdvanced2_OpenStream(IWMReaderAdvanced6 *iface,
TRACE("reader %p, stream %p, callback %p, context %p.\n", reader, stream, callback, context);
+ EnterCriticalSection(&reader->cs); EnterCriticalSection(&reader->reader.cs);
if (reader->reader.wg_parser) { LeaveCriticalSection(&reader->reader.cs); + LeaveCriticalSection(&reader->cs); WARN("Stream is already open; returning E_UNEXPECTED.\n"); return E_UNEXPECTED; } @@ -880,6 +887,7 @@ static HRESULT WINAPI WMReaderAdvanced2_OpenStream(IWMReaderAdvanced6 *iface, wm_reader_close(&reader->reader);
LeaveCriticalSection(&reader->reader.cs); + LeaveCriticalSection(&reader->cs); return hr; }
@@ -1741,6 +1749,8 @@ static void async_reader_destroy(struct wm_reader *iface)
reader->callback_cs.DebugInfo->Spare[0] = 0; DeleteCriticalSection(&reader->callback_cs); + reader->cs.DebugInfo->Spare[0] = 0; + DeleteCriticalSection(&reader->cs);
wm_reader_close(&reader->reader);
@@ -1773,6 +1783,8 @@ HRESULT WINAPI winegstreamer_create_wm_async_reader(IWMReader **reader) object->IWMReaderStreamClock_iface.lpVtbl = &WMReaderStreamClockVtbl; object->IWMReaderTypeNegotiation_iface.lpVtbl = &WMReaderTypeNegotiationVtbl;
+ InitializeCriticalSection(&object->cs); + object->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": async_reader.cs"); InitializeCriticalSection(&object->callback_cs); object->callback_cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": async_reader.callback_cs");
From: Rémi Bernon rbernon@codeweavers.com
--- dlls/winegstreamer/wm_asyncreader.c | 20 -------------------- dlls/winegstreamer/wm_reader.c | 15 +++++++++++++++ 2 files changed, 15 insertions(+), 20 deletions(-)
diff --git a/dlls/winegstreamer/wm_asyncreader.c b/dlls/winegstreamer/wm_asyncreader.c index 61ca6ce9681..11f225a759f 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -392,21 +392,11 @@ static HRESULT WINAPI WMReader_Open(IWMReader *iface, const WCHAR *url, reader, debugstr_w(url), callback, context);
EnterCriticalSection(&reader->cs); - EnterCriticalSection(&reader->reader.cs); - - if (reader->reader.wg_parser) - { - LeaveCriticalSection(&reader->reader.cs); - LeaveCriticalSection(&reader->cs); - WARN("Stream is already open; returning E_UNEXPECTED.\n"); - return E_UNEXPECTED; - }
if (SUCCEEDED(hr = wm_reader_open_file(&reader->reader, url)) && FAILED(hr = async_reader_open(reader, callback, context))) wm_reader_close(&reader->reader);
- LeaveCriticalSection(&reader->reader.cs); LeaveCriticalSection(&reader->cs); return hr; } @@ -872,21 +862,11 @@ static HRESULT WINAPI WMReaderAdvanced2_OpenStream(IWMReaderAdvanced6 *iface, TRACE("reader %p, stream %p, callback %p, context %p.\n", reader, stream, callback, context);
EnterCriticalSection(&reader->cs); - EnterCriticalSection(&reader->reader.cs); - - if (reader->reader.wg_parser) - { - LeaveCriticalSection(&reader->reader.cs); - LeaveCriticalSection(&reader->cs); - WARN("Stream is already open; returning E_UNEXPECTED.\n"); - return E_UNEXPECTED; - }
if (SUCCEEDED(hr = wm_reader_open_stream(&reader->reader, stream)) && FAILED(hr = async_reader_open(reader, callback, context))) wm_reader_close(&reader->reader);
- LeaveCriticalSection(&reader->reader.cs); LeaveCriticalSection(&reader->cs); return hr; } diff --git a/dlls/winegstreamer/wm_reader.c b/dlls/winegstreamer/wm_reader.c index 156f9d79446..243ebb20335 100644 --- a/dlls/winegstreamer/wm_reader.c +++ b/dlls/winegstreamer/wm_reader.c @@ -1558,6 +1558,13 @@ HRESULT wm_reader_open_stream(struct wm_reader *reader, IStream *stream)
EnterCriticalSection(&reader->cs);
+ if (reader->wg_parser) + { + LeaveCriticalSection(&reader->cs); + WARN("Stream is already open; returning E_UNEXPECTED.\n"); + return E_UNEXPECTED; + } + IStream_AddRef(reader->source_stream = stream); if (FAILED(hr = init_stream(reader, stat.cbSize.QuadPart))) { @@ -1591,6 +1598,14 @@ HRESULT wm_reader_open_file(struct wm_reader *reader, const WCHAR *filename)
EnterCriticalSection(&reader->cs);
+ if (reader->wg_parser) + { + LeaveCriticalSection(&reader->cs); + WARN("Stream is already open; returning E_UNEXPECTED.\n"); + CloseHandle(file); + return E_UNEXPECTED; + } + reader->file = file;
if (FAILED(hr = init_stream(reader, size.QuadPart)))
From: Rémi Bernon rbernon@codeweavers.com
--- dlls/winegstreamer/wm_asyncreader.c | 77 ++++++++++++++++++++++------- 1 file changed, 60 insertions(+), 17 deletions(-)
diff --git a/dlls/winegstreamer/wm_asyncreader.c b/dlls/winegstreamer/wm_asyncreader.c index 11f225a759f..a7d26aa131c 100644 --- a/dlls/winegstreamer/wm_asyncreader.c +++ b/dlls/winegstreamer/wm_asyncreader.c @@ -30,12 +30,22 @@ union async_op_data QWORD duration; void *context; } start; + struct + { + WCHAR *filename; + } open_file; + struct + { + IStream *stream; + } open_stream; };
struct async_op { enum async_op_type { + ASYNC_OP_OPEN_STREAM, + ASYNC_OP_OPEN_FILE, ASYNC_OP_START, ASYNC_OP_STOP, ASYNC_OP_CLOSE, @@ -77,7 +87,7 @@ struct async_reader CRITICAL_SECTION callback_cs; CONDITION_VARIABLE callback_cv;
- bool running; + bool opened, running; struct list async_ops;
bool user_clock; @@ -219,10 +229,7 @@ static DWORD WINAPI async_reader_callback_thread(void *arg) struct async_reader *reader = arg; static const DWORD zero; struct list *entry; - HRESULT hr = S_OK; - - IWMReaderCallback_OnStatus(reader->callback, WMT_OPENED, S_OK, - WMT_TYPE_DWORD, (BYTE *)&zero, reader->context); + HRESULT hr;
EnterCriticalSection(&reader->callback_cs);
@@ -236,6 +243,26 @@ static DWORD WINAPI async_reader_callback_thread(void *arg) hr = list_empty(&reader->async_ops) ? S_OK : E_ABORT; switch (op->type) { + case ASYNC_OP_OPEN_FILE: + if (SUCCEEDED(hr) && FAILED(hr = wm_reader_open_file(&reader->reader, op->u.open_file.filename))) + reader->running = false; + free(op->u.open_file.filename); + goto case_ASYNC_OP_OPEN; + + case ASYNC_OP_OPEN_STREAM: + if (SUCCEEDED(hr) && FAILED(hr = wm_reader_open_stream(&reader->reader, op->u.open_stream.stream))) + reader->running = false; + IStream_Release(op->u.open_stream.stream); + goto case_ASYNC_OP_OPEN; + + case_ASYNC_OP_OPEN: + reader->opened = SUCCEEDED(hr); + LeaveCriticalSection(&reader->callback_cs); + IWMReaderCallback_OnStatus(reader->callback, WMT_OPENED, hr, + WMT_TYPE_DWORD, (BYTE *)&zero, reader->context); + EnterCriticalSection(&reader->callback_cs); + break; + case ASYNC_OP_START: { reader->context = op->u.start.context; @@ -282,6 +309,10 @@ static DWORD WINAPI async_reader_callback_thread(void *arg)
LeaveCriticalSection(&reader->callback_cs);
+ if (reader->opened) + wm_reader_close(&reader->reader); + reader->opened = false; + TRACE("Reader is stopping; exiting.\n"); return 0; } @@ -386,18 +417,26 @@ static HRESULT WINAPI WMReader_Open(IWMReader *iface, const WCHAR *url, IWMReaderCallback *callback, void *context) { struct async_reader *reader = impl_from_IWMReader(iface); + union async_op_data data; HRESULT hr;
TRACE("reader %p, url %s, callback %p, context %p.\n", reader, debugstr_w(url), callback, context);
+ if (!(data.open_file.filename = wcsdup(url))) + return E_OUTOFMEMORY; + EnterCriticalSection(&reader->cs);
- if (SUCCEEDED(hr = wm_reader_open_file(&reader->reader, url)) - && FAILED(hr = async_reader_open(reader, callback, context))) - wm_reader_close(&reader->reader); + if (reader->opened) + hr = E_UNEXPECTED; + else if (SUCCEEDED(hr = async_reader_queue_op(reader, ASYNC_OP_OPEN_FILE, &data))) + hr = async_reader_open(reader, callback, context);
LeaveCriticalSection(&reader->cs); + + if (FAILED(hr)) + free(data.open_file.filename); return hr; }
@@ -410,11 +449,10 @@ static HRESULT WINAPI WMReader_Close(IWMReader *iface)
EnterCriticalSection(&reader->cs);
- if (SUCCEEDED(hr = async_reader_queue_op(reader, ASYNC_OP_CLOSE, NULL))) - { + if (!reader->callback_thread) + hr = NS_E_INVALID_REQUEST; + else if (SUCCEEDED(hr = async_reader_queue_op(reader, ASYNC_OP_CLOSE, NULL))) async_reader_close(reader); - hr = wm_reader_close(&reader->reader); - }
LeaveCriticalSection(&reader->cs);
@@ -857,17 +895,24 @@ static HRESULT WINAPI WMReaderAdvanced2_OpenStream(IWMReaderAdvanced6 *iface, IStream *stream, IWMReaderCallback *callback, void *context) { struct async_reader *reader = impl_from_IWMReaderAdvanced6(iface); + union async_op_data data; HRESULT hr;
TRACE("reader %p, stream %p, callback %p, context %p.\n", reader, stream, callback, context);
+ IStream_AddRef((data.open_stream.stream = stream)); + EnterCriticalSection(&reader->cs);
- if (SUCCEEDED(hr = wm_reader_open_stream(&reader->reader, stream)) - && FAILED(hr = async_reader_open(reader, callback, context))) - wm_reader_close(&reader->reader); + if (reader->opened) + hr = E_UNEXPECTED; + else if (SUCCEEDED(hr = async_reader_queue_op(reader, ASYNC_OP_OPEN_STREAM, &data))) + hr = async_reader_open(reader, callback, context);
LeaveCriticalSection(&reader->cs); + + if (FAILED(hr)) + IStream_Release(data.open_stream.stream); return hr; }
@@ -1732,8 +1777,6 @@ static void async_reader_destroy(struct wm_reader *iface) reader->cs.DebugInfo->Spare[0] = 0; DeleteCriticalSection(&reader->cs);
- wm_reader_close(&reader->reader); - wm_reader_cleanup(&reader->reader); free(reader); }
On 9/14/22 05:01, Rémi Bernon wrote:
@@ -236,6 +243,26 @@ static DWORD WINAPI async_reader_callback_thread(void *arg) hr = list_empty(&reader->async_ops) ? S_OK : E_ABORT; switch (op->type) {
case ASYNC_OP_OPEN_FILE:if (SUCCEEDED(hr) && FAILED(hr = wm_reader_open_file(&reader->reader, op->u.open_file.filename)))reader->running = false;free(op->u.open_file.filename);goto case_ASYNC_OP_OPEN;case ASYNC_OP_OPEN_STREAM:if (SUCCEEDED(hr) && FAILED(hr = wm_reader_open_stream(&reader->reader, op->u.open_stream.stream)))reader->running = false;IStream_Release(op->u.open_stream.stream);goto case_ASYNC_OP_OPEN;case_ASYNC_OP_OPEN:reader->opened = SUCCEEDED(hr);LeaveCriticalSection(&reader->callback_cs);IWMReaderCallback_OnStatus(reader->callback, WMT_OPENED, hr,WMT_TYPE_DWORD, (BYTE *)&zero, reader->context);EnterCriticalSection(&reader->callback_cs);break;
Please don't do this, it's unidiomatic and (especially with the current naming) misleading. In this case I think a helper function would be fine.
case ASYNC_OP_START: { reader->context = op->u.start.context;@@ -282,6 +309,10 @@ static DWORD WINAPI async_reader_callback_thread(void *arg)
LeaveCriticalSection(&reader->callback_cs);
- if (reader->opened)
wm_reader_close(&reader->reader);- reader->opened = false;
TRACE("Reader is stopping; exiting.\n"); return 0;@@ -386,18 +417,26 @@ static HRESULT WINAPI WMReader_Open(IWMReader *iface, const WCHAR *url, IWMReaderCallback *callback, void *context) { struct async_reader *reader = impl_from_IWMReader(iface);
union async_op_data data; HRESULT hr;
TRACE("reader %p, url %s, callback %p, context %p.\n", reader, debugstr_w(url), callback, context);
if (!(data.open_file.filename = wcsdup(url)))
return E_OUTOFMEMORY;
It's admittedly not exactly obvious to me why we need to execute wm_reader_open_file() / wm_reader_open_stream() on the callback thread. It wouldn't surprise me if that's how native behaves (not that I'm sure there's any way to tell?) and it doesn't seem *wrong* either, but it'd be nice to have clearer motivation, especially since it seems to add a bit of extra complexity.
EnterCriticalSection(&reader->cs);
- if (SUCCEEDED(hr = wm_reader_open_file(&reader->reader, url))
&& FAILED(hr = async_reader_open(reader, callback, context)))wm_reader_close(&reader->reader);
- if (reader->opened)
hr = E_UNEXPECTED;
We set reader->opened on the callback thread, but check it from the application thread. This actually *is* correct, I think (hooray, Windows), but it looks wrong, so it could do with a comment.
That or we just implement it the correct-looking way anyway, i.e. always set it from the application thread. In that case I don't think we even need the "opened" variable, but instead can use "reader->callback_thread" like in IWMReader::Close(). Having symmetry there probably wouldn't hurt either way.
(Resending because the gitlab bridge broke. Sorry for the delay.)
On 9/14/22 05:01, Rémi Bernon wrote:
@@ -236,6 +243,26 @@ static DWORD WINAPI async_reader_callback_thread(void *arg) hr = list_empty(&reader->async_ops) ? S_OK : E_ABORT; switch (op->type) {
case ASYNC_OP_OPEN_FILE:if (SUCCEEDED(hr) && FAILED(hr = wm_reader_open_file(&reader->reader, op->u.open_file.filename)))reader->running = false;free(op->u.open_file.filename);goto case_ASYNC_OP_OPEN;case ASYNC_OP_OPEN_STREAM:if (SUCCEEDED(hr) && FAILED(hr = wm_reader_open_stream(&reader->reader, op->u.open_stream.stream)))reader->running = false;IStream_Release(op->u.open_stream.stream);goto case_ASYNC_OP_OPEN;case_ASYNC_OP_OPEN:reader->opened = SUCCEEDED(hr);LeaveCriticalSection(&reader->callback_cs);IWMReaderCallback_OnStatus(reader->callback, WMT_OPENED, hr,WMT_TYPE_DWORD, (BYTE *)&zero, reader->context);EnterCriticalSection(&reader->callback_cs);break;
Please don't do this, it's unidiomatic and (especially with the current naming) misleading. In this case I think a helper function would be fine.
case ASYNC_OP_START: { reader->context = op->u.start.context;@@ -282,6 +309,10 @@ static DWORD WINAPI async_reader_callback_thread(void *arg)
LeaveCriticalSection(&reader->callback_cs);
- if (reader->opened)
wm_reader_close(&reader->reader);- reader->opened = false;
TRACE("Reader is stopping; exiting.\n"); return 0;@@ -386,18 +417,26 @@ static HRESULT WINAPI WMReader_Open(IWMReader *iface, const WCHAR *url, IWMReaderCallback *callback, void *context) { struct async_reader *reader = impl_from_IWMReader(iface);
union async_op_data data; HRESULT hr;
TRACE("reader %p, url %s, callback %p, context %p.\n", reader, debugstr_w(url), callback, context);
if (!(data.open_file.filename = wcsdup(url)))
return E_OUTOFMEMORY;
It's admittedly not exactly obvious to me why we need to execute wm_reader_open_file() / wm_reader_open_stream() on the callback thread. It wouldn't surprise me if that's how native behaves (not that I'm sure there's any way to tell?) and it doesn't seem *wrong* either, but it'd be nice to have clearer motivation, especially since it seems to add a bit of extra complexity.
EnterCriticalSection(&reader->cs);
- if (SUCCEEDED(hr = wm_reader_open_file(&reader->reader, url))
&& FAILED(hr = async_reader_open(reader, callback, context)))wm_reader_close(&reader->reader);
- if (reader->opened)
hr = E_UNEXPECTED;
We set reader->opened on the callback thread, but check it from the application thread. This actually *is* correct, I think (hooray, Windows), but it looks wrong, so it could do with a comment.
That or we just implement it the correct-looking way anyway, i.e. always set it from the application thread. In that case I don't think we even need the "opened" variable, but instead can use "reader->callback_thread" like in IWMReader::Close(). Having symmetry there probably wouldn't hurt either way.
-
Rémi Bernon -
Rémi Bernon (＠rbernon)
-
Zebediah Figura