[PATCH] amstream: Implement SSUPDATE_CONTINUOUS flag in IDirectDrawStreamSample::Update.

[Anton Baskanov]

Signed-off-by: Anton Baskanov 
---
 dlls/amstream/ddrawstream.c    | 36 ++++++++++++++---------
 dlls/amstream/tests/amstream.c | 53 ++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+), 14 deletions(-)

diff --git a/dlls/amstream/ddrawstream.c b/dlls/amstream/ddrawstream.c
index 5e6e1555d7c..0e5e54bd80d 100644
--- a/dlls/amstream/ddrawstream.c
+++ b/dlls/amstream/ddrawstream.c
@@ -75,10 +75,12 @@ struct ddraw_sample
     RECT rect;
     STREAM_TIME start_time;
     STREAM_TIME end_time;
+    BOOL continuous_update;
     CONDITION_VARIABLE update_cv;

struct list entry;
     HRESULT update_hr;
+    BOOL busy;
 };

static HRESULT ddrawstreamsample_create(struct ddraw_stream *parent, IDirectDrawSurface *surface,
@@ -86,6 +88,7 @@ static HRESULT ddrawstreamsample_create(struct ddraw_stream *parent, IDirectDraw

static void remove_queued_update(struct ddraw_sample *sample)
 {
+    sample->busy = FALSE;
     list_remove(&sample->entry);
     WakeConditionVariable(&sample->update_cv);
 }
@@ -1348,7 +1351,16 @@ static HRESULT WINAPI ddraw_meminput_Receive(IMemInputPin *iface, IMediaSample *
             sample->update_hr = process_update(sample, top_down_stride, top_down_pointer,
                     start_stream_time, end_stream_time);

-            remove_queued_update(sample);
+            if (sample->continuous_update && SUCCEEDED(sample->update_hr))
+            {
+                list_remove(&sample->entry);
+                list_add_tail(&sample->parent->update_queue, &sample->entry);
+            }
+            else
+            {
+                remove_queued_update(sample);
+            }
+
             LeaveCriticalSection(&stream->cs);
             return S_OK;
         }
@@ -1540,12 +1552,6 @@ static HRESULT WINAPI ddraw_sample_Update(IDirectDrawStreamSample *iface,
         return E_NOTIMPL;
     }

-    if (flags & ~SSUPDATE_ASYNC)
-    {
-        FIXME("Unsupported flags %#x.\n", flags);
-        return E_NOTIMPL;
-    }
-
     EnterCriticalSection(&sample->parent->cs);

if (sample->parent->state != State_Running)
@@ -1559,13 +1565,16 @@ static HRESULT WINAPI ddraw_sample_Update(IDirectDrawStreamSample *iface,
         LeaveCriticalSection(&sample->parent->cs);
         return MS_S_ENDOFSTREAM;
     }
-    if (MS_S_PENDING == sample->update_hr)
+    if (sample->busy)
     {
         LeaveCriticalSection(&sample->parent->cs);
         return MS_E_BUSY;
     }

-    sample->update_hr = MS_S_PENDING;
+    sample->continuous_update = (flags & SSUPDATE_ASYNC) && (flags & SSUPDATE_CONTINUOUS);
+
+    sample->update_hr = MS_S_NOUPDATE;
+    sample->busy = TRUE;
     list_add_tail(&sample->parent->update_queue, &sample->entry);
     WakeConditionVariable(&sample->parent->update_queued_cv);

@@ -1575,7 +1584,7 @@ static HRESULT WINAPI ddraw_sample_Update(IDirectDrawStreamSample *iface,
         return MS_S_PENDING;
     }

-    while (sample->update_hr == MS_S_PENDING)
+    while (sample->busy)
         SleepConditionVariableCS(&sample->update_cv, &sample->parent->cs, INFINITE);

LeaveCriticalSection(&sample->parent->cs);
@@ -1592,18 +1601,17 @@ static HRESULT WINAPI ddraw_sample_CompletionStatus(IDirectDrawStreamSample *ifa

EnterCriticalSection(&sample->parent->cs);

-    if (sample->update_hr == MS_S_PENDING)
+    if (sample->busy)
     {
         if (flags & (COMPSTAT_NOUPDATEOK | COMPSTAT_ABORT))
         {
-            sample->update_hr = MS_S_NOUPDATE;
             remove_queued_update(sample);
         }
         else if (flags & COMPSTAT_WAIT)
         {
             DWORD start_time = GetTickCount();
             DWORD elapsed = 0;
-            while (sample->update_hr == MS_S_PENDING && elapsed < milliseconds)
+            while (sample->busy && elapsed < milliseconds)
             {
                 DWORD sleep_time = milliseconds - elapsed;
                 if (!SleepConditionVariableCS(&sample->update_cv, &sample->parent->cs, sleep_time))
@@ -1613,7 +1621,7 @@ static HRESULT WINAPI ddraw_sample_CompletionStatus(IDirectDrawStreamSample *ifa
         }
     }

-    hr = sample->update_hr;
+    hr = sample->busy ? MS_S_PENDING : sample->update_hr;

LeaveCriticalSection(&sample->parent->cs);

diff --git a/dlls/amstream/tests/amstream.c b/dlls/amstream/tests/amstream.c
index 9dda5882a4f..6e6f7483b49 100644
--- a/dlls/amstream/tests/amstream.c
+++ b/dlls/amstream/tests/amstream.c
@@ -7665,6 +7665,59 @@ static void test_ddrawstreamsample_completion_status(void)
     hr = IAMMultiMediaStream_SetState(mmstream, STREAMSTATE_RUN);
     ok(hr == S_OK, "Got hr %#x.\n", hr);

+    hr = IDirectDrawStreamSample_Update(stream_sample1, SSUPDATE_ASYNC | SSUPDATE_CONTINUOUS, NULL, NULL, 0);
+    ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
+
+    hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, COMPSTAT_NOUPDATEOK, 0);
+    ok(hr == MS_S_NOUPDATE, "Got hr %#x.\n", hr);
+
+    hr = IDirectDrawStreamSample_Update(stream_sample1, SSUPDATE_ASYNC | SSUPDATE_CONTINUOUS, NULL, NULL, 0);
+    ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
+
+    hr = IDirectDrawStreamSample_Update(stream_sample2, SSUPDATE_ASYNC, NULL, NULL, 0);
+    ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
+
+    hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, 0, 0);
+    ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
+
+    media_sample = ammediastream_allocate_sample(&source, test_data, sizeof(test_data));
+    hr = IMemInputPin_Receive(source.source.pMemInputPin, media_sample);
+    ok(hr == S_OK, "Got hr %#x.\n", hr);
+    ref = IMediaSample_Release(media_sample);
+    ok(!ref, "Got outstanding refcount %d.\n", ref);
+
+    hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, 0, 0);
+    ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
+
+    media_sample = ammediastream_allocate_sample(&source, test_data, sizeof(test_data));
+    hr = IMemInputPin_Receive(source.source.pMemInputPin, media_sample);
+    ok(hr == S_OK, "Got hr %#x.\n", hr);
+    ref = IMediaSample_Release(media_sample);
+    ok(!ref, "Got outstanding refcount %d.\n", ref);
+
+    hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, 0, 0);
+    ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
+
+    hr = IDirectDrawStreamSample_CompletionStatus(stream_sample2, 0, 0);
+    ok(hr == S_OK, "Got hr %#x.\n", hr);
+
+    hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, COMPSTAT_NOUPDATEOK, 0);
+    ok(hr == S_OK, "Got hr %#x.\n", hr);
+
+    hr = IDirectDrawStreamSample_Update(stream_sample1, SSUPDATE_ASYNC | SSUPDATE_CONTINUOUS, NULL, NULL, 0);
+    ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
+
+    hr = IPin_EndOfStream(pin);
+    ok(hr == S_OK, "Got hr %#x.\n", hr);
+
+    hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, 0, 0);
+    ok(hr == MS_S_ENDOFSTREAM, "Got hr %#x.\n", hr);
+
+    hr = IAMMultiMediaStream_SetState(mmstream, STREAMSTATE_STOP);
+    ok(hr == S_OK, "Got hr %#x.\n", hr);
+    hr = IAMMultiMediaStream_SetState(mmstream, STREAMSTATE_RUN);
+    ok(hr == S_OK, "Got hr %#x.\n", hr);
+
     hr = IDirectDrawStreamSample_Update(stream_sample1, SSUPDATE_ASYNC, NULL, NULL, 0);
     ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);

[Zebediah Figura]

On 10/8/20 2:02 PM, Anton Baskanov wrote:
> Signed-off-by: Anton Baskanov
>
> dlls/amstream/ddrawstream.c    | 36 ++++++++++++++---------
>  dlls/amstream/tests/amstream.c | 53 ++++++++++++++++++++++++++++++++++
>  2 files changed, 75 insertions(+), 14 deletions(-)
>
> diff --git a/dlls/amstream/ddrawstream.c b/dlls/amstream/ddrawstream.c
> index 5e6e1555d7c..0e5e54bd80d 100644
> --- a/dlls/amstream/ddrawstream.c
> +++ b/dlls/amstream/ddrawstream.c
> @@ -75,10 +75,12 @@ struct ddraw_sample
>      RECT rect;
>      STREAM_TIME start_time;
>      STREAM_TIME end_time;
> -
>
> BOOL continuous_update;
> CONDITION_VARIABLE update_cv;
>
> struct list entry;
> HRESULT update_hr;
> -
>
> BOOL busy;

As far as I can tell, this is identical to "update_hr == MS_S_PENDING",
except that you set update_hr to MS_S_NOUPDATE in the continuous case
instead, right?

Would it be simpler, then, to keep most of the code handling update_hr
as it is, and instead handle COMPSTAT_NOUPDATEOK/MS_S_NOUPDATE specially
in ::CompletionStatus()?
> };
>
> static HRESULT ddrawstreamsample_create(struct ddraw_stream *parent, IDirectDrawSurface *surface,
> @@ -86,6 +88,7 @@ static HRESULT ddrawstreamsample_create(struct ddraw_stream *parent, IDirectDraw
>
> static void remove_queued_update(struct ddraw_sample *sample)
>  {
> - sample->busy = FALSE;
> list_remove(&sample->entry);
> WakeConditionVariable(&sample->update_cv);
>
> }
> @@ -1348,7 +1351,16 @@ static HRESULT WINAPI ddraw_meminput_Receive(IMemInputPin *iface, IMediaSample *
>              sample->update_hr = process_update(sample, top_down_stride, top_down_pointer,
>                      start_stream_time, end_stream_time);
> -
>        remove_queued_update(sample);
> -
>        if (sample->continuous_update && SUCCEEDED(sample->update_hr))
> -
>        {
> -
>            list_remove(&sample->entry);
> -
>            list_add_tail(&sample->parent->update_queue, &sample->entry);
> -
>        }
> -
>        else
> -
>        {
> -
>            remove_queued_update(sample);
> -
>        }
> -
>        LeaveCriticalSection(&stream->cs);
>        return S_OK;
>    }
>
> @@ -1540,12 +1552,6 @@ static HRESULT WINAPI ddraw_sample_Update(IDirectDrawStreamSample *iface,
>          return E_NOTIMPL;
>      }
> -
>
> if (flags & ~SSUPDATE_ASYNC)
> -
>
> {
> -
>    FIXME("Unsupported flags %#x.\n", flags);
> -
>    return E_NOTIMPL;
> -
>
> }
> -
>
> EnterCriticalSection(&sample->parent->cs);
>
> if (sample->parent->state != State_Running)
>
> @@ -1559,13 +1565,16 @@ static HRESULT WINAPI ddraw_sample_Update(IDirectDrawStreamSample *iface,
>          LeaveCriticalSection(&sample->parent->cs);
>          return MS_S_ENDOFSTREAM;
>      }
> - if (MS_S_PENDING == sample->update_hr)
> - if (sample->busy)
> {
>     LeaveCriticalSection(&sample->parent->cs);
>     return MS_E_BUSY;
> }
> - sample->update_hr = MS_S_PENDING;
> - sample->continuous_update = (flags & SSUPDATE_ASYNC) && (flags & SSUPDATE_CONTINUOUS);

This implies that specifying SSUPDATE_CONTINUOUS without SSUPDATE_ASYNC
is equivalent to zero flags, which seems surprising. Is that correct?

It may not exactly be easy to test, if it's not correct,
SSUPDATE_CONTINUOUS without SSUPDATE_ASYNC ends up doing a continuous
update but blocking (until EOS?), but if this line is correct as-is,
then I'd like to see a test for that.
> -
> - sample->update_hr = MS_S_NOUPDATE;
> - sample->busy = TRUE;
> list_add_tail(&sample->parent->update_queue, &sample->entry);
> WakeConditionVariable(&sample->parent->update_queued_cv);
>
> @@ -1575,7 +1584,7 @@ static HRESULT WINAPI ddraw_sample_Update(IDirectDrawStreamSample *iface,
>          return MS_S_PENDING;
>      }
> - while (sample->update_hr == MS_S_PENDING)
> -
>
> while (sample->busy)
>     SleepConditionVariableCS(&sample->update_cv, &sample->parent->cs, INFINITE);
>
> LeaveCriticalSection(&sample->parent->cs);
>
> @@ -1592,18 +1601,17 @@ static HRESULT WINAPI ddraw_sample_CompletionStatus(IDirectDrawStreamSample *ifa
>  EnterCriticalSection(&sample->parent->cs);
>
> - if (sample->update_hr == MS_S_PENDING)
> - if (sample->busy)
> {
>     if (flags & (COMPSTAT_NOUPDATEOK | COMPSTAT_ABORT))
>     {
> -
>        sample->update_hr = MS_S_NOUPDATE;
>        remove_queued_update(sample);
>    }
>    else if (flags & COMPSTAT_WAIT)
>    {
>        DWORD start_time = GetTickCount();
>        DWORD elapsed = 0;
> -
>        while (sample->update_hr == MS_S_PENDING && elapsed < milliseconds)
> -
>        while (sample->busy && elapsed < milliseconds)
>        {
>            DWORD sleep_time = milliseconds - elapsed;
>            if (!SleepConditionVariableCS(&sample->update_cv, &sample->parent->cs, sleep_time))
>
> @@ -1613,7 +1621,7 @@ static HRESULT WINAPI ddraw_sample_CompletionStatus(IDirectDrawStreamSample *ifa
>          }
>      }
> - hr = sample->update_hr;
> -
>
> hr = sample->busy ? MS_S_PENDING : sample->update_hr;
>
> LeaveCriticalSection(&sample->parent->cs);
>
> diff --git a/dlls/amstream/tests/amstream.c b/dlls/amstream/tests/amstream.c
> index 9dda5882a4f..6e6f7483b49 100644
> --- a/dlls/amstream/tests/amstream.c
> +++ b/dlls/amstream/tests/amstream.c
> @@ -7665,6 +7665,59 @@ static void test_ddrawstreamsample_completion_status(void)
>      hr = IAMMultiMediaStream_SetState(mmstream, STREAMSTATE_RUN);
>      ok(hr == S_OK, "Got hr %#x.\n", hr);
> - hr = IDirectDrawStreamSample_Update(stream_sample1, SSUPDATE_ASYNC | SSUPDATE_CONTINUOUS, NULL, NULL, 0);
> - ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
> -
> - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, COMPSTAT_NOUPDATEOK, 0);
> - ok(hr == MS_S_NOUPDATE, "Got hr %#x.\n", hr);
> -
> - hr = IDirectDrawStreamSample_Update(stream_sample1, SSUPDATE_ASYNC | SSUPDATE_CONTINUOUS, NULL, NULL, 0);
> - ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
> -
> - hr = IDirectDrawStreamSample_Update(stream_sample2, SSUPDATE_ASYNC, NULL, NULL, 0);
> - ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
> -
> - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, 0, 0);
> - ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
> -
> - media_sample = ammediastream_allocate_sample(&source, test_data, sizeof(test_data));
> - hr = IMemInputPin_Receive(source.source.pMemInputPin, media_sample);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> - ref = IMediaSample_Release(media_sample);
> - ok(!ref, "Got outstanding refcount %d.\n", ref);
> -
> - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, 0, 0);
> - ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);

Can we check that sample2 isn't satisfied here? [Or is native not
consistent?]
> -
> - media_sample = ammediastream_allocate_sample(&source, test_data, sizeof(test_data));
> - hr = IMemInputPin_Receive(source.source.pMemInputPin, media_sample);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> - ref = IMediaSample_Release(media_sample);
> - ok(!ref, "Got outstanding refcount %d.\n", ref);
> -
> - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, 0, 0);
> - ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
> -
> - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample2, 0, 0);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> -
> - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, COMPSTAT_NOUPDATEOK, 0);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> -
> - hr = IDirectDrawStreamSample_Update(stream_sample1, SSUPDATE_ASYNC | SSUPDATE_CONTINUOUS, NULL, NULL, 0);
> - ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
> -
> - hr = IPin_EndOfStream(pin);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> -
> - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, 0, 0);
> - ok(hr == MS_S_ENDOFSTREAM, "Got hr %#x.\n", hr);
> -
> - hr = IAMMultiMediaStream_SetState(mmstream, STREAMSTATE_STOP);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> - hr = IAMMultiMediaStream_SetState(mmstream, STREAMSTATE_RUN);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> - hr = IDirectDrawStreamSample_Update(stream_sample1, SSUPDATE_ASYNC, NULL, NULL, 0);
>    ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);

The DirectX documentation is rather vague about what "continuous" means,
or the behaviour surrounding this flag, and even after these tests I'm
still left with a lot of questions. In particular:

* CompletionStatus() with no flags evidently doesn't take the sample out
of continuous state, but does COMPSTAT_NOUPDATEOK? What about
COMPSTAT_ABORT?

* IPin::EndOfStream() takes the sample out of continuous state, as far
as I think it's possible to tell. The same applies to IPin::BeginFlush()
and IAMMediaStream::SetState(State_Stopped), right?

* What about calling Update() again, with the same flags, or different ones?

* COMPSTAT_WAIT blocks until the sample is taken out of continuous
state, right? This doesn't necessarily need to be tested if the answer
is "yes", since it'll be more than a little annoying; I just want to
make sure.

Also, can we test briefly that Receive() does update the ddraw surface
every time?

[Anton Baskanov]

On Friday, 9 October 2020 02:53:24 +07 you wrote:
> On 10/8/20 2:02 PM, Anton Baskanov wrote:
> > Signed-off-by: Anton Baskanov
> >
> > dlls/amstream/ddrawstream.c    | 36 ++++++++++++++---------
> >  dlls/amstream/tests/amstream.c | 53 ++++++++++++++++++++++++++++++++++
> >  2 files changed, 75 insertions(+), 14 deletions(-)
> >
> > diff --git a/dlls/amstream/ddrawstream.c b/dlls/amstream/ddrawstream.c
> > index 5e6e1555d7c..0e5e54bd80d 100644
> > --- a/dlls/amstream/ddrawstream.c
> > +++ b/dlls/amstream/ddrawstream.c
> > @@ -75,10 +75,12 @@ struct ddraw_sample
> >  RECT rect;
> >  STREAM_TIME start_time;
> >  STREAM_TIME end_time;
> >
> > -
> >
> > BOOL continuous_update;
> >
> > CONDITION_VARIABLE update_cv;
> >
> > struct list entry;
> > HRESULT update_hr;
> > -
> >
> > BOOL busy;
>
> As far as I can tell, this is identical to "update_hr == MS_S_PENDING",
> except that you set update_hr to MS_S_NOUPDATE in the continuous case
> instead, right?
>
> Would it be simpler, then, to keep most of the code handling update_hr
> as it is, and instead handle COMPSTAT_NOUPDATEOK/MS_S_NOUPDATE specially
> in ::CompletionStatus()?

It would require additional state anyway, because ::CompletionStatus with 
COMPSTAT_NOUPDATEOK returns different values depending on whether the sample 
was updated at least once or not (S_OK vs MS_S_NOUPDATE).
> > };
> >
> > static HRESULT ddrawstreamsample_create(struct ddraw_stream *parent,
> >  IDirectDrawSurface *surface,> 
> > @@ -86,6 +88,7 @@ static HRESULT ddrawstreamsample_create(struct
> > ddraw_stream *parent, IDirectDraw> 
> >  static void remove_queued_update(struct ddraw_sample *sample)
> >  {
> > -
> >
> > sample->busy = FALSE;
> >
> > list_remove(&sample->entry);
> > WakeConditionVariable(&sample->update_cv);
> >
> > }
> >
> > @@ -1348,7 +1351,16 @@ static HRESULT WINAPI
> > ddraw_meminput_Receive(IMemInputPin *iface, IMediaSample *> 
> >              sample->update_hr = process_update(sample, top_down_stride,
> >              top_down_pointer,>              
> >                      start_stream_time, end_stream_time);
> > -
> >        remove_queued_update(sample);
> > -
> >        if (sample->continuous_update &&
> >
> > SUCCEEDED(sample->update_hr))
> > -
> >        {
> > -
> >            list_remove(&sample->entry);
> > -
> >            list_add_tail(&sample->parent->update_queue,
> >
> > &sample->entry); +            }
> > -
> >        else
> > -
> >        {
> > -
> >            remove_queued_update(sample);
> > -
> >        }
> > -
> >        LeaveCriticalSection(&stream->cs);
> >        return S_OK;
> >
> >    }
> >
> > @@ -1540,12 +1552,6 @@ static HRESULT WINAPI
> > ddraw_sample_Update(IDirectDrawStreamSample *iface,> 
> >          return E_NOTIMPL;
> >  }
> >
> > -
> >
> > if (flags & ~SSUPDATE_ASYNC)
> > -
> >
> > {
> > -
> >    FIXME("Unsupported flags %#x.\n", flags);
> > -
> >    return E_NOTIMPL;
> > -
> >
> > }
> > -
> >
> > EnterCriticalSection(&sample->parent->cs);
> >
> > if (sample->parent->state != State_Running)
> >
> > @@ -1559,13 +1565,16 @@ static HRESULT WINAPI
> > ddraw_sample_Update(IDirectDrawStreamSample *iface,> 
> >          LeaveCriticalSection(&sample->parent->cs);
> >          return MS_S_ENDOFSTREAM;
> >  }
> >
> > - if (MS_S_PENDING == sample->update_hr)
> > -
> >
> > if (sample->busy)
> >
> > {
> > LeaveCriticalSection(&sample->parent->cs);
> > return MS_E_BUSY;
> >
> > }
> > - sample->update_hr = MS_S_PENDING;
> > - sample->continuous_update = (flags & SSUPDATE_ASYNC) && (flags &
> >
> > SSUPDATE_CONTINUOUS);
>
> This implies that specifying SSUPDATE_CONTINUOUS without SSUPDATE_ASYNC
> is equivalent to zero flags, which seems surprising. Is that correct?
>
> It may not exactly be easy to test, if it's not correct,
> SSUPDATE_CONTINUOUS without SSUPDATE_ASYNC ends up doing a continuous
> update but blocking (until EOS?), but if this line is correct as-is,
> then I'd like to see a test for that.

This should be correct as-is, added a test for it.
> > -
> > -
> >
> > sample->update_hr = MS_S_NOUPDATE;
> > -
> >
> > sample->busy = TRUE;
> >
> > list_add_tail(&sample->parent->update_queue, &sample->entry);
> > WakeConditionVariable(&sample->parent->update_queued_cv);
> >
> > @@ -1575,7 +1584,7 @@ static HRESULT WINAPI
> > ddraw_sample_Update(IDirectDrawStreamSample *iface,> 
> >          return MS_S_PENDING;
> >  }
> >
> > - while (sample->update_hr == MS_S_PENDING)
> > -
> >
> > while (sample->busy)
> > SleepConditionVariableCS(&sample->update_cv, &sample->parent->cs,
> > INFINITE);>      
> >
> > LeaveCriticalSection(&sample->parent->cs);
> >
> > @@ -1592,18 +1601,17 @@ static HRESULT WINAPI
> > ddraw_sample_CompletionStatus(IDirectDrawStreamSample *ifa> 
> >      EnterCriticalSection(&sample->parent->cs);
> > - if (sample->update_hr == MS_S_PENDING)
> > -
> >
> > if (sample->busy)
> >
> > {
> > if (flags & (COMPSTAT_NOUPDATEOK | COMPSTAT_ABORT))
> > {
> >
> > -
> >        sample->update_hr = MS_S_NOUPDATE;
> >
> >        remove_queued_update(sample);
> >
> >    }
> >    else if (flags & COMPSTAT_WAIT)
> >    {
> >
> >        DWORD start_time = GetTickCount();
> >        DWORD elapsed = 0;
> >
> > -
> >        while (sample->update_hr == MS_S_PENDING && elapsed <
> >
> > milliseconds) +            while (sample->busy && elapsed < milliseconds)
> >          {
> >
> >              DWORD sleep_time = milliseconds - elapsed;
> >              if (!SleepConditionVariableCS(&sample->update_cv,
> >              &sample->parent->cs, sleep_time))> 
> >
> > @@ -1613,7 +1621,7 @@ static HRESULT WINAPI
> > ddraw_sample_CompletionStatus(IDirectDrawStreamSample *ifa> 
> >          }
> >  }
> >
> > - hr = sample->update_hr;
> > -
> >
> > hr = sample->busy ? MS_S_PENDING : sample->update_hr;
> >
> > LeaveCriticalSection(&sample->parent->cs);
> >
> > diff --git a/dlls/amstream/tests/amstream.c
> > b/dlls/amstream/tests/amstream.c index 9dda5882a4f..6e6f7483b49 100644
> > --- a/dlls/amstream/tests/amstream.c
> > +++ b/dlls/amstream/tests/amstream.c
> > @@ -7665,6 +7665,59 @@ static void
> > test_ddrawstreamsample_completion_status(void)> 
> >      hr = IAMMultiMediaStream_SetState(mmstream, STREAMSTATE_RUN);
> >      ok(hr == S_OK, "Got hr %#x.\n", hr);
> > - hr = IDirectDrawStreamSample_Update(stream_sample1, SSUPDATE_ASYNC |
> >
> > SSUPDATE_CONTINUOUS, NULL, NULL, 0); +    ok(hr == MS_S_PENDING, "Got hr
> > %#x.\n", hr);
> > -
> > - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1,
> >
> > COMPSTAT_NOUPDATEOK, 0); +    ok(hr == MS_S_NOUPDATE, "Got hr %#x.\n",
> > hr);
> > -
> > - hr = IDirectDrawStreamSample_Update(stream_sample1, SSUPDATE_ASYNC |
> >
> > SSUPDATE_CONTINUOUS, NULL, NULL, 0); +    ok(hr == MS_S_PENDING, "Got hr
> > %#x.\n", hr);
> > -
> > - hr = IDirectDrawStreamSample_Update(stream_sample2, SSUPDATE_ASYNC,
> >
> > NULL, NULL, 0); +    ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
> > -
> > - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, 0, 0);
> > - ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
> > -
> > - media_sample = ammediastream_allocate_sample(&source, test_data,
> >
> > sizeof(test_data)); +    hr =
> > IMemInputPin_Receive(source.source.pMemInputPin, media_sample); +   
> > ok(hr == S_OK, "Got hr %#x.\n", hr);
> > - ref = IMediaSample_Release(media_sample);
> > - ok(!ref, "Got outstanding refcount %d.\n", ref);
> > -
> > - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, 0, 0);
> > - ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
>
> Can we check that sample2 isn't satisfied here? [Or is native not
> consistent?]

Done.
> > -
> > - media_sample = ammediastream_allocate_sample(&source, test_data,
> >
> > sizeof(test_data)); +    hr =
> > IMemInputPin_Receive(source.source.pMemInputPin, media_sample); +   
> > ok(hr == S_OK, "Got hr %#x.\n", hr);
> > - ref = IMediaSample_Release(media_sample);
> > - ok(!ref, "Got outstanding refcount %d.\n", ref);
> > -
> > - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, 0, 0);
> > - ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
> > -
> > - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample2, 0, 0);
> > - ok(hr == S_OK, "Got hr %#x.\n", hr);
> > -
> > - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1,
> >
> > COMPSTAT_NOUPDATEOK, 0); +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> > -
> > - hr = IDirectDrawStreamSample_Update(stream_sample1, SSUPDATE_ASYNC |
> >
> > SSUPDATE_CONTINUOUS, NULL, NULL, 0); +    ok(hr == MS_S_PENDING, "Got hr
> > %#x.\n", hr);
> > -
> > -
> >
> > hr = IPin_EndOfStream(pin);
> > -
> >
> > ok(hr == S_OK, "Got hr %#x.\n", hr);
> > -
> > -
> >
> > hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, 0, 0);
> > -
> >
> > ok(hr == MS_S_ENDOFSTREAM, "Got hr %#x.\n", hr);
> > -
> > -
> >
> > hr = IAMMultiMediaStream_SetState(mmstream, STREAMSTATE_STOP);
> > -
> >
> > ok(hr == S_OK, "Got hr %#x.\n", hr);
> > -
> >
> > hr = IAMMultiMediaStream_SetState(mmstream, STREAMSTATE_RUN);
> > -
> >
> > ok(hr == S_OK, "Got hr %#x.\n", hr);
> > -
> >
> > hr = IDirectDrawStreamSample_Update(stream_sample1, SSUPDATE_ASYNC,
> >    NULL, NULL, 0); ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
>
> The DirectX documentation is rather vague about what "continuous" means,
> or the behaviour surrounding this flag, and even after these tests I'm
> still left with a lot of questions. In particular:
> - CompletionStatus() with no flags evidently doesn't take the sample out
>
> of continuous state, but does COMPSTAT_NOUPDATEOK? What about
> COMPSTAT_ABORT?

Looking at the return values of ::CompletionStatus, I think they both do.
> - IPin::EndOfStream() takes the sample out of continuous state, as far
>
> as I think it's possible to tell. The same applies to IPin::BeginFlush()
> and IAMMediaStream::SetState(State_Stopped), right?

No, actually only EndOfStream does this. Added tests for it.
> - What about calling Update() again, with the same flags, or different ones?

This has no effect, returns MS_E_BUSY.
> - COMPSTAT_WAIT blocks until the sample is taken out of continuous
>
> state, right? This doesn't necessarily need to be tested if the answer
> is "yes", since it'll be more than a little annoying; I just want to
> make sure.

Added tests for this and it looks like COMPSTAT_WAIT takes the sample out of 
continuous state immediately and then waits for completion like with the 
normal update.
> Also, can we test briefly that Receive() does update the ddraw surface
> every time?

Done.

[Zebediah Figura]

On 10/9/20 3:55 PM, Anton Baskanov wrote:
> On Friday, 9 October 2020 02:53:24 +07 you wrote:
> > On 10/8/20 2:02 PM, Anton Baskanov wrote:
> > > Signed-off-by: Anton Baskanov
> > >
> > > dlls/amstream/ddrawstream.c    | 36 ++++++++++++++---------
> > >  dlls/amstream/tests/amstream.c | 53 ++++++++++++++++++++++++++++++++++
> > >  2 files changed, 75 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/dlls/amstream/ddrawstream.c b/dlls/amstream/ddrawstream.c
> > > index 5e6e1555d7c..0e5e54bd80d 100644
> > > --- a/dlls/amstream/ddrawstream.c
> > > +++ b/dlls/amstream/ddrawstream.c
> > > @@ -75,10 +75,12 @@ struct ddraw_sample
> > >  RECT rect;
> > >  STREAM_TIME start_time;
> > >  STREAM_TIME end_time;
> > >
> > > -
> > >
> > > BOOL continuous_update;
> > >
> > > CONDITION_VARIABLE update_cv;
> > >
> > > struct list entry;
> > > HRESULT update_hr;
> > > -
> > >
> > > BOOL busy;
> >
> > As far as I can tell, this is identical to "update_hr == MS_S_PENDING",
> > except that you set update_hr to MS_S_NOUPDATE in the continuous case
> > instead, right?
> >
> > Would it be simpler, then, to keep most of the code handling update_hr
> > as it is, and instead handle COMPSTAT_NOUPDATEOK/MS_S_NOUPDATE specially
> > in ::CompletionStatus()?
>
> It would require additional state anyway, because ::CompletionStatus with 
> COMPSTAT_NOUPDATEOK returns different values depending on whether the sample 
> was updated at least once or not (S_OK vs MS_S_NOUPDATE).

Hmm, I see what you mean. In that case, the way you have it currently is
probably best.
> > > };
> > >
> > > static HRESULT ddrawstreamsample_create(struct ddraw_stream *parent,
> > >  IDirectDrawSurface *surface,> 
> > > @@ -86,6 +88,7 @@ static HRESULT ddrawstreamsample_create(struct
> > > ddraw_stream *parent, IDirectDraw> 
> > >  static void remove_queued_update(struct ddraw_sample *sample)
> > >  {
> > > -
> > >
> > > sample->busy = FALSE;
> > >
> > > list_remove(&sample->entry);
> > > WakeConditionVariable(&sample->update_cv);
> > >
> > > }
> > >
> > > @@ -1348,7 +1351,16 @@ static HRESULT WINAPI
> > > ddraw_meminput_Receive(IMemInputPin *iface, IMediaSample *> 
> > >              sample->update_hr = process_update(sample, top_down_stride,
> > >              top_down_pointer,>              
> > >                      start_stream_time, end_stream_time);
> > > -
> > >        remove_queued_update(sample);
> > > -
> > >        if (sample->continuous_update &&
> > >
> > > SUCCEEDED(sample->update_hr))
> > > -
> > >        {
> > > -
> > >            list_remove(&sample->entry);
> > > -
> > >            list_add_tail(&sample->parent->update_queue,
> > >
> > > &sample->entry); +            }
> > > -
> > >        else
> > > -
> > >        {
> > > -
> > >            remove_queued_update(sample);
> > > -
> > >        }
> > > -
> > >        LeaveCriticalSection(&stream->cs);
> > >        return S_OK;
> > >
> > >    }
> > >
> > > @@ -1540,12 +1552,6 @@ static HRESULT WINAPI
> > > ddraw_sample_Update(IDirectDrawStreamSample *iface,> 
> > >          return E_NOTIMPL;
> > >  }
> > >
> > > -
> > >
> > > if (flags & ~SSUPDATE_ASYNC)
> > > -
> > >
> > > {
> > > -
> > >    FIXME("Unsupported flags %#x.\n", flags);
> > > -
> > >    return E_NOTIMPL;
> > > -
> > >
> > > }
> > > -
> > >
> > > EnterCriticalSection(&sample->parent->cs);
> > >
> > > if (sample->parent->state != State_Running)
> > >
> > > @@ -1559,13 +1565,16 @@ static HRESULT WINAPI
> > > ddraw_sample_Update(IDirectDrawStreamSample *iface,> 
> > >          LeaveCriticalSection(&sample->parent->cs);
> > >          return MS_S_ENDOFSTREAM;
> > >  }
> > >
> > > - if (MS_S_PENDING == sample->update_hr)
> > > -
> > >
> > > if (sample->busy)
> > >
> > > {
> > > LeaveCriticalSection(&sample->parent->cs);
> > > return MS_E_BUSY;
> > >
> > > }
> > > - sample->update_hr = MS_S_PENDING;
> > > - sample->continuous_update = (flags & SSUPDATE_ASYNC) && (flags &
> > >
> > > SSUPDATE_CONTINUOUS);
> >
> > This implies that specifying SSUPDATE_CONTINUOUS without SSUPDATE_ASYNC
> > is equivalent to zero flags, which seems surprising. Is that correct?
> >
> > It may not exactly be easy to test, if it's not correct,
> > SSUPDATE_CONTINUOUS without SSUPDATE_ASYNC ends up doing a continuous
> > update but blocking (until EOS?), but if this line is correct as-is,
> > then I'd like to see a test for that.
>
> This should be correct as-is, added a test for it.
> > > -
> > > -
> > >
> > > sample->update_hr = MS_S_NOUPDATE;
> > > -
> > >
> > > sample->busy = TRUE;
> > >
> > > list_add_tail(&sample->parent->update_queue, &sample->entry);
> > > WakeConditionVariable(&sample->parent->update_queued_cv);
> > >
> > > @@ -1575,7 +1584,7 @@ static HRESULT WINAPI
> > > ddraw_sample_Update(IDirectDrawStreamSample *iface,> 
> > >          return MS_S_PENDING;
> > >  }
> > >
> > > - while (sample->update_hr == MS_S_PENDING)
> > > -
> > >
> > > while (sample->busy)
> > > SleepConditionVariableCS(&sample->update_cv, &sample->parent->cs,
> > > INFINITE);>      
> > >
> > > LeaveCriticalSection(&sample->parent->cs);
> > >
> > > @@ -1592,18 +1601,17 @@ static HRESULT WINAPI
> > > ddraw_sample_CompletionStatus(IDirectDrawStreamSample *ifa> 
> > >      EnterCriticalSection(&sample->parent->cs);
> > > - if (sample->update_hr == MS_S_PENDING)
> > > -
> > >
> > > if (sample->busy)
> > >
> > > {
> > > if (flags & (COMPSTAT_NOUPDATEOK | COMPSTAT_ABORT))
> > > {
> > >
> > > -
> > >        sample->update_hr = MS_S_NOUPDATE;
> > >
> > >        remove_queued_update(sample);
> > >
> > >    }
> > >    else if (flags & COMPSTAT_WAIT)
> > >    {
> > >
> > >        DWORD start_time = GetTickCount();
> > >        DWORD elapsed = 0;
> > >
> > > -
> > >        while (sample->update_hr == MS_S_PENDING && elapsed <
> > >
> > > milliseconds) +            while (sample->busy && elapsed < milliseconds)
> > >          {
> > >
> > >              DWORD sleep_time = milliseconds - elapsed;
> > >              if (!SleepConditionVariableCS(&sample->update_cv,
> > >              &sample->parent->cs, sleep_time))> 
> > >
> > > @@ -1613,7 +1621,7 @@ static HRESULT WINAPI
> > > ddraw_sample_CompletionStatus(IDirectDrawStreamSample *ifa> 
> > >          }
> > >  }
> > >
> > > - hr = sample->update_hr;
> > > -
> > >
> > > hr = sample->busy ? MS_S_PENDING : sample->update_hr;
> > >
> > > LeaveCriticalSection(&sample->parent->cs);
> > >
> > > diff --git a/dlls/amstream/tests/amstream.c
> > > b/dlls/amstream/tests/amstream.c index 9dda5882a4f..6e6f7483b49 100644
> > > --- a/dlls/amstream/tests/amstream.c
> > > +++ b/dlls/amstream/tests/amstream.c
> > > @@ -7665,6 +7665,59 @@ static void
> > > test_ddrawstreamsample_completion_status(void)> 
> > >      hr = IAMMultiMediaStream_SetState(mmstream, STREAMSTATE_RUN);
> > >      ok(hr == S_OK, "Got hr %#x.\n", hr);
> > > - hr = IDirectDrawStreamSample_Update(stream_sample1, SSUPDATE_ASYNC |
> > >
> > > SSUPDATE_CONTINUOUS, NULL, NULL, 0); +    ok(hr == MS_S_PENDING, "Got hr
> > > %#x.\n", hr);
> > > -
> > > - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1,
> > >
> > > COMPSTAT_NOUPDATEOK, 0); +    ok(hr == MS_S_NOUPDATE, "Got hr %#x.\n",
> > > hr);
> > > -
> > > - hr = IDirectDrawStreamSample_Update(stream_sample1, SSUPDATE_ASYNC |
> > >
> > > SSUPDATE_CONTINUOUS, NULL, NULL, 0); +    ok(hr == MS_S_PENDING, "Got hr
> > > %#x.\n", hr);
> > > -
> > > - hr = IDirectDrawStreamSample_Update(stream_sample2, SSUPDATE_ASYNC,
> > >
> > > NULL, NULL, 0); +    ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
> > > -
> > > - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, 0, 0);
> > > - ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
> > > -
> > > - media_sample = ammediastream_allocate_sample(&source, test_data,
> > >
> > > sizeof(test_data)); +    hr =
> > > IMemInputPin_Receive(source.source.pMemInputPin, media_sample); +   
> > > ok(hr == S_OK, "Got hr %#x.\n", hr);
> > > - ref = IMediaSample_Release(media_sample);
> > > - ok(!ref, "Got outstanding refcount %d.\n", ref);
> > > -
> > > - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, 0, 0);
> > > - ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
> >
> > Can we check that sample2 isn't satisfied here? [Or is native not
> > consistent?]
>
> Done.
> > > -
> > > - media_sample = ammediastream_allocate_sample(&source, test_data,
> > >
> > > sizeof(test_data)); +    hr =
> > > IMemInputPin_Receive(source.source.pMemInputPin, media_sample); +   
> > > ok(hr == S_OK, "Got hr %#x.\n", hr);
> > > - ref = IMediaSample_Release(media_sample);
> > > - ok(!ref, "Got outstanding refcount %d.\n", ref);
> > > -
> > > - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, 0, 0);
> > > - ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
> > > -
> > > - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample2, 0, 0);
> > > - ok(hr == S_OK, "Got hr %#x.\n", hr);
> > > -
> > > - hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1,
> > >
> > > COMPSTAT_NOUPDATEOK, 0); +    ok(hr == S_OK, "Got hr %#x.\n", hr);
> > > -
> > > - hr = IDirectDrawStreamSample_Update(stream_sample1, SSUPDATE_ASYNC |
> > >
> > > SSUPDATE_CONTINUOUS, NULL, NULL, 0); +    ok(hr == MS_S_PENDING, "Got hr
> > > %#x.\n", hr);
> > > -
> > > -
> > >
> > > hr = IPin_EndOfStream(pin);
> > > -
> > >
> > > ok(hr == S_OK, "Got hr %#x.\n", hr);
> > > -
> > > -
> > >
> > > hr = IDirectDrawStreamSample_CompletionStatus(stream_sample1, 0, 0);
> > > -
> > >
> > > ok(hr == MS_S_ENDOFSTREAM, "Got hr %#x.\n", hr);
> > > -
> > > -
> > >
> > > hr = IAMMultiMediaStream_SetState(mmstream, STREAMSTATE_STOP);
> > > -
> > >
> > > ok(hr == S_OK, "Got hr %#x.\n", hr);
> > > -
> > >
> > > hr = IAMMultiMediaStream_SetState(mmstream, STREAMSTATE_RUN);
> > > -
> > >
> > > ok(hr == S_OK, "Got hr %#x.\n", hr);
> > > -
> > >
> > > hr = IDirectDrawStreamSample_Update(stream_sample1, SSUPDATE_ASYNC,
> > >    NULL, NULL, 0); ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
> >
> > The DirectX documentation is rather vague about what "continuous" means,
> > or the behaviour surrounding this flag, and even after these tests I'm
> > still left with a lot of questions. In particular:
> > - CompletionStatus() with no flags evidently doesn't take the sample out
> >
> > of continuous state, but does COMPSTAT_NOUPDATEOK? What about
> > COMPSTAT_ABORT?
>
> Looking at the return values of ::CompletionStatus, I think they both do.

I'm not sure I want to trust the return value to tell me that.
Fortunately, the MS_E_BUSY result from my third question below plus the
tests you have currently should prove it another way.
> > - IPin::EndOfStream() takes the sample out of continuous state, as far
> >
> > as I think it's possible to tell. The same applies to IPin::BeginFlush()
> > and IAMMediaStream::SetState(State_Stopped), right?
>
> No, actually only EndOfStream does this. Added tests for it.
> > - What about calling Update() again, with the same flags, or different ones?
>
> This has no effect, returns MS_E_BUSY.
> > - COMPSTAT_WAIT blocks until the sample is taken out of continuous
> >
> > state, right? This doesn't necessarily need to be tested if the answer
> > is "yes", since it'll be more than a little annoying; I just want to
> > make sure.
>
> Added tests for this and it looks like COMPSTAT_WAIT takes the sample out of 
> continuous state immediately and then waits for completion like with the 
> normal update.
> > Also, can we test briefly that Receive() does update the ddraw surface
> > every time?
>
> Done.