GNOME Bugzilla – Bug 394859
[audiopanorama] New simple method for adjusting the panorama
Last modified: 2007-01-16 08:29:13 UTC
Hi, the attached patches add the audiobalance element to the audiofx plugin, add a unit test and add the docs to the docs build system. The element and unit test is mostly a copy of audiopanorama with the only changes in the processing functions. Maybe this can be generalized into a new base class at some point. I hope it's not necessary to get this through plugins-bad before as it's just an audiofx addition :/ The difference between audiopanorama and audiobalance is, that audiopanorama moves the sound in the stereo panorama based and psychoacoutics while audiobalance only changes the volume of each channel. For some use cases the former is useful, for other's the latter and a balance element was requested more than once on IRC. Bye
Created attachment 79905 [details] [review] audiobalance-sources.diff
Created attachment 79906 [details] [review] audiobalance-tests.diff
Created attachment 79907 [details] [review] audiobalance-docs.diff
Created attachment 79922 [details] [review] audiobalance-makefile.diff I apperantly forgot to add the Makefile.am changes in the first patch... here they are ;)
> The element and unit test is mostly a copy of audiopanorama with the only > changes in the processing functions. Maybe this can be generalized into a new > base class at some point. Any reason not to make audiopanorama derive from audiobalance or vice versa then? Seems a bit unnecessary to duplicate all this code if only the processing funcs are different. > I hope it's not necessary to get this through plugins-bad before as it's just > an audiofx addition :/ Don't think this is a problem.
After talking with Stefan about it yesterday and some further thoughts I guess the best would be to implement it in the audiopanorama element and add a property to switch between both methods... obviously having the current one as default. This probably makes the most sense as the differences between the output of both methods are not that large and there's no clear definition of what pan is and what balance. The property should probably be called "method" with the values "psychoacoustic" and "naive" with the former as default. I'll attach a patch for this later today if there are no objections...
Created attachment 80058 [details] [review] audiopanorama-simple.diff
Created attachment 80059 [details] [review] audiopanorama-tests.diff
Here are the two patches, one adds the simple method to audiopanorama, the other adds unit tests for this method. Changing the method while processing is possible.
Created attachment 80171 [details] [review] audiopanora-simple.diff Not very useful to declare the method property as GST_CONTROLLABLE...
Thanks, committed: 2007-01-13 Tim-Philipp Müller <tim at centricular dot net> Patch by: Sebastian Dröge <slomo circular-chaos org> * gst/audiofx/audiopanorama.c: (gst_audio_panorama_method_get_type), (gst_audio_panorama_class_init), (gst_audio_panorama_init), (gst_audio_panorama_set_process_function), (gst_audio_panorama_set_property), (gst_audio_panorama_get_property), (gst_audio_panorama_set_caps), (gst_audio_panorama_transform_m2s_int_simple), (gst_audio_panorama_transform_s2s_int_simple), (gst_audio_panorama_transform_m2s_float_simple), (gst_audio_panorama_transform_s2s_float_simple): * gst/audiofx/audiopanorama.h: Add 'method' property and provide a simple (non-psychoacustic) processing method (#394859). * tests/check/elements/audiopanorama.c: (GST_START_TEST), (panorama_suite): Tests for new method.
* I would have used switch-case instead of if (filter->method == METHOD_SIMPLE), that helps future changes. * if you have pointers (URLs) to the differences of the algorithms, lets put them into the docs. * it wold also be good if we could give guidance when to use which method (is one faster that the other or is it purely matter of taste)
Created attachment 80297 [details] [review] audiopanorama-switch.diff patch for 1)
3) The simple method is probably marginally faster but IMHO not enough that someone can notice it... it's probably only a matter of taste and what your understanding of panning/balance is. 2) I don't have any concrete docs about it, only random webpages... some saying that panning/balance is what the old method does, some saying that it is what the simple method does in a very non-formal way... nothing worth quoting, sorry :/
Since we're talking about minute style details in gst_audio_panorama_set_process_function(): you could just as well use a function array, which would be considerably shorter ;)
Right... I'll do it tonight :P And I'll add to the docs that it's mostly a matter of taste which method one should use and that one should just compare both for their usage
Created attachment 80327 [details] [review] /audiopanorama-funcarray.diff here is it ;)
Thanks for the patch. Applied. 2007-01-16 Stefan Kost <ensonic@users.sf.net> Patch by: Sebastian Dröge <slomo circular-chaos org> * gst/audiofx/audiopanorama.c: (gst_audio_panorama_class_init), (gst_audio_panorama_set_process_function): Use a function array for process methods, add more docs and define the startindex of enums.