[PATCH 3/5] amstream: Add a second critical section for filter methods that are called from streams.
Anton Baskanov
baskanov at gmail.com
Fri Apr 9 14:02:44 CDT 2021
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.
More information about the wine-devel
mailing list