After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 109069 - [0.6.2] Fix for X window leak in xvideosink plugin
[0.6.2] Fix for X window leak in xvideosink plugin
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins
0.6.0
Other Linux
: Normal minor
: 0.6.2
Assigned To: Ronald Bultje
GStreamer Maintainers
: 93861 110330 114758 (view as bug list)
Depends on:
Blocks: 110330
 
 
Reported: 2003-03-24 07:44 UTC by janzen
Modified: 2004-12-22 21:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix for xvideosink.c initialization code (1.26 KB, patch)
2003-03-24 07:45 UTC, janzen
none Details | Review
backport to 0_6 of HEAD xvideosink code (23.83 KB, patch)
2003-05-17 13:01 UTC, Ronald Bultje
none Details | Review
fixage (1.49 KB, patch)
2003-06-09 13:36 UTC, Ronald Bultje
none Details | Review
you need this one too (609 bytes, patch)
2003-06-09 14:21 UTC, Ronald Bultje
none Details | Review
cumulative patch against 0.6 branch (2.00 KB, patch)
2003-06-09 15:00 UTC, Thomas Vander Stichele
none Details | Review
new patch for xvideosink (5.06 KB, patch)
2003-06-09 17:57 UTC, Ronald Bultje
none Details | Review
New patch for videotestsrc (1.64 KB, patch)
2003-06-09 17:57 UTC, Ronald Bultje
none Details | Review
final cumulative patch (8.65 KB, patch)
2003-06-09 18:50 UTC, Thomas Vander Stichele
none Details | Review

Description janzen 2003-03-24 07:44:47 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.
Comment 1 janzen 2003-03-24 07:45:51 UTC
Created attachment 15180 [details] [review]
Fix for xvideosink.c initialization code
Comment 2 Christian Fredrik Kalager Schaller 2003-03-31 08:11:28 UTC
Wim, I think we should get this one in for 0.6.1 (and for CVS head :)
Comment 3 Benjamin Otte (Company) 2003-04-04 17:59:23 UTC
Patch applied to HEAD.
Comment 4 Ronald Bultje 2003-04-04 18:00:41 UTC
Maked for 0.6.1 now.
Comment 5 David Schleef 2003-04-05 21:05:12 UTC
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.
Comment 6 Ronald Bultje 2003-04-06 22:02:45 UTC
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. ;).
Comment 7 Ronald Bultje 2003-04-07 17:09:44 UTC
Fixed in 0.6.1 CVS.
Comment 8 David Schleef 2003-04-08 23:42:30 UTC
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.
Comment 9 Benjamin Otte (Company) 2003-04-09 00:08:47 UTC
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.
Comment 10 Ronald Bultje 2003-04-09 05:33:50 UTC
It was reverted yesterday already. What's the real problem? A race?
Comment 11 Benjamin Otte (Company) 2003-04-09 16:30:07 UTC
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.
Comment 12 Ronald Bultje 2003-04-11 09:38:06 UTC
Unmarking for 0.6.1, this won't be fixed by then.
Comment 13 Ronald Bultje 2003-04-14 14:36:38 UTC
*** Bug 110330 has been marked as a duplicate of this bug. ***
Comment 14 Christian Fredrik Kalager Schaller 2003-05-12 13:39:03 UTC
What is the status of this now?
Comment 15 Benjamin Otte (Company) 2003-05-14 15:23:46 UTC
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
Comment 16 Ronald Bultje 2003-05-14 16:31:39 UTC
OK, let's try to fix this again. Maybe it'll work now.
Comment 17 Ronald Bultje 2003-05-17 13:01:26 UTC
Created attachment 16594 [details] [review]
backport to 0_6 of HEAD xvideosink code
Comment 18 Ronald Bultje 2003-05-17 13:05:33 UTC
*** Bug 93861 has been marked as a duplicate of this bug. ***
Comment 19 Ronald Bultje 2003-05-18 16:46:05 UTC
Applied. If this doesn't work, we'll reopen it again.
Comment 20 Ronald Bultje 2003-06-09 13:33:35 UTC
And, it doesn't work... What a surprise!

videotestsrc ! colorspace ! xvideosink crashes, working on a fix...
Comment 21 Ronald Bultje 2003-06-09 13:36:13 UTC
Created attachment 17346 [details] [review]
fixage
Comment 22 Ronald Bultje 2003-06-09 14:21:38 UTC
Created attachment 17353 [details] [review]
you need this one too
Comment 23 Ronald Bultje 2003-06-09 14:59:57 UTC
*** Bug 114758 has been marked as a duplicate of this bug. ***
Comment 24 Thomas Vander Stichele 2003-06-09 15:00:43 UTC
Created attachment 17356 [details] [review]
cumulative patch against 0.6 branch
Comment 25 Ronald Bultje 2003-06-09 17:57:14 UTC
Created attachment 17363 [details] [review]
new patch for xvideosink
Comment 26 Ronald Bultje 2003-06-09 17:57:43 UTC
Created attachment 17364 [details] [review]
New patch for videotestsrc
Comment 27 Ronald Bultje 2003-06-09 18:00:00 UTC
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.
Comment 28 Thomas Vander Stichele 2003-06-09 18:50:27 UTC
Created attachment 17369 [details] [review]
final cumulative patch
Comment 29 Ronald Bultje 2003-06-10 05:20:11 UTC
So, this seems applied now, closing again... Waiting for the next
reopen. ;).