[PATCH 1/6] mf/samplegrabber: Always release cancellation key when timer is invoked.
The sample grabber currently leaks the key on the last invokation of the timer, as it is only released when stream_schedule_sample is called. Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/mf/samplegrabber.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/dlls/mf/samplegrabber.c b/dlls/mf/samplegrabber.c index e0287e1e1db..dd48973b7b6 100644 --- a/dlls/mf/samplegrabber.c +++ b/dlls/mf/samplegrabber.c @@ -729,6 +729,12 @@ static HRESULT WINAPI sample_grabber_stream_timer_callback_Invoke(IMFAsyncCallba EnterCriticalSection(&grabber->cs); + if (grabber->cancel_key) + { + IUnknown_Release(grabber->cancel_key); + grabber->cancel_key = NULL; + } + LIST_FOR_EACH_ENTRY_SAFE(item, item2, &grabber->items, struct scheduled_item, entry) { if (item->type == ITEM_TYPE_MARKER) -- 2.33.1
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/mf/samplegrabber.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/dlls/mf/samplegrabber.c b/dlls/mf/samplegrabber.c index dd48973b7b6..086abcb0f9f 100644 --- a/dlls/mf/samplegrabber.c +++ b/dlls/mf/samplegrabber.c @@ -1458,6 +1458,14 @@ failed: static void sample_grabber_shutdown_object(void *user_context, IUnknown *obj) { struct sample_grabber_activate_context *context = user_context; + IMFMediaSink *sink; + + if (SUCCEEDED(IUnknown_QueryInterface(obj, &IID_IMFMediaSink, (void **)&sink))) + { + IMFMediaSink_Shutdown(sink); + IMFMediaSink_Release(sink); + } + context->shut_down = TRUE; } -- 2.33.1
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=104863 Your paranoid android. === debian11 (32 bit report) === mf: mf.c:3016: Test failed: Failed to get sink flags, hr 0xc00d3e85. mf.c:3019: Test failed: Failed to get type handler, hr 0xc00d4a38. mf.c:3023: Test failed: Failed to set media type, hr 0xc00d4a38. mf.c:3033: Test failed: Failed to get major type, hr 0xc00d4a38. mf.c:3034: Test failed: Unexpected major type {00989680-0000-0000-50c3-000001000000}. mf.c:3037: Test failed: Failed to get current type, hr 0xc00d4a38. mf.c:3038: Test failed: Unexpected media type. Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x004103e5). === debian11 (32 bit Chinese:China report) === mf: mf.c:3016: Test failed: Failed to get sink flags, hr 0xc00d3e85. mf.c:3019: Test failed: Failed to get type handler, hr 0xc00d4a38. mf.c:3023: Test failed: Failed to set media type, hr 0xc00d4a38. mf.c:3033: Test failed: Failed to get major type, hr 0xc00d4a38. mf.c:3034: Test failed: Unexpected major type {00989680-0000-0000-50c3-000001000000}. mf.c:3037: Test failed: Failed to get current type, hr 0xc00d4a38. mf.c:3038: Test failed: Unexpected media type. Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x004103e5). === debian11 (32 bit WoW report) === mf: mf.c:3016: Test failed: Failed to get sink flags, hr 0xc00d3e85. mf.c:3019: Test failed: Failed to get type handler, hr 0xc00d4a38. mf.c:3023: Test failed: Failed to set media type, hr 0xc00d4a38. mf.c:3033: Test failed: Failed to get major type, hr 0xc00d4a38. mf.c:3034: Test failed: Unexpected major type {00989680-0000-0000-50c3-000001000000}. mf.c:3037: Test failed: Failed to get current type, hr 0xc00d4a38. mf.c:3038: Test failed: Unexpected media type. Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x004103e5). === debian11 (64 bit WoW report) === mf: mf.c:3016: Test failed: Failed to get sink flags, hr 0xc00d3e85. mf.c:3019: Test failed: Failed to get type handler, hr 0xc00d4a38. mf.c:3023: Test failed: Failed to set media type, hr 0xc00d4a38. mf.c:3033: Test failed: Failed to get major type, hr 0xc00d4a38. mf.c:3034: Test failed: Unexpected major type {00000000-0000-0000-0000-000000000000}. mf.c:3037: Test failed: Failed to get current type, hr 0xc00d4a38. mf.c:3038: Test failed: Unexpected media type. Unhandled exception: page fault on read access to 0x000000010000000f in 64-bit code (0x000000000040e0cd).
I believe we have tests that show that activation object does not do that.
On 1/5/22 06:01, Nikolay Sivov wrote:
I believe we have tests that show that activation object does not do that.
Yeah, I saw this, it's quite weird. I wonder what is supposed to shut down the sink if not the activate object. The media session won't shut it down directly if the original topology just contained the activate object. Is the user of the media session supposed to manually shut it down? Would you happen to have any ideas here?
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/mf/session.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index e3d77864494..c1215998bbe 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -692,7 +692,7 @@ static void release_topo_node(struct topo_node *node) static void session_shutdown_current_topology(struct media_session *session) { - unsigned int shutdown, force_shutdown; + unsigned int noshutdown, force_shutdown; MF_TOPOLOGY_TYPE node_type; IMFStreamSink *stream_sink; IMFTopology *topology; @@ -712,10 +712,10 @@ static void session_shutdown_current_topology(struct media_session *session) if (SUCCEEDED(IMFTopologyNode_GetNodeType(node, &node_type)) && node_type == MF_TOPOLOGY_OUTPUT_NODE) { - shutdown = 1; - IMFTopologyNode_GetUINT32(node, &MF_TOPONODE_NOSHUTDOWN_ON_REMOVE, &shutdown); + noshutdown = 1; + IMFTopologyNode_GetUINT32(node, &MF_TOPONODE_NOSHUTDOWN_ON_REMOVE, &noshutdown); - if (force_shutdown || shutdown) + if (force_shutdown || !noshutdown) { if (SUCCEEDED(IMFTopologyNode_GetUnknown(node, &_MF_TOPONODE_IMFActivate, &IID_IMFActivate, (void **)&activate))) -- 2.33.1
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=104864 Your paranoid android. === debian11 (32 bit report) === mf: mf.c:3016: Test failed: Failed to get sink flags, hr 0xc00d3e85. mf.c:3019: Test failed: Failed to get type handler, hr 0xc00d4a38. mf.c:3023: Test failed: Failed to set media type, hr 0xc00d4a38. mf.c:3033: Test failed: Failed to get major type, hr 0xc00d4a38. mf.c:3034: Test failed: Unexpected major type {00989680-0000-0000-50c3-000001000000}. mf.c:3037: Test failed: Failed to get current type, hr 0xc00d4a38. mf.c:3038: Test failed: Unexpected media type. Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x004103e5). === debian11 (32 bit Chinese:China report) === mf: mf.c:3016: Test failed: Failed to get sink flags, hr 0xc00d3e85. mf.c:3019: Test failed: Failed to get type handler, hr 0xc00d4a38. mf.c:3023: Test failed: Failed to set media type, hr 0xc00d4a38. mf.c:3033: Test failed: Failed to get major type, hr 0xc00d4a38. mf.c:3034: Test failed: Unexpected major type {00989680-0000-0000-50c3-000001000000}. mf.c:3037: Test failed: Failed to get current type, hr 0xc00d4a38. mf.c:3038: Test failed: Unexpected media type. Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x004103e5). === debian11 (32 bit WoW report) === mf: mf.c:3016: Test failed: Failed to get sink flags, hr 0xc00d3e85. mf.c:3019: Test failed: Failed to get type handler, hr 0xc00d4a38. mf.c:3023: Test failed: Failed to set media type, hr 0xc00d4a38. mf.c:3033: Test failed: Failed to get major type, hr 0xc00d4a38. mf.c:3034: Test failed: Unexpected major type {00989680-0000-0000-50c3-000001000000}. mf.c:3037: Test failed: Failed to get current type, hr 0xc00d4a38. mf.c:3038: Test failed: Unexpected media type. Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x004103e5). === debian11 (64 bit WoW report) === mf: mf.c:3016: Test failed: Failed to get sink flags, hr 0xc00d3e85. mf.c:3019: Test failed: Failed to get type handler, hr 0xc00d4a38. mf.c:3023: Test failed: Failed to set media type, hr 0xc00d4a38. mf.c:3033: Test failed: Failed to get major type, hr 0xc00d4a38. mf.c:3034: Test failed: Unexpected major type {00000000-0000-0000-0000-000000000000}. mf.c:3037: Test failed: Failed to get current type, hr 0xc00d4a38. mf.c:3038: Test failed: Unexpected media type. Unhandled exception: page fault on read access to 0x000000010000000f in 64-bit code (0x000000000040e0cd).
This might as well be correct, but we'll need a test. I think a simple graph with dummy source + grabber sink (not an activation object), will show this - setting topology, shutdown the session, check if some sink methods for MF_E_SHUTDOWN.
I unsuccessfully tried to test this with simple topology of real source -> sample grabber using source type (no conversion), and Close(), or clearing topology does not trigger shutdown, ever. session->Shutdown() always shuts down actual sink when it's set as activation object, and again, never shuts down if it was set directly. This is pretty weird, I'll try some more tests. Maybe just SetTopology() does not transition session state enough, and it has to start playback even if just for a little while. It seems wrong if this automatic shutdown only happened when playback completes by reaching its end time. Derek, have you tried to test this in some way?
On 1/13/22 10:51, Nikolay Sivov wrote:
I unsuccessfully tried to test this with simple topology of real source -> sample grabber using source type (no conversion), and Close(), or clearing topology does not trigger shutdown, ever. session->Shutdown() always shuts down actual sink when it's set as activation object, and again, never shuts down if it was set directly.
This is pretty weird, I'll try some more tests. Maybe just SetTopology() does not transition session state enough, and it has to start playback even if just for a little while. It seems wrong if this automatic shutdown only happened when playback completes by reaching its end time.
Derek, have you tried to test this in some way? I've not yet tested this, but that is quite strange behavior you've described. I'll try using mftrace with a media engine that runs multiple topologies to try and get some insight here later today.
On 1/13/22 19:27, Derek Lesho wrote:
On 1/13/22 10:51, Nikolay Sivov wrote:
I unsuccessfully tried to test this with simple topology of real source -> sample grabber using source type (no conversion), and Close(), or clearing topology does not trigger shutdown, ever. session->Shutdown() always shuts down actual sink when it's set as activation object, and again, never shuts down if it was set directly.
This is pretty weird, I'll try some more tests. Maybe just SetTopology() does not transition session state enough, and it has to start playback even if just for a little while. It seems wrong if this automatic shutdown only happened when playback completes by reaching its end time.
Derek, have you tried to test this in some way? I've not yet tested this, but that is quite strange behavior you've described. I'll try using mftrace with a media engine that runs multiple topologies to try and get some insight here later today.
Please don't use API tracing for this.
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/mf/topology.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/dlls/mf/topology.c b/dlls/mf/topology.c index 34459912fb0..a92d7478440 100644 --- a/dlls/mf/topology.c +++ b/dlls/mf/topology.c @@ -2358,6 +2358,7 @@ static HRESULT topology_loader_resolve_branch(struct topoloader_context *context }; MF_TOPOLOGY_TYPE u_type, d_type; IMFTopologyNode *node; + HRESULT hr; TOPOID id; /* Downstream node might have already been cloned. */ @@ -2371,10 +2372,13 @@ static HRESULT topology_loader_resolve_branch(struct topoloader_context *context if (!connectors[u_type][d_type]) { WARN("Unsupported branch kind %d -> %d.\n", u_type, d_type); + IMFTopologyNode_Release(node); return E_FAIL; } - return connectors[u_type][d_type](context, upstream_node, output_index, node, input_index); + hr = connectors[u_type][d_type](context, upstream_node, output_index, node, input_index); + IMFTopologyNode_Release(node); + return hr; } static HRESULT topology_loader_resolve_nodes(struct topoloader_context *context, unsigned int *layer_size) @@ -2398,10 +2402,13 @@ static HRESULT topology_loader_resolve_nodes(struct topoloader_context *context, if (FAILED(IMFTopologyNode_GetOutput(orig_node, 0, &downstream_node, &input_index))) { IMFTopology_RemoveNode(context->output_topology, node); + IMFTopologyNode_Release(orig_node); + IMFTopologyNode_Release(node); continue; } hr = topology_loader_resolve_branch(context, node, 0, downstream_node, input_index); + IMFTopologyNode_Release(downstream_node); break; case MF_TOPOLOGY_TRANSFORM_NODE: case MF_TOPOLOGY_TEE_NODE: @@ -2412,6 +2419,8 @@ static HRESULT topology_loader_resolve_nodes(struct topoloader_context *context, } IMFTopologyNode_DeleteItem(node, &context->key); + IMFTopologyNode_Release(orig_node); + IMFTopologyNode_Release(node); if (FAILED(hr)) break; -- 2.33.1
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=104865 Your paranoid android. === debian11 (32 bit report) === mf: mf.c:3016: Test failed: Failed to get sink flags, hr 0xc00d3e85. mf.c:3019: Test failed: Failed to get type handler, hr 0xc00d4a38. mf.c:3023: Test failed: Failed to set media type, hr 0xc00d4a38. mf.c:3033: Test failed: Failed to get major type, hr 0xc00d4a38. mf.c:3034: Test failed: Unexpected major type {00989680-0000-0000-50c3-000001000000}. mf.c:3037: Test failed: Failed to get current type, hr 0xc00d4a38. mf.c:3038: Test failed: Unexpected media type. Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x004103e5). === debian11 (32 bit Chinese:China report) === mf: mf.c:3016: Test failed: Failed to get sink flags, hr 0xc00d3e85. mf.c:3019: Test failed: Failed to get type handler, hr 0xc00d4a38. mf.c:3023: Test failed: Failed to set media type, hr 0xc00d4a38. mf.c:3033: Test failed: Failed to get major type, hr 0xc00d4a38. mf.c:3034: Test failed: Unexpected major type {00989680-0000-0000-50c3-000001000000}. mf.c:3037: Test failed: Failed to get current type, hr 0xc00d4a38. mf.c:3038: Test failed: Unexpected media type. Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x004103e5). === debian11 (32 bit WoW report) === mf: mf.c:3016: Test failed: Failed to get sink flags, hr 0xc00d3e85. mf.c:3019: Test failed: Failed to get type handler, hr 0xc00d4a38. mf.c:3023: Test failed: Failed to set media type, hr 0xc00d4a38. mf.c:3033: Test failed: Failed to get major type, hr 0xc00d4a38. mf.c:3034: Test failed: Unexpected major type {00989680-0000-0000-50c3-000001000000}. mf.c:3037: Test failed: Failed to get current type, hr 0xc00d4a38. mf.c:3038: Test failed: Unexpected media type. Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x004103e5). === debian11 (64 bit WoW report) === mf: mf.c:3016: Test failed: Failed to get sink flags, hr 0xc00d3e85. mf.c:3019: Test failed: Failed to get type handler, hr 0xc00d4a38. mf.c:3023: Test failed: Failed to set media type, hr 0xc00d4a38. mf.c:3033: Test failed: Failed to get major type, hr 0xc00d4a38. mf.c:3034: Test failed: Unexpected major type {00000000-0000-0000-0000-000000000000}. mf.c:3037: Test failed: Failed to get current type, hr 0xc00d4a38. mf.c:3038: Test failed: Unexpected media type. Unhandled exception: page fault on execute access to 0x0000000000000000 in 64-bit code (0x0000000000000000).
Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/mfmediaengine/main.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/dlls/mfmediaengine/main.c b/dlls/mfmediaengine/main.c index f7f3afa2a05..c7ab664936c 100644 --- a/dlls/mfmediaengine/main.c +++ b/dlls/mfmediaengine/main.c @@ -167,6 +167,7 @@ struct media_engine } d3d11; } video_frame; CRITICAL_SECTION cs; + HANDLE session_closed; }; static void media_engine_release_video_frame_resources(struct media_engine *engine) @@ -917,6 +918,10 @@ static HRESULT WINAPI media_engine_session_events_Invoke(IMFAsyncCallback *iface IMFMediaEngineNotify_EventNotify(engine->callback, MF_MEDIA_ENGINE_EVENT_ENDED, 0, 0); break; + case MESessionClosed: + + SetEvent(engine->session_closed); + break; } failed: @@ -1297,6 +1302,7 @@ static void free_media_engine(struct media_engine *engine) } SysFreeString(engine->current_source); DeleteCriticalSection(&engine->cs); + CloseHandle(engine->session_closed); free(engine->video_frame.buffer); free(engine); } @@ -1956,6 +1962,8 @@ static HRESULT WINAPI media_engine_Shutdown(IMFMediaEngine *iface) else { media_engine_set_flag(engine, FLAGS_ENGINE_SHUT_DOWN, TRUE); + IMFMediaSession_Close(engine->session); + WaitForSingleObject(engine->session_closed, INFINITE); IMFMediaSession_Shutdown(engine->session); } LeaveCriticalSection(&engine->cs); @@ -2451,6 +2459,7 @@ static HRESULT init_media_engine(DWORD flags, IMFAttributes *attributes, struct engine->duration = NAN; engine->video_frame.pts = MINLONGLONG; InitializeCriticalSection(&engine->cs); + engine->session_closed = CreateEventW(NULL, TRUE, FALSE, NULL); hr = IMFAttributes_GetUnknown(attributes, &MF_MEDIA_ENGINE_CALLBACK, &IID_IMFMediaEngineNotify, (void **)&engine->callback); -- 2.33.1
On 1/4/22 20:35, Derek Lesho wrote:
failed: @@ -1297,6 +1302,7 @@ static void free_media_engine(struct media_engine *engine) } SysFreeString(engine->current_source); DeleteCriticalSection(&engine->cs); + CloseHandle(engine->session_closed); free(engine->video_frame.buffer); free(engine); } @@ -1956,6 +1962,8 @@ static HRESULT WINAPI media_engine_Shutdown(IMFMediaEngine *iface) else { media_engine_set_flag(engine, FLAGS_ENGINE_SHUT_DOWN, TRUE); + IMFMediaSession_Close(engine->session); + WaitForSingleObject(engine->session_closed, INFINITE); IMFMediaSession_Shutdown(engine->session); }
Could you explain what does this fix?
LeaveCriticalSection(&engine->cs); @@ -2451,6 +2459,7 @@ static HRESULT init_media_engine(DWORD flags, IMFAttributes *attributes, struct engine->duration = NAN; engine->video_frame.pts = MINLONGLONG; InitializeCriticalSection(&engine->cs); + engine->session_closed = CreateEventW(NULL, TRUE, FALSE, NULL);
hr = IMFAttributes_GetUnknown(attributes, &MF_MEDIA_ENGINE_CALLBACK, &IID_IMFMediaEngineNotify, (void **)&engine->callback);
On 1/5/22 06:09, Nikolay Sivov wrote:
On 1/4/22 20:35, Derek Lesho wrote:
failed: @@ -1297,6 +1302,7 @@ static void free_media_engine(struct media_engine *engine) } SysFreeString(engine->current_source); DeleteCriticalSection(&engine->cs); + CloseHandle(engine->session_closed); free(engine->video_frame.buffer); free(engine); } @@ -1956,6 +1962,8 @@ static HRESULT WINAPI media_engine_Shutdown(IMFMediaEngine *iface) else { media_engine_set_flag(engine, FLAGS_ENGINE_SHUT_DOWN, TRUE); + IMFMediaSession_Close(engine->session); + WaitForSingleObject(engine->session_closed, INFINITE); IMFMediaSession_Shutdown(engine->session); }
Could you explain what does this fix?
::Close releases the queued topology object, ::Shutdown doesn't. ::Release also closes the topology object, but there's a circular dependency here as the sample grabber sink node in the queued topology references the sample grabber media sink object, which references the sample grabber callback object, which, in the case of the media engine, contributes to the reference count of the media engine, and the media engine only releases the media session once it is fully released. Additionally, the documentation advises that you ::Close a session before shutting it down. https://docs.microsoft.com/en-us/windows/win32/medfound/step-7--shut-down-th...
LeaveCriticalSection(&engine->cs); @@ -2451,6 +2459,7 @@ static HRESULT init_media_engine(DWORD flags, IMFAttributes *attributes, struct engine->duration = NAN; engine->video_frame.pts = MINLONGLONG; InitializeCriticalSection(&engine->cs); + engine->session_closed = CreateEventW(NULL, TRUE, FALSE, NULL); hr = IMFAttributes_GetUnknown(attributes, &MF_MEDIA_ENGINE_CALLBACK, &IID_IMFMediaEngineNotify, (void **)&engine->callback);
A waiting work_item has two references, the initial reference from creation, and an additional reference associated with its presence pending_items list, freed through queue_release_pending_item. RtwqCancelWorkItem only releases the second reference. Signed-off-by: Derek Lesho <dlesho(a)codeweavers.com> --- dlls/rtworkq/queue.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dlls/rtworkq/queue.c b/dlls/rtworkq/queue.c index 58769a0be04..f77f2a602ca 100644 --- a/dlls/rtworkq/queue.c +++ b/dlls/rtworkq/queue.c @@ -866,6 +866,7 @@ static HRESULT queue_cancel_item(struct queue *queue, RTWQWORKITEM_KEY key) if (item->key == key) { key >>= 32; + queue_release_pending_item(item); if ((key & WAIT_ITEM_KEY_MASK) == WAIT_ITEM_KEY_MASK) { IRtwqAsyncResult_SetStatus(item->result, RTWQ_E_OPERATION_CANCELLED); @@ -876,7 +877,7 @@ static HRESULT queue_cancel_item(struct queue *queue, RTWQWORKITEM_KEY key) CloseThreadpoolTimer(item->u.timer_object); else WARN("Unknown item key mask %#x.\n", (DWORD)key); - queue_release_pending_item(item); + IUnknown_Release(&item->IUnknown_iface); hr = S_OK; break; } -- 2.33.1
On 1/4/22 20:35, Derek Lesho wrote:
A waiting work_item has two references, the initial reference from creation, and an additional reference associated with its presence pending_items list, freed through queue_release_pending_item. RtwqCancelWorkItem only releases the second reference.
...
@@ -866,6 +866,7 @@ static HRESULT queue_cancel_item(struct queue *queue, RTWQWORKITEM_KEY key) if (item->key == key) { key >>= 32; + queue_release_pending_item(item); if ((key & WAIT_ITEM_KEY_MASK) == WAIT_ITEM_KEY_MASK) { IRtwqAsyncResult_SetStatus(item->result, RTWQ_E_OPERATION_CANCELLED); @@ -876,7 +877,7 @@ static HRESULT queue_cancel_item(struct queue *queue, RTWQWORKITEM_KEY key) CloseThreadpoolTimer(item->u.timer_object); else WARN("Unknown item key mask %#x.\n", (DWORD)key); - queue_release_pending_item(item); + IUnknown_Release(&item->IUnknown_iface); hr = S_OK; break; } Yes, this looks correct, I think. Why did you have to move queue_release_pending_item() though?
On 1/10/22 09:20, Nikolay Sivov wrote:
On 1/4/22 20:35, Derek Lesho wrote:
A waiting work_item has two references, the initial reference from creation, and an additional reference associated with its presence pending_items list, freed through queue_release_pending_item. RtwqCancelWorkItem only releases the second reference.
...
@@ -866,6 +866,7 @@ static HRESULT queue_cancel_item(struct queue *queue, RTWQWORKITEM_KEY key) if (item->key == key) { key >>= 32; + queue_release_pending_item(item); if ((key & WAIT_ITEM_KEY_MASK) == WAIT_ITEM_KEY_MASK) { IRtwqAsyncResult_SetStatus(item->result, RTWQ_E_OPERATION_CANCELLED); @@ -876,7 +877,7 @@ static HRESULT queue_cancel_item(struct queue *queue, RTWQWORKITEM_KEY key) CloseThreadpoolTimer(item->u.timer_object); else WARN("Unknown item key mask %#x.\n", (DWORD)key); - queue_release_pending_item(item); + IUnknown_Release(&item->IUnknown_iface); hr = S_OK; break; } Yes, this looks correct, I think. Why did you have to move queue_release_pending_item() though?
I didn't have to, but I thought it made more sense not to contrast it with the behavior in waiting_item_cancelable_callback, as in both cases we don't need the item to stay in the pending list while we execute the callback.
On 1/4/22 20:35, Derek Lesho wrote:
The sample grabber currently leaks the key on the last invokation of the timer, as it is only released when stream_schedule_sample is called. It's also released when sample grabber sink is released. Is that not always enough? Or is that useful when sink is reused for next topology?
On 1/5/22 06:01, Nikolay Sivov wrote:
On 1/4/22 20:35, Derek Lesho wrote:
The sample grabber currently leaks the key on the last invokation of the timer, as it is only released when stream_schedule_sample is called. It's also released when sample grabber sink is released. Is that not always enough? Or is that useful when sink is reused for next topology?
The problem is that it introduces a circular dependency, the cancel key holds a reference to the timer callback, which contributes to the reference count of the sample grabber stream sink and media sink.
On 1/5/22 18:52, Derek Lesho wrote:
On 1/5/22 06:01, Nikolay Sivov wrote:
On 1/4/22 20:35, Derek Lesho wrote:
The sample grabber currently leaks the key on the last invokation of the timer, as it is only released when stream_schedule_sample is called. It's also released when sample grabber sink is released. Is that not always enough? Or is that useful when sink is reused for next topology?
The problem is that it introduces a circular dependency, the cancel key holds a reference to the timer callback, which contributes to the reference count of the sample grabber stream sink and media sink. I see, so this could be solved by releasing this key on shutdown, and ensuring that shutdown actually happens. I'll take a look at what's going on with its special IMFActivate vs normal shutdown method.
On 1/5/22 21:38, Nikolay Sivov wrote:
On 1/5/22 18:52, Derek Lesho wrote:
On 1/5/22 06:01, Nikolay Sivov wrote:
On 1/4/22 20:35, Derek Lesho wrote:
The sample grabber currently leaks the key on the last invokation of the timer, as it is only released when stream_schedule_sample is called. It's also released when sample grabber sink is released. Is that not always enough? Or is that useful when sink is reused for next topology?
The problem is that it introduces a circular dependency, the cancel key holds a reference to the timer callback, which contributes to the reference count of the sample grabber stream sink and media sink. I see, so this could be solved by releasing this key on shutdown, and ensuring that shutdown actually happens. I'll take a look at what's going on with its special IMFActivate vs normal shutdown method.
This better matches what I see from experimenting with the timer. I'll send it after 7.0. Question about properly shutting the sink down still remains.
participants (3)
-
Derek Lesho -
Marvin -
Nikolay Sivov