[PATCH] mfreadwrite: Introduce an internal source reader refcount.
To decide when to shutdown the media source. Otherwise the references hold by the callbacks from BeginGetEvent and others are keeping the source reader alive and the IMFMediaSource_Shutdown is never called. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/mfreadwrite/reader.c | 41 +++++++++++++++++++++++++-------- dlls/mfreadwrite/tests/mfplat.c | 8 +++++-- 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/dlls/mfreadwrite/reader.c b/dlls/mfreadwrite/reader.c index be77e1c5b1d..18509e067b3 100644 --- a/dlls/mfreadwrite/reader.c +++ b/dlls/mfreadwrite/reader.c @@ -153,6 +153,7 @@ struct source_reader IMFAsyncCallback source_events_callback; IMFAsyncCallback stream_events_callback; IMFAsyncCallback async_commands_callback; + LONG internal_refcount; LONG refcount; IMFMediaSource *source; IMFPresentationDescriptor *descriptor; @@ -203,6 +204,9 @@ static struct media_stream *impl_stream_from_IMFVideoSampleAllocatorNotify(IMFVi return CONTAINING_RECORD(iface, struct media_stream, notify_cb); } +static ULONG WINAPI src_reader_internal_addref(struct source_reader *reader); +static ULONG WINAPI src_reader_internal_release(struct source_reader *reader); + static HRESULT WINAPI source_reader_async_command_QueryInterface(IUnknown *iface, REFIID riid, void **obj) { if (IsEqualIID(riid, &IID_IUnknown)) @@ -325,13 +329,13 @@ static HRESULT WINAPI source_reader_callback_QueryInterface(IMFAsyncCallback *if static ULONG WINAPI source_reader_source_events_callback_AddRef(IMFAsyncCallback *iface) { struct source_reader *reader = impl_from_source_callback_IMFAsyncCallback(iface); - return IMFSourceReader_AddRef(&reader->IMFSourceReader_iface); + return src_reader_internal_addref(reader); } static ULONG WINAPI source_reader_source_events_callback_Release(IMFAsyncCallback *iface) { struct source_reader *reader = impl_from_source_callback_IMFAsyncCallback(iface); - return IMFSourceReader_Release(&reader->IMFSourceReader_iface); + return src_reader_internal_release(reader); } static HRESULT WINAPI source_reader_callback_GetParameters(IMFAsyncCallback *iface, @@ -611,13 +615,13 @@ static const IMFAsyncCallbackVtbl source_events_callback_vtbl = static ULONG WINAPI source_reader_stream_events_callback_AddRef(IMFAsyncCallback *iface) { struct source_reader *reader = impl_from_stream_callback_IMFAsyncCallback(iface); - return IMFSourceReader_AddRef(&reader->IMFSourceReader_iface); + return src_reader_internal_addref(reader); } static ULONG WINAPI source_reader_stream_events_callback_Release(IMFAsyncCallback *iface) { struct source_reader *reader = impl_from_stream_callback_IMFAsyncCallback(iface); - return IMFSourceReader_Release(&reader->IMFSourceReader_iface); + return src_reader_internal_release(reader); } static HRESULT source_reader_pull_stream_samples(struct source_reader *reader, struct media_stream *stream) @@ -888,13 +892,13 @@ static const IMFAsyncCallbackVtbl stream_events_callback_vtbl = static ULONG WINAPI source_reader_async_commands_callback_AddRef(IMFAsyncCallback *iface) { struct source_reader *reader = impl_from_async_commands_callback_IMFAsyncCallback(iface); - return IMFSourceReader_AddRef(&reader->IMFSourceReader_iface); + return src_reader_internal_addref(reader); } static ULONG WINAPI source_reader_async_commands_callback_Release(IMFAsyncCallback *iface) { struct source_reader *reader = impl_from_async_commands_callback_IMFAsyncCallback(iface); - return IMFSourceReader_Release(&reader->IMFSourceReader_iface); + return src_reader_internal_release(reader); } static struct stream_response * media_stream_detach_response(struct source_reader *reader, struct stream_response *response) @@ -1337,16 +1341,34 @@ static ULONG WINAPI src_reader_Release(IMFSourceReader *iface) { struct source_reader *reader = impl_from_IMFSourceReader(iface); ULONG refcount = InterlockedDecrement(&reader->refcount); - unsigned int i; TRACE("%p, refcount %u.\n", iface, refcount); if (!refcount) { - if (reader->async_callback) - IMFSourceReaderCallback_Release(reader->async_callback); if (reader->flags & SOURCE_READER_SHUTDOWN_ON_RELEASE) IMFMediaSource_Shutdown(reader->source); + src_reader_internal_release(reader); + } + + return refcount; +} + +static ULONG WINAPI src_reader_internal_addref(struct source_reader *reader) +{ + ULONG refcount = InterlockedIncrement(&reader->internal_refcount); + return refcount; +} + +static ULONG WINAPI src_reader_internal_release(struct source_reader *reader) +{ + ULONG refcount = InterlockedDecrement(&reader->internal_refcount); + unsigned int i; + + if (!refcount) + { + if (reader->async_callback) + IMFSourceReaderCallback_Release(reader->async_callback); if (reader->descriptor) IMFPresentationDescriptor_Release(reader->descriptor); if (reader->attributes) @@ -2267,6 +2289,7 @@ static HRESULT create_source_reader_from_source(IMFMediaSource *source, IMFAttri object->source_events_callback.lpVtbl = &source_events_callback_vtbl; object->stream_events_callback.lpVtbl = &stream_events_callback_vtbl; object->async_commands_callback.lpVtbl = &async_commands_callback_vtbl; + object->internal_refcount = 1; object->refcount = 1; list_init(&object->responses); if (shutdown_on_release) diff --git a/dlls/mfreadwrite/tests/mfplat.c b/dlls/mfreadwrite/tests/mfplat.c index 7e724344168..0791f51fa37 100644 --- a/dlls/mfreadwrite/tests/mfplat.c +++ b/dlls/mfreadwrite/tests/mfplat.c @@ -862,7 +862,8 @@ skip_read_sample: hr = IMFSourceReader_Flush(reader, MF_SOURCE_READER_ALL_STREAMS); ok(hr == S_OK, "Failed to flush all streams, hr %#x.\n", hr); - IMFSourceReader_Release(reader); + refcount = IMFSourceReader_Release(reader); + ok(refcount == 0, "Release returned %u.\n", refcount); /* Async mode. */ callback = create_async_callback(); @@ -883,7 +884,10 @@ todo_wine { } IMFAttributes_Release(attributes); if (hr == S_OK) - IMFSourceReader_Release(reader); + { + refcount = IMFSourceReader_Release(reader); + ok(refcount == 0, "Release returned %u.\n", refcount); + } IMFByteStream_Release(stream); } -- 2.34.1
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- I think that it might make sense to also check that the media source is released: you keep a reference to it (a reference is taken at the beginning of the test, but immediately released), and check that that too returns zero. This way you check that the source reader is really released (i.e., there are no other reference loops that prevent src_reader_internal_release() from being called the right number of times). Thanks, Giovanni. Il 09/02/22 15:45, Rémi Bernon ha scritto:
To decide when to shutdown the media source. Otherwise the references hold by the callbacks from BeginGetEvent and others are keeping the source reader alive and the IMFMediaSource_Shutdown is never called.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/mfreadwrite/reader.c | 41 +++++++++++++++++++++++++-------- dlls/mfreadwrite/tests/mfplat.c | 8 +++++-- 2 files changed, 38 insertions(+), 11 deletions(-)
diff --git a/dlls/mfreadwrite/reader.c b/dlls/mfreadwrite/reader.c index be77e1c5b1d..18509e067b3 100644 --- a/dlls/mfreadwrite/reader.c +++ b/dlls/mfreadwrite/reader.c @@ -153,6 +153,7 @@ struct source_reader IMFAsyncCallback source_events_callback; IMFAsyncCallback stream_events_callback; IMFAsyncCallback async_commands_callback; + LONG internal_refcount; LONG refcount; IMFMediaSource *source; IMFPresentationDescriptor *descriptor; @@ -203,6 +204,9 @@ static struct media_stream *impl_stream_from_IMFVideoSampleAllocatorNotify(IMFVi return CONTAINING_RECORD(iface, struct media_stream, notify_cb); }
+static ULONG WINAPI src_reader_internal_addref(struct source_reader *reader); +static ULONG WINAPI src_reader_internal_release(struct source_reader *reader); + static HRESULT WINAPI source_reader_async_command_QueryInterface(IUnknown *iface, REFIID riid, void **obj) { if (IsEqualIID(riid, &IID_IUnknown)) @@ -325,13 +329,13 @@ static HRESULT WINAPI source_reader_callback_QueryInterface(IMFAsyncCallback *if static ULONG WINAPI source_reader_source_events_callback_AddRef(IMFAsyncCallback *iface) { struct source_reader *reader = impl_from_source_callback_IMFAsyncCallback(iface); - return IMFSourceReader_AddRef(&reader->IMFSourceReader_iface); + return src_reader_internal_addref(reader); }
static ULONG WINAPI source_reader_source_events_callback_Release(IMFAsyncCallback *iface) { struct source_reader *reader = impl_from_source_callback_IMFAsyncCallback(iface); - return IMFSourceReader_Release(&reader->IMFSourceReader_iface); + return src_reader_internal_release(reader); }
static HRESULT WINAPI source_reader_callback_GetParameters(IMFAsyncCallback *iface, @@ -611,13 +615,13 @@ static const IMFAsyncCallbackVtbl source_events_callback_vtbl = static ULONG WINAPI source_reader_stream_events_callback_AddRef(IMFAsyncCallback *iface) { struct source_reader *reader = impl_from_stream_callback_IMFAsyncCallback(iface); - return IMFSourceReader_AddRef(&reader->IMFSourceReader_iface); + return src_reader_internal_addref(reader); }
static ULONG WINAPI source_reader_stream_events_callback_Release(IMFAsyncCallback *iface) { struct source_reader *reader = impl_from_stream_callback_IMFAsyncCallback(iface); - return IMFSourceReader_Release(&reader->IMFSourceReader_iface); + return src_reader_internal_release(reader); }
static HRESULT source_reader_pull_stream_samples(struct source_reader *reader, struct media_stream *stream) @@ -888,13 +892,13 @@ static const IMFAsyncCallbackVtbl stream_events_callback_vtbl = static ULONG WINAPI source_reader_async_commands_callback_AddRef(IMFAsyncCallback *iface) { struct source_reader *reader = impl_from_async_commands_callback_IMFAsyncCallback(iface); - return IMFSourceReader_AddRef(&reader->IMFSourceReader_iface); + return src_reader_internal_addref(reader); }
static ULONG WINAPI source_reader_async_commands_callback_Release(IMFAsyncCallback *iface) { struct source_reader *reader = impl_from_async_commands_callback_IMFAsyncCallback(iface); - return IMFSourceReader_Release(&reader->IMFSourceReader_iface); + return src_reader_internal_release(reader); }
static struct stream_response * media_stream_detach_response(struct source_reader *reader, struct stream_response *response) @@ -1337,16 +1341,34 @@ static ULONG WINAPI src_reader_Release(IMFSourceReader *iface) { struct source_reader *reader = impl_from_IMFSourceReader(iface); ULONG refcount = InterlockedDecrement(&reader->refcount); - unsigned int i;
TRACE("%p, refcount %u.\n", iface, refcount);
if (!refcount) { - if (reader->async_callback) - IMFSourceReaderCallback_Release(reader->async_callback); if (reader->flags & SOURCE_READER_SHUTDOWN_ON_RELEASE) IMFMediaSource_Shutdown(reader->source); + src_reader_internal_release(reader); + } + + return refcount; +} + +static ULONG WINAPI src_reader_internal_addref(struct source_reader *reader) +{ + ULONG refcount = InterlockedIncrement(&reader->internal_refcount); + return refcount; +} + +static ULONG WINAPI src_reader_internal_release(struct source_reader *reader) +{ + ULONG refcount = InterlockedDecrement(&reader->internal_refcount); + unsigned int i; + + if (!refcount) + { + if (reader->async_callback) + IMFSourceReaderCallback_Release(reader->async_callback); if (reader->descriptor) IMFPresentationDescriptor_Release(reader->descriptor); if (reader->attributes) @@ -2267,6 +2289,7 @@ static HRESULT create_source_reader_from_source(IMFMediaSource *source, IMFAttri object->source_events_callback.lpVtbl = &source_events_callback_vtbl; object->stream_events_callback.lpVtbl = &stream_events_callback_vtbl; object->async_commands_callback.lpVtbl = &async_commands_callback_vtbl; + object->internal_refcount = 1; object->refcount = 1; list_init(&object->responses); if (shutdown_on_release) diff --git a/dlls/mfreadwrite/tests/mfplat.c b/dlls/mfreadwrite/tests/mfplat.c index 7e724344168..0791f51fa37 100644 --- a/dlls/mfreadwrite/tests/mfplat.c +++ b/dlls/mfreadwrite/tests/mfplat.c @@ -862,7 +862,8 @@ skip_read_sample: hr = IMFSourceReader_Flush(reader, MF_SOURCE_READER_ALL_STREAMS); ok(hr == S_OK, "Failed to flush all streams, hr %#x.\n", hr);
- IMFSourceReader_Release(reader); + refcount = IMFSourceReader_Release(reader); + ok(refcount == 0, "Release returned %u.\n", refcount);
/* Async mode. */ callback = create_async_callback(); @@ -883,7 +884,10 @@ todo_wine { } IMFAttributes_Release(attributes); if (hr == S_OK) - IMFSourceReader_Release(reader); + { + refcount = IMFSourceReader_Release(reader); + ok(refcount == 0, "Release returned %u.\n", refcount); + }
IMFByteStream_Release(stream); }
On 2/9/22 20:23, Giovanni Mascellani wrote:
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- I think that it might make sense to also check that the media source is released: you keep a reference to it (a reference is taken at the beginning of the test, but immediately released), and check that that too returns zero.
I don't think it should necessarily be zero, it should release what's been taken.
This way you check that the source reader is really released (i.e., there are no other reference loops that prevent src_reader_internal_release() from being called the right number of times).
Thanks, Giovanni.
On 2/9/22 20:26, Giovanni Mascellani wrote:
Hi,
Il 09/02/22 18:25, Nikolay Sivov ha scritto:
I don't think it should necessarily be zero, it should release what's been taken.
What else could be keeping a reference to that media source?
The application that created it. It works differently, but I think I found a way.
Giovanni.
Hi, Il 10/02/22 14:17, Nikolay Sivov ha scritto:
The application that created it. It works differently, but I think I found a way.
Yes, my comment was in the context of that mfreadwrite test with Rémi's changes: what we want to check is that the source reader was correctly and fully released. With Rémi's changes checking that Release returns 0 is not enough, because there is now an internal reference counter that is not exposed. It might be that there are some internal references that are not correctly cleared. One convenient way to check if the source reader was fully released is to check whether the media source was released, because in the context of the test nothing else should hold a reference to the media source. Of course, other applications may want to keep a reference to the media source, nothing is preventing that. Giovanni.
participants (3)
-
Giovanni Mascellani -
Nikolay Sivov -
Rémi Bernon