GNOME Bugzilla – Bug 349207
[PLUGIN-ADD] audiopanorama
Last modified: 2006-08-20 13:10:47 UTC
I'd like to integrate this plugin to gst-plugins-good/gst http://buzztard.cvs.sourceforge.net/buzztard/gst-buzztard/src/audiopanorama/ any supporters?
I suggest at least a comprehensive patch against -good including everything required (plugin + documentation + test or unit test). Just committing stuff to -bad and getting it ready there might not be a bad idea either though IMHO (since then people already have it and we know everything is there and works fine and it just needs review ...)
Created attachment 69925 [details] [review] patch for new plugin
Unit test? :)
not yet :( try to do one tonight.
Created attachment 70263 [details] [review] patch for new plugin now with unit tests
Right, so let's move this on a bit: First the nitpicking (minor things only): - Makefile.am needs GST_CONTROLLER_CFLAGS and more importantly GST_CONTROLLER_LIBS (or whatever they are called) - in gst_audio_panorama_get_unit_size() the g_return_is_fail() is unnecessary IMHO, I'd either make it an assert or remove it completely, same for the 'ret' check there - gst_audio_panorama_set_caps: should probably set process function to NULL on error true (could probably be written easier to just return (filter->process_func != NULL) at the end. Also, goto label 'Error' should not be capitalised. I can't comment on the algorithm, but that's not really required anyway. Everything looks good to me otherwise: - unit test is there, - base class is used, - documentation is provided - you are willing to maintain it - no license issues - I've looked over the code and it's fine and conforms to our style so all the formal requirements seem to have been met. One other thing I was wondering is whether the plugin should really be called 'audiopanorama' or if we might want to plan a bit ahead and anticipate the addition of other 500-lines-of-code simple audio effects elements, which surely don't need to get their own plugin/.so. Dunno, just a thought. Oh, and float support would be nice too at some point ;)
> find . -name "*.c" -exec grep -l "g_return_val_if_fail (size, FALSE);" {} \; ./gst-plugins-base/gst/audioresample/gstaudioresample.c ./gst-plugins-base/gst/audioconvert/gstaudioconvert.c ./gst-plugins-base/gst/ffmpegcolorspace/gstffmpegcolorspace.c ./gst-plugins-base/gst/videoscale/gstvideoscale.c ./gst-plugins-good/ext/gdk_pixbuf/pixbufscale.c ./gst-plugins-good/gst/videobox/gstvideobox.c The g_return_if_fail() checks the given pointer location for the size result. I wouldn't remove it but an assert() should be fine. Its kind of stupid to call this with size=NULL, right?
commited to cvs