Testing on Windows confirmed that IGraphBuilder::RenderFile removes the source filter it adds if the file cannot be played. This was resulting in leaked file handles and threads if a program tried to play media without a proper set of filters installed.
Signed-off-by: Tim Clem tclem@codeweavers.com --- dlls/quartz/tests/filtergraph.c | 54 +++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)
diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c index fdcfcead0be..a509a07872c 100644 --- a/dlls/quartz/tests/filtergraph.c +++ b/dlls/quartz/tests/filtergraph.c @@ -4186,6 +4186,59 @@ static void test_ec_complete(void) ok(filter3.ref == 1, "Got outstanding refcount %d.\n", filter3.ref); }
+static void test_renderfile_failure(void) +{ + static const char bogus_data[20] = {0xde, 0xad, 0xbe, 0xef}; + + struct testfilter testfilter; + IFilterGraph2 *graph; + IEnumFilters *filterenum; + IBaseFilter *filter; + HRESULT hr; + BOOL saw_testfilter = FALSE; + FILTER_INFO filter_info; + const WCHAR *filename; + + /* Windows removes the source filter from the graph if a RenderFile + * call fails. It leaves the rest of the graph intact. */ + + graph = create_graph(); + testfilter_init(&testfilter, NULL, 0); + hr = IFilterGraph2_AddFilter(graph, &testfilter.IBaseFilter_iface, L"dummy"); + ok(hr == S_OK, "Got hr %#x.\n", hr); + + filename = create_file(L"test.nonsense", bogus_data, sizeof(bogus_data)); + hr = IFilterGraph2_RenderFile(graph, filename, NULL); + if (SUCCEEDED(hr)) { + skip("Expected RenderFile to fail on invalid file.\n"); + goto out; + } + + hr = IFilterGraph2_EnumFilters(graph, &filterenum); + ok(hr == S_OK, "Got hr %#x.\n", hr); + + while (IEnumFilters_Next(filterenum, 1, &filter, NULL) == S_OK) + { + if (!saw_testfilter && filter == &testfilter.IBaseFilter_iface) + saw_testfilter = TRUE; + else { + hr = IBaseFilter_QueryFilterInfo(filter, &filter_info); + ok(hr == S_OK, "Got hr %#x.\n", hr); + todo_wine ok(0, "Unexpected filter %p (%s) left in graph.\n", + filter, wine_dbgstr_w(filter_info.achName)); + } + + IBaseFilter_Release(filter); + } + IEnumFilters_Release(filterenum); + + ok(saw_testfilter, "Expected testfilter to remain in graph.\n"); + +out: + IFilterGraph2_Release(graph); + DeleteFileW(filename); +} + /* Remove and re-add the filter, to flush the graph's internal * IMediaSeeking cache. Don't expose IMediaSeeking when adding, to show * that it's only queried when needed. */ @@ -5645,6 +5698,7 @@ START_TEST(filtergraph) test_sync_source(); test_filter_state(); test_ec_complete(); + test_renderfile_failure(); test_graph_seeking(); test_default_sync_source(); test_add_source_filter();
Signed-off-by: Tim Clem tclem@codeweavers.com --- dlls/quartz/filtergraph.c | 7 ++++++- dlls/quartz/tests/filtergraph.c | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/dlls/quartz/filtergraph.c b/dlls/quartz/filtergraph.c index f509bdad332..026b9cb8f72 100644 --- a/dlls/quartz/filtergraph.c +++ b/dlls/quartz/filtergraph.c @@ -1430,8 +1430,13 @@ static HRESULT WINAPI FilterGraph2_RenderFile(IFilterGraph2 *iface, LPCWSTR lpcw } IEnumPins_Release(penumpins);
- if (!any) + if (!any) { + hr = IFilterGraph2_RemoveFilter(iface, preader); + if (FAILED(hr)) { + WARN("Unable to remove reader for unplayable source, hr: %#x\n", hr); + } hr = VFW_E_CANNOT_RENDER; + } else if (partial) hr = VFW_S_PARTIAL_RENDER; else diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c index a509a07872c..70ab490dbe3 100644 --- a/dlls/quartz/tests/filtergraph.c +++ b/dlls/quartz/tests/filtergraph.c @@ -4224,7 +4224,7 @@ static void test_renderfile_failure(void) else { hr = IBaseFilter_QueryFilterInfo(filter, &filter_info); ok(hr == S_OK, "Got hr %#x.\n", hr); - todo_wine ok(0, "Unexpected filter %p (%s) left in graph.\n", + ok(0, "Unexpected filter %p (%s) left in graph.\n", filter, wine_dbgstr_w(filter_info.achName)); }
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=93232
Your paranoid android.
=== debiant2 (build log) ===
winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind: winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind: winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind: winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind: winegstreamer: error: typefind: Die Art des Datenstroms konnte nicht ermittelt werden. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind: winegstreamer: error: typefind: Die Art des Datenstroms konnte nicht ermittelt werden. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind: winegstreamer: error: typefind: Impossible de déterminer le type du flux. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind: winegstreamer: error: typefind: Impossible de déterminer le type du flux. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind: winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind: winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind: winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind: winegstreamer: error: typefind: ストリームの種類を判別できません winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: ストリームの種類を判別できません winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: 无法确定流类型。 winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind: winegstreamer: error: typefind: 无法确定流类型。 winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind:
=== debiant2 (build log) ===
winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind: winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind: winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind: winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind:
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=93231
Your paranoid android.
=== debiant2 (build log) ===
winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind: winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind: winegstreamer: error: typefind: Die Art des Datenstroms konnte nicht ermittelt werden. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind: winegstreamer: error: typefind: Die Art des Datenstroms konnte nicht ermittelt werden. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind: winegstreamer: error: typefind: Impossible de déterminer le type du flux. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Impossible de déterminer le type du flux. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind: winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind: winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind: winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind: winegstreamer: error: typefind: ストリームの種類を判別できません winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: ストリームの種類を判別できません winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: 无法确定流类型。 winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind: winegstreamer: error: typefind: 无法确定流类型。 winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind
=== debiant2 (build log) ===
winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind: winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind: winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin8/GstDecodeBin:decodebin8/GstTypeFindElement:typefind: winegstreamer: error: typefind: Could not determine type of stream. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1161): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind winegstreamer: error: typefind: Internal data stream error. winegstreamer: error: typefind: ../plugins/elements/gsttypefindelement.c(1232): gst_type_find_element_loop (): /GstBin:bin9/GstDecodeBin:decodebin9/GstTypeFindElement:typefind:
On 6/28/21 1:46 PM, Tim Clem wrote:
Testing on Windows confirmed that IGraphBuilder::RenderFile removes the source filter it adds if the file cannot be played. This was resulting in leaked file handles and threads if a program tried to play media without a proper set of filters installed.
Signed-off-by: Tim Clem tclem@codeweavers.com
dlls/quartz/tests/filtergraph.c | 54 +++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)
diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c index fdcfcead0be..a509a07872c 100644 --- a/dlls/quartz/tests/filtergraph.c +++ b/dlls/quartz/tests/filtergraph.c @@ -4186,6 +4186,59 @@ static void test_ec_complete(void) ok(filter3.ref == 1, "Got outstanding refcount %d.\n", filter3.ref); }
+static void test_renderfile_failure(void) +{
- static const char bogus_data[20] = {0xde, 0xad, 0xbe, 0xef};
- struct testfilter testfilter;
- IFilterGraph2 *graph;
- IEnumFilters *filterenum;
- IBaseFilter *filter;
- HRESULT hr;
- BOOL saw_testfilter = FALSE;
- FILTER_INFO filter_info;
- const WCHAR *filename;
- /* Windows removes the source filter from the graph if a RenderFile
* call fails. It leaves the rest of the graph intact. */- graph = create_graph();
- testfilter_init(&testfilter, NULL, 0);
- hr = IFilterGraph2_AddFilter(graph, &testfilter.IBaseFilter_iface, L"dummy");
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- filename = create_file(L"test.nonsense", bogus_data, sizeof(bogus_data));
- hr = IFilterGraph2_RenderFile(graph, filename, NULL);
- if (SUCCEEDED(hr)) {
skip("Expected RenderFile to fail on invalid file.\n");goto out;- }
Any reason why this isn't "ok(hr == VFW_E_CANNOT_RENDER, ...)"?
- hr = IFilterGraph2_EnumFilters(graph, &filterenum);
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- while (IEnumFilters_Next(filterenum, 1, &filter, NULL) == S_OK)
- {
if (!saw_testfilter && filter == &testfilter.IBaseFilter_iface)saw_testfilter = TRUE;else {hr = IBaseFilter_QueryFilterInfo(filter, &filter_info);ok(hr == S_OK, "Got hr %#x.\n", hr);todo_wine ok(0, "Unexpected filter %p (%s) left in graph.\n",filter, wine_dbgstr_w(filter_info.achName));}IBaseFilter_Release(filter);- }
- IEnumFilters_Release(filterenum);
- ok(saw_testfilter, "Expected testfilter to remain in graph.\n");
Or, even easier:
ok(testfilter.graph == (IFilterGraph *)graph, "Got graph %p.\n", testfilter.graph);
+out:
- IFilterGraph2_Release(graph);
- DeleteFileW(filename);
Can you please check for failure from DeleteFile() here? I try to be extra paranoid that we aren't leaking handles to the file anywhere; we've done so in the past.
+}
- /* Remove and re-add the filter, to flush the graph's internal
- IMediaSeeking cache. Don't expose IMediaSeeking when adding, to show
- that it's only queried when needed. */
@@ -5645,6 +5698,7 @@ START_TEST(filtergraph) test_sync_source(); test_filter_state(); test_ec_complete();
- test_renderfile_failure(); test_graph_seeking(); test_default_sync_source(); test_add_source_filter();
On 6/28/21 2:45 PM, Zebediah Figura (she/her) wrote:
On 6/28/21 1:46 PM, Tim Clem wrote:
Testing on Windows confirmed that IGraphBuilder::RenderFile removes the source filter it adds if the file cannot be played. This was resulting in leaked file handles and threads if a program tried to play media without a proper set of filters installed.
Signed-off-by: Tim Clem tclem@codeweavers.com
dlls/quartz/tests/filtergraph.c | 54 +++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)
diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c index fdcfcead0be..a509a07872c 100644 --- a/dlls/quartz/tests/filtergraph.c +++ b/dlls/quartz/tests/filtergraph.c @@ -4186,6 +4186,59 @@ static void test_ec_complete(void) ok(filter3.ref == 1, "Got outstanding refcount %d.\n", filter3.ref); }
+static void test_renderfile_failure(void) +{
- static const char bogus_data[20] = {0xde, 0xad, 0xbe, 0xef};
- struct testfilter testfilter;
- IFilterGraph2 *graph;
- IEnumFilters *filterenum;
- IBaseFilter *filter;
- HRESULT hr;
- BOOL saw_testfilter = FALSE;
- FILTER_INFO filter_info;
- const WCHAR *filename;
- /* Windows removes the source filter from the graph if a RenderFile
* call fails. It leaves the rest of the graph intact. */- graph = create_graph();
- testfilter_init(&testfilter, NULL, 0);
- hr = IFilterGraph2_AddFilter(graph, &testfilter.IBaseFilter_iface, L"dummy");
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- filename = create_file(L"test.nonsense", bogus_data, sizeof(bogus_data));
- hr = IFilterGraph2_RenderFile(graph, filename, NULL);
- if (SUCCEEDED(hr)) {
skip("Expected RenderFile to fail on invalid file.\n");goto out;- }
Any reason why this isn't "ok(hr == VFW_E_CANNOT_RENDER, ...)"?
- hr = IFilterGraph2_EnumFilters(graph, &filterenum);
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- while (IEnumFilters_Next(filterenum, 1, &filter, NULL) == S_OK)
- {
if (!saw_testfilter && filter == &testfilter.IBaseFilter_iface)saw_testfilter = TRUE;else {hr = IBaseFilter_QueryFilterInfo(filter, &filter_info);ok(hr == S_OK, "Got hr %#x.\n", hr);todo_wine ok(0, "Unexpected filter %p (%s) left in graph.\n",filter, wine_dbgstr_w(filter_info.achName));}IBaseFilter_Release(filter);- }
- IEnumFilters_Release(filterenum);
- ok(saw_testfilter, "Expected testfilter to remain in graph.\n");
Or, even easier:
ok(testfilter.graph == (IFilterGraph *)graph, "Got graph %p.\n", testfilter.graph);
Actually, I guess I was misreading this test a bit; it (sensibly) does two things at once. I'd recommend something like:
hr = IFilterGraph2_EnumFilters(...); ok(hr == S_OK, "Got hr %#x.\n", hr);
hr = IEnumFilters_Next(...); ok(hr == S_OK, "Got hr %#x.\n", hr); ok(filter == &testfilter.IBaseFilter_iface, "Got unexpected filter %p.\n", filter);
hr = IEnumFilters_Next(...); todo_wine ok(hr == S_FALSE, "Got hr %#x.\n", hr);
IEnumFilters_Release(...);
No need for a loop.
+out:
- IFilterGraph2_Release(graph);
- DeleteFileW(filename);
Can you please check for failure from DeleteFile() here? I try to be extra paranoid that we aren't leaking handles to the file anywhere; we've done so in the past.
+}
- /* Remove and re-add the filter, to flush the graph's internal
- IMediaSeeking cache. Don't expose IMediaSeeking when adding, to show
- that it's only queried when needed. */
@@ -5645,6 +5698,7 @@ START_TEST(filtergraph) test_sync_source(); test_filter_state(); test_ec_complete();
- test_renderfile_failure(); test_graph_seeking(); test_default_sync_source(); test_add_source_filter();
June 28, 2021 12:46 PM, "Zebediah Figura (she/her)" zfigura@codeweavers.com wrote:
On 6/28/21 1:46 PM, Tim Clem wrote:
Testing on Windows confirmed that IGraphBuilder::RenderFile removes the source filter it adds if the file cannot be played. This was resulting in leaked file handles and threads if a program tried to play media without a proper set of filters installed. Signed-off-by: Tim Clem tclem@codeweavers.com
dlls/quartz/tests/filtergraph.c | 54 +++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c index fdcfcead0be..a509a07872c 100644 --- a/dlls/quartz/tests/filtergraph.c +++ b/dlls/quartz/tests/filtergraph.c @@ -4186,6 +4186,59 @@ static void test_ec_complete(void) ok(filter3.ref == 1, "Got outstanding refcount %d.\n", filter3.ref); }
+static void test_renderfile_failure(void)
+{
- static const char bogus_data[20] = {0xde, 0xad, 0xbe, 0xef};
- struct testfilter testfilter;
- IFilterGraph2 *graph;
- IEnumFilters *filterenum;
- IBaseFilter *filter;
- HRESULT hr;
- BOOL saw_testfilter = FALSE;
- FILTER_INFO filter_info;
- const WCHAR *filename;
- /* Windows removes the source filter from the graph if a RenderFile
- call fails. It leaves the rest of the graph intact. */
- graph = create_graph();
- testfilter_init(&testfilter, NULL, 0);
- hr = IFilterGraph2_AddFilter(graph, &testfilter.IBaseFilter_iface, L"dummy");
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- filename = create_file(L"test.nonsense", bogus_data, sizeof(bogus_data));
- hr = IFilterGraph2_RenderFile(graph, filename, NULL);
- if (SUCCEEDED(hr)) {
- skip("Expected RenderFile to fail on invalid file.\n");
- goto out;
- }
Any reason why this isn't "ok(hr == VFW_E_CANNOT_RENDER, ...)"?
Windows reports VFW_E_UNSUPPORTED_STREAM in this case. The documentation on the various return values is really vague, so I thought it was best to be non-specific here.
- hr = IFilterGraph2_EnumFilters(graph, &filterenum);
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- while (IEnumFilters_Next(filterenum, 1, &filter, NULL) == S_OK)
- {
- if (!saw_testfilter && filter == &testfilter.IBaseFilter_iface)
- saw_testfilter = TRUE;
- else {
- hr = IBaseFilter_QueryFilterInfo(filter, &filter_info);
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- todo_wine ok(0, "Unexpected filter %p (%s) left in graph.\n",
- filter, wine_dbgstr_w(filter_info.achName));
- }
- IBaseFilter_Release(filter);
- }
- IEnumFilters_Release(filterenum);
- ok(saw_testfilter, "Expected testfilter to remain in graph.\n");
Or, even easier:
ok(testfilter.graph == (IFilterGraph *)graph, "Got graph %p.\n", testfilter.graph);
I updated the test as per your other email. Much simpler. Thanks for the notes.
+out:
- IFilterGraph2_Release(graph);
- DeleteFileW(filename);
Can you please check for failure from DeleteFile() here? I try to be extra paranoid that we aren't leaking handles to the file anywhere; we've done so in the past.
Good idea, especially since this *is* leaking a file handle here before the patch - the source made by RenderFile will not be closed.
+}
/* Remove and re-add the filter, to flush the graph's internal
- IMediaSeeking cache. Don't expose IMediaSeeking when adding, to show
- that it's only queried when needed. */
@@ -5645,6 +5698,7 @@ START_TEST(filtergraph) test_sync_source(); test_filter_state(); test_ec_complete();
- test_renderfile_failure();
test_graph_seeking(); test_default_sync_source(); test_add_source_filter();
On 6/28/21 5:09 PM, Tim Clem wrote:
June 28, 2021 12:46 PM, "Zebediah Figura (she/her)" zfigura@codeweavers.com wrote:
On 6/28/21 1:46 PM, Tim Clem wrote:
Testing on Windows confirmed that IGraphBuilder::RenderFile removes the source filter it adds if the file cannot be played. This was resulting in leaked file handles and threads if a program tried to play media without a proper set of filters installed. Signed-off-by: Tim Clem tclem@codeweavers.com
dlls/quartz/tests/filtergraph.c | 54 +++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c index fdcfcead0be..a509a07872c 100644 --- a/dlls/quartz/tests/filtergraph.c +++ b/dlls/quartz/tests/filtergraph.c @@ -4186,6 +4186,59 @@ static void test_ec_complete(void) ok(filter3.ref == 1, "Got outstanding refcount %d.\n", filter3.ref); }
+static void test_renderfile_failure(void)
+{
- static const char bogus_data[20] = {0xde, 0xad, 0xbe, 0xef};
- struct testfilter testfilter;
- IFilterGraph2 *graph;
- IEnumFilters *filterenum;
- IBaseFilter *filter;
- HRESULT hr;
- BOOL saw_testfilter = FALSE;
- FILTER_INFO filter_info;
- const WCHAR *filename;
- /* Windows removes the source filter from the graph if a RenderFile
- call fails. It leaves the rest of the graph intact. */
- graph = create_graph();
- testfilter_init(&testfilter, NULL, 0);
- hr = IFilterGraph2_AddFilter(graph, &testfilter.IBaseFilter_iface, L"dummy");
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- filename = create_file(L"test.nonsense", bogus_data, sizeof(bogus_data));
- hr = IFilterGraph2_RenderFile(graph, filename, NULL);
- if (SUCCEEDED(hr)) {
- skip("Expected RenderFile to fail on invalid file.\n");
- goto out;
- }
Any reason why this isn't "ok(hr == VFW_E_CANNOT_RENDER, ...)"?
Windows reports VFW_E_UNSUPPORTED_STREAM in this case. The documentation on the various return values is really vague, so I thought it was best to be non-specific here.
Vague and not always accurate, especially when it comes to error codes ;-)
In any case I'd prefer to test the actual return value here, with todo_wine if necessary.
FWIW, I suspect that RenderFile() should be doing an unconditional translation of VFW_E_CANNOT_RENDER -> VFW_E_UNSUPPORTED_STREAM. Just from quick testing I see that happening both when there's no source filter registered, and when an intermediate filter fails to decode. Not sure why there's other error codes listed there, but I can't immediately think of ways to provoke them.
- hr = IFilterGraph2_EnumFilters(graph, &filterenum);
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- while (IEnumFilters_Next(filterenum, 1, &filter, NULL) == S_OK)
- {
- if (!saw_testfilter && filter == &testfilter.IBaseFilter_iface)
- saw_testfilter = TRUE;
- else {
- hr = IBaseFilter_QueryFilterInfo(filter, &filter_info);
- ok(hr == S_OK, "Got hr %#x.\n", hr);
- todo_wine ok(0, "Unexpected filter %p (%s) left in graph.\n",
- filter, wine_dbgstr_w(filter_info.achName));
- }
- IBaseFilter_Release(filter);
- }
- IEnumFilters_Release(filterenum);
- ok(saw_testfilter, "Expected testfilter to remain in graph.\n");
Or, even easier:
ok(testfilter.graph == (IFilterGraph *)graph, "Got graph %p.\n", testfilter.graph);
I updated the test as per your other email. Much simpler. Thanks for the notes.
+out:
- IFilterGraph2_Release(graph);
- DeleteFileW(filename);
Can you please check for failure from DeleteFile() here? I try to be extra paranoid that we aren't leaking handles to the file anywhere; we've done so in the past.
Good idea, especially since this *is* leaking a file handle here before the patch - the source made by RenderFile will not be closed.
+}
/* Remove and re-add the filter, to flush the graph's internal
- IMediaSeeking cache. Don't expose IMediaSeeking when adding, to show
- that it's only queried when needed. */
@@ -5645,6 +5698,7 @@ START_TEST(filtergraph) test_sync_source(); test_filter_state(); test_ec_complete();
- test_renderfile_failure();
test_graph_seeking(); test_default_sync_source(); test_add_source_filter();
-
Marvin -
Tim Clem -
Zebediah Figura (she/her)