[gstreamer-bugs] [Bug 631574] Add GstFilters library to gst-plugins-base

Mon Dec 20 17:16:11 PST 2010

https://bugzilla.gnome.org/show_bug.cgi?id=631574
  GStreamer | gst-plugins-base | git

--- Comment #9 from Youness Alaoui <youness.alaoui at collabora.co.uk> 2010-12-21 01:16:06 UTC ---
(In reply to comment #7)
> Just some high-level comments from looking at the docs
> 
> - Why do you have all these volume/audioconvert/etc filters? Isn't this
> essentially the same as a GstBinFilter with the corresponding description? I'm
> not sure that these are really needed... of course they don't hurt either :)
There are a few reasons :
1 - With the filters, you should forget about the elements, the idea is to
simplify it by providing a small set of specific filters providing specific
'features', instead of a huge list of elements that may or may not be
installed, etc.. It should be 'a filter for everything' and GstBinFilter is
only there for 'super advanced users' and should not really be used by anyone
2 - Most people see 'audioconvert' element and they put that in their pipeline
to convert their audio format.. then later on, they find bugs and realize they
needed to add an audioresample element too.. then they still get some bugs and
they don't understand why, only a few people know (and understand why) that you
actually need "audioconvert ! audioresample ! audioconvert" to properly convert
your audio format. The idea of the filters is to make everything easy and these
kind of low-level specific things are hidden by the 'audioconverter' filter,
you don't need to be a gstreamer expert in order to properly convert your audio
formats anymore
3 - In the case of the volume filter, you have one filter with one volume and
you can apply it everywhere, if in the UI, the user selects a volume for that
app, then you don't need to keep a list of volume elements, and set the right
volume in each one of them, the volume filter does that for you. In the case of
VoIP, the UI user only sees one 'volume slider', so the developer should only
set the value on only one 'volume filter', and not on every src-pad (from
farsight for example) per stream-ssrc-payload_type...
4 - GstBinFilter was added 3 weeks ago, but all these filters were creates
months ago. But still, I don't think GstBinFilter should be used for things
like that.
5- having them doesn't hurt :)

> - GstResolutionFilter should also have something to specify the PAR/DAR or to
> force keeping the DAR and things like that. And the name could be improved I
> guess... but I don't know a good name
That's a good point, The aspect ratio should be configurable, I'm not sure if
it needs to be anything fancy, maybe just 'stretch/keep_aspect_ratio' is
enough.
About the name, I don't know, maybe GstVideoSizeFilter ?

> - gst_filter_lock/unlock: Everywhere else in GStreamer we use macros for these
> things, e.g. GST_OBJECT_LOCK(). Would be nice to change that for consistency
Good point, I'll fix that.

> - GstFilter::handle_message(): please add to the documentation *which* messages
> will be passed to this. Is this only for gst_filter_manager_handle_message()?
> If it is, what exactly is the use case for this? An example in the
> documentation would help I guess :)

Well, the documentation says "Try to handle a message originally received on
the GstBus to the filter." and really, it's *ANY* and *ALL* messages that goes
through the bus.. the filter will return TRUE/FALSE depending on whether or not
it recognizes the message. Most filters don't need it and will always return
FALSE, others will look for a specific message (like the 'level' filter that
takes 'level' messages and transforms them into gobject signals) but you can't
know which message since it's filter-dependent.
If you use a GstFilterManager, then the gst_filter_manager_handle_message will
call that function for you (for all the filters in the manager), but if you use
the filter on its own without a filter manager, then you'd need to call that.
The examples/filters/test-level.c shows how it's used (through the filter
manager) :
http://git.collabora.co.uk/?p=user/kakaroto/gst-plugins-base.git;a=blob;f=tests/examples/filters/test-level.c;h=b600b68ee303e062e3cdb58371c3a441e9428557;hb=03fba0b1acd6cee1d9117100db4c05051c6d5e02#l47

Does that answer your question or did I misunderstand it ?

> - It might make sense to use GstObject as base class for GstFilter and
> GstFilterManager. This way you will get a) the mutex, b) parent/childs
> handling, c) floating refs
No, I didn't want to do that for multiple reasons, first of all, the purpose of
all this is mainly : "Make it easy".
Gstreamer is scary to a lot of people who have never used it, so I wanted to
keep this into the realm of what everybody knows by using GObject instead of
GstObject. I absolutely do not want floating refs, although I completely agree
that they are extremely useful and practical, I think that they add a certain
level of complexity to the refcounting system that could easily scare/confuse
people. Also there is no parent/child system here, a filter can be added to as
many filter managers as you want, so it's more of a "contains" rather than "is
a parent of". Finally, adding a mutex is easy, so it doesn't really warrant a
move to GstObject.
For all these reasons, I prefer it to be GObject as a base rather than
GstObject, I don't see any particularly important advantage in using GstObject
apart from the obvious "This is part of GStreamer so it has to be a GstObject"
argument.
What do you think ?

p.s: by the way, GObject also has support for floating refs.

> - You should add a note to the documentation that dynamic pipeline changes are
> not handled in all cases yet
What do you mean? what cases aren't handled for dynamic pipeline changes? It's
been a while so maybe it's just that my memory isn't fresh, but as far as I
remember, there were no issues with dynamic pipeline changes and all cases were
handled correctly.
The only thing I can see from this bug report's comments is about changing the
source/sink elements, but wouldn't changing the source only affect the sink
synchronization? the filters in between shouldn't really be affected since they
only transform data and it's rare for a filter (non-src/non-sink element) to
care/modify the timestamps ?
The only filter that has a sink in it is the Preview filter and that one uses
sync=false so it shouldn't bother it either.

> - gst_filter_add_standard_element() is badly named. Elements with one pad named
> "sink" and one pad named "src" are not really "standard elements". More
> something like filter elements, one-to-one elements, etc.
hehe, well, all the elements that get added are basically filters with
one-to-one pads, but since this is to be used internally for a filter, it
doesn't really make sense to call it gst_filter_add_filter_element. I know
there isn't really a 'standard' but it's become a 'kind of standard' for people
to name their pads 'src' and 'sink'.. I'm sure if you see an element with a
sink pad named "my_sink_pad", you would frown during code review and tell the
committer to name it correctly.
If you have a better suggestion for the function name, let me know though, I
don't mind changing it.

> - Is gst_filter_revert_bin() possible on a filter added by
> gst_filter_add_element_by_name()? If not, would such a feature be possible?
Good point.. I just checked the code and noticed that the
gst_filter_revert_standard_element and gst_filter_revert_bin use the exact same
code (apart from the gst_filter_revert_standard_element also unreffing/removing
from the 'elements' GList), so I should probably make the revert_bin just call
revert_standard_element with the argument GList *elements=NULL.
About the 'add_element_by_name', it's mostly used to specify an element that
uses pads named differently from 'src/sink', but I guess it would take 5
minutes to write a 'revert' that takes src_pad_name and sink_pad_name arguments
instead of hardcoding "src" and "sink.
Then I could just make the gst_filter_revert_standard_element use that directly
giving it "src" and "sink" arguments for the names.

> - GstMultiFilterManager says that you must not do anything with it other than
> listening to signals. Is this really true? You can't call any of the
> GstFilterManager interface functions?
Euhh, no, I think you were mislead by the big green 'Note' in the gtk-doc. The
GstMultiFilterManager can be used normally using the same GstFilterManager
interface functions.. what you can't play with is the GstSingleFilterManager
that is used internally by the MultiFM. The "you must not do anything with it
other than listening to signals" is actually a note on the 'applied' signal of
the Multi filter manager.
Whenever you 'apply' a MultiFM to a bin+pad, it will create a new SingleFM
internally, and append all the filters to it and then 'apply' that SingleFM on
that bin+pad. It will make sure all internal SingleFM are synchronized, so if
you move/replace/add filters to the MultiFM, all the internal SingleFMs get
modified the same way, that's why if you start playing with the internal
SingleFM, it will screw it all up.
The SingleFM is accessible because when you apply the MultiFM it will send a
'applied' signal giving you the internal SingleFM it created for this bin+pad :
http://people.collabora.co.uk/~kakaroto/gpb-docs/GstMultiFilterManager.html#GstMultiFilterManager-applied
If for example, you later want to add a new filter to your MultiFM, it will get
added in all the internal SingleFMs, and maybe it will fail to apply in one of
them, that's why you may want to know about this by listening to signals on the
internal SingleFMs.

(In reply to comment #8)
> (In reply to comment #5)
> > How would you suggest telling the elements in the filter manager about the
> > changed source/sink ? The filters (and filter manager) have a _handle_message
> > API method, so maybe sending some kind of message through that ?
> 
> I think the elements in the filter/filter manager shouldn't even notice that
> the source/sink was replaced, other than maybe seeing a newsegment event. But
> now that I looked at the docs of GstFilter it seems that this is only meant for
> elements between the source and sink and not for the sinks/sources themselves.
> In that case ignore it for now :)
> 
Yes, the filters are 'filters' in the gstreamer sense, one input = one output,
so you can't really use a GstFilter as a source/sink (but a filter could have
an internal src/sink like the Preview filter)

> What remains is that you need to tell newly added elements to a running
> pipeline about the current running time, otherwise you're going to break QoS
> and synchronization in some cases.

Humm, the whole timing/synchronization/segment/latency/QoS stuff in gstreamer
is my weak point, I'm having a bit of a hard time getting my mind around it.
I'm a bit clueless as to why this is needed, or how to do it properly, could
you enlighten me?
If you have a solution that I can just apply, I can fix it, but figuring out
what needs to be done is giving me a headache...

Thanks for taking the time to look at this :)

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.