tinyzimmer / go-gst

Gstreamer bindings and utilities for golang
GNU Lesser General Public License v2.1
130 stars 37 forks source link

gst: add get request pad function #16

Closed brucekim closed 3 years ago

brucekim commented 3 years ago

@tinyzimmer Getting request pad is necessary for multiplexer like compositor, aggregator to get a template pad that is created on demand, not automatic.

I found other one also wants to this function in go-gst. https://github.com/tinyzimmer/go-gst/issues/14

Following is excerpt from gstreamer manual regarding request pad. https://gstreamer.freedesktop.org/documentation/application-development/basics/pads.html?gi-language=c

Request pads An element can also have request pads. These pads are not created automatically but are only created on demand. This is very useful for multiplexers, aggregators and tee elements. Aggregators are elements that merge the content of several input streams together into one output stream. Tee elements are the reverse: they are elements that have one input stream and copy this stream to each of their output pads, which are created on request. Whenever an application needs another copy of the stream, it can simply request a new output pad from the tee element. The gst_element_get_request_pad () method can be used to get a pad from the element based on the name of the pad template.

brucekim commented 3 years ago

@tinyzimmer Is it ok for you about latest revision, supplement comment?

tinyzimmer commented 3 years ago

I still have the question about that pad= nil - What's it for? Specifically, under many circumstances it could cause a runtime panic when the finalizer finally tries to do its work.

brucekim commented 3 years ago

pad = nil in ReleaseRequestPad() is to let go handle it by garbage collecting. ReleaseRequestPad() is intentionally to make request pad free.

For example, let consider a scenario in which four 'testvideosource' elements are liked with a compositor. Each testvideosource would be linked with the compositor through a sink pad respectively. The sink pad is type of request pad.

Then, let consider that , we want to remove one testvideosource linked when pipeline running. In order to do that, first of all, we unlink the testvideosource from the piprline, requiring block, unlink, unblock steps. In addition, we need to release resource of the sink pad that was connected to the testvideosource if we dont use it anymore.

Here, I think RequestRequestPad() is necessary to free the sink pad, which calls gst_element_release_request_pad(). since gstreamer is implemented in C, such explicit release call is necessary. In the other hand, we may consider new Unlink() for request pad which may performs release_request_pad in automatic way. Here again, pad=nil make the reference of the pad at go side null for garbage collection.

In summary,

It would be appreciated if you share any good idea.

tinyzimmer commented 3 years ago

My concern is that it feels very opinionated and forces something that would otherwise happen naturally, depending on the structure of the code. Perhaps if you show me an example of you using it this way it would give me better context. It might make more sense to me what you are describing.


From the gstreamer docs you linked:

This does not unref the pad. If the pad was created by using gst_element_request_pad, gst_element_release_request_pad needs to be followed by gst_object_unref to free the pad.

That's fine. Once the pad leaves scope it will eventually get unrefed by the finalizer placed on it from FromGstPadUnsafeFull. In my head the only thing that would keep that from happening is the way in which you write the code. If the structure of someone's code demands that pad = nil after Releasing, otherwise finalizers won't run, then I feel it would be better for the caller themselves do that instead of the function.

So basically, it feels possible the user's code can be refactored in a way to make this not necessary. If I see an example that proves otherwise that might sway me.

brucekim commented 3 years ago

Thanks for your review. Yes, I agree with your concern. I checked the finalizer eventually does unref, which was placed from FromGstPadUnsafeFull(). I updated an example and a patch that removes pad=nil line.

It would be appreciated if you share how you think about it.

tinyzimmer commented 3 years ago

LGTM - I'm a bit busy today but will merge later