GNOME Bugzilla – Bug 109069
[0.6.2] Fix for X window leak in xvideosink plugin
Last modified: 2004-12-22 21:47:04 UTC
In sys/xvideo/xvideosink.c, the xvideosink->window pointer is not initialized to NULL. Later in the code, there is the following commented-out code: /* FIXME destroying the window causes a crash, so we'll let it leak for now */ /*_gst_xwindow_destroy (xvideosink->window);*/ These are almost certainly related. :) I've fixed the initialization and restored the _gst_xwindow_destroy() call, and everything seems to work fine for me. See attached patch.
Created attachment 15180 [details] [review] Fix for xvideosink.c initialization code
Wim, I think we should get this one in for 0.6.1 (and for CVS head :)
Patch applied to HEAD.
Maked for 0.6.1 now.
This should be tested in HEAD before putting it in a release. xvideosink->window was already set to NULL by GObject initialization, so I don't think anything has been done to actually fix the problem.
As far as I know, gobject initialization doesn't set anything to NULL. Please corect me if I'm wrong, I'm not sure either. But as far as I know, unless you explicitely set it to NULL in <object>_init(), it's not set to NULL. Anyway, we'll test it before applying, don't worry. ;).
Fixed in 0.6.1 CVS.
Objects are allocated in g_type_create_instance() using g_malloc0: instance = g_malloc0 (node->data->instance.instance_size); So unless the constructor is overridden, xvideosink->window will default to NULL. Please revert this change.
Fixed in HEAD. This required lots of code, because simply changing windows wasn't expected originally. btw: Isn't 110330 a duplicate? I don't see a difference there.
It was reverted yesterday already. What's the real problem? A race?
If you create a new GstXWindow, you obviously create a new X window, too. However, shared memory in X's shared memory extension is coupled to the X window. And since XVideosink uses a bufferpool with preallocated shared memory images you have to make sure to clean the bufferpool and other preallocated memory upon using a new window. Without the patch old buffers will simply be drawn to the old window, which isn't destroyed, so it works (you might lose buffers though, since they're all painted to the old window). With the patch attached here, the window is destroyed but you still get buffers with memory destined for the old window, which is freed, which breaks. With the fixes in the patch I wanted to have supplied to HEAD, but which hasn't shown up on the cvs-list (too tired to do the commit?), this is fixed and the images are properly freed when requesting a new window.
Unmarking for 0.6.1, this won't be fixed by then.
*** Bug 110330 has been marked as a duplicate of this bug. ***
What is the status of this now?
HEAD has this bug fixed since somewhere around the 0.6.1 release time and I haven't heard of any problems so far, so it should be ok to port it back to 0.6
OK, let's try to fix this again. Maybe it'll work now.
Created attachment 16594 [details] [review] backport to 0_6 of HEAD xvideosink code
*** Bug 93861 has been marked as a duplicate of this bug. ***
Applied. If this doesn't work, we'll reopen it again.
And, it doesn't work... What a surprise! videotestsrc ! colorspace ! xvideosink crashes, working on a fix...
Created attachment 17346 [details] [review] fixage
Created attachment 17353 [details] [review] you need this one too
*** Bug 114758 has been marked as a duplicate of this bug. ***
Created attachment 17356 [details] [review] cumulative patch against 0.6 branch
Created attachment 17363 [details] [review] new patch for xvideosink
Created attachment 17364 [details] [review] New patch for videotestsrc
The above patches also remove the GST_CAPS_CHAINED() weirdity in both, and just loop over the patch. I prefer this as behaviour, although I'm not 100% sure whether the old behaviour was really "wrong". Also, videotestsrc should return "DONE", not "OK", because otherwise, it will still set the *total* caps on the pipeline and thus the first element hereof on the next element (colorspace), which breaks the whole reason of the loop in the first place (find the right one). Anyway, I suggest applying these for 0.6.2, "videotestsrc ! colorspace ! xvideosink" works now.
Created attachment 17369 [details] [review] final cumulative patch
So, this seems applied now, closing again... Waiting for the next reopen. ;).