After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 349207 - [PLUGIN-ADD] audiopanorama
[PLUGIN-ADD] audiopanorama
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 0.10.5
Assigned To: Stefan Sauer (gstreamer, gtkdoc dev)
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2006-07-29 18:32 UTC by Stefan Sauer (gstreamer, gtkdoc dev)
Modified: 2006-08-20 13:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch for new plugin (17.90 KB, patch)
2006-07-30 21:11 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
patch for new plugin (33.76 KB, patch)
2006-08-05 15:30 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review

Description Stefan Sauer (gstreamer, gtkdoc dev) 2006-07-29 18:32:50 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?
Comment 1 Tim-Philipp Müller 2006-07-30 14:53:41 UTC
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 ...)
Comment 2 Stefan Sauer (gstreamer, gtkdoc dev) 2006-07-30 21:11:48 UTC
Created attachment 69925 [details] [review]
patch for new plugin
Comment 3 Tim-Philipp Müller 2006-08-01 07:13:55 UTC
Unit test? :)
Comment 4 Stefan Sauer (gstreamer, gtkdoc dev) 2006-08-01 08:03:56 UTC
not yet :( try to do one tonight.
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2006-08-05 15:30:51 UTC
Created attachment 70263 [details] [review]
patch for new plugin

now with unit tests
Comment 6 Tim-Philipp Müller 2006-08-18 09:55:14 UTC
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 ;)

Comment 7 Stefan Sauer (gstreamer, gtkdoc dev) 2006-08-20 11:16:55 UTC
> 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?
Comment 8 Stefan Sauer (gstreamer, gtkdoc dev) 2006-08-20 13:10:47 UTC
commited to cvs