The Legend of Heroes: Trails of Cold Steel II crashes on its intro videos because it mistakenly destroys and frees its own filter when the IMediaSeeking object is destroyed, which is retrieved as a separate object when we query for it, with a refcount of 1 regardless of the filter's refcount. It does so with msvcrt operator delete, even if the filter had outstanding ref counts.
It happens to work on Windows because Windows caches the object exposing the interface.
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com --- dlls/quartz/filtergraph.c | 42 ++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 18 deletions(-)
diff --git a/dlls/quartz/filtergraph.c b/dlls/quartz/filtergraph.c index c659cf9..0de16c4 100644 --- a/dlls/quartz/filtergraph.c +++ b/dlls/quartz/filtergraph.c @@ -153,6 +153,7 @@ struct filter { struct list entry, sorted_entry; IBaseFilter *filter; + IMediaSeeking *seeking; WCHAR *name; BOOL sorting; }; @@ -569,6 +570,15 @@ static IBaseFilter *find_filter_by_name(IFilterGraphImpl *graph, const WCHAR *na return NULL; }
+static IMediaSeeking *get_filter_seeking(struct filter *filter) +{ + /* Cache the IMediaSeeking object as some broken apps actually depend on this. */ + if (!filter->seeking) + if (FAILED(IBaseFilter_QueryInterface(filter->filter, &IID_IMediaSeeking, (void**)&filter->seeking))) + filter->seeking = NULL; + return filter->seeking; +} + static BOOL has_output_pins(IBaseFilter *filter) { IEnumPins *enumpins; @@ -593,24 +603,19 @@ static BOOL has_output_pins(IBaseFilter *filter) return FALSE; }
-static BOOL is_renderer(IBaseFilter *filter) +static BOOL is_renderer(struct filter *filter) { IAMFilterMiscFlags *flags; - IMediaSeeking *seeking; BOOL ret = FALSE;
- if (SUCCEEDED(IBaseFilter_QueryInterface(filter, &IID_IAMFilterMiscFlags, (void **)&flags))) + if (SUCCEEDED(IBaseFilter_QueryInterface(filter->filter, &IID_IAMFilterMiscFlags, (void **)&flags))) { if (IAMFilterMiscFlags_GetMiscFlags(flags) & AM_FILTER_MISC_FLAGS_IS_RENDERER) ret = TRUE; IAMFilterMiscFlags_Release(flags); } - else if (SUCCEEDED(IBaseFilter_QueryInterface(filter, &IID_IMediaSeeking, (void **)&seeking))) - { - IMediaSeeking_Release(seeking); - if (!has_output_pins(filter)) - ret = TRUE; - } + else if (get_filter_seeking(filter) && !has_output_pins(filter->filter)) + ret = TRUE; return ret; }
@@ -675,12 +680,13 @@ static HRESULT WINAPI FilterGraph2_AddFilter(IFilterGraph2 *iface, }
IBaseFilter_AddRef(entry->filter = filter); + entry->seeking = NULL; list_add_head(&graph->filters, &entry->entry); list_add_head(&graph->sorted_filters, &entry->sorted_entry); entry->sorting = FALSE; ++graph->version;
- if (is_renderer(filter)) + if (is_renderer(entry)) ++graph->nRenderers;
return duplicate_name ? VFW_S_DUPLICATE_NAME : hr; @@ -755,11 +761,13 @@ static HRESULT WINAPI FilterGraph2_RemoveFilter(IFilterGraph2 *iface, IBaseFilte hr = IBaseFilter_JoinFilterGraph(pFilter, NULL, NULL); if (SUCCEEDED(hr)) { - if (is_renderer(pFilter)) + if (is_renderer(entry)) --This->nRenderers;
IBaseFilter_SetSyncSource(pFilter, NULL); IBaseFilter_Release(pFilter); + if (entry->seeking) + IMediaSeeking_Release(entry->seeking); list_remove(&entry->entry); list_remove(&entry->sorted_entry); CoTaskMemFree(entry->name); @@ -2219,13 +2227,11 @@ static HRESULT all_renderers_seek(IFilterGraphImpl *This, fnFoundSeek FoundSeek,
LIST_FOR_EACH_ENTRY(filter, &This->filters, struct filter, entry) { - IMediaSeeking *seek = NULL; + IMediaSeeking *seek = get_filter_seeking(filter);
- IBaseFilter_QueryInterface(filter->filter, &IID_IMediaSeeking, (void **)&seek); if (!seek) continue; hr = FoundSeek(This, seek, arg); - IMediaSeeking_Release(seek); if (hr_return != E_NOTIMPL) allnotimpl = FALSE; if (hr_return == S_OK || (FAILED(hr) && hr != E_NOTIMPL && SUCCEEDED(hr_return))) @@ -2429,11 +2435,11 @@ static HRESULT WINAPI MediaSeeking_GetStopPosition(IMediaSeeking *iface, LONGLON
LIST_FOR_EACH_ENTRY(filter, &graph->filters, struct filter, entry) { - if (FAILED(IBaseFilter_QueryInterface(filter->filter, &IID_IMediaSeeking, (void **)&seeking))) + seeking = get_filter_seeking(filter); + if (!seeking) continue;
filter_hr = IMediaSeeking_GetStopPosition(seeking, &filter_stop); - IMediaSeeking_Release(seeking); if (SUCCEEDED(filter_hr)) { hr = S_OK; @@ -2539,12 +2545,12 @@ static HRESULT WINAPI MediaSeeking_SetPositions(IMediaSeeking *iface, LONGLONG * { LONGLONG current = current_ptr ? *current_ptr : 0, stop = stop_ptr ? *stop_ptr : 0;
- if (FAILED(IBaseFilter_QueryInterface(filter->filter, &IID_IMediaSeeking, (void **)&seeking))) + seeking = get_filter_seeking(filter); + if (!seeking) continue;
filter_hr = IMediaSeeking_SetPositions(seeking, ¤t, current_flags | AM_SEEKING_ReturnTime, &stop, stop_flags); - IMediaSeeking_Release(seeking); if (SUCCEEDED(filter_hr)) { hr = S_OK;
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com --- dlls/quartz/tests/filtergraph.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c index 9988dca..af659a9 100644 --- a/dlls/quartz/tests/filtergraph.c +++ b/dlls/quartz/tests/filtergraph.c @@ -1165,6 +1165,7 @@ struct testfilter ULONG misc_flags;
IMediaSeeking IMediaSeeking_iface; + LONG seeking_ref; DWORD seek_caps; BOOL support_testguid; LONGLONG seek_duration, seek_current, seek_stop; @@ -1525,12 +1526,14 @@ static HRESULT WINAPI testseek_QueryInterface(IMediaSeeking *iface, REFIID iid, static ULONG WINAPI testseek_AddRef(IMediaSeeking *iface) { struct testfilter *filter = impl_from_IMediaSeeking(iface); + InterlockedIncrement(&filter->seeking_ref); return InterlockedIncrement(&filter->ref); }
static ULONG WINAPI testseek_Release(IMediaSeeking *iface) { struct testfilter *filter = impl_from_IMediaSeeking(iface); + InterlockedDecrement(&filter->seeking_ref); return InterlockedDecrement(&filter->ref); }
@@ -3530,14 +3533,20 @@ static void test_ec_complete(void) hr = check_ec_complete(graph, &filter1.IBaseFilter_iface); ok(hr == S_OK, "Got hr %#x.\n", hr);
+ ok(filter1.seeking_ref > 0, "Unexpected seeking refcount %d.\n", filter1.seeking_ref); IFilterGraph2_RemoveFilter(graph, &filter1.IBaseFilter_iface); + ok(filter1.seeking_ref == 0, "Unexpected seeking refcount %d.\n", filter1.seeking_ref); + filter1_pin.dir = PINDIR_OUTPUT; IFilterGraph2_AddFilter(graph, &filter1.IBaseFilter_iface, NULL);
hr = check_ec_complete(graph, &filter1.IBaseFilter_iface); ok(hr == E_ABORT, "Got hr %#x.\n", hr);
+ ok(filter1.seeking_ref > 0, "Unexpected seeking refcount %d.\n", filter1.seeking_ref); IFilterGraph2_RemoveFilter(graph, &filter1.IBaseFilter_iface); + ok(filter1.seeking_ref == 0, "Unexpected seeking refcount %d.\n", filter1.seeking_ref); + filter1.IMediaSeeking_iface.lpVtbl = NULL; filter1_pin.dir = PINDIR_INPUT; filter1.pin_count = 1; @@ -3769,6 +3778,8 @@ static void test_graph_seeking(void) hr = IMediaSeeking_GetCapabilities(seeking, &caps); ok(hr == S_OK, "Got hr %#x.\n", hr); ok(caps == AM_SEEKING_CanDoSegments, "Got caps %#x.\n", caps); + ok(filter1.seeking_ref > 0, "Unexpected seeking refcount %d.\n", filter1.seeking_ref); + ok(filter2.seeking_ref > 0, "Unexpected seeking refcount %d.\n", filter2.seeking_ref);
caps = AM_SEEKING_CanDoSegments | AM_SEEKING_CanGetCurrentPos; hr = IMediaSeeking_CheckCapabilities(seeking, &caps); @@ -4082,10 +4093,16 @@ static void test_graph_seeking(void) IMediaFilter_Release(filter); IMediaControl_Release(control); IMediaSeeking_Release(seeking); + + ok(filter1.seeking_ref > 0, "Unexpected seeking refcount %d.\n", filter1.seeking_ref); + ok(filter2.seeking_ref > 0, "Unexpected seeking refcount %d.\n", filter2.seeking_ref); + ref = IFilterGraph2_Release(graph); ok(!ref, "Got outstanding refcount %d.\n", hr); ok(filter1.ref == 1, "Got outstanding refcount %d.\n", filter1.ref); ok(filter2.ref == 1, "Got outstanding refcount %d.\n", filter2.ref); + ok(filter1.seeking_ref == 0, "Unexpected seeking refcount %d.\n", filter1.seeking_ref); + ok(filter2.seeking_ref == 0, "Unexpected seeking refcount %d.\n", filter2.seeking_ref); }
static void test_default_sync_source(void)
On 4/7/20 4:05 PM, Gabriel Ivăncescu wrote:
-static BOOL is_renderer(IBaseFilter *filter) +static BOOL is_renderer(struct filter *filter) { IAMFilterMiscFlags *flags;
IMediaSeeking *seeking; BOOL ret = FALSE;
if (SUCCEEDED(IBaseFilter_QueryInterface(filter, &IID_IAMFilterMiscFlags, (void **)&flags)))
- if (SUCCEEDED(IBaseFilter_QueryInterface(filter->filter, &IID_IAMFilterMiscFlags, (void **)&flags))) { if (IAMFilterMiscFlags_GetMiscFlags(flags) & AM_FILTER_MISC_FLAGS_IS_RENDERER) ret = TRUE; IAMFilterMiscFlags_Release(flags); }
- else if (SUCCEEDED(IBaseFilter_QueryInterface(filter, &IID_IMediaSeeking, (void **)&seeking)))
- {
IMediaSeeking_Release(seeking);if (!has_output_pins(filter))ret = TRUE;- }
- else if (get_filter_seeking(filter) && !has_output_pins(filter->filter))
ret = TRUE; return ret;@@ -675,12 +680,13 @@ static HRESULT WINAPI FilterGraph2_AddFilter(IFilterGraph2 *iface, }
IBaseFilter_AddRef(entry->filter = filter);
- entry->seeking = NULL; list_add_head(&graph->filters, &entry->entry); list_add_head(&graph->sorted_filters, &entry->sorted_entry); entry->sorting = FALSE; ++graph->version;
- if (is_renderer(filter))
- if (is_renderer(entry)) ++graph->nRenderers;
Helper name does not imply important state change, will it work if you queried once right after IBaseFilter_AddRef()? This looks like the only place when filter is added to the list. After that you can simply make is_renderer() check for null pointer.
On 07/04/2020 16:47, Nikolay Sivov wrote:
On 4/7/20 4:05 PM, Gabriel Ivăncescu wrote:
-static BOOL is_renderer(IBaseFilter *filter) +static BOOL is_renderer(struct filter *filter) { IAMFilterMiscFlags *flags; - IMediaSeeking *seeking; BOOL ret = FALSE; - if (SUCCEEDED(IBaseFilter_QueryInterface(filter, &IID_IAMFilterMiscFlags, (void **)&flags))) + if (SUCCEEDED(IBaseFilter_QueryInterface(filter->filter, &IID_IAMFilterMiscFlags, (void **)&flags))) { if (IAMFilterMiscFlags_GetMiscFlags(flags) & AM_FILTER_MISC_FLAGS_IS_RENDERER) ret = TRUE; IAMFilterMiscFlags_Release(flags); } - else if (SUCCEEDED(IBaseFilter_QueryInterface(filter, &IID_IMediaSeeking, (void **)&seeking))) - { - IMediaSeeking_Release(seeking); - if (!has_output_pins(filter)) - ret = TRUE; - } + else if (get_filter_seeking(filter) && !has_output_pins(filter->filter)) + ret = TRUE; return ret; } @@ -675,12 +680,13 @@ static HRESULT WINAPI FilterGraph2_AddFilter(IFilterGraph2 *iface, } IBaseFilter_AddRef(entry->filter = filter); + entry->seeking = NULL; list_add_head(&graph->filters, &entry->entry); list_add_head(&graph->sorted_filters, &entry->sorted_entry); entry->sorting = FALSE; ++graph->version; - if (is_renderer(filter)) + if (is_renderer(entry)) ++graph->nRenderers;
Helper name does not imply important state change, will it work if you queried once right after IBaseFilter_AddRef()? This looks like the only place when filter is added to the list. After that you can simply make is_renderer() check for null pointer.
Hi Nikolay,
I guess it would work for this particular case, but it would be different from how Windows does it, so I'd rather not. I don't think the state change is concerning, because it should be transparent, the rest of the code shouldn't have to be cluttered with such corner case IMO. I believe it should be enough that I commented on the helper why it's done the way it is (plus the tests on next patch).
Note that if I do it on AddRef, there's a possibility of adding side-effects if the MediaSeeking would otherwise not be used at all in another app, for example. In this case it would be queried and eventually released, which might introduce other bugs (because currently, it isn't, and neither on Windows). I personally don't think it's worth it.
Thanks, Gabriel
On 4/7/20 9:17 AM, Gabriel Ivăncescu wrote:
On 07/04/2020 16:47, Nikolay Sivov wrote:
On 4/7/20 4:05 PM, Gabriel Ivăncescu wrote:
-static BOOL is_renderer(IBaseFilter *filter) +static BOOL is_renderer(struct filter *filter) { IAMFilterMiscFlags *flags; - IMediaSeeking *seeking; BOOL ret = FALSE; - if (SUCCEEDED(IBaseFilter_QueryInterface(filter, &IID_IAMFilterMiscFlags, (void **)&flags))) + if (SUCCEEDED(IBaseFilter_QueryInterface(filter->filter, &IID_IAMFilterMiscFlags, (void **)&flags))) { if (IAMFilterMiscFlags_GetMiscFlags(flags) & AM_FILTER_MISC_FLAGS_IS_RENDERER) ret = TRUE; IAMFilterMiscFlags_Release(flags); } - else if (SUCCEEDED(IBaseFilter_QueryInterface(filter, &IID_IMediaSeeking, (void **)&seeking))) - { - IMediaSeeking_Release(seeking); - if (!has_output_pins(filter)) - ret = TRUE; - } + else if (get_filter_seeking(filter) && !has_output_pins(filter->filter)) + ret = TRUE; return ret; } @@ -675,12 +680,13 @@ static HRESULT WINAPI FilterGraph2_AddFilter(IFilterGraph2 *iface, } IBaseFilter_AddRef(entry->filter = filter); + entry->seeking = NULL; list_add_head(&graph->filters, &entry->entry); list_add_head(&graph->sorted_filters, &entry->sorted_entry); entry->sorting = FALSE; ++graph->version; - if (is_renderer(filter)) + if (is_renderer(entry)) ++graph->nRenderers;
Helper name does not imply important state change, will it work if you queried once right after IBaseFilter_AddRef()? This looks like the only place when filter is added to the list. After that you can simply make is_renderer() check for null pointer.
Hi Nikolay,
I guess it would work for this particular case, but it would be different from how Windows does it, so I'd rather not. I don't think the state change is concerning, because it should be transparent, the rest of the code shouldn't have to be cluttered with such corner case IMO. I believe it should be enough that I commented on the helper why it's done the way it is (plus the tests on next patch).
Note that if I do it on AddRef, there's a possibility of adding side-effects if the MediaSeeking would otherwise not be used at all in another app, for example. In this case it would be queried and eventually released, which might introduce other bugs (because currently, it isn't, and neither on Windows). I personally don't think it's worth it.
Broadly I don't think it's worth spending effort trying to match Windows' query patterns exactly, even if applications do occasionally mess up COM in some very creative ways. If we do, we'd want at least a comment or tests explaining why we're not using simpler code. Note also that this patch isn't doing a whole lot, since is_renderer() is called in AddFilter() and thus IMediaSeeking will end up being queried anyway for every filter that doesn't expose IAMFilterMiscFlags.
Thanks, Gabriel
-
Gabriel Ivăncescu -
Nikolay Sivov -
Zebediah Figura