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

Display the used volume normalization mode/values instead of target #651

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kepstin
Copy link

@kepstin kepstin commented Sep 14, 2024

Displaying the target loudness in the stream details display isn't very useful, since that's a user-configured setting and does not change depending on what is being played. In place of that number, show information about what volume normalization is currently being applied.

If measurements are available, and are being used, it will display either the track or album measured loudness depending on whether prefer_album_loudness is set.

Otherwise, it will display "Dynamic volume normalization" if loudnorm is being used in dynamic mode - or "Fixed gain correction" if fixed gain is being applied.

If no volume normalization is being applied, the loudness information row will not be displayed.

Screenshot from 2024-09-14 14-28-11

@kepstin
Copy link
Author

kepstin commented Sep 15, 2024

This PR conflicts with the changes made in music-assistant/server#1662 - the API no longer returns the loudness or loudness_album fields.

It looks like there's also an issue regarding types in some of the comparisons - I'm checking if loudness or loudness_album are === null to see if the fields have been populated, but this check doesn't work correctly when the field is omitted (which i think makes it undefined instead of null?)

@marcelveldt
Copy link
Member

This PR conflicts with the changes made in music-assistant/server#1662 - the API no longer returns the loudness or loudness_album fields.

Since then I patched the HA client so we could consider adding those fields back by removing that temporary patch in the serialization. I agree it would be useful to display what is being applied. In fact I invision this dialog to be much more informative at some point:

Audio source
codec/content type
sample rate, bit depth (or bitrate in case of lossy codec)
Provider
loudness measurement (if any)

Audio target/output
What kind of volume normalization is applied ?
What gain adjustment is being made to the audio (in case of volume normalization)
What filters are active ?
What sample rate and bit depth is sent to the final destination ?
Maybe also note the intermediate step in the chain where we use f32
What codec do we use to send the audio to the player ?

Maybe we should just extend the streamdetails object with extra details.
I guess we can iterate in baby steps and your contribution is a great step forward!

Let me know if you want me to reinstate the loudness measurement on the streamdetails object now before merging this one or merge this one first and then do an extra step in a new PR.

@marcelveldt
Copy link
Member

marcelveldt commented Sep 16, 2024

It looks like there's also an issue regarding types in some of the comparisons - I'm checking if loudness or loudness_album are === null to see if the fields have been populated, but this check doesn't work correctly when the field is omitted (which i think makes it undefined instead of null?)

If you check for null it will also catch undefined. For now they are undefined due to that hack to fix HA-side after I changed the value type of the loudness measurement but now that HA is patched we could revert that.

EDIT: If you want to catch both undefined and null you need to check blah == null and not blah === null

// computed properties
const streamDetails = computed(() => {
return store.activePlayerQueue?.current_item?.streamdetails;
});
const loudness = computed(() => {
let sd = streamDetails.value;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let sd = streamDetails.value;
const sd = streamDetails.value;

@OzGav
Copy link
Contributor

OzGav commented Oct 14, 2024

@kepstin Is this still being progressed?

@marcelveldt
Copy link
Member

This PR conflicts with the changes made in music-assistant/server#1662 - the API no longer returns the loudness or loudness_album fields.

These values have been restored

@OzGav
Copy link
Contributor

OzGav commented Dec 21, 2024

@kepstin are you able to progress this?

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.

3 participants