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 ---
This is needed to behave like Windows and pass the tests in the next patch, because is_renderer queries the filter for IMediaSeeking.
dlls/quartz/filtergraph.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/dlls/quartz/filtergraph.c b/dlls/quartz/filtergraph.c index 0de16c4..4b6d8d5 100644 --- a/dlls/quartz/filtergraph.c +++ b/dlls/quartz/filtergraph.c @@ -686,9 +686,7 @@ static HRESULT WINAPI FilterGraph2_AddFilter(IFilterGraph2 *iface, entry->sorting = FALSE; ++graph->version;
- if (is_renderer(entry)) - ++graph->nRenderers; - + graph->nRenderers = -1; return duplicate_name ? VFW_S_DUPLICATE_NAME : hr; }
@@ -761,9 +759,7 @@ static HRESULT WINAPI FilterGraph2_RemoveFilter(IFilterGraph2 *iface, IBaseFilte hr = IBaseFilter_JoinFilterGraph(pFilter, NULL, NULL); if (SUCCEEDED(hr)) { - if (is_renderer(entry)) - --This->nRenderers; - + This->nRenderers = -1; IBaseFilter_SetSyncSource(pFilter, NULL); IBaseFilter_Release(pFilter); if (entry->seeking) @@ -5258,6 +5254,14 @@ static HRESULT WINAPI MediaFilter_Run(IMediaFilter *iface, REFERENCE_TIME start) stream_start += 500000; }
+ if (graph->nRenderers == -1) + { + graph->nRenderers = 0; + LIST_FOR_EACH_ENTRY(filter, &graph->filters, struct filter, entry) + if (is_renderer(filter)) + ++graph->nRenderers; + } + LIST_FOR_EACH_ENTRY(filter, &graph->sorted_filters, struct filter, sorted_entry) { filter_hr = IBaseFilter_Run(filter->filter, stream_start);
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com --- dlls/quartz/tests/filtergraph.c | 45 +++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)
diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c index 9988dca..2c787c4 100644 --- a/dlls/quartz/tests/filtergraph.c +++ b/dlls/quartz/tests/filtergraph.c @@ -26,6 +26,35 @@ #include "wine/heap.h" #include "wine/test.h"
+#define DEFINE_EXPECT(func) \ + static BOOL expect_ ## func = FALSE, called_ ## func = FALSE + +#define SET_EXPECT(func) \ + do { called_ ## func = FALSE; expect_ ## func = TRUE; } while(0) + +#define CHECK_EXPECT2(func) \ + do { \ + ok(expect_ ##func, "unexpected call " #func "\n"); \ + called_ ## func = TRUE; \ + }while(0) + +#define CHECK_EXPECT(func) \ + do { \ + CHECK_EXPECT2(func); \ + expect_ ## func = FALSE; \ + }while(0) + +#define CHECK_CALLED(func) \ + do { \ + ok(called_ ## func, "expected " #func "\n"); \ + expect_ ## func = called_ ## func = FALSE; \ + }while(0) + +#define CLEAR_CALLED(func) \ + expect_ ## func = called_ ## func = FALSE + +DEFINE_EXPECT(QI_IMediaSeeking); + static const GUID testguid = {0xabbccdde};
typedef struct TestFilterImpl @@ -1273,6 +1302,7 @@ static HRESULT WINAPI testfilter_QueryInterface(IBaseFilter *iface, REFIID iid, } else if (IsEqualGUID(iid, &IID_IMediaSeeking) && filter->IMediaSeeking_iface.lpVtbl) { + CHECK_EXPECT2(QI_IMediaSeeking); *out = &filter->IMediaSeeking_iface; } else if (IsEqualGUID(iid, &IID_IReferenceClock) && filter->IReferenceClock_iface.lpVtbl) @@ -3345,8 +3375,10 @@ todo_wine static HRESULT check_ec_complete(IFilterGraph2 *graph, IBaseFilter *filter) { IMediaEventSink *eventsink; + BOOL has_seeking = FALSE; LONG_PTR param1, param2; IMediaControl *control; + IMediaSeeking *seeking; IMediaEvent *eventsrc; HRESULT hr, ret_hr; LONG code; @@ -3355,7 +3387,17 @@ static HRESULT check_ec_complete(IFilterGraph2 *graph, IBaseFilter *filter) IFilterGraph2_QueryInterface(graph, &IID_IMediaEvent, (void **)&eventsrc); IFilterGraph2_QueryInterface(graph, &IID_IMediaEventSink, (void **)&eventsink);
+ SET_EXPECT(QI_IMediaSeeking); + if (SUCCEEDED(IBaseFilter_QueryInterface(filter, &IID_IMediaSeeking, (void **)&seeking))) + { + IMediaSeeking_Release(seeking); + has_seeking = TRUE; + } + CLEAR_CALLED(QI_IMediaSeeking); + + if (has_seeking) SET_EXPECT(QI_IMediaSeeking); IMediaControl_Run(control); + if (has_seeking) CHECK_CALLED(QI_IMediaSeeking);
hr = IMediaEvent_GetEvent(eventsrc, &code, ¶m1, ¶m2, 0); ok(hr == E_ABORT, "Got hr %#x.\n", hr); @@ -3766,9 +3808,12 @@ static void test_graph_seeking(void)
filter1.seek_caps = AM_SEEKING_CanDoSegments | AM_SEEKING_CanGetCurrentPos; filter2.seek_caps = AM_SEEKING_CanDoSegments | AM_SEEKING_CanGetDuration; + + SET_EXPECT(QI_IMediaSeeking); hr = IMediaSeeking_GetCapabilities(seeking, &caps); ok(hr == S_OK, "Got hr %#x.\n", hr); ok(caps == AM_SEEKING_CanDoSegments, "Got caps %#x.\n", caps); + CHECK_CALLED(QI_IMediaSeeking);
caps = AM_SEEKING_CanDoSegments | AM_SEEKING_CanGetCurrentPos; hr = IMediaSeeking_CheckCapabilities(seeking, &caps);
On 4/6/20 8:07 AM, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com
dlls/quartz/tests/filtergraph.c | 45 +++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)
diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c index 9988dca..2c787c4 100644 --- a/dlls/quartz/tests/filtergraph.c +++ b/dlls/quartz/tests/filtergraph.c @@ -26,6 +26,35 @@ #include "wine/heap.h" #include "wine/test.h"
+#define DEFINE_EXPECT(func) \
- static BOOL expect_ ## func = FALSE, called_ ## func = FALSE
+#define SET_EXPECT(func) \
- do { called_ ## func = FALSE; expect_ ## func = TRUE; } while(0)
+#define CHECK_EXPECT2(func) \
- do { \
ok(expect_ ##func, "unexpected call " #func "\n"); \called_ ## func = TRUE; \- }while(0)
+#define CHECK_EXPECT(func) \
- do { \
CHECK_EXPECT2(func); \expect_ ## func = FALSE; \- }while(0)
+#define CHECK_CALLED(func) \
- do { \
ok(called_ ## func, "expected " #func "\n"); \expect_ ## func = called_ ## func = FALSE; \- }while(0)
+#define CLEAR_CALLED(func) \
- expect_ ## func = called_ ## func = FALSE
+DEFINE_EXPECT(QI_IMediaSeeking);
static const GUID testguid = {0xabbccdde};
typedef struct TestFilterImpl @@ -1273,6 +1302,7 @@ static HRESULT WINAPI testfilter_QueryInterface(IBaseFilter *iface, REFIID iid, } else if (IsEqualGUID(iid, &IID_IMediaSeeking) && filter->IMediaSeeking_iface.lpVtbl) {
CHECK_EXPECT2(QI_IMediaSeeking); *out = &filter->IMediaSeeking_iface;@@ -3345,8 +3375,10 @@ todo_wine static HRESULT check_ec_complete(IFilterGraph2 *graph, IBaseFilter *filter) { IMediaEventSink *eventsink;
- BOOL has_seeking = FALSE; LONG_PTR param1, param2; IMediaControl *control;
- IMediaSeeking *seeking; IMediaEvent *eventsrc; HRESULT hr, ret_hr; LONG code;
@@ -3355,7 +3387,17 @@ static HRESULT check_ec_complete(IFilterGraph2 *graph, IBaseFilter *filter) IFilterGraph2_QueryInterface(graph, &IID_IMediaEvent, (void **)&eventsrc); IFilterGraph2_QueryInterface(graph, &IID_IMediaEventSink, (void **)&eventsink);
SET_EXPECT(QI_IMediaSeeking);
if (SUCCEEDED(IBaseFilter_QueryInterface(filter, &IID_IMediaSeeking, (void **)&seeking)))
{
IMediaSeeking_Release(seeking);has_seeking = TRUE;}
CLEAR_CALLED(QI_IMediaSeeking);
if (has_seeking) SET_EXPECT(QI_IMediaSeeking); IMediaControl_Run(control);
if (has_seeking) CHECK_CALLED(QI_IMediaSeeking);
hr = IMediaEvent_GetEvent(eventsrc, &code, ¶m1, ¶m2, 0); ok(hr == E_ABORT, "Got hr %#x.\n", hr);
@@ -3766,9 +3808,12 @@ static void test_graph_seeking(void)
filter1.seek_caps = AM_SEEKING_CanDoSegments | AM_SEEKING_CanGetCurrentPos; filter2.seek_caps = AM_SEEKING_CanDoSegments | AM_SEEKING_CanGetDuration;
SET_EXPECT(QI_IMediaSeeking); hr = IMediaSeeking_GetCapabilities(seeking, &caps); ok(hr == S_OK, "Got hr %#x.\n", hr); ok(caps == AM_SEEKING_CanDoSegments, "Got caps %#x.\n", caps);
CHECK_CALLED(QI_IMediaSeeking);
caps = AM_SEEKING_CanDoSegments | AM_SEEKING_CanGetCurrentPos; hr = IMediaSeeking_CheckCapabilities(seeking, &caps);
Does "The Legend of Heroes" actually depend on IMediaSeeking being queried at a certain time, or is this just a way of testing that it's not queried every time we do a seeking call?
If the latter, I think it would be better to test the part that actually matters, i.e. that IMediaSeeking::Release() is only called once, and only when the graph is torn down. That way you don't need patch 2/3.
(As an aside, I've always felt sort of lukewarm about the CHECK_EXPECT framework; I think a simple global variable such as is used in the qasf:dmowrapper tests is more transparent and flexible.)
On 06/04/2020 20:09, Zebediah Figura wrote:
On 4/6/20 8:07 AM, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com
dlls/quartz/tests/filtergraph.c | 45 +++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)
diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c index 9988dca..2c787c4 100644 --- a/dlls/quartz/tests/filtergraph.c +++ b/dlls/quartz/tests/filtergraph.c @@ -26,6 +26,35 @@ #include "wine/heap.h" #include "wine/test.h"
+#define DEFINE_EXPECT(func) \
- static BOOL expect_ ## func = FALSE, called_ ## func = FALSE
+#define SET_EXPECT(func) \
- do { called_ ## func = FALSE; expect_ ## func = TRUE; } while(0)
+#define CHECK_EXPECT2(func) \
- do { \
ok(expect_ ##func, "unexpected call " #func "\n"); \called_ ## func = TRUE; \- }while(0)
+#define CHECK_EXPECT(func) \
- do { \
CHECK_EXPECT2(func); \expect_ ## func = FALSE; \- }while(0)
+#define CHECK_CALLED(func) \
- do { \
ok(called_ ## func, "expected " #func "\n"); \expect_ ## func = called_ ## func = FALSE; \- }while(0)
+#define CLEAR_CALLED(func) \
- expect_ ## func = called_ ## func = FALSE
+DEFINE_EXPECT(QI_IMediaSeeking);
static const GUID testguid = {0xabbccdde};
typedef struct TestFilterImpl
@@ -1273,6 +1302,7 @@ static HRESULT WINAPI testfilter_QueryInterface(IBaseFilter *iface, REFIID iid, } else if (IsEqualGUID(iid, &IID_IMediaSeeking) && filter->IMediaSeeking_iface.lpVtbl) {
CHECK_EXPECT2(QI_IMediaSeeking); *out = &filter->IMediaSeeking_iface; } else if (IsEqualGUID(iid, &IID_IReferenceClock) && filter->IReferenceClock_iface.lpVtbl)@@ -3345,8 +3375,10 @@ todo_wine static HRESULT check_ec_complete(IFilterGraph2 *graph, IBaseFilter *filter) { IMediaEventSink *eventsink;
- BOOL has_seeking = FALSE; LONG_PTR param1, param2; IMediaControl *control;
- IMediaSeeking *seeking; IMediaEvent *eventsrc; HRESULT hr, ret_hr; LONG code;
@@ -3355,7 +3387,17 @@ static HRESULT check_ec_complete(IFilterGraph2 *graph, IBaseFilter *filter) IFilterGraph2_QueryInterface(graph, &IID_IMediaEvent, (void **)&eventsrc); IFilterGraph2_QueryInterface(graph, &IID_IMediaEventSink, (void **)&eventsink);
SET_EXPECT(QI_IMediaSeeking);
if (SUCCEEDED(IBaseFilter_QueryInterface(filter, &IID_IMediaSeeking, (void **)&seeking)))
{
IMediaSeeking_Release(seeking);has_seeking = TRUE;}
CLEAR_CALLED(QI_IMediaSeeking);
if (has_seeking) SET_EXPECT(QI_IMediaSeeking); IMediaControl_Run(control);
if (has_seeking) CHECK_CALLED(QI_IMediaSeeking);
hr = IMediaEvent_GetEvent(eventsrc, &code, ¶m1, ¶m2, 0); ok(hr == E_ABORT, "Got hr %#x.\n", hr);
@@ -3766,9 +3808,12 @@ static void test_graph_seeking(void)
filter1.seek_caps = AM_SEEKING_CanDoSegments | AM_SEEKING_CanGetCurrentPos; filter2.seek_caps = AM_SEEKING_CanDoSegments | AM_SEEKING_CanGetDuration;
SET_EXPECT(QI_IMediaSeeking); hr = IMediaSeeking_GetCapabilities(seeking, &caps); ok(hr == S_OK, "Got hr %#x.\n", hr); ok(caps == AM_SEEKING_CanDoSegments, "Got caps %#x.\n", caps);
CHECK_CALLED(QI_IMediaSeeking);
caps = AM_SEEKING_CanDoSegments | AM_SEEKING_CanGetCurrentPos; hr = IMediaSeeking_CheckCapabilities(seeking, &caps);
Does "The Legend of Heroes" actually depend on IMediaSeeking being queried at a certain time, or is this just a way of testing that it's not queried every time we do a seeking call?
If the latter, I think it would be better to test the part that actually matters, i.e. that IMediaSeeking::Release() is only called once, and only when the graph is torn down. That way you don't need patch 2/3.
(As an aside, I've always felt sort of lukewarm about the CHECK_EXPECT framework; I think a simple global variable such as is used in the qasf:dmowrapper tests is more transparent and flexible.)
Hi Zeb,
You're right, it's only on the Release, I just wanted to make it closer to Windows, but it seems it might be too much of an implementation detail.
For the tests, I don't think calling Release itself multiple times is the real problem, but only when the ref count becomes 0, so I'll need to add a separate ref count just for testing the IMediaSeeking.
Thanks, Gabriel
-
Gabriel Ivăncescu -
Zebediah Figura