[PATCH 0/2] MR3067: mf: Shutdown handling of activate objects in session.

[PATCH 1/2] mf/tests: Add tests for samplegrabber shutdown state.

[Bernhard Kölbl]

From: Bernhard Kölbl

---
 dlls/mf/tests/mf.c | 444 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 443 insertions(+), 1 deletion(-)

diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c
index 010c8b2bd12..995f48a7eed 100644
--- a/dlls/mf/tests/mf.c
+++ b/dlls/mf/tests/mf.c
@@ -2838,6 +2838,7 @@ struct test_grabber_callback
     IMFCollection *samples;
     HANDLE ready_event;
     HANDLE done_event;
+    BOOL is_shutdown;
 };

static struct test_grabber_callback *impl_from_IMFSampleGrabberSinkCallback(IMFSampleGrabberSinkCallback *iface)
@@ -2946,6 +2947,9 @@ static HRESULT WINAPI test_grabber_callback_OnProcessSample(IMFSampleGrabberSink

static HRESULT WINAPI test_grabber_callback_OnShutdown(IMFSampleGrabberSinkCallback *iface)
 {
+    struct test_grabber_callback *grabber = CONTAINING_RECORD(iface, struct test_grabber_callback, IMFSampleGrabberSinkCallback_iface);
+    grabber->is_shutdown = TRUE;
+
     return S_OK;
 }

@@ -5064,7 +5068,6 @@ static void test_sample_grabber_orientation(GUID subtype)
     ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);

IMFActivate_Release(sink_activate);
-    IMFMediaSession_Release(session);
     IMFMediaSource_Release(source);

hr = MFShutdown();
@@ -5073,6 +5076,444 @@ static void test_sample_grabber_orientation(GUID subtype)
     IMFSampleGrabberSinkCallback_Release(&grabber_callback->IMFSampleGrabberSinkCallback_iface);
 }

+struct activate_object_shim
+{
+    IMFActivate IMFActivate_iface;
+    LONG refcount;
+    
+    IMFActivate *inner;
+    BOOL is_shutdown;
+};
+
+static struct activate_object_shim *impl_from_IMFActivate(IMFActivate *iface)
+{
+    return CONTAINING_RECORD(iface, struct activate_object_shim, IMFActivate_iface);
+}
+
+static HRESULT WINAPI activate_object_shim_QueryInterface(IMFActivate *iface, REFIID riid, void **obj)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+
+    if (IsEqualIID(riid, &IID_IMFActivate) ||
+            IsEqualIID(riid, &IID_IMFAttributes) ||
+            IsEqualIID(riid, &IID_IUnknown))
+    {
+        *obj = &activate->IMFActivate_iface;
+        IMFActivate_AddRef(*obj);
+        return S_OK;
+    }
+
+    *obj = NULL;
+    return E_NOINTERFACE;
+}
+
+static ULONG WINAPI activate_object_shim_AddRef(IMFActivate *iface)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    ULONG ref = InterlockedIncrement(&activate->refcount);
+    return ref;
+}
+
+static ULONG WINAPI activate_object_shim_Release(IMFActivate *iface)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    ULONG ref = InterlockedDecrement(&activate->refcount);
+
+    if (!ref)
+    {
+        if (activate->inner)
+            IMFActivate_Release(activate->inner);
+        free(activate);
+    }
+
+    return ref;
+}
+
+static HRESULT WINAPI activate_object_shim_GetItem(IMFActivate *iface, REFGUID key, PROPVARIANT *value)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_GetItem(activate->inner, key, value);
+}
+
+static HRESULT WINAPI activate_object_shim_GetItemType(IMFActivate *iface, REFGUID key, MF_ATTRIBUTE_TYPE *type)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_GetItemType(activate->inner, key, type);
+}
+
+static HRESULT WINAPI activate_object_shim_CompareItem(IMFActivate *iface, REFGUID key, REFPROPVARIANT value, BOOL *result)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_CompareItem(activate->inner, key, value, result);
+}
+
+static HRESULT WINAPI activate_object_shim_Compare(IMFActivate *iface, IMFAttributes *theirs, MF_ATTRIBUTES_MATCH_TYPE type,
+        BOOL *result)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_Compare(activate->inner, theirs, type, result);
+}
+
+static HRESULT WINAPI activate_object_shim_GetUINT32(IMFActivate *iface, REFGUID key, UINT32 *value)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_GetUINT32(activate->inner, key, value);
+}
+
+static HRESULT WINAPI activate_object_shim_GetUINT64(IMFActivate *iface, REFGUID key, UINT64 *value)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_GetUINT64(activate->inner, key, value);
+}
+
+static HRESULT WINAPI activate_object_shim_GetDouble(IMFActivate *iface, REFGUID key, double *value)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_GetDouble(activate->inner, key, value);
+}
+
+static HRESULT WINAPI activate_object_shim_GetGUID(IMFActivate *iface, REFGUID key, GUID *value)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_GetGUID(activate->inner, key, value);
+}
+
+static HRESULT WINAPI activate_object_shim_GetStringLength(IMFActivate *iface, REFGUID key, UINT32 *length)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_GetStringLength(activate->inner, key, length);
+}
+
+static HRESULT WINAPI activate_object_shim_GetString(IMFActivate *iface, REFGUID key, WCHAR *value,
+        UINT32 size, UINT32 *length)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_GetString(activate->inner, key, value, size, length);
+}
+
+static HRESULT WINAPI activate_object_shim_GetAllocatedString(IMFActivate *iface, REFGUID key,
+        WCHAR **value, UINT32 *length)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_GetAllocatedString(activate->inner, key, value, length);
+}
+
+static HRESULT WINAPI activate_object_shim_GetBlobSize(IMFActivate *iface, REFGUID key, UINT32 *size)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_GetBlobSize(activate->inner, key, size);
+}
+
+static HRESULT WINAPI activate_object_shim_GetBlob(IMFActivate *iface, REFGUID key, UINT8 *buf,
+        UINT32 bufsize, UINT32 *blobsize)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_GetBlob(activate->inner, key, buf, bufsize, blobsize);
+}
+
+static HRESULT WINAPI activate_object_shim_GetAllocatedBlob(IMFActivate *iface, REFGUID key, UINT8 **buf, UINT32 *size)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_GetAllocatedBlob(activate->inner, key, buf, size);
+}
+
+static HRESULT WINAPI activate_object_shim_GetUnknown(IMFActivate *iface, REFGUID key, REFIID riid, void **ppv)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_GetUnknown(activate->inner, key, riid, ppv);
+}
+
+static HRESULT WINAPI activate_object_shim_SetItem(IMFActivate *iface, REFGUID key, REFPROPVARIANT value)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_SetItem(activate->inner, key, value);
+}
+
+static HRESULT WINAPI activate_object_shim_DeleteItem(IMFActivate *iface, REFGUID key)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_DeleteItem(activate->inner, key);
+}
+
+static HRESULT WINAPI activate_object_shim_DeleteAllItems(IMFActivate *iface)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_DeleteAllItems(activate->inner);
+}
+
+static HRESULT WINAPI activate_object_shim_SetUINT32(IMFActivate *iface, REFGUID key, UINT32 value)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_SetUINT32(activate->inner, key, value);
+}
+
+static HRESULT WINAPI activate_object_shim_SetUINT64(IMFActivate *iface, REFGUID key, UINT64 value)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_SetUINT64(activate->inner, key, value);
+}
+
+static HRESULT WINAPI activate_object_shim_SetDouble(IMFActivate *iface, REFGUID key, double value)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_SetDouble(activate->inner, key, value);
+}
+
+static HRESULT WINAPI activate_object_shim_SetGUID(IMFActivate *iface, REFGUID key, REFGUID value)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_SetGUID(activate->inner, key, value);
+}
+
+static HRESULT WINAPI activate_object_shim_SetString(IMFActivate *iface, REFGUID key, const WCHAR *value)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_SetString(activate->inner, key, value);
+}
+
+static HRESULT WINAPI activate_object_shim_SetBlob(IMFActivate *iface, REFGUID key, const UINT8 *buf, UINT32 size)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_SetBlob(activate->inner, key, buf, size);
+}
+
+static HRESULT WINAPI activate_object_shim_SetUnknown(IMFActivate *iface, REFGUID key, IUnknown *unknown)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_SetUnknown(activate->inner, key, unknown);
+}
+
+static HRESULT WINAPI activate_object_shim_LockStore(IMFActivate *iface)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_LockStore(activate->inner);
+}
+
+static HRESULT WINAPI activate_object_shim_UnlockStore(IMFActivate *iface)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_UnlockStore(activate->inner);
+}
+
+static HRESULT WINAPI activate_object_shim_GetCount(IMFActivate *iface, UINT32 *count)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_GetCount(activate->inner, count);
+}
+
+static HRESULT WINAPI activate_object_shim_GetItemByIndex(IMFActivate *iface, UINT32 index, GUID *key, PROPVARIANT *value)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_GetItemByIndex(activate->inner, index, key, value);
+}
+
+static HRESULT WINAPI activate_object_shim_CopyAllItems(IMFActivate *iface, IMFAttributes *dest)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_CopyAllItems(activate->inner, dest);
+}
+
+static HRESULT WINAPI activate_object_shim_ActivateObject(IMFActivate *iface, REFIID riid, void **obj)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_ActivateObject(activate->inner, riid, obj);
+}
+
+static HRESULT WINAPI activate_object_shim_ShutdownObject(IMFActivate *iface)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+
+    activate->is_shutdown = TRUE;
+    return IMFActivate_ShutdownObject(activate->inner);
+}
+
+static HRESULT WINAPI activate_object_shim_DetachObject(IMFActivate *iface)
+{
+    struct activate_object_shim *activate = impl_from_IMFActivate(iface);
+    return IMFActivate_DetachObject(activate->inner);
+}
+
+static const IMFActivateVtbl activate_object_shim_vtbl =
+{
+    activate_object_shim_QueryInterface,
+    activate_object_shim_AddRef,
+    activate_object_shim_Release,
+    activate_object_shim_GetItem,
+    activate_object_shim_GetItemType,
+    activate_object_shim_CompareItem,
+    activate_object_shim_Compare,
+    activate_object_shim_GetUINT32,
+    activate_object_shim_GetUINT64,
+    activate_object_shim_GetDouble,
+    activate_object_shim_GetGUID,
+    activate_object_shim_GetStringLength,
+    activate_object_shim_GetString,
+    activate_object_shim_GetAllocatedString,
+    activate_object_shim_GetBlobSize,
+    activate_object_shim_GetBlob,
+    activate_object_shim_GetAllocatedBlob,
+    activate_object_shim_GetUnknown,
+    activate_object_shim_SetItem,
+    activate_object_shim_DeleteItem,
+    activate_object_shim_DeleteAllItems,
+    activate_object_shim_SetUINT32,
+    activate_object_shim_SetUINT64,
+    activate_object_shim_SetDouble,
+    activate_object_shim_SetGUID,
+    activate_object_shim_SetString,
+    activate_object_shim_SetBlob,
+    activate_object_shim_SetUnknown,
+    activate_object_shim_LockStore,
+    activate_object_shim_UnlockStore,
+    activate_object_shim_GetCount,
+    activate_object_shim_GetItemByIndex,
+    activate_object_shim_CopyAllItems,
+    activate_object_shim_ActivateObject,
+    activate_object_shim_ShutdownObject,
+    activate_object_shim_DetachObject,
+};
+
+struct IMFActivate *create_activate_object_shim(void)
+{
+    struct activate_object_shim *object;
+
+    if (!(object = calloc(1, sizeof(*object))))
+        return NULL;
+
+    object->IMFActivate_iface.lpVtbl = &activate_object_shim_vtbl;
+    object->refcount = 1;
+
+    return &object->IMFActivate_iface;
+}
+
+static void test_sample_grabber_shutdown(void)
+{
+    media_type_desc video_rgb32_desc =
+    {
+        ATTR_GUID(MF_MT_MAJOR_TYPE, MFMediaType_Video),
+        ATTR_GUID(MF_MT_SUBTYPE, MFVideoFormat_RGB32),
+    };
+
+    struct test_grabber_callback *grabber_callback;
+    struct activate_object_shim *activate_shim;
+    IMFTopologyNode *src_node, *sink_node;
+    IMFPresentationDescriptor *pd;
+    IMFAsyncCallback *callback;
+    IMFMediaType *output_type;
+    IMFMediaSession *session;
+    IMFStreamDescriptor *sd;
+    IMFMediaSource *source;
+    IMFTopology *topology;
+    PROPVARIANT propvar;
+    BOOL selected;
+    HRESULT hr;
+    ULONG ref;
+    DWORD res;
+
+    hr = MFStartup(MF_VERSION, MFSTARTUP_FULL);
+    ok(hr == S_OK, "Failed to start up, hr %#lx.\n", hr);
+
+    if (!(source = create_media_source(L"test.mp4", L"video/mp4")))
+    {
+        win_skip("MP4 media source is not supported, skipping tests.\n");
+        return;
+    }
+
+    callback = create_test_callback(TRUE);
+    grabber_callback = impl_from_IMFSampleGrabberSinkCallback(create_test_grabber_callback());
+    activate_shim = impl_from_IMFActivate(create_activate_object_shim());
+
+    grabber_callback->ready_event = CreateEventW(NULL, FALSE, FALSE, NULL);
+    ok(!!grabber_callback->ready_event, "CreateEventW failed, error %lu\n", GetLastError());
+    grabber_callback->done_event = CreateEventW(NULL, FALSE, FALSE, NULL);
+    ok(!!grabber_callback->done_event, "CreateEventW failed, error %lu\n", GetLastError());
+
+    hr = MFCreateMediaSession(NULL, &session);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+
+    hr = MFCreateTopologyNode(MF_TOPOLOGY_OUTPUT_NODE, &sink_node);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    hr = MFCreateTopologyNode(MF_TOPOLOGY_SOURCESTREAM_NODE, &src_node);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+
+    hr = MFCreateTopology(&topology);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    hr = IMFTopology_AddNode(topology, sink_node);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    hr = IMFTopology_AddNode(topology, src_node);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    hr = IMFTopologyNode_ConnectOutput(src_node, 0, sink_node, 0);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+
+    hr = IMFMediaSource_CreatePresentationDescriptor(source, &pd);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    hr = IMFPresentationDescriptor_GetStreamDescriptorByIndex(pd, 0, &selected, &sd);
+    ok(selected, "got selected %u.\n", !!selected);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    init_source_node(source, -1, src_node, pd, sd);
+    IMFTopologyNode_Release(src_node);
+    IMFPresentationDescriptor_Release(pd);
+    IMFStreamDescriptor_Release(sd);
+
+    hr = MFCreateMediaType(&output_type);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    init_media_type(output_type, video_rgb32_desc, -1);
+    hr = MFCreateSampleGrabberSinkActivate(output_type, &grabber_callback->IMFSampleGrabberSinkCallback_iface, &activate_shim->inner);
+    ok(hr == S_OK, "Failed to create grabber sink, hr %#lx.\n", hr);
+    IMFMediaType_Release(output_type);
+
+    hr = IMFTopologyNode_SetObject(sink_node, (IUnknown *)&activate_shim->IMFActivate_iface);
+    ok(hr == S_OK, "Failed to set object, hr %#lx.\n", hr);
+    hr = IMFTopologyNode_SetUINT32(sink_node, &MF_TOPONODE_CONNECT_METHOD, MF_CONNECT_ALLOW_DECODER);
+    ok(hr == S_OK, "Failed to set connect method, hr %#lx.\n", hr);
+    IMFTopologyNode_Release(sink_node);
+
+    hr = IMFTopology_SetUINT32(topology, &MF_TOPOLOGY_ENUMERATE_SOURCE_TYPES, TRUE);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+
+    hr = IMFMediaSession_SetTopology(session, 0, topology);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    IMFTopology_Release(topology);
+
+    propvar.vt = VT_EMPTY;
+    hr = IMFMediaSession_Start(session, &GUID_NULL, &propvar);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+
+    res = WaitForSingleObject(grabber_callback->ready_event, 5000);
+    ok(!res, "WaitForSingleObject returned %#lx\n", res);
+    CloseHandle(grabber_callback->ready_event);
+    grabber_callback->ready_event = NULL;
+
+    SetEvent(grabber_callback->done_event);
+
+    hr = IMFMediaSession_Close(session);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    hr = wait_media_event(session, callback, MESessionClosed, 1000, &propvar);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    ok(propvar.vt == VT_EMPTY, "got vt %u\n", propvar.vt);
+
+    hr = IMFMediaSession_Shutdown(session);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    todo_wine ok(grabber_callback->is_shutdown, "Samplegrabber wasn't shut down.\n");
+    todo_wine ok(!activate_shim->is_shutdown, "Activate Object was shut down.\n");
+
+    hr = IMFMediaSource_Shutdown(source);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    IMFMediaSource_Release(source);
+
+    hr = IMFActivate_ShutdownObject(&activate_shim->IMFActivate_iface);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    ref = IMFActivate_Release(&activate_shim->IMFActivate_iface);
+    ok(!ref, "Unexpected ref %lu.\n", ref);
+    ref = IMFMediaSession_Release(session);
+    todo_wine ok(!ref || broken(ref == 1) /* Windows rarely fails this. */, "Unexpected ref %lu.\n", ref);
+
+    hr = MFShutdown();
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+
+    IMFSampleGrabberSinkCallback_Release(&grabber_callback->IMFSampleGrabberSinkCallback_iface);
+}
+
 static void test_quality_manager(void)
 {
     IMFPresentationClock *clock;
@@ -6779,6 +7220,7 @@ START_TEST(mf)
     test_sample_grabber_is_mediatype_supported();
     test_sample_grabber_orientation(MFVideoFormat_RGB32);
     test_sample_grabber_orientation(MFVideoFormat_NV12);
+    test_sample_grabber_shutdown();
     test_quality_manager();
     test_sar();
     test_evr();

[PATCH 2/2] mf: Don't make stream sink shutdown dependent on IMFActivate presence in node.

[Bernhard Kölbl]

From: Bernhard Kölbl

Nodes in the presentation topology only hold a reference to an activate object,
when they are a sink anyway, see session_bind_output_nodes() for reference.

Prevents IMFMediaSink objects from getting leaked, even when IMFMediaSession_Shutdown() was called.

Signed-off-by: Bernhard Kölbl 
---
 dlls/mf/session.c  | 2 +-
 dlls/mf/tests/mf.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/dlls/mf/session.c b/dlls/mf/session.c
index 8b29c8487fb..219c9f859f0 100644
--- a/dlls/mf/session.c
+++ b/dlls/mf/session.c
@@ -762,7 +762,7 @@ static void session_shutdown_current_topology(struct media_session *session)
                         WARN("Failed to shut down activation object for the sink, hr %#lx.\n", hr);
                     IMFActivate_Release(activate);
                 }
-                else if (SUCCEEDED(topology_node_get_object(node, &IID_IMFStreamSink, (void **)&stream_sink)))
+                if (SUCCEEDED(topology_node_get_object(node, &IID_IMFStreamSink, (void **)&stream_sink)))
                 {
                     if (SUCCEEDED(IMFStreamSink_GetMediaSink(stream_sink, &sink)))
                     {
diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c
index 995f48a7eed..c476daac886 100644
--- a/dlls/mf/tests/mf.c
+++ b/dlls/mf/tests/mf.c
@@ -5494,7 +5494,7 @@ static void test_sample_grabber_shutdown(void)

hr = IMFMediaSession_Shutdown(session);
     ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
-    todo_wine ok(grabber_callback->is_shutdown, "Samplegrabber wasn't shut down.\n");
+    ok(grabber_callback->is_shutdown, "Samplegrabber wasn't shut down.\n");
     todo_wine ok(!activate_shim->is_shutdown, "Activate Object was shut down.\n");

hr = IMFMediaSource_Shutdown(source);
@@ -5506,7 +5506,7 @@ static void test_sample_grabber_shutdown(void)
     ref = IMFActivate_Release(&activate_shim->IMFActivate_iface);
     ok(!ref, "Unexpected ref %lu.\n", ref);
     ref = IMFMediaSession_Release(session);
-    todo_wine ok(!ref || broken(ref == 1) /* Windows rarely fails this. */, "Unexpected ref %lu.\n", ref);
+    ok(!ref || broken(ref == 1) /* Windows rarely fails this. */, "Unexpected ref %lu.\n", ref);

hr = MFShutdown();
     ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);

[Nikolay Sivov (＠nsivov)]

Strangely, I don't see sink shutdown happening on Windows on MFSESSION_SETTOPOLOGY_CLEAR_CURRENT. I'll see what happens with custom sink next, maybe it has some awareness of the grabber.

[Bernhard Kölbl (＠besentv)]

On Mon Jun 19 16:55:37 2023 +0000, Nikolay Sivov wrote:
> Strangely, I don't see sink shutdown happening on Windows on
> MFSESSION_SETTOPOLOGY_CLEAR_CURRENT. I'll see what happens with custom
> sink next, maybe it has some awareness of the grabber.

Yeah this sounds plausible, I'll update the MR once you know more.

[Yuxuan Shui (＠yshui)]

On Mon Jun 19 16:55:37 2023 +0000, Bernhard Kölbl wrote:
> Yeah this sounds plausible, I'll update the MR once you know more.

I think this is at least correct when the session is being shutdown via `mfsession_Shutdown`. If I limit this to `if (session->state == SESSION_STATE_SHUT_DOWN)`, would you merge this?

And there is a deadlock issue with this MR which I am investigating.

[Bernhard Kölbl (＠besentv)]

On Wed Oct 25 17:19:15 2023 +0000, Yuxuan Shui wrote:
> I think this is at least correct when the session is being shutdown via
> `mfsession_Shutdown`. If I limit this to `if (session->state ==
> SESSION_STATE_SHUT_DOWN)`, would you merge this?
> And there is a deadlock issue with this MR which I am investigating.

Assigned this to you as you're investigating this.

[Yuxuan Shui (＠yshui)]

On Wed Oct 25 17:58:55 2023 +0000, Bernhard Kölbl wrote:
> Assigned this to you as you're investigating this.

The deadlock is this:

media_engine_Shutdown (holds `engine->cs`) -> session Shutdown -> samplegrabber media sink Shutdown (wants `grabber->cs`)

vs

sample_grabber_stream_timer_callback_Invoke (holds `grabber->cs`) -> media_engine_grabber_callback_OnProcessSample (wants `engine->cs`)

I think we can make `engine->flags` atomic and don't try to acquire `enigne->cs` in case the engine is shutdown.

[Yuxuan Shui (＠yshui)]

On Wed Oct 25 18:35:41 2023 +0000, Yuxuan Shui wrote:
> The deadlock is this:
> media_engine_Shutdown (holds `engine->cs`) -> session Shutdown ->
> samplegrabber media sink Shutdown (wants `grabber->cs`)
> vs
> sample_grabber_stream_timer_callback_Invoke (holds `grabber->cs`) ->
> media_engine_grabber_callback_OnProcessSample (wants `engine->cs`)
> I think we can make `engine->flags` atomic and don't try to acquire
> `enigne->cs` in case the engine is shutdown.

I think this can be part of !4133

[Yuxuan Shui (＠yshui)]

On Wed Oct 25 18:36:26 2023 +0000, Yuxuan Shui wrote:
> I think this can be part of !4133

So I had a think. It seems to be a bad idea in general to call potentially user controlled code (like `_OnProcessSample`, or `IMFMediaSink_Shutdown`) while holding a lock.

Because the code we call into can do anything, this would be a minefield of recursive locks, and lock inversions. I think it might be a good idea to avoid doing this altogether.

@nsivov do you think this is the right direction?

[Yuxuan Shui (＠yshui)]

Summary of what I found so far:

1. `MediaSession::Shutdown` definitely calls `MediaSink::Shutdown`. And it doesn't call `MFActivate::ShutdownObject`.
2. When topology is resolved, neither the original topology nodes nor the resolved topology nodes keep a reference to the `MFActivate` objects, but something else does. I am not sure what's keeping the references. Those references are released when the session is shutdown.
3. I am not sure what the stored `MFActivate` is used for. If `ActivateObject` hasn't been called when session is shut down, obviously `ShutdownObject` won't be called; but when `ActivateObject` has been called, the created objects are shutdown directly via `MediaSink::Shutdown`. `ShutdownObject` isn't called either way.
4. And BTW, we are calling `ActivateObject` too early. I think native doesn't call it until `MediaSession::Start`.
5. `MediaSession::SetTopology` with `MFSESSION_SETTOPOLOGY_CLEAR_CURRENT` doesn't shutdown the cleared topology. In fact, the cleared topology continues to run, `ProcessSample` is still called, etc.
6. `ClearTopologies()` doesn't generate a `MESessionTopologiesCleared` if there's no queued topologies.

[Yuxuan Shui (＠yshui)]

On Fri Oct 27 22:13:17 2023 +0000, Yuxuan Shui wrote:
> Summary of what I found so far:
> - `MediaSession::Shutdown` definitely calls `MediaSink::Shutdown`. And
>
> it doesn't call `MFActivate::ShutdownObject`.
> 2. When topology is resolved, neither the original topology nodes nor
> the resolved topology nodes keep a reference to the `MFActivate`
> objects, but something else does. I am not sure what's keeping the
> references. Those references are released when the session is shutdown.
> 3. I am not sure what the stored `MFActivate` is used for. If
> `ActivateObject` hasn't been called when session is shut down, obviously
> `ShutdownObject` won't be called; but when `ActivateObject` has been
> called, the created objects are shutdown directly via
> `MediaSink::Shutdown`. `ShutdownObject` isn't called either way.
> 4. And BTW, we are calling `ActivateObject` too early. I think native
> doesn't call it until `MediaSession::Start`.
> 5. `MediaSession::SetTopology` with
> `MFSESSION_SETTOPOLOGY_CLEAR_CURRENT` doesn't shutdown the cleared
> topology. In fact, the cleared topology continues to run,
> `ProcessSample` is still called, etc.
> 6. `ClearTopologies()` doesn't generate a `MESessionTopologiesCleared`
> if there's no queued topologies.

oh, i forgot one:

7. `IMFTopology` has an `MF_TOPOLOGY_RESOLUTION_STATUS` attribute which we are missing.