Re: [Bug-gnupod] Patch to support ReplayGain / mp3gain

[H. Langos]

Hi Richard,

On Fri, Apr 17, 2009 at 11:02:22PM +0200, Richard van den Berg wrote:
> I'm successfully using gnupod on my iPod video 30GB. Thanks to everyone
> who contributed to this great tool!
>
> I have all my mp3s tagged for ReplayGain volume leveling using mp3gain.
> This tool adds ReplayGain information using APE tags. Making gnupod read
> these tags and apply them as SoundCheck values was surprisingly easy.
> Boy, I love open source software. :-)

Thank you very much for your feedback and your contribution.

> There might be a side effect if you have APE tags that also contain
> (wrong) title and artist information. I don't have such files to test
> with, but implementing a --disable-ape switch might be a good idea. Let
> me know if you need me to provide a patch for that as well.

This would indeed be very helpful.
Until now gnupod didn't pay any attention to APE tags in mp3 files.
Therefore I am reluctant to make the new behavior a default and only
have a "noAPE" option.

Usually it would be a clear case against the new default behaviour.
On the other hand a "--noAPE" or "--noAPEtag" would fit in better with the
current set of options. Also the next release of gnupod will probably
contain some new tools and we might get away with changing some aspects of
the existing tools in the "dust cloud generated by the rush on those glorious
new tools" (I wrote them, so I can be ironic about them without stepping on
anybody's toes :-) )

> This patch is against the current CVS version.

Thanks! That makes things easier. I personally use git but I keep it in sync
with the official CVS. Could you use unified patch format (diff -u) ?
I find that more readable as the changes are closer together in the
diff output.

> It will prefer ReplayGain
> APE/id3v2 tags over iTunNORM id3v2 comments. The ReplayGain algorithm is
> far superior over the peak based SoundCheck algorithm iTunes implements,
> so it wouldn't make sense the other way around.

But in the end they both produce one integer number that is applied as a
simple scaling factor, right? So the "superiority" of the the ReplayGain
algorithm is something that depends on the software used to produces those
tags. Therefore I would like to give the user some control over this. One
(crude) way of doing it, is the --noAPEtag option.

I think it should be "noAPEtag" instead of "noAPE" as the program that takes
this option (from the users perspective) is gnupod_addsong and "noAPE" might
suggest that APE format files are to be excluded.

Here are some suggestions for your patch in detail:

- Rename "convertReplayGainToSoundCheck" to "_parse_ReplayGain".

- Make the sub first check if the argument is undef. That saves you the
  'if($hs->{REPLAYGAIN_TRACK_GAIN} ne "")' line which probably is not what you
  want to check for in the first place.

- Make the sub return undef if the argument was undef or if the argument could
  not be parsed.

- Change the call to something like this: (I'm improvising here so don't
  use it as it is...)

  $soundcheck = ( _parse_ReplayGain($hs->{REPLAYGAIN_TRACK_GAIN}) || 
_parse_iTunNORM($hs->{COMM} || $hs->{COM} || $h->{COMMENT}) )

  This will probably fail if _parse_ReplayGain() ever returns a valid value
  of 0. So you might have to write a more elaborate version like this:

  $soundcheck = _parse_iTunNORM($hs->{COMM} || $hs->{COM} || $h->{COMMENT})  
unless defined ($soundcheck = _parse_ReplayGain($hs->{REPLAYGAIN_TRACK_GAIN})) ;

  But taking a second look at that sub, it probably never does produce 0. So 
  the first and easier to understand version should be ok.

- Some words of documentation in pod format would be great. (Just
  cut'n'paste'n'edit and try "perldoc src/ext/FileMagic.pm" to see the result)

- I like the round() sub. Elegant usage of "<=>" operator. (I never got the
  hang of it). However, a sub that is only called once, could probably be
  left out. If you want to maintain readability, you might put the round 
  operation in a separate asignment. Even if you put it on one line, the
  usage of $number in the <=> operation can be replaced by "-$gain" or 
  "$gain", as it only depends on the sign, right?

cheers
-henrik

PS: Please don't take the number of suggestions as an indication of poor
quality. Quite the opposite is true. You might well know more perl than I do.
If the patch was poor I would have probably just fixed those things myself. 
Instead I hope you take it as motivation to continue work on gnupod. We could 
do with some fresh blood. :-)