From: Zebediah Figura <zfigura@codeweavers.com>
Subject: Re: [PATCH 4/5] amstream: Implement AMDirectDrawStream::Receive.
Message-Id: <39c613dc-e9e9-91c5-b28a-b11f41916c0b@codeweavers.com>
Date: Wed, 23 Sep 2020 15:25:31 -0500
In-Reply-To: <20200923185500.9458-4-baskanov@gmail.com>
References: <20200923185500.9458-1-baskanov@gmail.com> <20200923185500.9458-4-baskanov@gmail.com>

On 9/23/20 1:54 PM, Anton Baskanov wrote:
> Signed-off-by: Anton Baskanov <baskanov@gmail.com>
> ---
>  dlls/amstream/ddrawstream.c    |  25 ++++++-
>  dlls/amstream/tests/amstream.c | 118 +++++++++++++++++++++++++++++++++
>  2 files changed, 141 insertions(+), 2 deletions(-)
> 
> diff --git a/dlls/amstream/ddrawstream.c b/dlls/amstream/ddrawstream.c
> index e706841c8fc..856c8775445 100644
> --- a/dlls/amstream/ddrawstream.c
> +++ b/dlls/amstream/ddrawstream.c
> @@ -59,6 +59,7 @@ struct ddraw_stream
>      struct format format;
>      FILTER_STATE state;
>      BOOL eos;
> +    HANDLE update_queued_event;
>  };
>  
>  static HRESULT ddrawstreamsample_create(struct ddraw_stream *parent, IDirectDrawSurface *surface,
> @@ -150,6 +151,7 @@ static ULONG WINAPI ddraw_IAMMediaStream_Release(IAMMediaStream *iface)
>          DeleteCriticalSection(&stream->cs);
>          if (stream->ddraw)
>              IDirectDraw_Release(stream->ddraw);
> +        CloseHandle(stream->update_queued_event);
>          HeapFree(GetProcessHeap(), 0, stream);
>      }
>  
> @@ -268,6 +270,8 @@ static HRESULT WINAPI ddraw_IAMMediaStream_SetState(IAMMediaStream *iface, FILTE
>  
>      EnterCriticalSection(&stream->cs);
>  
> +    if (state == State_Stopped)
> +        SetEvent(stream->update_queued_event);
>      if (stream->state == State_Stopped)
>          stream->eos = FALSE;
>  
> @@ -1190,8 +1194,24 @@ static HRESULT WINAPI ddraw_meminput_GetAllocatorRequirements(IMemInputPin *ifac
>  
>  static HRESULT WINAPI ddraw_meminput_Receive(IMemInputPin *iface, IMediaSample *sample)
>  {
> -    FIXME("iface %p, sample %p, stub!\n", iface, sample);
> -    return E_NOTIMPL;
> +    struct ddraw_stream *stream = impl_from_IMemInputPin(iface);
> +
> +    TRACE("stream %p, sample %p.\n", stream, sample);
> +
> +    EnterCriticalSection(&stream->cs);
> +
> +    for (;;)
> +    {
> +        if (stream->state == State_Stopped)
> +        {
> +            LeaveCriticalSection(&stream->cs);
> +            return S_OK;
> +        }
> +
> +        LeaveCriticalSection(&stream->cs);
> +        WaitForSingleObject(stream->update_queued_event, INFINITE);
> +        EnterCriticalSection(&stream->cs);
> +    }
>  }

This is the sort of pattern that makes me think that a condition
variable would be a good choice here.

On a sort of related note, the point of the event is not exactly clear
with this patch. I think it may make sense to merge this patch with the
next; the two functions are pretty tightly tied together.

>  
>  static HRESULT WINAPI ddraw_meminput_ReceiveMultiple(IMemInputPin *iface,
> @@ -1236,6 +1256,7 @@ HRESULT ddraw_stream_create(IUnknown *outer, void **out)
>      object->IMemInputPin_iface.lpVtbl = &ddraw_meminput_vtbl;
>      object->IPin_iface.lpVtbl = &ddraw_sink_vtbl;
>      object->ref = 1;
> +    object->update_queued_event = CreateEventW(NULL, FALSE, FALSE, NULL);
>  
>      object->format.width = 100;
>      object->format.height = 100;
> diff --git a/dlls/amstream/tests/amstream.c b/dlls/amstream/tests/amstream.c
> index df9d148493f..d0c20f6253a 100644
> --- a/dlls/amstream/tests/amstream.c
> +++ b/dlls/amstream/tests/amstream.c
> @@ -5076,6 +5076,123 @@ static void test_ddrawstream_set_format(void)
>      ok(!ref, "Got outstanding refcount %d.\n", ref);
>  }
>  
> +static IAMMultiMediaStream *ddrawstream_mmstream;
> +static STREAM_STATE ddrawstream_state;
> +
> +static DWORD CALLBACK ddrawstream_set_state(void *param)
> +{
> +    HRESULT hr;
> +
> +    Sleep(100);
> +    hr = IAMMultiMediaStream_SetState(ddrawstream_mmstream, ddrawstream_state);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +
> +    return 0;
> +}

I'm inclined to think it makes things clearer to make Receive()
asynchronous, since that's the thing that blocks. It also allows you to
explicitly test that the function blocks, and to avoid the Sleep().

> +
> +static void test_ddrawstream_receive(void)
> +{
> +    ALLOCATOR_PROPERTIES properties =
> +    {
> +        .cBuffers = 1,
> +        .cbBuffer = 16,
> +        .cbAlign = 1,
> +    };
> +
> +    IAMMultiMediaStream *mmstream = create_ammultimediastream();
> +    ALLOCATOR_PROPERTIES actual;
> +    IMediaStreamFilter *filter;
> +    struct testfilter source;
> +    IMemAllocator *allocator;
> +    IGraphBuilder *graph;
> +    IMediaStream *stream;
> +    IMediaSample *sample;
> +    FILTER_STATE state;
> +    HANDLE thread;
> +    HRESULT hr;
> +    ULONG ref;
> +    IPin *pin;
> +
> +    hr = IAMMultiMediaStream_Initialize(mmstream, STREAMTYPE_READ, 0, NULL);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +    hr = IAMMultiMediaStream_GetFilter(mmstream, &filter);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +    ok(!!filter, "Expected non-null filter.\n");
> +    hr = IAMMultiMediaStream_AddMediaStream(mmstream, NULL, &MSPID_PrimaryVideo, 0, &stream);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +    hr = IMediaStream_QueryInterface(stream, &IID_IPin, (void **)&pin);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +    hr = IAMMultiMediaStream_GetFilterGraph(mmstream, &graph);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +    ok(graph != NULL, "Expected non-NULL graph.\n");
> +    testfilter_init(&source);
> +    hr = IGraphBuilder_AddFilter(graph, &source.filter.IBaseFilter_iface, NULL);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +    hr = CoCreateInstance(&CLSID_MemoryAllocator, NULL, CLSCTX_INPROC_SERVER, &IID_IMemAllocator, (void **)&allocator);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +    hr = IMemAllocator_SetProperties(allocator, &properties, &actual);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +    hr = IMemAllocator_Commit(allocator);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +
> +    hr = IGraphBuilder_ConnectDirect(graph, &source.source.pin.IPin_iface, pin, &rgb32_mt);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +
> +    hr = IMemAllocator_GetBuffer(allocator, &sample, NULL, NULL, 0);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +    hr = IMemInputPin_Receive(source.source.pMemInputPin, sample);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +    ref = IMediaSample_Release(sample);
> +    ok(!ref, "Got outstanding refcount %d.\n", ref);
> +
> +    hr = IAMMultiMediaStream_SetState(mmstream, STREAMSTATE_RUN);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +
> +    hr = IMemAllocator_GetBuffer(allocator, &sample, NULL, NULL, 0);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +
> +    ddrawstream_mmstream = mmstream;
> +    ddrawstream_state = STREAMSTATE_STOP;
> +    thread = CreateThread(NULL, 0, ddrawstream_set_state, NULL, 0, NULL);
> +
> +    hr = IMemInputPin_Receive(source.source.pMemInputPin, sample);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +    hr = IMediaStreamFilter_GetState(filter, 0, &state);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +    ok(state == State_Stopped, "Got state %#x.\n", state);
> +
> +    ok(!WaitForSingleObject(thread, 2000), "Wait timed out.\n");
> +    CloseHandle(thread);
> +
> +    ref = IMediaSample_Release(sample);
> +    ok(!ref, "Got outstanding refcount %d.\n", ref);
> +
> +    hr = IMemAllocator_GetBuffer(allocator, &sample, NULL, NULL, 0);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +    hr = IMemInputPin_Receive(source.source.pMemInputPin, sample);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +    ref = IMediaSample_Release(sample);
> +    ok(!ref, "Got outstanding refcount %d.\n", ref);
> +
> +    IGraphBuilder_Disconnect(graph, pin);
> +    IGraphBuilder_Disconnect(graph, &source.source.pin.IPin_iface);
> +
> +    hr = IMemAllocator_Decommit(allocator);
> +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> +
> +    ref = IAMMultiMediaStream_Release(mmstream);
> +    ok(!ref, "Got outstanding refcount %d.\n", ref);
> +    ref = IGraphBuilder_Release(graph);
> +    ok(!ref, "Got outstanding refcount %d.\n", ref);
> +    ref = IMediaStreamFilter_Release(filter);
> +    ok(!ref, "Got outstanding refcount %d.\n", ref);
> +    IPin_Release(pin);
> +    ref = IMediaStream_Release(stream);
> +    ok(!ref, "Got outstanding refcount %d.\n", ref);
> +    ref = IMemAllocator_Release(allocator);
> +    ok(!ref, "Got outstanding refcount %d.\n", ref);
> +}
> +
>  static void check_ammediastream_join_am_multi_media_stream(const CLSID *clsid)
>  {
>      IAMMultiMediaStream *mmstream = create_ammultimediastream();
> @@ -6880,6 +6997,7 @@ START_TEST(amstream)
>      test_ddrawstream_create_sample();
>      test_ddrawstream_get_format();
>      test_ddrawstream_set_format();
> +    test_ddrawstream_receive();
>  
>      test_ddrawstreamsample_get_media_stream();
>  
> 


-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEENTo75Twe9rbPART3DZ01igeheEAFAl9rrzsACgkQDZ01igeh
eEBWQwf/Vi2VVdxVaolKmS2t1vVos59r3wBqa78bWHKo2BrsXb9O1KL9QOkVL39s
lPNTayJDCSNUz8Bfil6BPP2u45FKyX4LwJI4P48Mt8SmHRa/2XxqAg991c/LKcUe
+fP/yT6QfsaM0XAuNbfBxG9Yl39NxFwZwtQ3pyx1/h7/yNt4JNm8+gksA4+LVl9s
/+57vEIRFAspTGRb0OjDG9jRUiWfktlbQjx5iWiwcFSKY9Ih0Ht8xci37vb50C3n
gPdRK6kNwr2rAcpKL+GMan0tmFBrAmSSrpBq2sIvnROeCf7w/AnmaoU2WL+UCQtY
CN4js0jFqiCqeE/mIXX7h+6CFptG/Q==
=tC4Y
-----END PGP SIGNATURE-----