[PATCH v3 0/5] MR625: qasf: Configure, start and stop the WMReader in the ASF Reader filter.
-- v3: qasf: Configure WMReader stream selection in asf_reader_init_stream. qasf: Configure WMReader stream format in asf_reader_init_stream. qasf: Start/stop the WM reader in asf_reader_init/cleanup_stream. qasf: Implement ASF Reader filter init_stream and cleanup_stream. qasf: Wait for IWMReader_Open to complete in ASF Reader Load. https://gitlab.winehq.org/wine/wine/-/merge_requests/625
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/qasf/asfreader.c | 52 ++++++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 16 deletions(-) diff --git a/dlls/qasf/asfreader.c b/dlls/qasf/asfreader.c index 43a7a3534a8..47333ad5ed9 100644 --- a/dlls/qasf/asfreader.c +++ b/dlls/qasf/asfreader.c @@ -41,6 +41,10 @@ struct asf_reader AM_MEDIA_TYPE media_type; WCHAR *file_name; + HRESULT result; + WMT_STATUS status; + CONDITION_VARIABLE status_cv; + IWMReaderCallback *callback; IWMReader *reader; @@ -269,18 +273,33 @@ static HRESULT WINAPI file_source_Load(IFileSourceFilter *iface, LPCOLESTR file_ if (!file_name) return E_POINTER; - if (filter->file_name) + EnterCriticalSection(&filter->filter.filter_cs); + + if (filter->file_name || !(filter->file_name = wcsdup(file_name))) + { + LeaveCriticalSection(&filter->filter.filter_cs); return E_FAIL; + } - if (!(filter->file_name = wcsdup(file_name))) - return E_OUTOFMEMORY; + if (media_type && FAILED(hr = CopyMediaType(&filter->media_type, media_type))) + { + LeaveCriticalSection(&filter->filter.filter_cs); + return hr; + } - if (media_type) - CopyMediaType(&filter->media_type, media_type); + if (SUCCEEDED(hr = IWMReader_Open(filter->reader, filter->file_name, filter->callback, NULL))) + { + filter->status = -1; + while (filter->status != WMT_OPENED) + SleepConditionVariableCS(&filter->status_cv, &filter->filter.filter_cs, INFINITE); + hr = filter->result; + } - if (FAILED(hr = IWMReader_Open(filter->reader, filter->file_name, filter->callback, NULL))) + if (FAILED(hr)) WARN("Failed to open WM reader, hr %#lx.\n", hr); + LeaveCriticalSection(&filter->filter.filter_cs); + return S_OK; } @@ -378,29 +397,25 @@ static ULONG WINAPI reader_callback_Release(IWMReaderCallback *iface) return ref; } -static HRESULT WINAPI reader_callback_OnStatus(IWMReaderCallback *iface, WMT_STATUS status, HRESULT hr, +static HRESULT WINAPI reader_callback_OnStatus(IWMReaderCallback *iface, WMT_STATUS status, HRESULT result, WMT_ATTR_DATATYPE type, BYTE *value, void *context) { struct asf_reader *filter = impl_from_IWMReaderCallback(iface)->filter; AM_MEDIA_TYPE stream_media_type = {0}; DWORD i, stream_count; WCHAR name[MAX_PATH]; + HRESULT hr; - TRACE("iface %p, status %d, hr %#lx, type %d, value %p, context %p.\n", - iface, status, hr, type, value, context); + TRACE("iface %p, status %d, result %#lx, type %d, value %p, context %p.\n", + iface, status, result, type, value, context); switch (status) { case WMT_OPENED: - if (FAILED(hr)) - { - ERR("Failed to open WMReader, hr %#lx.\n", hr); - break; - } if (FAILED(hr = IWMReader_GetOutputCount(filter->reader, &stream_count))) { ERR("Failed to get WMReader output count, hr %#lx.\n", hr); - break; + stream_count = 0; } if (stream_count > ARRAY_SIZE(filter->streams)) { @@ -424,9 +439,14 @@ static HRESULT WINAPI reader_callback_OnStatus(IWMReaderCallback *iface, WMT_STA strmbase_source_init(&stream->source, &filter->filter, name, &source_ops); } filter->stream_count = stream_count; - LeaveCriticalSection(&filter->filter.filter_cs); + + filter->result = result; + filter->status = WMT_OPENED; + WakeConditionVariable(&filter->status_cv); BaseFilterImpl_IncrementPinVersion(&filter->filter); + + LeaveCriticalSection(&filter->filter.filter_cs); break; default: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/625
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/qasf/asfreader.c | 52 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/dlls/qasf/asfreader.c b/dlls/qasf/asfreader.c index 47333ad5ed9..ecfc24ba78c 100644 --- a/dlls/qasf/asfreader.c +++ b/dlls/qasf/asfreader.c @@ -192,11 +192,63 @@ static HRESULT asf_reader_query_interface(struct strmbase_filter *iface, REFIID return E_NOINTERFACE; } +static HRESULT asf_reader_init_stream(struct strmbase_filter *iface) +{ + struct asf_reader *filter = impl_from_strmbase_filter(iface); + HRESULT hr = S_OK; + int i; + + TRACE("iface %p\n", iface); + + for (i = 0; i < filter->stream_count; ++i) + { + struct asf_stream *stream = filter->streams + i; + + if (!stream->source.pin.peer) + continue; + + if (FAILED(hr = IMemAllocator_Commit(stream->source.pAllocator))) + { + WARN("Failed to commit stream %u allocator, hr %#lx\n", i, hr); + break; + } + } + + return hr; +} + +static HRESULT asf_reader_cleanup_stream(struct strmbase_filter *iface) +{ + struct asf_reader *filter = impl_from_strmbase_filter(iface); + HRESULT hr = S_OK; + int i; + + TRACE("iface %p\n", iface); + + for (i = 0; i < filter->stream_count; ++i) + { + struct asf_stream *stream = filter->streams + i; + + if (!stream->source.pin.peer) + continue; + + if (FAILED(hr = IMemAllocator_Decommit(stream->source.pAllocator))) + { + WARN("Failed to decommit stream %u allocator, hr %#lx\n", i, hr); + break; + } + } + + return hr; +} + static const struct strmbase_filter_ops filter_ops = { .filter_get_pin = asf_reader_get_pin, .filter_destroy = asf_reader_destroy, .filter_query_interface = asf_reader_query_interface, + .filter_init_stream = asf_reader_init_stream, + .filter_cleanup_stream = asf_reader_cleanup_stream, }; static HRESULT WINAPI asf_reader_DecideBufferSize(struct strmbase_source *iface, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/625
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/qasf/asfreader.c | 67 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/dlls/qasf/asfreader.c b/dlls/qasf/asfreader.c index ecfc24ba78c..0ce86d3b01f 100644 --- a/dlls/qasf/asfreader.c +++ b/dlls/qasf/asfreader.c @@ -200,6 +200,9 @@ static HRESULT asf_reader_init_stream(struct strmbase_filter *iface) TRACE("iface %p\n", iface); + while (filter->status == -1) + SleepConditionVariableCS(&filter->status_cv, &filter->filter.filter_cs, INFINITE); + for (i = 0; i < filter->stream_count; ++i) { struct asf_stream *stream = filter->streams + i; @@ -212,8 +215,28 @@ static HRESULT asf_reader_init_stream(struct strmbase_filter *iface) WARN("Failed to commit stream %u allocator, hr %#lx\n", i, hr); break; } + + if (FAILED(hr = IPin_NewSegment(stream->source.pin.peer, 0, 0, 1))) + { + WARN("Failed to start stream %u new segment, hr %#lx\n", i, hr); + break; + } } + if (FAILED(hr)) + return hr; + + if (SUCCEEDED(hr = IWMReader_Start(filter->reader, 0, 0, 1, NULL))) + { + filter->status = -1; + while (filter->status != WMT_STARTED) + SleepConditionVariableCS(&filter->status_cv, &filter->filter.filter_cs, INFINITE); + hr = filter->result; + } + + if (FAILED(hr)) + WARN("Failed to start WMReader %p, hr %#lx\n", filter->reader, hr); + return hr; } @@ -225,6 +248,20 @@ static HRESULT asf_reader_cleanup_stream(struct strmbase_filter *iface) TRACE("iface %p\n", iface); + while (filter->status == -1) + SleepConditionVariableCS(&filter->status_cv, &filter->filter.filter_cs, INFINITE); + + if (SUCCEEDED(hr = IWMReader_Stop(filter->reader))) + { + filter->status = -1; + while (filter->status != WMT_STOPPED) + SleepConditionVariableCS(&filter->status_cv, &filter->filter.filter_cs, INFINITE); + hr = filter->result; + } + + if (FAILED(hr)) + WARN("Failed to stop WMReader %p, hr %#lx\n", filter->reader, hr); + for (i = 0; i < filter->stream_count; ++i) { struct asf_stream *stream = filter->streams + i; @@ -501,6 +538,36 @@ static HRESULT WINAPI reader_callback_OnStatus(IWMReaderCallback *iface, WMT_STA LeaveCriticalSection(&filter->filter.filter_cs); break; + case WMT_END_OF_STREAMING: + EnterCriticalSection(&filter->filter.filter_cs); + for (i = 0; i < filter->stream_count; ++i) + { + struct asf_stream *stream = filter->streams + i; + + if (!stream->source.pin.peer) + continue; + + IPin_EndOfStream(stream->source.pin.peer); + } + LeaveCriticalSection(&filter->filter.filter_cs); + break; + + case WMT_STARTED: + EnterCriticalSection(&filter->filter.filter_cs); + filter->result = result; + filter->status = WMT_STARTED; + LeaveCriticalSection(&filter->filter.filter_cs); + WakeConditionVariable(&filter->status_cv); + break; + + case WMT_STOPPED: + EnterCriticalSection(&filter->filter.filter_cs); + filter->result = result; + filter->status = WMT_STOPPED; + LeaveCriticalSection(&filter->filter.filter_cs); + WakeConditionVariable(&filter->status_cv); + break; + default: WARN("Ignoring status %#x.\n", status); break; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/625
On 8/11/22 11:45, Rémi Bernon wrote:
@@ -225,6 +248,20 @@ static HRESULT asf_reader_cleanup_stream(struct strmbase_filter *iface)
TRACE("iface %p\n", iface);
+ while (filter->status == -1) + SleepConditionVariableCS(&filter->status_cv, &filter->filter.filter_cs, INFINITE);
We're always waiting for state changes to complete after making them; why do we need this?
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/qasf/asfreader.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/dlls/qasf/asfreader.c b/dlls/qasf/asfreader.c index 0ce86d3b01f..852787e1fe8 100644 --- a/dlls/qasf/asfreader.c +++ b/dlls/qasf/asfreader.c @@ -206,6 +206,7 @@ static HRESULT asf_reader_init_stream(struct strmbase_filter *iface) for (i = 0; i < filter->stream_count; ++i) { struct asf_stream *stream = filter->streams + i; + IWMOutputMediaProps *props; if (!stream->source.pin.peer) continue; @@ -216,6 +217,22 @@ static HRESULT asf_reader_init_stream(struct strmbase_filter *iface) break; } + if (FAILED(hr = IWMReader_GetOutputFormat(filter->reader, stream->index, 0, &props))) + { + WARN("Failed to get stream %u output format, hr %#lx\n", i, hr); + break; + } + + hr = IWMOutputMediaProps_SetMediaType(props, (WM_MEDIA_TYPE *)&stream->source.pin.mt); + if (SUCCEEDED(hr)) + hr = IWMReader_SetOutputProps(filter->reader, stream->index, props); + IWMOutputMediaProps_Release(props); + if (FAILED(hr)) + { + WARN("Failed to set stream %u output format, hr %#lx\n", i, hr); + break; + } + if (FAILED(hr = IPin_NewSegment(stream->source.pin.peer, 0, 0, 1))) { WARN("Failed to start stream %u new segment, hr %#lx\n", i, hr); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/625
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/qasf/asfreader.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/dlls/qasf/asfreader.c b/dlls/qasf/asfreader.c index 852787e1fe8..e564d26ed30 100644 --- a/dlls/qasf/asfreader.c +++ b/dlls/qasf/asfreader.c @@ -195,6 +195,9 @@ static HRESULT asf_reader_query_interface(struct strmbase_filter *iface, REFIID static HRESULT asf_reader_init_stream(struct strmbase_filter *iface) { struct asf_reader *filter = impl_from_strmbase_filter(iface); + WMT_STREAM_SELECTION selections[ARRAY_SIZE(filter->streams)]; + WORD stream_numbers[ARRAY_SIZE(filter->streams)]; + IWMReaderAdvanced *reader_advanced; HRESULT hr = S_OK; int i; @@ -203,11 +206,17 @@ static HRESULT asf_reader_init_stream(struct strmbase_filter *iface) while (filter->status == -1) SleepConditionVariableCS(&filter->status_cv, &filter->filter.filter_cs, INFINITE); + if (FAILED(hr = IWMReader_QueryInterface(filter->reader, &IID_IWMReaderAdvanced, (void **)&reader_advanced))) + return hr; + for (i = 0; i < filter->stream_count; ++i) { struct asf_stream *stream = filter->streams + i; IWMOutputMediaProps *props; + stream_numbers[i] = i + 1; + selections[i] = WMT_OFF; + if (!stream->source.pin.peer) continue; @@ -238,8 +247,16 @@ static HRESULT asf_reader_init_stream(struct strmbase_filter *iface) WARN("Failed to start stream %u new segment, hr %#lx\n", i, hr); break; } + + selections[i] = WMT_ON; } + if (SUCCEEDED(hr) && FAILED(hr = IWMReaderAdvanced_SetStreamsSelected(reader_advanced, + filter->stream_count, stream_numbers, selections))) + WARN("Failed to set reader %p stream selection, hr %#lx\n", filter->reader, hr); + + IWMReaderAdvanced_Release(reader_advanced); + if (FAILED(hr)) return hr; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/625
On Thu Aug 11 16:53:21 2022 +0000, **** wrote:
Zebediah Figura (she/her) replied on the mailing list: ``` On 8/11/22 11:45, R��mi Bernon wrote:
@@ -225,6 +248,20 @@ static HRESULT asf_reader_cleanup_stream(struct strmbase_filter *iface)
TRACE("iface %p\n", iface);
+ while (filter->status == -1) + SleepConditionVariableCS(&filter->status_cv, &filter->filter.filter_cs, INFINITE); We're always waiting for state changes to complete after making them; why do we need this?
wine-gitlab mailing list -- wine-gitlab(a)winehq.org To unsubscribe send an email to wine-gitlab-leave(a)winehq.org ``` I think there's always a possibility that Start / Stop gets called concurrently, and because we're leaving the filter CS while waiting for the change it's then possible to initiate another state change concurrently.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/625#note_6102
On 8/11/22 11:56, Rémi Bernon (@rbernon) wrote:
On Thu Aug 11 16:53:21 2022 +0000, **** wrote:
Zebediah Figura (she/her) replied on the mailing list: ``` On 8/11/22 11:45, Rémi Bernon wrote:
@@ -225,6 +248,20 @@ static HRESULT asf_reader_cleanup_stream(struct strmbase_filter *iface)
TRACE("iface %p\n", iface);
+ while (filter->status == -1) + SleepConditionVariableCS(&filter->status_cv, &filter->filter.filter_cs, INFINITE); We're always waiting for state changes to complete after making them; why do we need this?
I think there's always a possibility that Start / Stop gets called concurrently, and because we're leaving the filter CS while waiting for the change it's then possible to initiate another state change concurrently.
I missed this the first time, but we really shouldn't be doing that. We should use a separate CS to synchronize with the streaming thread.
On Thu Aug 11 17:09:14 2022 +0000, **** wrote:
Zebediah Figura (she/her) replied on the mailing list: ``` On 8/11/22 11:56, R��mi Bernon (@rbernon) wrote:
On Thu Aug 11 16:53:21 2022 +0000, **** wrote:
Zebediah Figura (she/her) replied on the mailing list: \`\`\` On 8/11/22 11:45, R��mi Bernon wrote:
@@ -225,6 +248,20 @@ static HRESULT asf_reader_cleanup_stream(struct strmbase_filter *iface)
TRACE("iface %p\n", iface);
+ while (filter->status == -1) + SleepConditionVariableCS(&filter->status_cv, &filter->filter.filter_cs, INFINITE); We're always waiting for state changes to complete after making them; why do we need this? \`\`\` I think there's always a possibility that Start / Stop gets called concurrently, and because we're leaving the filter CS while waiting for the change it's then possible to initiate another state change concurrently.
I missed this the first time, but we really shouldn't be doing that. We should use a separate CS to synchronize with the streaming thread. _______________________________________________ wine-gitlab mailing list -- wine-gitlab(a)winehq.org To unsubscribe send an email to wine-gitlab-leave(a)winehq.org ``` I don't think the same, we need to enter the filter CS in the callbacks to safely access the filter streams and other members in an explicit way.
Although maybe there's some hidden guarantee that we safely can, after `init_stream` and before `cleanup_stream`, I think it's not explicit and therefore risky. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/625#note_6105
On 8/11/22 12:19, Rémi Bernon (@rbernon) wrote:
On Thu Aug 11 17:09:14 2022 +0000, **** wrote:
Zebediah Figura (she/her) replied on the mailing list: ``` On 8/11/22 11:56, Rémi Bernon (@rbernon) wrote:
On Thu Aug 11 16:53:21 2022 +0000, **** wrote:
Zebediah Figura (she/her) replied on the mailing list: \`\`\` On 8/11/22 11:45, Rémi Bernon wrote:
@@ -225,6 +248,20 @@ static HRESULT asf_reader_cleanup_stream(struct strmbase_filter *iface)
TRACE("iface %p\n", iface);
+ while (filter->status == -1) + SleepConditionVariableCS(&filter->status_cv, &filter->filter.filter_cs, INFINITE); We're always waiting for state changes to complete after making them; why do we need this? \`\`\` I think there's always a possibility that Start / Stop gets called concurrently, and because we're leaving the filter CS while waiting for the change it's then possible to initiate another state change concurrently.
I missed this the first time, but we really shouldn't be doing that. We should use a separate CS to synchronize with the streaming thread. _______________________________________________ wine-gitlab mailing list -- wine-gitlab(a)winehq.org To unsubscribe send an email to wine-gitlab-leave(a)winehq.org ``` I don't think the same, we need to enter the filter CS in the callbacks to safely access the filter streams and other members in an explicit way.
Although maybe there's some hidden guarantee that we safely can, after `init_stream` and before `cleanup_stream`, I think it's not explicit and therefore risky.
If there's members we need to share with the streaming thread we should use something other than filter_cs to do it—whether that means using a different mutex, or relying on the fact that the stream list can't change while the filter is running. Somewhat awkwardly, other quartz code violates this rule, but that's also something we should fix. I do not think it is at all workable to make state change methods any less than fully serialized.
participants (2)
-
Rémi Bernon -
Zebediah Figura (she/her)