Created attachment 91435 [details] [review]
Patch for BPM support

In widgets/rb-song-info.c, there is a
+	bpm = g_ascii_strtod (bpm_str, &endptr);
used to convert a value you got from user input in a gtk widget, shouldn't it be g_strtod ? (ie is the string you get using the locale floating point separator, or is it always returned with '.' as a separator?).

I'm not really sure as to why someone would want to modify the BPM value of a song? How do you calculate this value?

>I'm not really sure as to why someone would want to modify the BPM value of a
>song? How do you calculate this value?

Most songs doesn't have these tags set, so you may want to use an external software (automatic or having the user tap a key to the beat) to calculate it. 

And about the g_ascii_strtod the glib documentation states:
"This function should seldomly be used. The normal situation when reading numbers not for human consumption is to use g_ascii_strtod()."

And for the same type of conversion i the same file g_ascii_strto... is used, so I used that also. But I agree... since it is from a use field it feels strange.. however, since it is only numbers the only valid chars here are in the 128-bit range and hence same for both utf-8 and ascii.

//Mattias

(In reply to comment #3)
> >I'm not really sure as to why someone would want to modify the BPM value of a
> >song? How do you calculate this value?
> 
> Most songs doesn't have these tags set, so you may want to use an external
> software (automatic or having the user tap a key to the beat) to calculate it.

Then the software should set the tag automatically once it has calculated it, the user shouldn't have to manually enter it once it has been calculated ;)
 
> 
> And for the same type of conversion i the same file g_ascii_strto... is used,
> so I used that also. But I agree... since it is from a use field it feels
> strange.. however, since it is only numbers the only valid chars here are in
> the 128-bit range and hence same for both utf-8 and ascii.

The problem is not handling utf-8 chars, it's about being able to do the conversion. *_strtod functions have to be able to recognize 12.34 as a valid floating point string, but must error out on stuff like 12A34. And the thing is that the floating point separator isn't necessarily '.' in some locales, on a LC_ALL=C system, the numerical value 12.34 is written "12.34" while in a LC_ALL="fr_FR" system, 12.34 is "12,34". g_ascii_strtod parses strings where the floating point separator must be '.' (which should be the case for stuff written to a file) while g_strtod uses a different separator depending on the locale ('.' in LC_ALL=C, ',' when LC_ALL=fr_FR, ...). 
When LC_ALL=fr_FR and you try to parse "12,34" with g_ascii_strtod, you'll get 12 since g_ascii_strtod doesn't take into account htat the decimal separator is ',' in fr_FR locale.

Ok I see. 
I don't have any problems with that change so just go ahead with that... 

>> Most songs doesn't have these tags set, so you may want to use an external
>> software (automatic or having the user tap a key to the beat) to calculate it.
>
>Then the software should set the tag automatically once it has calculated it,
>the user shouldn't have to manually enter it once it has been calculated ;)

Unfortunatly software analyzers can count quite wrong for songs without a clear beat... 
And hopefully someone will write a auto-bpm plugin with optional tap support to change the bpm. Then this entry could be removed :)

//Mattias

GST_TAG_BEATS_PER_MINUTE was added in GStreamer 0.10.12; our current minimum requirement is 0.10.4, so we'd need #ifdefs around any code using it, like we have for GST_TAG_MUSICBRAINZ_TRACKID.

or just bump the minimum requirement ;)

g_ascii_strtod was the cause I couldn't set the bpm value to 59.99 (got 59.00) since I used , as a delimiter. When I wrote 59.99 it worked...

//Mattias

Created attachment 91530 [details] [review]
New patch fixing the ifdef and g_strtod

There's at least a #ifdef missing in rb-encoder-gst.c I think, and it seems that the BPM field will be shown in the UI even if when the BPM value can't be read/set. If that's correct, I'd #ifdef that as well.

Ok... I'll fix this later... 

//Mattias

It seems like my time just got very limited for the month to come, so I will not have the time to fix those small issues. But since it's very small issues I hope you will be able to fix them as you apply the patch.

//Mattias

Created attachment 92167 [details] [review]
New patch with defines

New patch with a define ENABLE_BPM that will be defined if gstreamer is 0.10.12 or more. 
I have ifdefs around the visible BPM parts, but I have keep the internals to not mess upp the database just because the BPM is toggled.

Just a ping to see if this could get included before trunk diverges to much?

//Snaggen

It is almost a month since I fixed the remaning issues with the patch.... I have asked for a comment but havent heard a word. Is bugzilla the wrong place for patches? Should I send it to the mailing list?!

//Snaggen

I guess the maintainers are busy atm :-/ Sending a mail to the mailing list might be helpful, though that won't change anything if everyone is busy ;) 

The only remaining (extremely minor) issue is that you've used c++ style comments in rb-song-info.c.

The main reason I'm reluctant to commit this (and anything that adds new fields to RhythmDBEntry) is that it increases memory usage for everyone, whether they use it or not.

What proportion of the tracks in your library would have BPM tags?

I see two kind of use cases for BPM tags, DJ:ing and workout music. I have tagged all my music just to be able to have music with a good beat when I'm running. I guess you may tag some part (like 10%) if you only tag music for workout... but if a tagging plugin would exists that could analyze and autotag with the option for manual tagging by tapping I guess that these kind of users would tag most of its music library.

//Snaggen

Created attachment 94413 [details] [review]
New patch without c++ comments

Uploaded my bzr branch to launchpad, so it easier to get it if someone wants it.

https://code.launchpad.net/rhythmbox

Will this ever be included in the main branch? I would definitely find it very useful. Lack of BPM support is actually the main reason why I'm NOT using rhythmbox at the moment.

I agree that the BPM support would be an important feature.
I'm an occasional DJ, using RB for prelistening, and the patch from Mattias is now obsolete. I hope this issue will rise in importance.
As Mattias proposed, a compiler flag (inactive by default) might be the best solution. Anyways, I do not think the memory issue would be a problem nowadays.

Laurent

I updated my branch to work with the latest rhythmbox
https://code.launchpad.net/~snaggen/rhythmbox/bpm

bzr branch lp:~snaggen/rhythmbox/bpm 

to get it.

Thanks Matthias,
you updated branch is working well, this is helping me a lot !

I might be stuck to this revision r5131, but to me, this BPM support is more important than any other RB evolution. I hope the developers will consider to integrate it into the main release.

To answer Jonathan's last question (3 years ago), 100% of my tracks have BPMs, because I'm using them to select the music during live events.

Regards
Laurent

Laurent, I'm just happy that someone uses this patch. And if it isn't that much work and someone wants me to, I might update this branch when I find some spare time.
The best would be if it got merged I guess...

I probably will merge this soon.

Johathan, great! Do you wan't me to remove the ifdef ENABLE_BPM that just makes the code ugly and really isn't needed for gstreamer support any more?
That would also remove the autoconf stuff...

Please do.  I want to get BPM, album-artist, and perhaps composer tags added fairly soon, and having an up to date and clean patch will help.

the conditional part is removed and the branch is synced with trunk.

Small problem: the changes to the transcoding backend will not work.  rhythmdb_entry_get_string(entry, RHYTHMDB_PROP_BPM) will result in an assertion failure, since it's not a string property.

Instead, we need to retrieve the double value and (maybe) decide whether to write it or not using a numeric comparison.  We could either always write it, in which case transcoded files will end up with BPM values of 0, or only write if greater than a very small nonzero value, in which case it will be impossible to write a BPM value of 0.

pushed (with some minor changes and additions) as commit 512f875.

Did I ever thank you Jonathan and Mattias ?
I just installed rb 0.13.0 on my production environment, instead of Mattias's branch, and it works very fine !
Thanks again