[PATCH 1/2] quartz/tests: Test IGraphBuilder::RenderFile cleanup on failure

newer
[PATCH v2 1/2] quartz/tests: Test...

Tim Clem

June 28, 2021

1:46 p.m.

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(a)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(); -- 2.32.0

Show replies by date

Tim Clem

June 2021

1:46 p.m.

New subject: [PATCH 2/2] quartz: Remove source filter added by IGraphBuilder::RenderFile on failure

Signed-off-by: Tim Clem <tclem(a)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)); } -- 2.32.0

Marvin

2:11 p.m.

New subject: [PATCH 2/2] quartz: Remove source filter added by IGraphBuilder::RenderFile on failure

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:

Marvin

1:58 p.m.

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:

Zebediah Figura (she/her)

2:45 p.m.

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(a)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();

Zebediah Figura (she/her)

2:51 p.m.

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(a)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();

Tim Clem

5:09 p.m.

June 28, 2021 12:46 PM, "Zebediah Figura (she/her)" <zfigura(a)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(a)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();

Zebediah Figura (she/her)

5:24 p.m.

On 6/28/21 5:09 PM, Tim Clem wrote:
> June 28, 2021 12:46 PM, "Zebediah Figura (she/her)" <zfigura(a)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(a)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();

1731

Age (days ago)

1731

Last active (days ago)

List overview

7 comments

3 participants

participants (3)

Marvin
Tim Clem
Zebediah Figura (she/her)