[PATCH v6 0/7] MR140: qasf: More ASF reader filter implementation.

[Zebediah Figura]

On 7/5/22 14:02, Rémi Bernon (@rbernon) wrote:
>> 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.
> 
> We wait for the streaming thread, in WMReader_Stop, so while holding the
> filter_cs. Which may block the streaming thread if the callbacks need
> to enter the filter_cs as well.
> 
>> 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.
> 
> It is completely non obvious, and imho very brittle. I don't think
> the WMReader_Stop call should ever block and wait for the streaming
> thread. The reader is asynchronous after all, it should notify the
> thread and the callbacks would be called eventually.
> 
> The tests clearly suggest that native doesn't block there, as there's
> an event and a WaitForSingleObject after the Stop call. Changing the
> timeout also shows that the call is fully async.
> 

It is brittle, and that's why DirectShow is terrible :-)

The above message seems to contain some degree of confusion. There are 
essentially two places where we do the same thing:

* in DirectShow, we must clean up the streaming thread in 
IMediaFilter::Stop(), and wait for it to complete. In strmbase terms 
this translates to the cleanup_stream() callback. The API requires this.

* in wmvcore, we currently wait for the streaming thread in 
IWMReader::Stop(). As you correctly point out, the API actually 
*doesn't* require this. However, we are almost certainly going to need 
to make sure the streaming thread is stopped before destroying the 
reader object, and some tests imply that it should be done in 
IWMReader::Close() as well [namely, WMT_STOPPED will not be received 
after IWMReader::Close().]

So yes, we could not block in IWMReader::Stop(), but that's not 
ultimately going to help anything. We do still need to wait for the 
streaming thread to stop at some point.