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.