LibVLC Deadlock on libvlc_media_player stop (win32) [Fixed]

[Rumpel]

Okay, i will test it and tell you the result

[bratao]

Just to report.
I´m using in a C++ project and i have exactly the same deadlock.
Both in 1.1.7 and in 1.2 nightly

[Rumpel]

So i removed the vlc_testcancel() call and my app works fine for me at the moment i have no problems. But on other machines with different CPUs i have still the problem with the deadlocks

[Rémi Denis-Courmont]

I told you so. vlc_testcancel() has nothing to do with the problem. It just alters timing a bit which can change the odds of hitting the deadlock depending on the hardware.

[solud]

I have fixed this "bug" in my software!

I no longer believe this is a bug in LibVLC, per se -- more like the way the library is used. I'll try to explain what I've found, though since my experience in this type of thing is limited I may still be wrong (but currently deadlock-free)!

The problem seems to be with Win32-LibVLC's use of WaitForSingleObjectEx(...) during vlc_join(...). If you call this within a thread that has child windows, it seems you risk deadlocking the application. Here's a quote from the docs:

Use caution when calling the wait functions and code that directly or indirectly creates windows. If a thread creates any windows, it must process messages. Message broadcasts are sent to all windows in the system. A thread that uses a wait function with no time-out interval may cause the system to become deadlocked. Two examples of code that indirectly creates windows are DDE and the CoInitialize function. Therefore, if you have a thread that creates windows, use MsgWaitForMultipleObjects or MsgWaitForMultipleObjectsEx, rather than WaitForSingleObjectEx.

With a timeout specified as INFINITE in the following code, a deadlock can occur randomly since no messages will be able to be processed:

Code: Select all

do vlc_testcancel (); while (WaitForSingleObjectEx (handle, INFINITE, TRUE) == WAIT_IO_COMPLETION);

When my application called libvlc_media_player_stop(Handle) from within the main thread, which definitely has child windows, it ran the high risk of deadlocking when it hit vlc_join(...). And from my testing this only happened on the call to stop the 2nd player, I can only assume it was because in the time it was stopping the first player a window message was sent that couldn't get processed thus causing WaitForSingleObjectEx(...) to deadlock.

The solution for me, since I don't think it necessary to modify LibVLC code, was to place the libvlc_media_player_stop(Handle) calls in a separate thread starting from _FormClosing(...). Problem solved!

I tested this on a workstation that would deadlock almost instantly (old CPU, slow) and was able to open and close a form with two players repeatedly for 10 minutes without deadlock. I'll be doing further testing later in the day, and if I find anything else I'll be sure to add more information here.

[Rumpel]

solud can you show me a littel bit of code form you as example??

[solud]

Code: Select all

private void StopVideo() { VlcMediaPlayer p1 = player1MediaPlayer; VlcMediaPlayer p2 = player2MediaPlayer; if (p1 != null) p1.Stop(); if (p2 != null) p2.Stop(); } private void VideoForm_FormClosing(object sender, FormClosingEventArgs e) { Thread t = new Thread(new ThreadStart(StopVideo)); t.Start(); }

[Rumpel]

Thank you

[rvs75]

Seems to work for me too. I add a "t.join()" after "t.start()" because i have a libvlc_media_player_set_media, libvlc_media_player_set_hwnd and libvlc_media_player_play after the libvlc_media_player_stop.

Thanx

[Rumpel]

I have still problems but i think it is the right way.

EDIT:
I have a question. So i get no deadlocks but in some cases if i will stop a video the libvlc_media_player_stop() function needs exactly 30sec to close but only in a few cases not all. Why?

EDIT 2:
Sorry now i read in the forum that RTSP is very slow sometimes but in thread is no solution. Any tipps?

[Rémi Denis-Courmont]

For RTSP, there is no out-of-the-box solution. Either live555 needs to be fixed, or the VLC RTSP input must be rewritten to use a better library.

[Sébastien Escudier]

this should be fixed in 1.2

[bratao]

Sébastien, whats will be fixed, the deadlock problem or the RTSP one ?

[Rémi Denis-Courmont]

RTSP. I don't see how VLC would fix a limitation of Win32...

[bratao]

Rémi, if fact you can fix it in libvlc. Just replacing the WaitForSingleObjectEx for a MsgWait equivalent:

Code: Select all

ret = MsgWaitForMultipleObjects (1, &handle, INFINITE, NULL, NULL); if (ret == WAIT_OBJECT_0 + 1) { /* PROCESS THE MESSAGE */ BOOL bRet; while ((bRet = PeekMessage (&msg, NULL, 0, 0, PM_REMOVE)) != 0) { TranslateMessage (&msg); DispatchMessage (&msg); } LOOP again until we receive WAIT_OBJECT_0 }

[Rémi Denis-Courmont]

That might be a good idea. That would be then MsgWaitForMultipleObjectsEx because we need an alertable function.

However, I'm a bit suspicious that this will raise surprise reentrancy problems in VLC and VLC-based applications :-/

[ispy]

If this could be fixed it would help my project a lot - i get about 20 error reports of VLC deadlocking on stop a day. I've tried every workaround I can find/ think of.

[Rémi Denis-Courmont]

I am not an expert in Win32 API and I might have missed something but...

• It is not clear to me that the Win32 documentation would require the use of MsgWaitForMultipleObjects(Ex) instead of WaitForMultipleObjects(Ex), WaitForSingleObject(Ex) and Sleep(Ex) in threads that have created windows. In particular, LibVLC should not need any window messages to wait for another thread to complete.
• The proposed change may reentrancy problems in the window events procedures that did not get called from LibVLC and now would get called.
• It might interfere with the main loop of the applications, if they did anything other than just TranslateMessage/DispatchMessage.
• Nobody has reviewied the proposed patch on vlc-devel.

So at this point, I am going to assume that this is a bug in some applications and not in LibVLC. The ball is yours.

[bratao]

I think that this path is very important.

It is not clear to me that the Win32 documentation would require the use of MsgWaitForMultipleObjects(Ex) instead of WaitForMultipleObjects(Ex), WaitForSingleObject(Ex) and Sleep(Ex) in threads that have created windows. In particular, LibVLC should not need any window messages to wait for another thread to complete.

The documentation cleary state that if a thread process messages, it NEED use MsgWaitForMultipleObjects
From http://msdn.microsoft.com/en-us/library ... 85%29.aspx

If a thread creates any windows, it must process messages. Message broadcasts are sent to all windows in the system. A thread that uses a wait function with no time-out interval may cause the system to become deadlocked. Two examples of code that indirectly creates windows are DDE and the CoInitialize function. Therefore, if you have a thread that creates windows, use MsgWaitForMultipleObjects or MsgWaitForMultipleObjectsEx, rather than WaitForMultipleObjects.

The proposed change may reentrancy problems in the window events procedures that did not get called from LibVLC and now would get called.

I don´t see how this could generate any reentrancy problems. Could explain it ?

It might interfere with the main loop of the applications, if they did anything other than just TranslateMessage/DispatchMessage.

No you got it wrong. The main loop of applications could do anything, if it not create window, MsgWait.. will behave exactly as WaitForMultipleObjects.

Nobody has reviewied the proposed patch on vlc-devel.

The patch is here : http://mailman.videolan.org/pipermail/v ... 81125.html
It is wrong. With this code, i understand why you should see that it may have reentrancy problems, and if fact, they will.
The MsgWaitForMultipleObjects should wait normaly as WaitForMultipleObjects but if it recieve a message, it should process it, and then go back to wait state.
This way, if a thread do not create windows it works exactly as WaitForMultipleObjects. If it create a windows, it NEED MsgWaitForMultipleObjects.
The patch should be in like this:

Code: Select all

--- src/win32/thread.c | 14 +++++++++++++- 1 files changed, 13 insertions(+), 1 deletions(-) diff --git a/src/win32/thread.c b/src/win32/thread.c index 00c2781..f4521b1 100644 --- a/src/win32/thread.c +++ b/src/win32/thread.c @@ -125,7 +125,19 @@ BOOL WINAPI DllMain (HINSTANCE hinstDll, DWORD fdwReason, LPVOID lpvReserved) static DWORD vlc_WaitForMultipleObjects (DWORD count, const HANDLE *handles, DWORD delay) { - DWORD ret = WaitForMultipleObjectsEx (count, handle, FALSE, delay, TRUE); + DWORD ret; + for (;;) + { + ret = MsgWaitForMultipleObjectsEx (count, handle, delay, + QS_ALLINPUT, MWMO_ALTERTABLE); + if (ret == WAIT_OBJECT_0 + count)/* We have a message to process, dispatch it, then come back to processing */ + { + MSG msg; + + while (PeekMessage (&msg, NULL, 0, 0, PM_REMOVE)) + { + TranslateMessage (&msg); + DispatchMessage (&msg); + } + } + else + break; + } /* We do not abandon objects... this would be a bug */ assert (ret < WAIT_ABANDONED_0 || WAIT_ABONDONED_0 + count - 1 < ret); --

I did´t check the VLC Code, but it objects use to be Abandoned , the code should subtract delay on each loop. But if it really need to wait for the object the code is correctly.
A remark is that on the assert (at last on the maillist patch) have a typo on WAIT_ABONDONED_0.

[Rémi Denis-Courmont]

The documentation cleary state that if a thread process messages, it NEED use MsgWaitForMultipleObjects

The point is, there is no reason why the the thread would have process a message in the first place. Waiting for another VLC thread to exit does not require processing messages to make any progress. Thus the thread should be perfectly fine postponing the message processing until LibVLC unwinds and the calling application main loop is reached.

I don´t see how this could generate any reentrancy problems. Could explain it ?

It's obvious! You'd be calling the window procedure from a site, within LibVLC, where it was not called earlier.

It might interfere with the main loop of the applications, if they did anything other than just TranslateMessage/DispatchMessage.

No you got it wrong. The main loop of applications could do anything, if it not create window, MsgWait.. will behave exactly as WaitForMultipleObjects.

If it does not create a window, then it won't make a difference indeed. But this is a stupid irrelevant point.

If the assumption were that the thread does not create any window, we wouldn't need to have this discussion in the first place...