On Sat, Jul 9, 2022 at 9:52 AM Brendan McGrath <brendan@redmandi.com> wrote:

Unravel Two adds a reference to the IMediaSample during its callback.

This patch adds a test that checks if an application does do this, that it can
free it later and the reference count will finish at zero.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51616
Signed-off-by: Brendan McGrath <brendan@redmandi.com>
---

Thanks Zeb for taking a look. Agree with your feedback. I guess we need
to prove the workaround is incorrect.

This test I think replicates what Unravel Two does, and it does demonstrate
that the workaround will cause an issue in this scenario.

But it would be good to hear back from the original author; although 12
years is a long time!

Unrelated to this patchset - but I noticed I get a seg fault when running the
mediadet tests (the others seem ok). It happens right at the end:
Segmentation fault (core dumped)
make: *** [Makefile:120413: dlls/qedit/tests/mediadet.ok] Error 139

I didn't see it on the Wine test servers, but I'm wondering if you get
the same when you run locally?

Yes, I also get that.

I've created a wine bug for this:
https://bugs.winehq.org/show_bug.cgi?id=53556

I've added a lot more detail there, and an experimental fix. I'd be curious to know if the segfault you are seeing is the same thing.

Anyway, thanks again. Appreciate your time.


 dlls/qedit/tests/mediadet.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/dlls/qedit/tests/mediadet.c b/dlls/qedit/tests/mediadet.c
index 3202baca9244..3ea752176618 100644
--- a/dlls/qedit/tests/mediadet.c
+++ b/dlls/qedit/tests/mediadet.c
@@ -866,6 +866,11 @@ static void test_put_filter(void)
     ok(!ref, "Got outstanding refcount %ld.\n", ref);
 }

+struct MySample {
+  IMediaSample sample;
+  LONG refcount;
+};
+
 static HRESULT WINAPI ms_QueryInterface(IMediaSample *iface, REFIID riid,
         void **ppvObject)
 {
@@ -874,12 +879,18 @@ static HRESULT WINAPI ms_QueryInterface(IMediaSample *iface, REFIID riid,

 static ULONG WINAPI ms_AddRef(IMediaSample *iface)
 {
-    return 2;
+    struct MySample *my_sample = (struct MySample*) iface;
+    ULONG refcount = InterlockedIncrement(&my_sample->refcount);
+
+    return refcount;
 }

 static ULONG WINAPI ms_Release(IMediaSample *iface)
 {
-    return 1;
+    struct MySample *my_sample = (struct MySample*) iface;
+    ULONG refcount = InterlockedDecrement(&my_sample->refcount);
+
+    return refcount;
 }

 static HRESULT WINAPI ms_GetPointer(IMediaSample *iface, BYTE **ppBuffer)
@@ -989,7 +1000,7 @@ static const IMediaSampleVtbl my_sample_vt = {
     ms_SetMediaTime
 };

-static IMediaSample my_sample = { &my_sample_vt };
+static struct MySample my_sample = { {&my_sample_vt}, 0 };

 static BOOL samplecb_called = FALSE;

@@ -1012,8 +1023,9 @@ static ULONG WINAPI sgcb_Release(ISampleGrabberCB *iface)
 static HRESULT WINAPI sgcb_SampleCB(ISampleGrabberCB *iface, double SampleTime,
         IMediaSample *pSample)
 {
-    ok(pSample == &my_sample, "Got wrong IMediaSample: %p, expected %p\n", pSample, &my_sample);
+    ok(pSample == &my_sample.sample, "Got wrong IMediaSample: %p, expected %p\n", pSample, &my_sample);
     samplecb_called = TRUE;
+    IMediaSample_AddRef(pSample);
     return E_NOTIMPL;
 }

@@ -1043,6 +1055,7 @@ static void test_samplegrabber(void)
     IEnumPins *pins;
     HRESULT hr;
     FILTER_STATE fstate;
+    ULONG refcount;

     /* Invalid RIID */
     hr = CoCreateInstance(&CLSID_SampleGrabber, NULL, CLSCTX_INPROC_SERVER, &IID_IClassFactory,
@@ -1074,10 +1087,14 @@ static void test_samplegrabber(void)
     hr = IPin_QueryInterface(pin, &IID_IMemInputPin, (void**)&inpin);
     ok(hr == S_OK, "Got hr %#lx.\n", hr);

-    hr = IMemInputPin_Receive(inpin, &my_sample);
+    hr = IMemInputPin_Receive(inpin, &my_sample.sample);
     ok(hr == S_OK, "Got hr %#lx.\n", hr);
     ok(samplecb_called == TRUE, "SampleCB should have been called\n");

+    // Release the Ref we added in the callback
+    refcount = IUnknown_Release(&my_sample.sample);
+    todo_wine ok(refcount == 0, "refcount should be zero\n");
+
     IMemInputPin_Release(inpin);
     IPin_Release(pin);

--
2.34.1