[PATCH] amstream: Implement SSUPDATE_CONTINUOUS flag in IDirectDrawStreamSample::Update.
Zebediah Figura
zfigura at codeweavers.com
Mon Oct 12 22:22:46 CDT 2020
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 <baskanov at gmail.com>
>>> ---
>>>
>>> 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.
>
>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20201012/53957c2e/attachment-0001.sig>
More information about the wine-devel
mailing list