Re: [PATCH 1/2] amstream: Implement IDirectDrawMediaStream::SetDirectDraw().

[Zebediah Figura]

On 6/23/20 5:32 PM, Gijs Vermeulen wrote:
> Signed-off-by: Gijs Vermeulen
>
> dlls/amstream/ddrawstream.c    | 34 ++++++++++--
>  dlls/amstream/tests/amstream.c | 95 ++++++++++++++++------------------
>  2 files changed, 77 insertions(+), 52 deletions(-)
>
> diff --git a/dlls/amstream/ddrawstream.c b/dlls/amstream/ddrawstream.c
> index ca6a910f3f..7f42879254 100644
> --- a/dlls/amstream/ddrawstream.c
> +++ b/dlls/amstream/ddrawstream.c
> @@ -35,6 +35,7 @@ struct ddraw_stream
>      IMemInputPin IMemInputPin_iface;
>      IPin IPin_iface;
>      LONG ref;
> -
>
> LONG sample_refs;
>
> IMultiMediaStream* parent;
> MSPID purpose_id;
>
> @@ -393,11 +394,34 @@ static HRESULT WINAPI ddraw_IDirectDrawMediaStream_GetDirectDraw(IDirectDrawMedi
>  }
>
> static HRESULT WINAPI ddraw_IDirectDrawMediaStream_SetDirectDraw(IDirectDrawMediaStream *iface,
> -
>    IDirectDraw *pDirectDraw)
> -
>    IDirectDraw *ddraw)
>
> {
> - FIXME("(%p)->(%p) stub!\n", iface, pDirectDraw);
> - struct ddraw_stream *stream = impl_from_IDirectDrawMediaStream(iface);
> - return E_NOTIMPL;
> - TRACE("stream %p, ddraw %p.\n", stream, ddraw);
> -
> - EnterCriticalSection(&stream->cs);
> -
> - if (stream->sample_refs)
> - {
> -
>    LeaveCriticalSection(&stream->cs);
> -
>    return ((IDirectDraw *)stream->ddraw == ddraw) ? S_OK : MS_E_SAMPLEALLOC;

Unfortunately that won't work. You can't just cast between IDirectDraw
and IDirectDraw7 (in fact, you can't do it in either direction; despite
the name they don't inherit from each other).

I think the right thing to do is use IDirectDraw internally rather than
IDirectDraw7. I know I expressed reservation before about this, but on
reflection I think my concerns aren't relevant for amstream (in
particular, we don't ever need a 3D device).

Note also that you drop the lock before accessing stream->ddraw here,
which is probably not what you meant to do.
> - }
> -
> - if (stream->ddraw)
> -
>    IDirectDraw7_Release(stream->ddraw);
> -
> - if (ddraw)
> - {
> -
>    IDirectDraw_AddRef(ddraw);
> -
>    stream->ddraw = (IDirectDraw7 *) ddraw;

Same here.
> - }
> - else
> -
>    stream->ddraw = NULL;
> -
> - LeaveCriticalSection(&stream->cs);
> -
> - return S_OK;
>
> }
>
> static HRESULT WINAPI ddraw_IDirectDrawMediaStream_CreateSample(IDirectDrawMediaStream *iface,
> @@ -948,6 +972,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->sample_refs = 0;
>
> InitializeCriticalSection(&object->cs);
>
> @@ -1010,6 +1035,8 @@ static ULONG WINAPI ddraw_sample_Release(IDirectDrawStreamSample *iface)
>  TRACE("(%p)->(): new ref = %u\n", iface, ref);
>
> - InterlockedDecrement(&sample->parent->sample_refs);
> - if (!ref)
>    {
>        if (sample->surface)
>
> @@ -1127,6 +1154,7 @@ static HRESULT ddrawstreamsample_create(struct ddraw_stream *parent, IDirectDraw
>      object->IDirectDrawStreamSample_iface.lpVtbl = &DirectDrawStreamSample_Vtbl;
>      object->ref = 1;
>      object->parent = parent;
> -
>
> InterlockedIncrement(&parent->sample_refs);
>
> if (surface)
> {
>
> diff --git a/dlls/amstream/tests/amstream.c b/dlls/amstream/tests/amstream.c
> index 03c8660a42..8f474215b5 100644
> --- a/dlls/amstream/tests/amstream.c
> +++ b/dlls/amstream/tests/amstream.c
> @@ -4798,7 +4798,7 @@ static void test_ddrawstream_getsetdirectdraw(void)
>  /* The current ddraw is released when SetDirectDraw() is called. */
>  hr = IDirectDrawMediaStream_SetDirectDraw(ddraw_stream, NULL);
> - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
> -
>
> ok(hr == S_OK, "Got hr %#x.\n", hr);
> EXPECT_REF(ddraw, 2);
>
> hr = IDirectDrawMediaStream_GetDirectDraw(ddraw_stream, &ddraw3);
>
> @@ -4807,65 +4807,62 @@ static void test_ddrawstream_getsetdirectdraw(void)
>      if (ddraw3) IDirectDraw_Release(ddraw3);
>  hr = IDirectDrawMediaStream_SetDirectDraw(ddraw_stream, ddraw2);
> - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
> - todo_wine EXPECT_REF(ddraw, 3);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> - EXPECT_REF(ddraw, 3);
> - if (hr == S_OK)
> - {
> -
>    hr = IDirectDrawMediaStream_GetDirectDraw(ddraw_stream, &ddraw3);
> -
>    ok(hr == S_OK, "Got hr %#x.\n", hr);
> -
>    ok(ddraw3 == ddraw2, "Expected ddraw %p, got %p.\n", ddraw2, ddraw3);
> -
>    EXPECT_REF(ddraw, 4);
> -
>    IDirectDraw_Release(ddraw3);
> -
>    EXPECT_REF(ddraw, 3);
> - hr = IDirectDrawMediaStream_GetDirectDraw(ddraw_stream, &ddraw3);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> - ok(ddraw3 == ddraw2, "Expected ddraw %p, got %p.\n", ddraw2, ddraw3);
> - EXPECT_REF(ddraw, 4);
> - IDirectDraw_Release(ddraw3);
> - EXPECT_REF(ddraw, 3);
> -
>    hr = IDirectDrawMediaStream_CreateSample(ddraw_stream, NULL, NULL, 0, &sample);
> -
>    ok(hr == S_OK, "Got hr %#x.\n", hr);
> - hr = IDirectDrawMediaStream_CreateSample(ddraw_stream, NULL, NULL, 0, &sample);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> -
>     /* SetDirectDraw() doesn't take an extra reference to the ddraw object
> -
>      * if there are samples extant. */
> -
>    hr = IDirectDrawMediaStream_SetDirectDraw(ddraw_stream, ddraw2);
> -
>    ok(hr == S_OK, "Got hr %#x.\n", hr);
> -
>    EXPECT_REF(ddraw, 3);
> -
> /* SetDirectDraw() doesn't take an extra reference to the ddraw object
> -
>  * if there are samples extant. */
> - hr = IDirectDrawMediaStream_SetDirectDraw(ddraw_stream, ddraw2);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> - EXPECT_REF(ddraw, 3);
> -
>    hr = DirectDrawCreate(NULL, &ddraw3, NULL);
> -
>    ok(hr == S_OK, "Got hr %#x.\n", hr);
> -
>    hr = IDirectDraw_SetCooperativeLevel(ddraw3, GetDesktopWindow(), DDSCL_NORMAL);
> -
>    ok(hr == DD_OK, "Got hr %#x.\n", hr);
> -
>    EXPECT_REF(ddraw3, 1);
> - hr = DirectDrawCreate(NULL, &ddraw3, NULL);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> - hr = IDirectDraw_SetCooperativeLevel(ddraw3, GetDesktopWindow(), DDSCL_NORMAL);
> - ok(hr == DD_OK, "Got hr %#x.\n", hr);
> - EXPECT_REF(ddraw3, 1);
> -
>    hr = IDirectDrawMediaStream_SetDirectDraw(ddraw_stream, ddraw3);
> -
>    ok(hr == MS_E_SAMPLEALLOC, "Got hr %#x.\n", hr);
> - hr = IDirectDrawMediaStream_SetDirectDraw(ddraw_stream, ddraw3);
> - ok(hr == MS_E_SAMPLEALLOC, "Got hr %#x.\n", hr);
> -
>    hr = IDirectDrawMediaStream_GetDirectDraw(ddraw_stream, &ddraw4);
> -
>    ok(hr == S_OK, "Got hr %#x.\n", hr);
> -
>    ok(ddraw4 == ddraw2, "Expected ddraw %p, got %p.\n", ddraw2, ddraw4);
> -
>    EXPECT_REF(ddraw, 4);
> -
>    IDirectDraw_Release(ddraw4);
> -
>    EXPECT_REF(ddraw, 3);
> - hr = IDirectDrawMediaStream_GetDirectDraw(ddraw_stream, &ddraw4);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> - ok(ddraw4 == ddraw2, "Expected ddraw %p, got %p.\n", ddraw2, ddraw4);
> - EXPECT_REF(ddraw, 4);
> - IDirectDraw_Release(ddraw4);
> - EXPECT_REF(ddraw, 3);
> -
>    ref = IDirectDrawStreamSample_Release(sample);
> -
>    ok(!ref, "Got outstanding refcount %d.\n", ref);
> - ref = IDirectDrawStreamSample_Release(sample);
> - ok(!ref, "Got outstanding refcount %d.\n", ref);
> -
>    hr = IDirectDrawMediaStream_SetDirectDraw(ddraw_stream, ddraw3);
> -
>    ok(hr == S_OK, "Got hr %#x.\n", hr);
> -
>    EXPECT_REF(ddraw, 2);
> -
>    EXPECT_REF(ddraw3, 2);
> - hr = IDirectDrawMediaStream_SetDirectDraw(ddraw_stream, ddraw3);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> - EXPECT_REF(ddraw, 2);
> - EXPECT_REF(ddraw3, 2);
> -
>    hr = IDirectDrawMediaStream_GetDirectDraw(ddraw_stream, &ddraw4);
> -
>    ok(hr == S_OK, "Got hr %#x.\n", hr);
> -
>    ok(ddraw4 == ddraw3, "Expected ddraw %p, got %p.\n", ddraw3, ddraw4);
> -
>    EXPECT_REF(ddraw3, 3);
> -
>    IDirectDraw_Release(ddraw4);
> -
>    EXPECT_REF(ddraw3, 2);
> - hr = IDirectDrawMediaStream_GetDirectDraw(ddraw_stream, &ddraw4);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> - ok(ddraw4 == ddraw3, "Expected ddraw %p, got %p.\n", ddraw3, ddraw4);
> - EXPECT_REF(ddraw3, 3);
> - IDirectDraw_Release(ddraw4);
> - EXPECT_REF(ddraw3, 2);
> -
>    hr = IDirectDrawMediaStream_SetDirectDraw(ddraw_stream, NULL);
> -
>    ok(hr == S_OK, "Got hr %#x.\n", hr);
> -
>    EXPECT_REF(ddraw3, 1);
> - hr = IDirectDrawMediaStream_SetDirectDraw(ddraw_stream, NULL);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> - EXPECT_REF(ddraw3, 1);
> -
>    ref = IDirectDraw_Release(ddraw3);
> -
>    ok(!ref, "Got outstanding refcount %d.\n", ref);
> - }
> -
>
> ref = IDirectDraw_Release(ddraw3);
> -
>
> ok(!ref, "Got outstanding refcount %d.\n", ref);
>
> EXPECT_REF(stream, 3);
> IDirectDrawMediaStream_Release(ddraw_stream);