[gstreamer-bugs] [Bug 166142] [PATCH] [x/xvimagesink] not threadsafe

[bugzilla-daemon at bugzilla.gnome.org]

Please DO NOT reply to this by email. All additional comments should be made in
the comments box of this bug report.

 http://bugzilla.gnome.org/show_bug.cgi?id=166142
 GStreamer | gst-plugins | Ver: HEAD CVS





------- Additional Comments From Ronald Bultje  2005-02-06 18:20 -------
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.

------- You are receiving this mail because: -------
You are the assignee for the bug.
You are the QA contact for the bug.