[PATCH 3/5] amstream: Add a second critical section for filter methods that are called from streams.
Zebediah Figura (she/her)
zfigura at codeweavers.com
Thu Apr 8 10:12:35 CDT 2021
On 4/8/21 1:14 AM, Anton Baskanov wrote:
> On среда, 7 апреля 2021 г. 22:27:33 +07 you wrote:
>> On 4/6/21 1:04 PM, Anton Baskanov wrote:
>>> Signed-off-by: Anton Baskanov <baskanov at gmail.com>
>>> ---
>>> This is required to avoid deadlocks, e.g. when the filter is stopped
>>> while EndOfStream is being called.
>>
>> So, if I understand correctly, the race is:
>>
>>
>> Main thread Streaming thread
>> ------------------------------------------------
>> IMediaControl::Stop() IPin::EndOfStream()
>> lock graph->cs IMediaStreamFilter::EndOfStream()
>> stop parser filter
>> wait for streaming thread try to grab graph->cs
>>
>>
>> Shouldn't patch 0005 be enough to avoid this? We don't hold filter->cs,
>> or any amstream-specific locks, while waiting for streaming threads to stop.
>
> Actually, the deadlock is:
>
> Main thread Streaming thread
> ------------------------------------------------
> IMediaStreamFilter::Stop() IPin::EndOfStream()
> lock filter->cs lock stream->cs
> IAMMediaStream::SetState() IMediaStreamFilter::EndOfStream()
> try to lock stream->cs try to lock filter->cs
>
> In theory, it should be possible to move SetState() calls out of the filter
> critical section, but there are other possible deadlocks, like:
>
> Main thread Streaming thread
> ------------------------------------------------
> IMediaStreamFilter::GetStopPosition() IPin::EndOfStream()
> lock filter->cs lock stream->cs
> get_seeking()
> IPin_ConnectedTo() IMediaStreamFilter::EndOfStream()
> try to lock stream->cs try to lock filter->cs
>
>
>
Ah. Yeah, lock ordering would be a problem. Fortunately it only seems to
be a problem there.
Could we instead call IMediaStreamFilter::EndOfStream() not inside
stream->cs? That solution seems potentially simpler, judging by your
not-yet-submitted commit 4ad440dde6. It should probably be accompanied
by a comment; probably we should add a similar comment to
IStreamSample::GetSampleTimes().
More information about the wine-devel
mailing list