[PATCH 3/4] amstream: Implement AMDirectDrawStream::BeginFlush and ::EndFlush.
Zebediah Figura
zfigura at codeweavers.com
Thu Oct 1 23:52:15 CDT 2020
On 10/1/20 12:57 PM, Anton Baskanov wrote:
> On Thursday, 1 October 2020 00:06:48 +07 you wrote:
>> On 9/29/20 2:09 PM, Anton Baskanov wrote:
>>> Signed-off-by: Anton Baskanov <baskanov at gmail.com>
>>> ---
>>>
>>> dlls/amstream/ddrawstream.c | 36 ++++++++--
>>> dlls/amstream/tests/amstream.c | 128 ++++++++++++++++++++++++++++++++-
>>> 2 files changed, 157 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/dlls/amstream/ddrawstream.c b/dlls/amstream/ddrawstream.c
>>> index 4bd1aa230b4..d940bf88cdc 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;
>>>
>>> + BOOL flushing;
>>>
>>> CONDITION_VARIABLE update_queued_cv;
>>> struct list update_queue;
>>>
>>> };
>>>
>>> @@ -1142,7 +1143,7 @@ static HRESULT WINAPI ddraw_sink_EndOfStream(IPin
>>> *iface)>
>>> EnterCriticalSection(&stream->cs);
>>>
>>> - if (stream->eos)
>>> + if (stream->eos || stream->flushing)
>>>
>>> {
>>>
>>> LeaveCriticalSection(&stream->cs);
>>> return E_FAIL;
>>>
>>> @@ -1159,14 +1160,34 @@ static HRESULT WINAPI ddraw_sink_EndOfStream(IPin
>>> *iface)>
>>> static HRESULT WINAPI ddraw_sink_BeginFlush(IPin *iface)
>>> {
>>>
>>> - FIXME("iface %p, stub!\n", iface);
>>> - return E_NOTIMPL;
>>> + struct ddraw_stream *stream = impl_from_IPin(iface);
>>> +
>>> + TRACE("stream %p.\n", stream);
>>> +
>>> + EnterCriticalSection(&stream->cs);
>>> +
>>> + stream->flushing = TRUE;
>>> + stream->eos = FALSE;
>>> + WakeConditionVariable(&stream->update_queued_cv);
>>> +
>>> + LeaveCriticalSection(&stream->cs);
>>> +
>>> + return S_OK;
>>>
>>> }
>>>
>>> static HRESULT WINAPI ddraw_sink_EndFlush(IPin *iface)
>>> {
>>>
>>> - FIXME("iface %p, stub!\n", iface);
>>> - return E_NOTIMPL;
>>> + struct ddraw_stream *stream = impl_from_IPin(iface);
>>> +
>>> + TRACE("stream %p.\n", stream);
>>> +
>>> + EnterCriticalSection(&stream->cs);
>>> +
>>> + stream->flushing = FALSE;
>>> +
>>> + LeaveCriticalSection(&stream->cs);
>>> +
>>> + return S_OK;
>>>
>>> }
>>
>> This implementation worries me a bit (and, admittedly, is one of the
>> drawbacks of using a condition variable), because it's vulnerable to an
>> ABA problem. In particular, it's possible for a main thread to call
>> BeginFlush() and EndFlush() in quick succession, while a thread sleeping
>> in Receive() doesn't wake up until after EndFlush() and hence goes back
>> to sleep. I don't know if Windows actually guarantees a wakeup here, but
>> unless it demonstrably doesn't, we should probably make sure the queue
>> is actually empty in either BeginFlush() or EndFlush(). I think we can
>> use the same CV for that.
>
> Fixed this with another CV and a boolean flag, which is kind of ugly, but I
> couldn't think of a better solution.
Hmm.
On the one hand, I don't think the flag is particularly ugly, but.
On the other hand, I'm a little bothered now by that statement of mine;
I'm not actually sure it's necessary. [In my defense, DirectShow
synchronization is hard, and extremely underspecified.] I remember
dealing with a similar problem when trying to improve synchronization in
the base renderer a while ago, and I came to the conclusion that a patch
like this wasn't necessary.
In particular, I think the actual requirements for flushing are that a
filter cannot send old samples after EndFlush(), or allow them to be
rendered. In practice, if a filter doesn't buffer, it can "pass the
buck" up to the upstream filter. Put another way, if filters don't
buffer, the only filter that actually needs to wait for the streaming
thread to finish is the parser filter, and the parser filter basically
always needs to wait *anyway*.
But, just in case, I tested, and surprisingly enough, native amstream's
Receive() won't be unblocked if BeginFlush() and EndFlush() are called
in quick succession, but if I add a small Sleep() between those, it will
be unblocked. Quite surprising for several reasons, but I think that
seals the case.
So I'd be perfectly happy to sign off on the original version of this
patch, rebased on the other changes ;-)
>
>>
>> (Incidentally, patches 2 and 4 of this series look fine.)
>>
>>> static HRESULT WINAPI ddraw_sink_NewSegment(IPin *iface, REFERENCE_TIME
>>> start, REFERENCE_TIME stop, double rate)>
>>> @@ -1299,6 +1320,11 @@ static HRESULT WINAPI
>>> ddraw_meminput_Receive(IMemInputPin *iface, IMediaSample *>
>>> LeaveCriticalSection(&stream->cs);
>>> return S_OK;
>>>
>>> }
>>>
>>> + if (stream->flushing)
>>> + {
>>> + LeaveCriticalSection(&stream->cs);
>>> + return S_FALSE;
>>> + }
>>>
>>> if (!list_empty(&stream->update_queue))
>>> {
>>>
>>> struct ddraw_sample *sample =
>>> LIST_ENTRY(list_head(&stream->update_queue), struct
>>> ddraw_sample, entry);>
>>> diff --git a/dlls/amstream/tests/amstream.c
>>> b/dlls/amstream/tests/amstream.c index 19461d921f6..653c184f0b4 100644
>>> --- a/dlls/amstream/tests/amstream.c
>>> +++ b/dlls/amstream/tests/amstream.c
>>> @@ -4005,6 +4005,7 @@ static IPin *ammediastream_pin;
>>>
>>> static IMemInputPin *ammediastream_mem_input_pin;
>>> static IMediaSample *ammediastream_media_sample;
>>> static DWORD ammediastream_sleep_time;
>>>
>>> +static HRESULT ammediastream_expected_hr;
>>>
>>> static DWORD CALLBACK ammediastream_end_of_stream(void *param)
>>> {
>>>
>>> @@ -4012,7 +4013,7 @@ static DWORD CALLBACK
>>> ammediastream_end_of_stream(void *param)>
>>> Sleep(ammediastream_sleep_time);
>>> hr = IPin_EndOfStream(ammediastream_pin);
>>>
>>> - ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> + ok(hr == ammediastream_expected_hr, "Got hr %#x.\n", hr);
>>>
>>> return 0;
>>>
>>> }
>>>
>>> @@ -4023,7 +4024,7 @@ static DWORD CALLBACK ammediastream_receive(void
>>> *param)>
>>> Sleep(ammediastream_sleep_time);
>>> hr = IMemInputPin_Receive(ammediastream_mem_input_pin,
>>> ammediastream_media_sample);>
>>> - ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> + ok(hr == ammediastream_expected_hr, "Got hr %#x.\n", hr);
>>>
>>> return 0;
>>>
>>> }
>>>
>>> @@ -4197,6 +4198,7 @@ static void test_audiostreamsample_update(void)
>>>
>>> ammediastream_mem_input_pin = mem_input_pin;
>>> ammediastream_media_sample = media_sample1;
>>> ammediastream_sleep_time = 100;
>>>
>>> + ammediastream_expected_hr = S_OK;
>>>
>>> thread = CreateThread(NULL, 0, ammediastream_receive, NULL, 0, NULL);
>>>
>>> hr = IAudioStreamSample_Update(stream_sample, 0, NULL, NULL, 0);
>>>
>>> @@ -4216,6 +4218,7 @@ static void test_audiostreamsample_update(void)
>>>
>>> ammediastream_pin = pin;
>>> ammediastream_sleep_time = 100;
>>>
>>> + ammediastream_expected_hr = S_OK;
>>>
>>> thread = CreateThread(NULL, 0, ammediastream_end_of_stream, NULL, 0,
>>> NULL);
>>>
>>> hr = IAudioStreamSample_Update(stream_sample, 0, NULL, NULL, 0);
>>>
>>> @@ -5157,6 +5160,7 @@ static void test_ddrawstream_receive(void)
>>>
>>> ammediastream_mem_input_pin = source.source.pMemInputPin;
>>> ammediastream_media_sample = sample;
>>> ammediastream_sleep_time = 0;
>>>
>>> + ammediastream_expected_hr = S_OK;
>>>
>>> thread = CreateThread(NULL, 0, ammediastream_receive, NULL, 0, NULL);
>>>
>>> ok(WaitForSingleObject(thread, 100) == WAIT_TIMEOUT, "Receive
>>> returned prematurely.\n");>
>>> @@ -5196,6 +5200,121 @@ static void test_ddrawstream_receive(void)
>>>
>>> ok(!ref, "Got outstanding refcount %d.\n", ref);
>>>
>>> }
>>>
>>> +static void test_ddrawstream_begin_flush_end_flush(void)
>>> +{
>>> + IAMMultiMediaStream *mmstream = create_ammultimediastream();
>>> + IDirectDrawStreamSample *stream_sample;
>>> + IDirectDrawMediaStream *ddraw_stream;
>>> + IMediaSample *media_sample;
>>> + IMediaFilter *media_filter;
>>> + struct testfilter source;
>>> + IGraphBuilder *graph;
>>> + IMediaStream *stream;
>>> + VIDEOINFO video_info;
>>> + AM_MEDIA_TYPE mt;
>>> + 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_AddMediaStream(mmstream, NULL,
>>> &MSPID_PrimaryVideo, 0, &stream); + ok(hr == S_OK, "Got hr %#x.\n",
>>> hr);
>>> + hr = IMediaStream_QueryInterface(stream, &IID_IDirectDrawMediaStream,
>>> (void **)&ddraw_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");
>>> + hr = IGraphBuilder_QueryInterface(graph, &IID_IMediaFilter, (void
>>> **)&media_filter); + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> + testfilter_init(&source);
>>> + hr = IGraphBuilder_AddFilter(graph, &source.filter.IBaseFilter_iface,
>>> NULL); + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> +
>>> + hr = IMediaFilter_SetSyncSource(media_filter, NULL);
>>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> +
>>> + video_info = rgb32_video_info;
>>> + video_info.bmiHeader.biWidth = 3;
>>> + video_info.bmiHeader.biHeight = 1;
>>> + mt = rgb32_mt;
>>> + mt.pbFormat = (BYTE *)&video_info;
>>> + hr = IGraphBuilder_ConnectDirect(graph,
>>> &source.source.pin.IPin_iface, pin, &mt); + ok(hr == S_OK, "Got hr
>>> %#x.\n", hr);
>>> +
>>> + hr = IDirectDrawMediaStream_CreateSample(ddraw_stream, NULL, NULL, 0,
>>> &stream_sample); + 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 = BaseOutputPinImpl_GetDeliveryBuffer(&source.source,
>>> &media_sample, NULL, NULL, 0); + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> +
>>> + ammediastream_mem_input_pin = source.source.pMemInputPin;
>>> + ammediastream_media_sample = media_sample;
>>> + ammediastream_sleep_time = 0;
>>> + ammediastream_expected_hr = S_FALSE;
>>> + thread = CreateThread(NULL, 0, ammediastream_receive, NULL, 0, NULL);
>>> +
>>> + hr = IPin_BeginFlush(pin);
>>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> +
>>> + ok(!WaitForSingleObject(thread, 2000), "Wait timed out.\n");
>>> + CloseHandle(thread);
>>> +
>>> + ref = IMediaSample_Release(media_sample);
>>> + ok(!ref, "Got outstanding refcount %d.\n", ref);
>>> +
>>> + hr = BaseOutputPinImpl_GetDeliveryBuffer(&source.source,
>>> &media_sample, NULL, NULL, 0); + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> + hr = IMemInputPin_Receive(source.source.pMemInputPin, media_sample);
>>> + ok(hr == S_FALSE, "Got hr %#x.\n", hr);
>>> +
>>> + ref = IMediaSample_Release(media_sample);
>>> + ok(!ref, "Got outstanding refcount %d.\n", ref);
>>> +
>>> + hr = IDirectDrawStreamSample_Update(stream_sample, SSUPDATE_ASYNC,
>>> NULL, NULL, 0); + ok(hr == MS_S_PENDING, "Got hr %#x.\n", hr);
>>> +
>>> + hr = IPin_EndOfStream(pin);
>>> + ok(hr == E_FAIL, "Got hr %#x.\n", hr);
>>> +
>>> + hr = IPin_EndFlush(pin);
>>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> +
>>> + hr = BaseOutputPinImpl_GetDeliveryBuffer(&source.source,
>>> &media_sample, NULL, NULL, 0); + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> + 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 = IPin_EndOfStream(pin);
>>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> +
>>> + hr = IAMMultiMediaStream_SetState(mmstream, STREAMSTATE_STOP);
>>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> +
>>> + IGraphBuilder_Disconnect(graph, pin);
>>> + IGraphBuilder_Disconnect(graph, &source.source.pin.IPin_iface);
>>> +
>>> + ref = IDirectDrawStreamSample_Release(stream_sample);
>>> + ok(!ref, "Got outstanding refcount %d.\n", ref);
>>> + ref = IAMMultiMediaStream_Release(mmstream);
>>> + ok(!ref, "Got outstanding refcount %d.\n", ref);
>>> + IMediaFilter_Release(media_filter);
>>> + ref = IGraphBuilder_Release(graph);
>>> + ok(!ref, "Got outstanding refcount %d.\n", ref);
>>> + IPin_Release(pin);
>>> + IDirectDrawMediaStream_Release(ddraw_stream);
>>> + ref = IMediaStream_Release(stream);
>>> + ok(!ref, "Got outstanding refcount %d.\n", ref);
>>> +}
>>> +
>>>
>>> static void check_ammediastream_join_am_multi_media_stream(const CLSID
>>> *clsid) {
>>>
>>> IAMMultiMediaStream *mmstream = create_ammultimediastream();
>>>
>>> @@ -7062,6 +7181,7 @@ static void test_ddrawstreamsample_update(void)
>>>
>>> ammediastream_mem_input_pin = mem_input_pin;
>>> ammediastream_media_sample = media_sample;
>>> ammediastream_sleep_time = 0;
>>>
>>> + ammediastream_expected_hr = S_OK;
>>>
>>> thread = CreateThread(NULL, 0, ammediastream_receive, NULL, 0, NULL);
>>>
>>> Sleep(100);
>>>
>>> @@ -7109,6 +7229,7 @@ static void test_ddrawstreamsample_update(void)
>>>
>>> ammediastream_mem_input_pin = mem_input_pin;
>>> ammediastream_media_sample = media_sample;
>>> ammediastream_sleep_time = 0;
>>>
>>> + ammediastream_expected_hr = S_OK;
>>>
>>> thread = CreateThread(NULL, 0, ammediastream_receive, NULL, 0, NULL);
>>>
>>> Sleep(100);
>>>
>>> @@ -7161,6 +7282,7 @@ static void test_ddrawstreamsample_update(void)
>>>
>>> ammediastream_mem_input_pin = mem_input_pin;
>>> ammediastream_media_sample = media_sample;
>>> ammediastream_sleep_time = 100;
>>>
>>> + ammediastream_expected_hr = S_OK;
>>>
>>> thread = CreateThread(NULL, 0, ammediastream_receive, NULL, 0, NULL);
>>>
>>> hr = IDirectDrawStreamSample_Update(stream_sample, 0, NULL, NULL, 0);
>>>
>>> @@ -7183,6 +7305,7 @@ static void test_ddrawstreamsample_update(void)
>>>
>>> ammediastream_pin = pin;
>>> ammediastream_sleep_time = 100;
>>>
>>> + ammediastream_expected_hr = S_OK;
>>>
>>> thread = CreateThread(NULL, 0, ammediastream_end_of_stream, NULL, 0,
>>> NULL);
>>>
>>> hr = IDirectDrawStreamSample_Update(stream_sample, 0, NULL, NULL, 0);
>>>
>>> @@ -7702,6 +7825,7 @@ START_TEST(amstream)
>>>
>>> test_ddrawstream_get_format();
>>> test_ddrawstream_set_format();
>>> test_ddrawstream_receive();
>>>
>>> + test_ddrawstream_begin_flush_end_flush();
>>>
>>> test_ddrawstreamsample_get_media_stream();
>>> test_ddrawstreamsample_update();
>
>
>
>
>
-------------- 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/20201001/62083da5/attachment-0001.sig>
More information about the wine-devel
mailing list