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

Album gain #10085

Merged
merged 6 commits into from
Aug 18, 2023
Merged

Album gain #10085

merged 6 commits into from
Aug 18, 2023

Conversation

TelepathicWalrus
Copy link
Contributor

@TelepathicWalrus TelepathicWalrus commented Aug 5, 2023

Add album gain. This will use the smallest gain from the album the track is from instead of the individual track gain. This keeps the volume range between tracks in an album.

Changes
Add album gain dto

Frontend: jellyfin/jellyfin-web#4741

@github-actions
Copy link

github-actions bot commented Aug 5, 2023

No changes to OpenAPI specification found. See history of this comment for previous changes.

@nielsvanvelzen
Copy link
Member

Wouldn't it be better to add this to the BaseItem of the album itself instead of adding another property? That way we can eventually add it for playlist/collection without adding even more.

@cvium
Copy link
Member

cvium commented Aug 5, 2023

Wouldn't it be better to add this to the BaseItem of the album itself instead of adding another property? That way we can eventually add it for playlist/collection without adding even more.

Should probably be a computed property at the very least? (make sure it doesn't hammer the DB though)

@nielsvanvelzen
Copy link
Member

Wouldn't it be better to add this to the BaseItem of the album itself instead of adding another property? That way we can eventually add it for playlist/collection without adding even more.

Should probably be a computed property at the very least? (make sure it doesn't hammer the DB though)

It could either be added in the db while scanning or with the includeItemFields parameter most API's have.

@TelepathicWalrus
Copy link
Contributor Author

Ive changed it to be added when scanning, hopefully this will be better?

use baseitem LUFS for album
@lonebyte
Copy link
Contributor

lonebyte commented Aug 5, 2023

Thanks for this! But it seems like I underestimated it. It is actually not enough to use the smallest value, see https://github.com/beetbox/beets/blob/a4e61e8bc7983df412f9bccec930c7b0818a4a08/beetsplug/replaygain.py#L303
Basically you have to consider the album to be one continuos song

@TelepathicWalrus
Copy link
Contributor Author

Thanks for this! But it seems like I underestimated it. It is actually not enough to use the smallest value, see https://github.com/beetbox/beets/blob/a4e61e8bc7983df412f9bccec930c7b0818a4a08/beetsplug/replaygain.py#L303 Basically you have to consider the album to be one continuos song

This is possible to do using ffmpeg i think. you can use concat to concatenate all the songs of the album into one then get the lufs from that one long track. Only issue is I'm not sure where i can put this in the code. Where i have it now doesn't have access to librarymanager or encoderpath. If someone could help there that would be awesome.
Also i know it isn't true album gain as it is but it does do what is required, i.e keeping all songs in an album to be the same relative loudness so maybe it is good enough??

@lonebyte
Copy link
Contributor

lonebyte commented Aug 6, 2023

Also i know it isn't true album gain as it is but it does do what is required, i.e keeping all songs in an album to be the same relative loudness so maybe it is good enough??

Unfortunately not. I have one album with the last song being complete silence. The calculated LUFS is -70, which would not be healthy when listening to the other songs 😅. I thing the easiest solution for now would be to actually use the maximum LUFS for the album, which in my example is -10.2. The true value is -12.7 LUFS, so not too far away.

I think in my original comment I misinterpreted the LUFS to already be the gain, in which case the smallest would be the 'correct' one to choose.

@TelepathicWalrus
Copy link
Contributor Author

TelepathicWalrus commented Aug 6, 2023

Also i know it isn't true album gain as it is but it does do what is required, i.e keeping all songs in an album to be the same relative loudness so maybe it is good enough??

Unfortunately not. I have one album with the last song being complete silence. The calculated LUFS is -70, which would not be healthy when listening to the other songs sweat_smile. I thing the easiest solution for now would be to actually use the maximum LUFS for the album, which in my example is -10.2. The true value is -12.7 LUFS, so not too far away.

I think in my original comment I misinterpreted the LUFS to already be the gain, in which case the smallest would be the 'correct' one to choose.

I am already taking the maximum LUFS value for the smallest gain value. Have you tried it with some of your albums out of interest? it would be interesting to see if it works for that album with the silence at the end.

I was thinking that maybe a weighted average of the track LUFS values might be better though what are your thoughts?

@lonebyte
Copy link
Contributor

lonebyte commented Aug 6, 2023

I am already taking the maximum LUFS value for the smallest gain value.

My bad, I didn't look close enough at the code.

Have you tried it with some of your albums out of interest? it would be interesting to see if it works for that album with the silence at the end.

I tested it now and I think it is close enough for now. Thanks a lot! With track gain, the silent song receives a gain of 400, but doesn't seem to be a problem. But maybe there should be some sanity check, maybe limit it to 10 in jellyfin-web?

I was thinking that maybe a weighted average of the track LUFS values might be better though what are your thoughts?

Weighted by what metric? The unweighted average for that particular album (https://musicbrainz.org/release-group/8183834a-6c8a-39ef-a7bc-a82f79b929e9 btw) would be -19, which is further away from the ideal (-12.7 LUFS) than what is currently used (-10.2 LUFS). So I think averaging does more harm than good.

@TelepathicWalrus
Copy link
Contributor Author

Weighted by what metric? The unweighted average for that particular album (https://musicbrainz.org/release-group/8183834a-6c8a-39ef-a7bc-a82f79b929e9 btw) would be -19, which is further away from the ideal (-12.7 LUFS) than what is currently used (-10.2 LUFS). So I think averaging does more harm than good.

By weighted I mean if you take the LUFS value for the track and multiply it by the runtime of the track/total runtime of album, then add these values together to give average weighted LUFS. Example:
track 1 = 10seconds, LUFS = -10
track 2 = 15seconds, LUFS = -15
track 3 = 30seconds, LUFS = -30
Then total runtime is 55seconds and weighted average would be (10/55)-10+(15/55)-15+(30/55)*-30=-22.3 db.

@lonebyte
Copy link
Contributor

lonebyte commented Aug 6, 2023

That would result in -15.6 LUFS, already better. But what if you had an album with quiet long tracks and loud short ones? The gain would become too high again. IMHO taking the maximum LUFS is already good enough. Then at least you don't run the risk of blowing the headphones off people's heads.

@TelepathicWalrus
Copy link
Contributor Author

That would result in -15.6 LUFS, already better. But what if you had an album with quiet long tracks and loud short ones? The gain would become too high again. IMHO taking the maximum LUFS is already good enough. Then at least you don't run the risk of blowing the headphones off people's heads.

Yeah I agree with you on that, I think it is good enough and gives the desired effect.

Move dto for all types
@Bond-009 Bond-009 merged commit 4c7fb8f into jellyfin:master Aug 18, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants