GNOME Bugzilla – Bug 683995
[0.11] [API change] No parent parameter in GstPadLinkFunction
Last modified: 2012-09-24 18:27:47 UTC
Created attachment 224288 [details] [review] pad: Add parent parameter to link function While looking at the GstPadLinkFunction, I found two strange things: 1. There is no parent pointer passed, even though every user in the main modules seems to fetch its parent as its first action. 2. The documentation says that a link function on a src pad should call the one on it's peer sink pad, that sound pretty painful to implement safely, as the object lock is released some other thread could be unlinking/releasing the sink pad while this happens. If there is a good reason for either of those behaviours ? I'm attaching a proposed patch that adds a parent pointer, calls both functions independently and rejects the link if the parent is required but gone. Please discuss. Also, please don't merge this patch without updating the other modules, if you think it's a good idea, I can do the fixing. The unlink function happens under the object lock, so none of these problems apply.
(In reply to comment #0) > Created an attachment (id=224288) [details] [review] > pad: Add parent parameter to link function > > While looking at the GstPadLinkFunction, I found two strange things: > > 1. There is no parent pointer passed, even though every user in the main > modules seems to fetch its parent as its first action. It didn't think it was important enough to add, but yes, it would be good. > 2. The documentation says that a link function on a src pad should call the one > on it's peer sink pad, that sound pretty painful to implement safely, as the > object lock is released some other thread could be unlinking/releasing the sink > pad while this happens. > > If there is a good reason for either of those behaviours ? It was done in an attempt to make linking/unlinking threadsafe. The idea is that the linkfunction of the srcpad holds some sort of lock while it calls the peerpad link function. This makes sure that you only have 1 thread calling both the link/unlink function of the srcpad and the sinkpad and both pads would be sure that they are linked together. If you call both link functions separately, by the time you have called the linkfunction on the srcpad, the sinkpad could already have been linked to another pad. Then you have a situation where the srcpad thinks it is linked to a sinkpad while it really isn't. You could check this after calling the link function and undo the link but that is messy and also racy. Unfortunately we could not use the object lock on the srcpad for this because you can't hold it while calling into the link/unlink function of the peer pad without nasty deadlocks (if the peer sinkpad, for example, would do anything with the srcpad). So, the proper way to implement a link/unlink function is to make a separate lock that you take before calling the peer link function. It's been a while since I thought about this so I might be missing something but that's what I remember. Link/unlink functions are really not used much (and when they are, they can probably go). > > I'm attaching a proposed patch that adds a parent pointer, calls both functions > independently and rejects the link if the parent is required but gone. Please > discuss. > > Also, please don't merge this patch without updating the other modules, if you > think it's a good idea, I can do the fixing. > > The unlink function happens under the object lock, so none of these problems > apply.
(In reply to comment #1) > (In reply to comment #0) > > Created an attachment (id=224288) [details] [review] [details] [review] > > pad: Add parent parameter to link function > > > > While looking at the GstPadLinkFunction, I found two strange things: > > > > 1. There is no parent pointer passed, even though every user in the main > > modules seems to fetch its parent as its first action. > > It didn't think it was important enough to add, but yes, it would be good. So let's add this part (the parent pointer) ASAP? :) Are all other pad functions having one or is another one missing?
commit de635d089f10816668957ac08b19ae78761d7ab2 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Mon Sep 17 13:09:58 2012 +0200 pad: Add parent parameter to the link and unlink functions Fixes part of bug #683995. All other functions have the parent parameter. Now what shall we do with the remaining part of this bug?
> Now what shall we do with the > remaining part of this bug? WONTFIX ? Is there a problem that needs solving or is solved by that?
Was mostly a matter of consistency, let's declare it fixed then.