-
-
Notifications
You must be signed in to change notification settings - Fork 799
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
ReplayGain support + audio normalization (web player) #1988
Conversation
Nice! I'll take a look and give you feedback on this as soon as I can. Thanks! |
Hey @kgarner7 , does this take into consideration transcoding? I think we need to make |
I'm not entirely sure how transcoding would play a role. Does that change the replaygain properties of the track (e.g., if I started with flac and converted it to ogg, would those values change)? The tags come from the original file (and are stored in the db). |
I may be wrong, but I think we just have to reuse the tags from the original file in the transcoded one. I think |
Ah, if the concern is whether ffmpeg will expose those same tags to the new file, I believe the answer is yes (when I transcoded my library to Play:Sub, which does replaygain, I could see gain info). |
- extract ReplayGain tags from files, expose via native api - use metadata to normalize audio in web player
Download the artifacts for this pull request: |
I play my music transcoded from FLAC to Opus, and the gain was working correctly in the web player too. |
@rsammelson My questioning is about other clients, the Subsonic clients. Will they benefit from this? Base on @kgarner7 , seems that this works as well |
Ahhh,I understand now. I don't think so. The reason why Play:Sub worked was because it read the tags from file. It isn't exposed on subsonic routes (I guess I could modify childFromMediaFile?), but I'm not sure that is used... |
Yes, that's how Subsonic clients handle ReplayGain. We just need to be sure that whatever RG tags are in the files, they remain intact after transcoding. My understanding is that is what currently happens (based on your test with play:Sub), right? To be sure, you could also download a track with transcoding ON and check if |
I downloaded a transcoded track, and I can confirm that the replaygain attributes are still there (I also manually transcoded with ffmpeg for same results). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Just need to test it locally and see how the UX feels.
alter table media_file add | ||
album_gain real; | ||
alter table media_file add | ||
album_peak real; | ||
alter table media_file add | ||
track_gain real; | ||
alter table media_file add | ||
track_peak real; | ||
`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a rg_
prefix to these columns?
model/mediafile.go
Outdated
AlbumGain float64 `structs:"album_gain" json:"albumGain"` | ||
AlbumPeak float64 `structs:"album_peak" json:"albumPeak"` | ||
TrackGain float64 `structs:"track_gain" json:"trackGain"` | ||
TrackPeak float64 `structs:"track_peak" json:"trackPeak"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, please add a RG
prefix (ex: RGAlbumGain
)
scanner/metadata/metadata.go
Outdated
func (t Tags) AlbumGain() float64 { return t.getGainValue("replaygain_album_gain") } | ||
func (t Tags) AlbumPeak() float64 { return t.getPeakValue("replaygain_album_peak") } | ||
func (t Tags) TrackGain() float64 { return t.getGainValue("replaygain_track_gain") } | ||
func (t Tags) TrackPeak() float64 { return t.getPeakValue("replaygain_track_peak") } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here (prefix RG)
ui/src/common/SongInfo.js
Outdated
}, | ||
}) | ||
const useStyles = config.enableReplayGain | ||
? makeStyles({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a better (more MUI) way to achieve this. Ex:
navidrome/ui/src/common/SongDatagrid.js
Line 48 in 31c598d
visibility: (props) => (props.isDesktop ? 'hidden' : 'visible'), |
Additional feature: ability to toggle preamp gain |
"none": "Disabled", | ||
"album": "Normalize albums", | ||
"track": "Normalize tracks" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions for better descriptions here are appreciated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer "Use Album Gain" and "Use Track Gain".
Thats f.e. how the DSub Android Client states it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, makes sense @metalheim
* ReplayGain support - extract ReplayGain tags from files, expose via native api - use metadata to normalize audio in web player * make pre-push happy * remove unnecessary prints * remove another unnecessary print * add tooltips, see metadata * address comments, use settings instead * remove console.log * use better language for gain modes
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. |
resolve #1850, resolve #233, and part of #1036
This PR seeks to add extraction of ReplayGain tags, the ability to get this data via the native API, and audio normalization in the web player.
The web player is unchanged, I have just elected to use the Web Audio API to wrap around the
<audio>
element. I'm not sure if the gainNode with values > 1 is problematic or not yet (maybe they need to be normalized to 1?).I'd be happy to have some feedback/additional testing (or if someone has a different player, now might be a good time to switch).