Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Adds Audio Channel Metadata - #1036 #1294

Merged
merged 3 commits into from
Sep 20, 2021

Conversation

mayanez
Copy link
Contributor

@mayanez mayanez commented Aug 21, 2021

First off awesome project! I'm looking to get familiar with the codebase to start adding a few missing features for classical music (#1036). I'm starting off small and figured this would be a good contribution although not in the linked issue since it is especially for those with multi-channel versions of the same track in their library.

I'm fairly new to Go as well so let me know if anything is off on that front. With regards to the current changes two things I wasn't able to piece together:

  • the naming convention for the db/migration file
  • the configuration for prettier. It looks like SongList.js needs to run it.

If I missed anything regarding the contributing conventions let me know.

Cheers!

@deluan
Copy link
Member

deluan commented Aug 21, 2021

Hey, thanks for this!

First off awesome project! I'm looking to get familiar with the codebase to start adding a few missing features for classical music (#1036). I'm starting off small and figured this would be a good contribution although not in the linked issue since it is especially for those with multi-channel versions of the same track in their library.

Is channels really that important? Can you send me an example of file with more than 2 channels?

I'm fairly new to Go as well so let me know if anything is off on that front. With regards to the current changes two things I wasn't able to piece together:

  • the naming convention for the db/migration file

To create an empty migration skeleton, run make migration. Ex: make migration name=add_mediafile_channels

  • the configuration for prettier. It looks like SongList.js needs to run it.

Yeah, it is breaking the build. Usually you install a prettier plugin in your IDE and enable autoformat with it. You can also run npm run prettier inside the /ui folder. To be sure you don't push broken code, setup your git hooks (make setup-git)

If I missed anything regarding the contributing conventions let me know.

I'll review the PR as soon as I can

Copy link
Member

@deluan deluan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just missing the support for channels in the ffmpeg extractor. Can you please implement it?

@mayanez
Copy link
Contributor Author

mayanez commented Aug 21, 2021

Thanks for the feedback. I'll try to address your comments as best I can and will circle around if I uncover any issues.

Is channels really that important? Can you send me an example of file with more than 2 channels?
Its more common for classical music and other live music recordings. The primary sources for this are SACD, Blu-ray, and DVD.

You can find sample tracks here: https://oppodigital.com/hra/dsd-by-davidelias.aspx

@mayanez mayanez force-pushed the metadata-enhancements/audio-channels branch from a56b76c to 16f2fbb Compare August 22, 2021 20:29
@mayanez
Copy link
Contributor Author

mayanez commented Aug 22, 2021

While going over the ffmpeg scanner I found that using ffprobe, which is part of the ffmpeg, packages can significantly simplify parsing. I've chosen to make it output json so that it's easily parse-able and refactored the necessary parts. It should be more robust to any future ffmpeg version changes as well.

@deluan
Copy link
Member

deluan commented Aug 22, 2021

Thanks for taking a stab at the ffmpeg extractor. Unfortunately there's one important thing you overlooked: batching.

Originally I tried to use ffprobe for extracting metadata (you may find references to it in GitHub history), for the same reasons you pointed above, but it was too slow as it has to exec one ffprobe command for each track file. For a few hundred songs it will be slow, but may not have a huge impact. But for libraries over 50K songs (we have users with >1Mi songs), this is simply unacceptable.

The way I found to speed up this was to call ffmpeg passing all files in a directory in one command only, drastically reducing the time required to import all songs. Unfortunately ffprobe does not support multiple input files, nor ffmpeg outputs json or other more parseable formats...

Anyway, I'm afraid I won't be able to accept this change. If you are having trouble parsing ffmpeg's output with regex, please reach ou to me through Discord and I can help with that.

@mayanez mayanez force-pushed the metadata-enhancements/audio-channels branch 2 times, most recently from 678d47e to b9d69bd Compare August 25, 2021 02:22
@mayanez mayanez requested a review from deluan August 26, 2021 01:41
@deluan
Copy link
Member

deluan commented Aug 26, 2021

Thanks for the changes, and sorry, I didn't have time to work on the project the last couple of days. I'll take a look at this as soon as possible.

@deluan
Copy link
Member

deluan commented Sep 3, 2021

Hey, sorry for the delay. I already reviewed it and looks good. Will try it this weekend and if everything is fine, will merge this

Copy link
Member

@deluan deluan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the contribution!

Copy link
Member

@deluan deluan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I just tried the ffmpeg extractor, and it is not able to get channels from some files (M4A and OGG). Here are some samples:

01-01 The Hollow.ogg.zip
02 I Get Around.m4a.zip

@mayanez mayanez force-pushed the metadata-enhancements/audio-channels branch 2 times, most recently from baf14b8 to 4f2062a Compare September 12, 2021 20:44
@mayanez
Copy link
Contributor Author

mayanez commented Sep 12, 2021

Actually I just tried the ffmpeg extractor, and it is not able to get channels from some files (M4A and OGG). Here are some samples:

01-01 The Hollow.ogg.zip
02 I Get Around.m4a.zip

Looks like it's possible to have a language in the stream. I've updated the regex to account for it and added appropriate test cases.

@mayanez mayanez requested a review from deluan September 12, 2021 20:45
@deluan deluan merged commit 15ae3d4 into navidrome:master Sep 20, 2021
@mayanez mayanez deleted the metadata-enhancements/audio-channels branch September 22, 2021 23:37
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants