Re: [Bug-gnupod] Patch to support ReplayGain / mp3gain
On Sun, May 24, 2009 at 10:22:17PM +0200, Richard van den Berg wrote: On 5/24/09 2:33 AM, H. Langos wrote: One thing to keep in mind is that the --min-vol-adj and --max-vol-adj now only affect the RVA/RVAD tag information instead of something extracted from RVA2 tags. (I need to update the help text about that.) Since RVA2 now changes the SoundCheck field in the iTunesDB it can still be ignored by disabling Sound Check on the iPod. Ok. I didn't know that. I'll leave it as it is then and the final thing I'll do about this topic will be changing the output format for the soundcheck field. I'll make gnupod_find report the adjustment in dB instead of the raw integer value. This should also help spotting bugs in that area. cheers -henrik ___ Bug-gnupod mailing list Bug-gnupod@nongnu.org http://lists.nongnu.org/mailman/listinfo/bug-gnupod
Re: [Bug-gnupod] Patch to support ReplayGain / mp3gain
On Sat, May 09, 2009 at 08:34:57PM +0200, H. Langos wrote: Yes. We would leave volume alone and go for soundcheck instead. We would stop using that crude one byte and start using that 32bit dBm value. At least thats what it is supposed to encode. I just greped through my mp3s and I have only found positive values ranging from 21 to 4219 (= 4.219 dB ) Sorry. This conversion was total BS. soundcheck 4219 = -6.252 dB soundcheck 21 = +16.7 dB I would suggest that you take some stuff that is too loud i.e. louder than the magic 89dB, feed it to iTunes and see how iTunes encodes negative adjustments. I shouldn't write email after spending the whole day sitting in the sun.. ;-) cheers -henrik ___ Bug-gnupod mailing list Bug-gnupod@nongnu.org http://lists.nongnu.org/mailman/listinfo/bug-gnupod
Re: [Bug-gnupod] Patch to support ReplayGain / mp3gain
Hi Richard,
On Sun, May 10, 2009 at 10:53:42PM +0200, H. Langos wrote:
On Sun, May 10, 2009 at 10:35:57AM +0200, Richard van den Berg wrote:
Do you want to wait until we have a all of the above working before you
commit my current ReplayGain patches to CVS?
I hope I get around to commit those changes tomorrow.
I made a couple of changes before commiting your patches.
First of all I changes the option --no-ape to --no-ape-tag. .ape is also
an audio file extention and naming the option only --no-ape could suggest
that it is about the file format instead of the tag.
NOT: This is only a change in the user interface. Internally the hash key is
still called noAPE.
Then I changes the regex in _parse_ReplayGain as already discussed:
|return undef unless defined($gain);
|if($gain =~ /(.*?)\s*dB$/) {
I also changed a regex further down:
|return undef unless ($gain =~ /^\s*-?\d+(\.\d+)?\s*$/);
This is a little more strict as it will not allow stuff like -4. or
15.. I.e. if there is a dot, it insists on finding at least one digit
after the dot.
I tested it with an mp3 that contains a TXXX tag with replay_gain_track
and replay_gain_album where converted to soundcheck attribute as expected.
I'll add the version check in autoconf.ac for MP3::Info before I commit
the change though.
cheers
-henrik
___
Bug-gnupod mailing list
Bug-gnupod@nongnu.org
http://lists.nongnu.org/mailman/listinfo/bug-gnupod
Re: [Bug-gnupod] Patch to support ReplayGain / mp3gain
On 5/9/09 9:14 AM, Frank Blendinger wrote:
I'll talk to the mpd developers to see if they are willing to support
RVA2/RGAD and/or APE tags.
I used mpd before switching to a Squeezebox. IIRC it is written in perl.
If they are using MP3::Info adding APE support is easy; just add an
extra option to the get_mp3tag() call.
Do I understand you correctly that this could lead to have two gain
values added on top of each other? I don't think you'd want that...
That depends. It gives the user some manual control over the desired
volume to use. If someone feels that applying the automatic algorithm
makes a track too loud or too quiet, he can manually set an RVA value to
compensate for it. This could be preferred over adjusting the normalized
value directly because when the files are rescanned by the normalization
algorithm, the RVA tag will not be reset. This is how iTunes does it
(iTunNORM is set by the Sound Check algorithm, while RVAD can be set
manually with a volume slider, they are both combined at playback). I'm
not saying I prefer combining these tags, but it's how the iPod works:
the Sound Check and Volume values are combined at playback (which Sound
Check is enabled on the iPod). With my patch the Replay Gain data is
converted to the iTunesDB Sound Check value while gnupod already
translates RVA2 values the iTunesDB Volume value.
So far we've only established that the normalize software writes RVA2
tags (iTunes writes RVAD which gnupod currently ignores). Using both
normalize and mp3gain is silly and makes no sense, but would indeed
result in these two algorithms being used on top of each other unless
you disable Sound Check on the iPod.
I think this would destroy the whole have all songs at the same average
volume level concept, when you apply something else on top of the
ReplayGain data.
I agree, and I think Apple implemented it this way because Sound Check
uses a crappy algorithm. When you use Replay Gain there is really no
need for manual adjustments or combining the values.
To have only ReplayGain and nothing else applied in the iPod, I will
have to not use the --max-vol-adj parameter and not use Sound Check,
right?
ReplayGain is stored as a Sound Check value, so you will have to enable
Sound Check on your iPod. Leave the --max-vol-adj alone so gnupod
ignores the RVA tags (0 is the default).
At least according to the mpd guys, those TXXX tags are quite common, so
it might help some people if those are supported in gnupod. Personally,
I'm going to keep the APE tags on my files, so I don't care that much.
I just checked and MP3::Info returns each TXXX tag as an individual
value (not a hash). So TXXX:replaygain_track_gain=-2 dB becomes
$hs{REPLAYGAIN_TRACK_GAIN}=-2 dB. This means the TXXX tags overwrite
the APE ReplayGain tags by default. I just tested this, and it works;
when ReplayGain data is set differently in the APE tag and TXXX tag, the
TXXX is used by gnupod. Sweet.
Cheers,
Richard
___
Bug-gnupod mailing list
Bug-gnupod@nongnu.org
http://lists.nongnu.org/mailman/listinfo/bug-gnupod
Re: [Bug-gnupod] Patch to support ReplayGain / mp3gain
On 5/9/09 9:14 AM, Frank Blendinger wrote:
[...]
gnupod_convert_OGG.pl uses Ogg::Vorbis::Header which may or may not
support the RG tags. If you send me an ogg file (privately) that
contains RG info I can check (and patch gnupod_convert_OGG.pl).
I've sent you a private mail on this, it should be easy to do.
Thanks for the file. The attached patch (adding 4 lines of code) takes
care of it.
Cheers,
Richard
diff -ru gnupod-rg-mp3/src/ext/FileMagic.pm gnupod-cvs/src/ext/FileMagic.pm
--- gnupod-rg-mp3/src/ext/FileMagic.pm 2009-05-09 11:17:53.0 +0200
+++ gnupod-cvs/src/ext/FileMagic.pm 2009-05-09 13:01:46.0 +0200
@@ -180,6 +180,10 @@
my $cf = ((split(/\//,$file))[-1]);
my @songa = pss($metastuff-{_TRACKNUM});
+ # Use track ReplayGain by default, use album ReplayGain if requested
+ my $rgtag = _REPLAYGAIN_TRACK_GAIN;
+ $rgtag = _REPLAYGAIN_ALBUM_GAIN if($flags-{'rgalbum'});
+
$rh{artist}= getutf8($metastuff-{_ARTIST} || Unknown Artist);
$rh{album} = getutf8($metastuff-{_ALBUM} || Unknown Album);
$rh{title} = getutf8($metastuff-{_TITLE} || $cf || Unknown
Title);
@@ -188,6 +192,7 @@
$rh{songnum} = int($songa[0]);
$rh{comment} = getutf8($metastuff-{_COMMENT} ||
$metastuff-{FORMAT}. file);
$rh{fdesc} = getutf8($metastuff-{_VENDOR} || Converted using
$encoder);
+ $rh{soundcheck} = _parse_ReplayGain($metastuff-{$rgtag}) || ;
$rh{mediatype} = int($metastuff-{_MEDIATYPE} || MEDIATYPE_AUDIO);
return {ref=\%rh, encoder=$encoder,
codec=$NN_HEADERS-{$magic}-{ftyp} };
}
diff -ru gnupod-rg-mp3/src/gnupod_convert_OGG.pl
gnupod-cvs/src/gnupod_convert_OGG.pl
--- gnupod-rg-mp3/src/gnupod_convert_OGG.pl 2009-05-09 12:42:25.0
+0200
+++ gnupod-cvs/src/gnupod_convert_OGG.pl2009-05-09 12:40:27.0
+0200
@@ -62,6 +62,8 @@
print _TRACKNUM:.( ($ftag-comment('tracknum'))[0] |
($ftag-comment('tracknumber'))[0] ).\n;
print _COMMENT:.($ftag-comment('comment'))[0].\n;
+print
_REPLAYGAIN_TRACK_GAIN:.($ftag-comment('REPLAYGAIN_TRACK_GAIN'))[0].\n;
+print
_REPLAYGAIN_ALBUM_GAIN:.($ftag-comment('REPLAYGAIN_ABLUM_GAIN'))[0].\n;
print _MEDIATYPE:.(GNUpod::FileMagic::MEDIATYPE_AUDIO).\n;
print FORMAT:OGG\n;
}
___
Bug-gnupod mailing list
Bug-gnupod@nongnu.org
http://lists.nongnu.org/mailman/listinfo/bug-gnupod
Re: [Bug-gnupod] Patch to support ReplayGain / mp3gain
On Fri, May 08, 2009 at 04:16:45PM +0200, Richard van den Berg wrote: On 5/8/09 2:26 PM, H. Langos wrote: Yeap. And if mp3gain can't be changed to do write RVA2 tags by itself, maybe there is a way to make mp3gain only do the analysis and pass its result to a programm that can write those RVA2 tags? That would be possible using: $ mp3gain -o -q -s s foo.mp3 FileMP3 gaindB gainMax AmplitudeMax global_gainMin global_gain foo.mp3-7-10.5038780.534185255145 Album-7-10.5038780.534185255145 Well, then thats the way to go. Just write some awk or perl wrapper around it, push those values into your favorite tagger and there you go. Do you know if the author of mp3gain is still active? reachable? alife? I have no clue.. in the thread from 2004 they mention he was unresponsive. So how do we get mp3gain's analysis results into RVA2 / XRVA tags? By changing the mp3gain C code. It's open source after all. I just don't have the stomach or time for it at the moment. Perhaps using the tagging code from normalize will make it easier. However, there could be a good reason not to attempt it. From http://jwz.livejournal.com/370342.html?thread=4232358#t4232358 Anway, whats not clear to me is if the RVA2 value could be used with ReplayGain. Both ReplayGain and RVA2 are applied as a constant value across the complete track, but I'm not sure the units are the same as the RVA2 tag. Well, the gain is given in dB relative to a reference level that most times is 83dB (But sometimes might be 89dB. The nasty details are here: http://www.mars.org/mailman/public/mad-dev/2004-February/000993.html ) I assume that mp3gain uses 83dB. But it wouldn't hurt to check. The units are in both cases dB. The encoding is different. mp3gain writes some prosa while the RVA2/XRVA tags are more terse encoding, but they both deal with relative volume changes in dB. Are you sure about the non desctructivnes of mp3gain? You can run mp3gain in several modes, either (often reversible) changing the volume of the file or just tagging it. I use it only for tagging. From the Debian man page: mp3gain actually changes your file’s gain only when you use one of the options -r, -a, -g, or -l. If none of these options is given, only a tag denoting the recommended gain change is written to the file. Thats good to know! Thank you! And thank you very much for testing mp3gain's undo feature. cheers -henrik ___ Bug-gnupod mailing list Bug-gnupod@nongnu.org http://lists.nongnu.org/mailman/listinfo/bug-gnupod
Re: [Bug-gnupod] Patch to support ReplayGain / mp3gain
Sorry about that. It's fixed now.
cheers
-henrik
On Sat, May 09, 2009 at 01:04:15PM +0200, Richard van den Berg wrote:
On 5/8/09 2:26 PM, H. Langos wrote:
BTW: I just added XRVA support as it is a too simple to let it pass.
Always test before commit. ;-)
Global symbol %hs requires explicit package name at
/opt/local/lib/perl5/5.8.9/darwin-2level/GNUpod/FileMagic.pm line 542.
$hs{RVA2} = $hs-{XRVA} if (!defined($hs-{RVA2}) defined($hs-{XRVA}));
should be
$hs-{RVA2} = $hs-{XRVA} if (!defined($hs-{RVA2})
defined($hs-{XRVA}));
Cheers,
Richard
___
Bug-gnupod mailing list
Bug-gnupod@nongnu.org
http://lists.nongnu.org/mailman/listinfo/bug-gnupod
Re: [Bug-gnupod] Patch to support ReplayGain / mp3gain
On 5/9/09 4:41 PM, H. Langos wrote: Well, the gain is given in dB relative to a reference level that most times is 83dB (But sometimes might be 89dB. The nasty details are here: http://www.mars.org/mailman/public/mad-dev/2004-February/000993.html ) I assume that mp3gain uses 83dB. But it wouldn't hurt to check. From that URL: reached the conclusion that as the vast majority of implementations are using 89dB, we should take this infortunate situation into consideration and update the standard to 89dB Since you already came to the conclusion that mp3gain is about the only implementation of the Replay Gain algorithm, I'm sure it uses 89dB as well. From mp3gain.c: /* the TAG version of the suggested Track Gain should ALWAYS be based on the 89dB standard. So we don't modify the suggested gain change until this point */ So I guess it is safe to store Replay Gain info inside RVA2 tags. The more I think about it, the more it makes sense to use the nid3lib code from normalize to make mp3gain support ID3 tags. I might attempt this when I have some free time (it could be a while until that happens). Cheers, Richard ___ Bug-gnupod mailing list Bug-gnupod@nongnu.org http://lists.nongnu.org/mailman/listinfo/bug-gnupod
Re: [Bug-gnupod] Patch to support ReplayGain / mp3gain
Hi Frank, On Fri, May 08, 2009 at 12:14:53PM +0200, Frank Blendinger wrote: On Thu 2009-05-07 21:16, Richard van den Berg rich...@vdberg.org proclaimed: On 5/7/09 5:06 PM, H. Langos wrote: What's important is the fact that there seems to be a standard that defines how volume adjustment data can be stored in id3 tags. Nice research. RVA2 is definitely the standard to use, and gnupod already has support for it. The problem is that there is not a lot of software that writes RVA2 tags. (The only ones I know about are iTunes and normalize). mp3gain should really be changed to write RVA2 tags instead of APE ones. You might want to take a look at this python script: http://mpd.wikia.com/wiki/Hack:ape2id3.py It will read the ReplayGain settings from APE tags and add it to ID3v2 tags. It has worked without any problems for me so far. Oh, this is a new one .. it takes the prose that mp3gain and vorbisgain generate and stores it in a prose id3 tag ... So to sum this up, we could have to deal with: * APE tags * RVA2 in ID3v2.4 or XRV/XRVA in ID3v2.3 or RGAD in ID3v2.3 * iTunNORM as Comment in either ID3v2.3 or v2.4 Did I miss anything? You missed the id3v2.3 RVAD tags. The definition in the standard is IMHO a horrible read. Little wonder the apple guys used it in a way that certainly was not intended by the standard's authors... https://savannah.nongnu.org/support/?105294 The intended use stays unclear. But maybe my hifi karma is not good enough to grasp it ;-) All of those could potential be present at once in a file. There can only be either an ID3v2.3 or an ID3v.4 as far as I can tell. But there can always be that iTunNORM comment in addition to the RG tags, and also additional APE tags. I have actually seen files in the wild that have all of this present. [1] So how will gnupod handle this? I suppose the iPod will just use iTunNORM and ignore anything else. Will gnupod use a present iTunNORM comment or will it always create one from ID3v2/APE information? Currently gnupod writes the soundcheck attribute it gets from iTunNorm comments as well as the volume attribute computed from the RVA2/XRVA tag. The volume attribute however is limited in its scale. it can only go from -100% to +100%. The lower bound is ok as it represents silence, but the upper bound is BS as it only allows gains of 6dB. So I guess we might want to abandon that in favor of manipulating the soundcheck attribute: http://www.id3.org/iTunes_Normalization_settings As I see it now. I am wasting a lot of time on this. I'll think about it but I guess I will not put too much work into it myself. cheers -henrik ___ Bug-gnupod mailing list Bug-gnupod@nongnu.org http://lists.nongnu.org/mailman/listinfo/bug-gnupod
Re: [Bug-gnupod] Patch to support ReplayGain / mp3gain
On Sat, May 09, 2009 at 12:13:01PM +0200, Richard van den Berg wrote:
On 5/9/09 9:14 AM, Frank Blendinger wrote:
I'll talk to the mpd developers to see if they are willing to support
RVA2/RGAD and/or APE tags.
I used mpd before switching to a Squeezebox. IIRC it is written in perl.
If they are using MP3::Info adding APE support is easy; just add an
extra option to the get_mp3tag() call.
mpd is written in C.
It already has APE tag support but I don't know if that support is
limited to certain file formats.
Do I understand you correctly that this could lead to have two gain
values added on top of each other? I don't think you'd want that...
...
I think this would destroy the whole have all songs at the same average
volume level concept, when you apply something else on top of the
ReplayGain data.
I agree, and I think Apple implemented it this way because Sound Check
uses a crappy algorithm. When you use Replay Gain there is really no
need for manual adjustments or combining the values.
I guess we should go this way:
1. Use RVA2/XRVA or (if the former is missing) REPLAY_GAIN_x to compute a
new soundcheck value.
If the first step didn't yield any information:
2. Use iTunNORM's soundcheck value and RVA volume adjustment.
This would probably work for most people.
At least according to the mpd guys, those TXXX tags are quite common, so
it might help some people if those are supported in gnupod. Personally,
I'm going to keep the APE tags on my files, so I don't care that much.
I just checked and MP3::Info returns each TXXX tag as an individual
value (not a hash). So TXXX:replaygain_track_gain=-2 dB becomes
$hs{REPLAYGAIN_TRACK_GAIN}=-2 dB. This means the TXXX tags overwrite
the APE ReplayGain tags by default. I just tested this, and it works;
when ReplayGain data is set differently in the APE tag and TXXX tag, the
TXXX is used by gnupod. Sweet.
Great. Did you check if that works with the parameters that FileMagic.pm uses
for its get_mp3tag() call?
cheers
-henrik
___
Bug-gnupod mailing list
Bug-gnupod@nongnu.org
http://lists.nongnu.org/mailman/listinfo/bug-gnupod
Re: [Bug-gnupod] Patch to support ReplayGain / mp3gain
On 5/8/09 12:14 PM, Frank Blendinger wrote: You might want to take a look at this python script: http://mpd.wikia.com/wiki/Hack:ape2id3.py It will read the ReplayGain settings from APE tags and add it to ID3v2 tags. It has worked without any problems for me so far. That script seems to create custom TXXX tags. This is not a standard and it surprises me that players support it. I know rockbox (alternative iPod firmware) does as well. Besides, I don't need to store the data I already have in APE tags in yet another format. This would require me to always run a conversion script after I run mp3gain, and I would have to add the script to abcde (the ripper script I use) as well. So to sum this up, we could have to deal with: * APE tags * RVA2 in ID3v2.4 or XRV/XRVA in ID3v2.3 or RGAD in ID3v2.3 * iTunNORM as Comment in either ID3v2.3 or v2.4 Did I miss anything? Yes, the following TXXX tags: TXXX:replaygain_album_gain TXXX:replaygain_album_peak TXXX:replaygain_track_gain TXXX:replaygain_track_peak So how will gnupod handle this? I suppose the iPod will just use iTunNORM and ignore anything else. Will gnupod use a present iTunNORM comment or will it always create one from ID3v2/APE information? The patch I created will favor ReplayGain info found in an APE tag over iTunNORM since RG is a superior algorithm. RVA2 tags are converted to the iTunesDB volume value when --max-vol-adj is used with gnupod_addsong.pl. It is combined with normalization info (if also present) when Sound Check is enabled on the iPod. What will be used if both ID3v2 and APE is present? I read about a (possible?) --ignore-ape option, but I didn't really get what the default behaviour will be. The new default behavior will be to read APE tags, including ReplayGain information. ID3 tags with the same name of APE tags will overwrite the APE tags. ID3v2 tags are favored over ID3v1 tags. Perhaps we should also read TXXX:replaygain_album_gain and TXXX:replaygain_track_gain from ID3v2 tags to replace the respective APE tags. I, for one, would propose to use RVA2 if present and APE only as a fallback, as it is more standardize and also more often present in actual files, APE does not seem to be supported by many (software) players (and I guess not by a single hardware device), while some do support ReplayGain tags in ID3v2. My Neo 35 running OpenNeo reads RG info in APE tags just fine. As do my various Squeezeboxes (powered by SqueezeCenter). One of the problems is that normalization data like RG APE tags and iTunNORM comments is sometimes combined with RVA2 volume information. iTunes is one of the software that does this, and SqueezeCenter also combines the tags (but there is some debate to change that). So favoring RVA2 over RG info should really be selectable by the user. As explained above, iPods will combine the two when both are present in the iTunesDB and Sound Check is enabled. And one last thing: what about support for the music formats covered by the gnupod_convert_* scripts? Those can carry ReplayGain tags, too, so it would be nice to keep/convert them when the audio gets converted. The tags of non-native file formats are spit out by the GET_META part of the gnupod_convert_* scripts. If they spit out the correct soundcheck value, that would work. So if you can show us how to get ReplayGain information from those non-native files, we can probably make it work. So far I only have experience with OGG Vorbis files. There is tool similar to mp3gain called vorbisgain to calculate the gain data and write the tags, and they can be read with the vorbiscomment tool. So I guess it should not be that hard to read those tags as it is done with the common artist/title/... tags and add them to the converted mp3 files. gnupod_convert_OGG.pl uses Ogg::Vorbis::Header which may or may not support the RG tags. If you send me an ogg file (privately) that contains RG info I can check (and patch gnupod_convert_OGG.pl). I have read that there is also support for ReplayGain tags in FLAC files, but I have no experience with that. We'll leave that for someone that actually uses FLAC (I don't). Cheers, Richard ___ Bug-gnupod mailing list Bug-gnupod@nongnu.org http://lists.nongnu.org/mailman/listinfo/bug-gnupod
Re: [Bug-gnupod] Patch to support ReplayGain / mp3gain
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. :-)
___
Bug-gnupod mailing list
Bug-gnupod@nongnu.org
http://lists.nongnu.org/mailman/listinfo/bug-gnupod
Re: [Bug-gnupod] Patch to support ReplayGain / mp3gain
Hi Henrik, On Mon, April 20, 2009 15:32, H. Langos wrote: 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 agree with your reasoning in principle (and will provide a patch for the --noAPEtag option) but if you had ever tried to use iTunes' SoundCheck and compare it against ReplayGain, you wouldn't be proposing this at all. :-) The SoundCheck algorithm is peak based, while ReplayGain uses the RMS energy. The difference between the two is that ReplayGain actually works (makes all tracks sound equally loud) while SoundCheck doesn't. (There is even a commercial product named iVolume that (badly) integrates ReplayGain into iTunes.) For more info see http://replaygain.hydrogenaudio.org/ PS: Please don't take the number of suggestions as an indication of poor quality. Not at all. They are good suggestions. I am a functional programmer before anything else. Now if you told me my code didn't work, I would be hurt. ;-) Besides (after hours of googling for the syntax of the iTunNORM tag) I borrowed most of the code from http://projects.robinbowes.com/flac2mp3/trac/ticket/30 I hope you take it as motivation to continue work on gnupod. If there are things missing or broken I'll definitely work on it some more. Right now, I'm very pleased with gnupod as it does everything I need! I'll send you a new patch when I have some more time. Cheers, Richard ___ Bug-gnupod mailing list Bug-gnupod@nongnu.org http://lists.nongnu.org/mailman/listinfo/bug-gnupod
