[PATCH 3/5] amstream: Add a second critical section for filter methods that are called from streams.

[Zebediah Figura (she/her)]

On 4/9/21 2:02 PM, Anton Baskanov wrote:
> On четверг, 8 апреля 2021 г. 22:12:35 +07 you wrote:
>> 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().
> 
> Looks like this should work. There will be a race condition where
> IMediaStreamFilter::Flush is called before MediaStreamFilter::EndOfStream.
> AFAICS this should not cause any issues other than making eos_count negative
> briefly.
> 
> 

There is a race there, yeah, but I don't think it's meaningful.

Nice job anticipating my "is it valid to decrement the counter past 
zero" question in 203580, by the way :D