arturoc / gstreamer1.0-rs

Idiomatic bindings for Gstreamer on Rust.
MIT License
36 stars 14 forks source link

I suspect that Bin::add should not take ownership #17

Closed Yoric closed 8 years ago

Yoric commented 8 years ago

According to the definition of GStreamer, I need to add my Elements to a Pipeline before I can link with them. However, the only way I have found to add an Element is to use Bin::add, which takes ownership. Once this is done, I cannot call link.

arturoc commented 8 years ago

you can use to_element() on any element to create a copy that will be ref'd. something like:

let mut element1 = gst::Element::new("somelement","").unwrap();
let mut element2 = gst::Element::new("otherlement","").unwrap();
let mut pipeline = gst::Pipeline::new("pipeline").unwrap();
pipeline.add(element1.to_element());
pipeline.add(element2.to_element());
element1.link(&mut element2);

there could probably be a different method for this but i'm not sure on what's the best option, if using clone which would clone the object and use gst_object_ref under the hood or something like ref() to use a syntax more similar to gstreamer's.

Yoric commented 8 years ago

@arturoc, is there any reason to not do the to_element() under the hood inside add?

arturoc commented 8 years ago

the gstreamer docs say:

Adds the given element to the bin. Sets the element's parent, and thus takes ownership of the element. An element can only be added to one bin.

i think making the add method move the element instead of borrowing it is more coherent with taking ownership of the element

Yoric commented 8 years ago

That really look like two different notions of ownership. If you really want to encode having a parent (or not) as part of the type system, you might wish to write something along the lines of

pub trait NeedsParent;

pub struct HasParent;
impl NeedsParent for HasParent;

pub struct NoParent;
impl NeedsParent for HasParent;

pub trait Element<T> where T: NeedsParent {
  ..
}

impl Bin {
  pub fn add(&mut self, item: Element<NoParent>) -> Result<Element<HasParent>, Element<NoParent>> {
   ...
  }
}

But that looks like overkill. Also, I haven't checked, but what's the behavior if an element is added to two bins? Does the second add steal parentship or does it fail?

arturoc commented 8 years ago

i've added a decodebin example showing how to do this + some things that were missing that are useful to create a pipeline element by element, like being able to connect to signals...

in general i think using move for things that take ownership seems a good pattern using borrow for this api seems counter intuitive to me.

not sure what happens if you add an object to 2 bins i guess it'll fail mostly if the first pipeline is in play state

Yoric commented 8 years ago

I don't quite understand how using move is a good pattern here, since we pretty much always need to reuse the element afterwards. Can you elaborate?

arturoc commented 8 years ago

I've done some changes related to this, there's now a gst::Ref that can be created to get a reference to an object, that way the original when moved into a bin can't be used any more but you can still hold a Ref to use the object later. Ref acts as a pointer object so you can call any of the original object methods on it directly.

There's also some utility methods in Bin, add_and_link and add_and_link_many that allow to add and link at once which avoids having to use Ref at all in this particular case.

So now you can do:

bin.add_and_link_many(vec![filesrc, decodebin, sink]);

and

let decodebin_ref = gst::Ref::new(&decodebin);
bin.add_and_link(filesrc, decodebin):
decodebin_ref.get::<i32>("some-property");

In general my idea about with this wrapper is to use rust ownership model to represent the ownership of the gstreamer objects. In c there's no such thing but having rust a really well defined ownership semantics i think it makes a lot of sense to use them to represent the equivalent gstreamer ownership.