mfmediaengine: Be a bit more conservative with locks in engine Shutdown.
During engine shutdown we acquire engine lock first, then locks of its constituents (e.g. sample grabbers); whereas normally the order is the other way around (e.g. timer callback -> acquire sample grabber lock -> OnProcessSample callback -> engine lock). This is deadlock prone.
With this commit, engine lock is released before we shutdown the inner media session.
Merge request reports
Activity
assigned to @yshui
I think this deserves more consideration. First we don't currently shut down sinks the same way Windows does, that sort of prevents seeing what happens with this grabber. We should fix that first.
Second, for the grabber, locking does not match what we do. On Windows, blocking in OnProcessSample() does not prevent shut down sequence. So this needs some more manual testing of the sample grabber, to see what happens when OnProcessSample() is blocked, but clock state callbacks are called for example, do they produce events or not, that sort of thing.
After that we'll see if changing call order in the media engine is still useful.
First we don't currently shut down sinks the same way Windows does.
From my observations, Windows calls
MediaSink::Shutdown
inMediaSession::Shutdown
, and doesn't callShutdownObject
; and it doesn't shutdown sinks at all when changing/clearing topologies. Does that match what you know? Is that what you suggest we do?On Windows, blocking in OnProcessSample() does not prevent shut down sequence.
That would suggestOnProcessSample
is called on a different thread, wouldn't it? And that doesn't say anything about locking.Edited by Yuxuan ShuiWait, sorry I got things mixed up, yeah OnProcessSample is not called from Shutdown...
🤦 Did you mean locking engine in OnProcessSample doesn't block shutdown on native? Or did you mean something else by "blocking"? Because that would be expected right? With this MR shutdown won't be blocked in that case either, just like native.
Edited by Yuxuan ShuiOr maybe you mean like putting an
while(true)
insideOnProcessSample
? In that case, I guess this means eitherOnProcessSample
is called without the grabber locked, or shutting down a grabber doesn't require its lock.I think the latter is less likely, the former is achievable independent of this MR.
So, I tried, and it's difficult to validate if engine is locked or not during session shutdown, as there isn't many opportunity to hook into the engine.
But that also mean it's hard for the application to observe this fact, in which case this MR still stands. And I want to argue for its inclusion.
- In general, it's a good idea to avoid nested locks. Of course, we should avoid holding grabber locks while calling
OnProcessSample
, but: - That fix is less universal then this one. The problem arise from the lock order being reversed during shutdown. So there could be more potential deadlocks on top of this engine/grabber deadlock. This MR would fix all of them.
I would also open an MR to do what you suggested, but I think this MR should be merged as well.
- In general, it's a good idea to avoid nested locks. Of course, we should avoid holding grabber locks while calling
I was able to block engine shutdown by injecting a media source via
IMFMediaEngineExtension
. While the engine shutdown is blocked, I was able to call engine shutdown again from a different thread, and the second shutdown returnsMF_E_SHUTDOWN
immediately.This mean either engine lock is not held; or engine shutdown flag is not protected by a lock, maybe an atomic. I think this is more evidence this MR should be accepted.
removed review request for @besentv