BPM metadata enhancement #1087

brianschrameck · 2021-05-04T13:49:10Z

Closes #1086 and is related to #1036.

Adds BPM to the stored metadata about MediaFiles.

Displays BPM in the following locations:

Listing songs in the song list (desktop, sortable)
Listing songs in playlists (desktop, sortable)
Listing songs in albums (desktop)
Expanding song details

When listing, shows a blank field if no BPM is present. When showing song details, shows a question mark.

Updates test MP3 file to have BPM tag. Updated test to ensure tag is read correctly.

Updated localization files. Most languages just use "BPM" as discovered during research on Wikipedia. However, a couple use some different nomenclature. Spanish uses PPM and Japanese uses M.M.

Related to navidrome#1036. Adds BPM to the stored metadata about MediaFiles. Displays BPM in the following locations: - Listing songs in the song list (desktop, sortable) - Listing songs in playlists (desktop, sortable) - Listing songs in albums (desktop) - Expanding song details When listing, shows a blank field if no BPM is present. When showing song details, shows a question mark. Updates test MP3 file to have BPM tag. Updated test to ensure tag is read correctly. Updated localization files. Most languages just use "BPM" as discovered during research on Wikipedia. However, a couple use some different nomenclature. Spanish uses PPM and Japanese uses M.M.

brianschrameck · 2021-05-04T13:51:52Z

Also, note that the localization files were reformatted in alignment with the style enforced by the included Prettier extensions in the dev environment. I suggest hiding whitespace changes during review. 😉

brianschrameck · 2021-05-04T15:30:57Z

Making this WIP; as discussed, BPM could be a float and TagLib isn't handling that scenario.

ghost · 2021-05-04T15:44:25Z

Would there be a way to not show this field at all?

brianschrameck · 2021-05-04T15:46:27Z

Would there be a way to not show this field at all?

That's being addressed via PR #923.

- Supports reading floating point BPM (still storing it as an integer) and FFmpeg as the extractor - Replaces existing .ogg test file with one that shouldn't fail randomly - Adds supporting tests for both FFmpeg and TagLib

deluan

Thanks for this! Looks good in general, just a few minor things.

Updated localization files. Most languages just use "BPM" as discovered during research on Wikipedia. However, a couple use some different nomenclature. Spanish uses PPM and Japanese uses M.M.

Thanks for the research! One thing that I'd like to point out is that you don't need to add the new string in all translations, usually you only add the English one. I prefer to handle the translations in our POEditor project, so it can be reviewed by the translators responsible for each language. Adding the translations in PRs make my life harder, as I have to manually update the POEditor project 😞

So, if you don't mind, can you please remove the translations? At least the ones that are not really translated (still BPM), as ND falls back automatically to English. Thanks!

deluan · 2021-05-05T03:35:48Z

ui/src/common/SongDetails.js

@@ -33,6 +39,13 @@ export const SongDetails = (props) => {
    updatedAt: <DateField record={record} source="updatedAt" showTime />,
    playCount: <TextField record={record} source="playCount" />,
    comment: <MultiLineTextField record={record} source="comment" />,
+    bpm: (


I think comment should always be the last field here, as it can have multiple lines. Maybe put bpm after genre?

deluan · 2021-05-05T03:37:57Z

ui/src/common/SongDetails.js

+      <FunctionField
+        record={record}
+        source="bpm"
+        render={(r) => r.bpm || '?'}


Any reason why show a "?" when the fiend is empty? Why not do the same as discSubtitle and comment, remove the field?

Definitely missed that it does remove those fields dynamically. Will remove it instead.

deluan · 2021-05-05T03:41:02Z

model/mediafile.go

@@ -39,6 +39,7 @@ type MediaFile struct {
 	Compilation          bool      `json:"compilation"`
 	Comment              string    `json:"comment"`
 	Lyrics               string    `json:"lyrics"`
+	Bpm                  int       `json:"bpm"`


I think we should tag the field as json:"bpm,omitempty". This way we don't need <FunctionField source="bpm" render={(r) => r.bpm || ''} /> as it will be empty, not zero, simplifying the Datagrids (just need a simple <TextField/> field

Good call, much cleaner.

deluan · 2021-05-05T03:42:23Z

db/migration/20210430212322_add_bpm_metadata.go

+func upAddBpmMetadata(tx *sql.Tx) error {
+	_, err := tx.Exec(`
+alter table media_file
+add bpm integer;


As this field will appear in the Datagrids, it will be sortable, so we should add an index for it.

deluan · 2021-05-05T03:44:45Z

scanner/metadata/ffmpeg_test.go

+		Input #0, ogg, from 'tests/fixtures/test.ogg':
+  		  Metadata:
+	        FBPM            : 141.7`
+		md, _ := e.extractMetadata("tests/fixtures/test.mp3", output)


Small nitpick: I know the path here is irrelevant, but I think for clarity sake it should match the file path passed to the function (.ogg and not .mp3)

Whoops, copy/paste error. Agreed.

brianschrameck · 2021-05-05T11:37:35Z

All good call outs. First time using Go (and this code!) so definitely still learning. Will make changes today, thanks.

- Adds index for BPM. Removes drop column as it's not supported by SQLite (duh). - Removes localizations for BPM as those will be done in POEditor. - Moves BPM before Comment in Song Details and removes BPM altogether if it's empty. - Omits empty BPM in JSON responses, eliminating need for FunctionField. - Fixes copy/paste error in ffmpeg_test.

deluan

I think we got everything in place. But I think I was not 100% clear about the translations: We do need the new field added to the en.json file, this is the only language handled in the code. The other languages are the ones handled in the POEEditor. Please add the en.json string back if you can.

I'll do some manual testing tonight before merging.

brianschrameck · 2021-05-05T16:45:18Z

We do need the new field added to the en.json file, this is the only language handled in the code.

Correct me if I'm wrong, but the en.json file is still in there with the changes.

deluan · 2021-05-05T16:48:05Z

Correct me if I'm wrong, but the en.json file is still in there with the changes.

Ops, my bad! I've previously marked it as "Viewed" and inadvertently skipped it this time.

deluan · 2021-05-06T01:33:17Z

Merging. This just makes it even more important to have #923 in place before next release.

github-actions · 2023-03-16T01:59:51Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

brianschrameck changed the title ~~BPM metadata enhancement~~ WIP: BPM metadata enhancement May 4, 2021

Enhances support for BPM metadata extraction

cab6675

- Supports reading floating point BPM (still storing it as an integer) and FFmpeg as the extractor - Replaces existing .ogg test file with one that shouldn't fail randomly - Adds supporting tests for both FFmpeg and TagLib

brianschrameck changed the title ~~WIP: BPM metadata enhancement~~ BPM metadata enhancement May 4, 2021

deluan requested changes May 5, 2021

View reviewed changes

brianschrameck requested a review from deluan May 5, 2021 12:59

deluan reviewed May 5, 2021

View reviewed changes

deluan approved these changes May 5, 2021

View reviewed changes

deluan merged commit 30bb3f7 into navidrome:master May 6, 2021

brianschrameck deleted the bpm-metadata-enhancements branch May 6, 2021 21:08

github-actions bot added the frozen-due-to-age label Mar 16, 2023

github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BPM metadata enhancement #1087

BPM metadata enhancement #1087

brianschrameck commented May 4, 2021 •

edited

brianschrameck commented May 4, 2021 •

edited

brianschrameck commented May 4, 2021

ghost commented May 4, 2021

brianschrameck commented May 4, 2021

deluan left a comment •

edited

deluan May 5, 2021

deluan May 5, 2021

brianschrameck May 5, 2021

deluan May 5, 2021

brianschrameck May 5, 2021

deluan May 5, 2021 •

edited

deluan May 5, 2021

brianschrameck May 5, 2021

brianschrameck commented May 5, 2021

deluan left a comment

brianschrameck commented May 5, 2021

deluan commented May 5, 2021

deluan commented May 6, 2021

github-actions bot commented Mar 16, 2023

BPM metadata enhancement #1087

BPM metadata enhancement #1087

Conversation

brianschrameck commented May 4, 2021 • edited

brianschrameck commented May 4, 2021 • edited

brianschrameck commented May 4, 2021

ghost commented May 4, 2021

brianschrameck commented May 4, 2021

deluan left a comment • edited

Choose a reason for hiding this comment

deluan May 5, 2021

Choose a reason for hiding this comment

deluan May 5, 2021

Choose a reason for hiding this comment

brianschrameck May 5, 2021

Choose a reason for hiding this comment

deluan May 5, 2021

Choose a reason for hiding this comment

brianschrameck May 5, 2021

Choose a reason for hiding this comment

deluan May 5, 2021 • edited

Choose a reason for hiding this comment

deluan May 5, 2021

Choose a reason for hiding this comment

brianschrameck May 5, 2021

Choose a reason for hiding this comment

brianschrameck commented May 5, 2021

deluan left a comment

Choose a reason for hiding this comment

brianschrameck commented May 5, 2021

deluan commented May 5, 2021

deluan commented May 6, 2021

github-actions bot commented Mar 16, 2023

brianschrameck commented May 4, 2021 •

edited

brianschrameck commented May 4, 2021 •

edited

deluan left a comment •

edited

deluan May 5, 2021 •

edited