Re: [PATCH v5 0/6] MR140: qasf: More ASF reader filter implementation.

[Rémi Bernon]

On Mon Jul  4 20:26:47 2022 +0000, **** wrote:
> Zebediah Figura replied on the mailing list:
> ```
> On 6/15/22 01:58, Rémi Bernon wrote:
> > +static HRESULT asf_reader_init_stream(struct strmbase_filter *iface)
> > +{
> > +    struct asf_reader *filter = impl_from_strmbase_filter(iface);
> > +    HRESULT hr;
> > +    int i;
> > +
> > +    TRACE("iface %p\n", iface);
> > +
> > +    for (i = 0; i < filter->stream_count; ++i)
> > +    {
> > +        struct asf_stream *stream = filter->streams + i;
> > +
> > +        if (!stream->source.pin.peer)
> > +            continue;
> > +
> > +        hr = IMemAllocator_Commit(stream->source.pAllocator);
> > +        if (FAILED(hr))
> > +        {
> > +            WARN("Failed to commit stream %u allocator, hr %#lx\n",
> > i, hr);
> > +            continue;
> > +        }
> > +
> > +        hr = IPin_NewSegment(stream->source.pin.peer, 0, 0, 1);
> > +        if (FAILED(hr))
> > +        {
> > +            WARN("Failed to start stream %u new segment, hr %#lx\n",
> > i, hr);
> > +            continue;
> > +        }
> > +    }
> > +
> > +    return IWMReader_Start(filter->reader, 0, 0, 1, NULL);
> > +}
> > +
> > +static HRESULT asf_reader_cleanup_stream(struct strmbase_filter *iface)
> > +{
> > +    struct asf_reader *filter = impl_from_strmbase_filter(iface);
> > +    int i;
> > +
> > +    TRACE("iface %p\n", iface);
> > Shouldn't the reader be stopped here?
> Maybe, but it causes all sort of deadlocks which I've stopped trying to fix, and in practice it seems to work without it.

The deadlock is because `WMReader_Stop` calling `stop_streaming` which needs to enter the streaming thread CS to stop it, and then waits for it.

It can either be that the streaming thread is blocked in `wm_reader_get_stream_sample`, which, for some reason may be waiting for samples which never come.

Or, that the streaming thread calls the callbacks before exiting, and that the callbacks need to enter the filter CS, which is currently held by asf_reader_cleanup_stream. For instance either because there's a sample that's been received, or because of the `WMT_END_OF_STREAMING` notification.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/140#note_3304

[PATCH v5 0/6] MR140: qasf: More ASF reader filter implementation.

[Zebediah Figura]

On 7/5/22 00:46, Rémi Bernon (@rbernon) wrote:
> On Mon Jul  4 20:26:47 2022 +0000, **** wrote:
> > Zebediah Figura replied on the mailing list:
> > ```
> > On 6/15/22 01:58, Rémi Bernon wrote:
> > > +static HRESULT asf_reader_init_stream(struct strmbase_filter *iface)
> > > +{
> > > +    struct asf_reader *filter = impl_from_strmbase_filter(iface);
> > > +    HRESULT hr;
> > > +    int i;
> > > +
> > > +    TRACE("iface %p\n", iface);
> > > +
> > > +    for (i = 0; i < filter->stream_count; ++i)
> > > +    {
> > > +        struct asf_stream *stream = filter->streams + i;
> > > +
> > > +        if (!stream->source.pin.peer)
> > > +            continue;
> > > +
> > > +        hr = IMemAllocator_Commit(stream->source.pAllocator);
> > > +        if (FAILED(hr))
> > > +        {
> > > +            WARN("Failed to commit stream %u allocator, hr %#lx\n",
> > > i, hr);
> > > +            continue;
> > > +        }
> > > +
> > > +        hr = IPin_NewSegment(stream->source.pin.peer, 0, 0, 1);
> > > +        if (FAILED(hr))
> > > +        {
> > > +            WARN("Failed to start stream %u new segment, hr %#lx\n",
> > > i, hr);
> > > +            continue;
> > > +        }
> > > +    }
> > > +
> > > +    return IWMReader_Start(filter->reader, 0, 0, 1, NULL);
> > > +}
> > > +
> > > +static HRESULT asf_reader_cleanup_stream(struct strmbase_filter *iface)
> > > +{
> > > +    struct asf_reader *filter = impl_from_strmbase_filter(iface);
> > > +    int i;
> > > +
> > > +    TRACE("iface %p\n", iface);
> > > Shouldn't the reader be stopped here?
> > Maybe, but it causes all sort of deadlocks which I've stopped trying to fix, and in practice it seems to work without it.
>
> The deadlock is because `WMReader_Stop` calling `stop_streaming` which needs to enter the streaming thread CS to stop it, and then waits for it.

We don't wait for the streaming thread while holding stream_cs, though. 
Do you mean the reader CS? If we're taking the reader CS from the stream 
thread, that's also a bug that'll need to be solved somehow.
> It can either be that the streaming thread is blocked in `wm_reader_get_stream_sample`, which, for some reason may be waiting for samples which never come.

If that's happening there's a bug, I think. If we're EOS then 
wm_reader_get_stream_sample() should return immediately.
> Or, that the streaming thread calls the callbacks before exiting, and that the callbacks need to enter the filter CS, which is currently held by asf_reader_cleanup_stream. For instance either because there's a sample that's been received, or because of the `WMT_END_OF_STREAMING` notification.

There's a (unfortunately as yet unwritten) rule that nothing from the 
streaming thread is allowed to ever grab the filter CS.

I failed to notice this during review the first time, but that means 
that patch 3/7 is broken. It shouldn't be necessary to take the filter 
CS, though, since pin connection state can't change while the filter is 
running.