GNOME Bugzilla – Bug 467666
[ELEMENT-MOVE] Move lpwsinc and bpwsinc to gst-plugins-good
Last modified: 2008-02-07 22:01:13 UTC
Hi, it would be nice if the lpwsinc and bpwsinc element from the filter plugin in gst-plugins-bad are moved to the audiofx plugin in gst-plugins-bad. The reasons for only moving the elements and not the plugin are these two: a) It would be a bit insane to have two plugins containing audio filters in gst-plugins-good. One with more elements makes much more sense. b) The remaining element from the filter plugin, iir, is completely broken (simply segfaults for me) and it's IMHO not very useful at all, even if it works. There seems to be no interest in fixing the element and nobody seems to care about it at all. When moving the elements we should rename them to something more consistent IMHO. All audiofx plugins have a prefix "audio" and lpwsinc is not only a lowpass filter but also a highpass filter, same goes for bpwsinc which is not only a bandpass filter but also a bandreject filter. The currently only lowpass/highpass filter in audiofx is called audiochebyshevfreqlimit, the bandpass/bandreject filter audiochebyshevfreqband so I would propose a renaming to audiowsincfreqlimit and audiowsincfreqband.
+1 from my side
when moving those, it would be good if the doc-blurb could give some info about the differences between chebyshev and sinc, when to use which, pros and cons.
Makes sense, I'll add this later to the current docs already.
OK, I've added some docs about this to all 4 elements.
> b) The remaining element from the filter plugin, iir, is completely broken > (simply segfaults for me) and it's IMHO not very useful at all, even if it > works. There seems to be no interest in fixing the element and nobody seems to > care about it at all. Not that it really matters for the move (if we don't guarantee stability or API stability for elements in -bad in any way, I don't see why we can't rename plugins, rename elements, move elements around or even move only some elements to -good and leave some in -bad), or that I care about this element at all, but: someone did care about it enough to port it to GStreamer-0.10 at some point, and probably used it for something, so it seems a bit bold to me to claim that no one cares about it (it doesn't seem to crash for me, FWIW; whether it's useful or not, I don't know).
If you use more than one stage it will crash. You can only provide one a and one b coefficient in the element, while the stuff in iir.c wants an array with stages elements. Whatever, for filter testing it might be useful (if it is fixed) :) I might take a look at fixing it in the next days, there's much to do apart from the stuff mentioned above though and I would definitely prefer to have lpwsinc/bpwsinc moved to good/audiofx nonetheless. Sorry if I insulted someone with the statement that nobody cares about it, I just got this impression while looking at the sources. But you're right, someone cared enough to port it to 0.10.
I think the audiochebyshevfreqlimit is overly long as a name and was a mistake. I definately don't want to see these other two elements mangled to have such a long name.
> I think the audiochebyshevfreqlimit is overly long as a name and was a mistake. There hasn't been a -good release since that was added, so it could still be changed.
Well, we found no better name. Suggestions are welcome though. audio : because there could be one for video too chebyshev: because thats the algorithm used freqlimit: because its lowpass/highpass we could drop 'freq' as that is what audio filter usualy process we could abbreviate 'chebychev' to 'cheb' audiochebyshevfreqlimit -> audiocheblimit similar change shold be made to audiochebyshevfreqband of course
I would like to have them renamed too, at least if a better name is found that still doesn't drop any aspects. audiocheblimit and audiochebband sounds better imho, does anybody else have suggestions? Whatever, lpwsinc and bpwsinc should nonetheless be renamed into something better as those names only reflect one mode they can work in. What about audiowsinclimit and audiowsincband then?
Yes lets also rename the lpwsinc and bpwsinc as you suggested. @Thomas: are the names okay for you?
sure, sounds fine. Stefan, there are chebychev filters for video ?
Thomas, sure, the lowpass one can be used as a noisefilter too. Just two random google results: http://www.edacafe.com/books/phdThesis/Chapter-2.7.php http://www.edn.com/article/CA179572.html
So everybody is fine with renaming stuff: lpwsinc -> audiowsinclimit bpwsinc -> audiowsincband audiochebyshevfreqlimit -> audiocheblimit audiochebyshevfreqband -> audiochebband ??
One thing I've noticed, that I've noticed with other elements, is that the docs for the custom enum properties are really bad: The "window" property "window" GstLPWSincWindow : Read / Write Window function to use. Default value: Hamming window (default) The don't give any idea about the other possible values, like gst-inspect does. The description for lpwsinc should probably change: "Windows Sinc low pass and high pass filter" -> "Windowing Sinc low pass and high pass filter" Otherwise, docs, tests & code look good +1 for move
The description of lpwsinc is fixed in CVS now.
Elements transplanted to good: 2008-02-07 Jan Schmidt <jan.schmidt@sun.com> * docs/plugins/Makefile.am: * docs/plugins/gst-plugins-good-plugins-docs.sgml: * docs/plugins/gst-plugins-good-plugins-sections.txt: * docs/plugins/gst-plugins-good-plugins.args: * docs/plugins/inspect/plugin-audiofx.xml: * gst/audiofx/Makefile.am: * gst/audiofx/audiofx.c: * gst/audiofx/audiowsincband.c: * gst/audiofx/audiowsincband.h: * gst/audiofx/audiowsinclimit.c: * gst/audiofx/audiowsinclimit.h: * tests/check/Makefile.am: * tests/check/elements/audiowsincband.c: * tests/check/elements/audiowsinclimit.c: Move the lpwsinc and bpwsinc elements from gst-plugins-bad into the audiofx plugin, and rename to audiowsinclimit and audiowsincband respectively. Fixes: #467666