GNOME Bugzilla – Bug 166142
[PATCH] [x/xvimagesink] not threadsafe
Last modified: 2005-06-30 15:50:42 UTC
xvimagesink (and probably ximagesink, too) are not interface-threadsafe. This indicates that by using public interfaces, it is easily possible to crash it, e.g. by setting the xwindow-ID (XOverlay interface) in the PLAYING state while iterating in another thread. If it doesn´t crash, it will still regularly give X window errors, not so much because we intermingle X calls, but because we simply propose arguments that don´t exist. Examples include the xvimage created in the _chain() function, which can be free´ed in the xoverlay_set_xwindow_id() function right after, before it´s sent to X using XvPutImage(). Attached patch fixes it by changing from an internal locking scheme to a caller locking scheme (which is how locking is supposed to be done anyway). ximagesink probably requires a similar patch. Sdlvideosink is fine (or so my tests indicate).
Created attachment 36908 [details] [review] patch
Created attachment 36910 [details] [review] patch2 Same patch for ximagesink.
*** Bug 164165 has been marked as a duplicate of this bug. ***
Created attachment 37075 [details] [review] Proposed patch implementing a stream lock and keeping the x locks where they are I am attaching this patch which will be easier to port to 0.9 and contains some other fixes as well. Please try it, tell me if it fixes your issue and then i ll commit it. Thanks.
Julien, trying to keep compat between the threaded branch and 0.8 is pointless, we'll break it within no-time. Please consider the original patch and feel free to rename x_lock to stream_lock or something. I don't want to debug tens of different versions of threading issues for the pointless sake of being able to not port patches over anyway.
OK, so that wasn´t nice... I´ll clarify: identifying this bug was a bitch. It is not reproduceable in gdb for me, and I get random X errors or sometimes segfaults (with no core file). That sucks. Also, since it solely happens in threaded apps, debug logs are useless (other threads continue running for a while). Now, after a while I started seeing that it was not a bug in my app and started debugging specific pieces of code in xvimagesink. It took me a long while to figure out the problem. Now, after that I´ve analyzed all code and relocated all locks to protect critical regions, which is what locks (mutexes) are there for. I´m fairly sure that I´ve done this correctly, you can mark my words on that, I´ve got years of experience in threaded programming and debugging (see e.g. mjpegtools, which is a fully threaded media processing suite that is hellishly stable). However, it still is a bitch to check and test. I can just say that it no longer crashes. I cannot say that it is bugfree, because I cannot know. Your code does something different. That is fine, but pointless, I´ll need to come up with new testcases, analyze code again and (probably) again fix mistakes that I already fixed (both in xvimagesink and ximagesink). That´s a waste of time, and it definately is not my hobby to do threaded debugging. It is no fun. Also, your stream_lock is always an encompasser around functions using the x_lock. This essentially makes the x_lock pointless. You can rename x_lock like I proposed and that would do the same thing in practice, assuming your patch is correct. The reason that you choose here, keeping compat with 0.9, is pointless. There is no compat. We´ll rewrite large pieces of videosinks, too, and you know that. We will have to port over all patches manually, and back too. That´s just the way it is and it is the downside of a large core restructuring. Given all of the above, I propose to just re-look at my patch, possibly rename x_lock to stream_lock or something and go with that.
Bug Fixed in HEAD
Created attachment 37451 [details] [review] Test case demonstrating several threads using the xoverlay API Compile with this gcc `pkg-config --cflags --libs gstreamer-0.8 gstreamer-interfaces-0.8 gdk-2.0` thread_test.c -o thread_test