Created attachment 73149 [details] [review]
New element.

Looks pretty good.

As noted on irc, for the record:

 - please clarify licensing of the analysis code with the original authors.
 - we prefer to use gbooleans rather than 1-bit bitfields, and you should use logical operations (||) rather than bitwise ones (|) on them.

Fix up those and we can throw it in -bad. Also, good work on comprehensive tests!

Sent a polite mail to original author requesting verification of licensing.  Will provide the result of the correspondence and updated patch when I receive an affirmative answer.

Thanks for reviewing this.

In the light of a new day, I see things a bit clearer.  Aside from changing the bitfields to gboolean, I have to dismiss all you claims:

The ||= operator you mentioned on IRC does not seem to exist, at least my compiler does not seem to know about it.  Rewriting the part that uses |= to use || would create more problems than it solves I think.

About the licensing: I changed the comment about the origin to explicitly state that it is originally based on gain_analysis.c from vorbisgain version 0.34.  This file states that it is under LGPL (like vorbisgain itself).

Yes, there are other programs that incorporate derivations of that file (mp3gain, wavegain, *whatever*gain), and yes, some of these programs are GPL instead of LGPL.  Your concern seems to stem from this fact.  Yes, it would be illegal to distribute this file as LGPL if it was GPL originally (without permission from the original authors).  One can turn any piece of LGPL code into GPL but not the other way around (except for the copyright holders of course).

But there is no indication that this was done.  All instances of the file state that they are LGPL.  Even if there *was* a variant of the file somewhere that got turned into GPL (which anybody can do legally), there would be no doubt since there is at least one instance with LGPL.  If we cannot trust the copyright/license header in this regard, well, we cannot trust anything.  By the same paranoid logic, the email address could be fake and the person answering my confirmation could just pretend to be the copyright holder.

The truth is most probably that there is no such conspiracy going on and that the code is simply LGPL, as is correctly stated _everywhere_.  Glen Sawyer probably thinks I'm a complete moron for making him confirm the obvious.

I hope you agree and the slightly revised variant I will provide can go in (and will end up in -good and not -ugly in the future).  Otherwise, please point out the flaw in my argumentation or reformulate your concern.  Thank you.

Created attachment 73756 [details] [review]
Revised patch

Turn the guint bitfields into full-width gbooleans.  Turn uses of g_assert into g_return_if_fail.  Be more explicit about the origin of the analysis code (state a single program name, version and filename).

One assertion in comment #4 is wrong: vorbisgain is not LGPL, they apparently have the wrong COPYING in their tarball.  On Ubuntu Dapper, a file /usr/share/doc/vorbisgain/copyright is installed alongside vorbisgain which ends with the following lines:

[...]
> vorbisgain itself is under the GNU General Public License, version 2.
> The complete text of the GNU General Public License can be found under
> /usr/share/common-licenses/GPL-2 on most Debian systems.
>
> The implementation of the ReplayGain algorithm is under the GNU Lesser
> General Public License, version 2.1.  The complete text of the LGPL
> v2.1 can be found under /usr/share/doc/common-licenses/LGPL-2.1 on
> most Debian systems.

The revised variant of my patch explicitly states that I only used parts of the algorithm (file "gain_analysis.c"), which clearly is under LGPL.

While there most probably have been changes to the implementation of the algorithm in a GPL context, no one took the opportunity to change the license of the file from LGPL to GPL (which the LGPL permits anybody to do).  I have therefore no doubt that the file is LGPL just like is stated everywhere.

I refresh my urge to either rephrase any remaining concerns or point out flaws in my argumentation.

This all looks fine now.

It's ready to go into -bad as soon  as someone is motivated enough/has time to actually commit it, I think. 

Committed, thanks a lot!

 2006-10-06  Tim-Philipp Müller  <tim at centricular dot net>

	Patch by: René Stadler  <mail at renestadler de>

	* configure.ac:
	* docs/plugins/Makefile.am:
	* docs/plugins/gst-plugins-bad-plugins-docs.sgml:
	* docs/plugins/gst-plugins-bad-plugins-sections.txt:
	* gst/replaygain/Makefile.am:
	* gst/replaygain/gstrganalysis.c: (gst_rg_analysis_base_init),
	(gst_rg_analysis_class_init), (gst_rg_analysis_init),
	(gst_rg_analysis_set_property), (gst_rg_analysis_get_property),
	(gst_rg_analysis_start), (gst_rg_analysis_set_caps),
	(gst_rg_analysis_transform_ip), (gst_rg_analysis_event),
	(gst_rg_analysis_stop), (gst_rg_analysis_handle_tags),
	(gst_rg_analysis_handle_eos), (gst_rg_analysis_track_result),
	(gst_rg_analysis_album_result), (plugin_init):
	* gst/replaygain/gstrganalysis.h:
	* gst/replaygain/rganalysis.c: (yule_filter), (butter_filter),
	(apply_filters), (reset_filters), (accumulator_add),
	(accumulator_clear), (accumulator_result), (rg_analysis_new),
	(rg_analysis_set_sample_rate), (rg_analysis_destroy),
	(rg_analysis_analyze_mono_float),
	(rg_analysis_analyze_stereo_float),
	(rg_analysis_analyze_mono_int16),
	(rg_analysis_analyze_stereo_int16), (rg_analysis_analyze),
	(rg_analysis_track_result), (rg_analysis_album_result),
	(rg_analysis_reset_album), (rg_analysis_reset):
	* gst/replaygain/rganalysis.h:
	  Add ReplayGain analysis element (#357069).

	* tests/check/Makefile.am:
	* tests/check/elements/.cvsignore:
	* tests/check/elements/rganalysis.c: (get_expected_gain),
	(setup_rganalysis), (cleanup_rganalysis), (set_playing_state),
	(send_eos_event), (send_tag_event), (poll_eos), (poll_tags),
	(fail_unless_track_gain), (fail_unless_track_peak),
	(fail_unless_album_gain), (fail_unless_album_peak),
	(fail_if_track_tags), (fail_if_album_tags),
	(fail_unless_num_tracks), (test_buffer_const_float_mono),
	(test_buffer_const_float_stereo), (test_buffer_const_int16_mono),
	(test_buffer_const_int16_stereo), (test_buffer_square_float_mono),
	(test_buffer_square_float_stereo), (test_buffer_square_int16_mono),
	(test_buffer_square_int16_stereo), (push_buffer), (GST_START_TEST),
	(rganalysis_suite), (main):
	  Unit tests for the new replaygain element.