feat: Allow logarithmic amplification (needs input on API)

[daalfox]

Adds volume control in decibels discussed in #714

[daalfox]

This is my first contribution on this crate. Just checking in with you about the general approach for this. Is it ok?

[dvdsk]

I understand the choices you made however I think we can do better. Let me know what you think of the feedback.

Further more did you look at the resource Roderick linked in the issue? It features a slightly more involved calculation that leads to a nicer volume curve. We could use that instead or have both the current log and that nicer one.

[daalfox]

Yeah I wanted to keep myself in the same page with you. I just skimmed through the article I will check it out. Thanks for review

[roderickvd]

You can look at the code I linked to, which compresses that article into 10 lines of code. You can copy and/or modify those lines from me under public domain.

[daalfox]

You can look at the code I linked to, which compresses that article into 10 lines of code. You can copy and/or modify those lines from me under public domain.

@roderickvd Do I add this to Sink, SpacialSink and ChannelVolume and call it set_normalized_volume or something? Maybe I open a new PR for that?

[dvdsk]

Take a look at source/mod.rs. Do we want a amplify_decibel() in there?

[dvdsk]

@roderickvd Do I add this to Sink, SpacialSink and ChannelVolume and call it set_normalized_volume or something? Maybe I open a new PR for that?

I would also add it to amplify. But you are right, there are many places we deal with volume atm, we will probably want it there too.

[roderickvd]

@roderickvd Do I add this to Sink, SpacialSink and ChannelVolume and call it set_normalized_volume or something? Maybe I open a new PR for that?

I would also add it to amplify. But you are right, there are many places we deal with volume atm, we will probably want it there too.

This is why I liked @PetrGlad's suggestion here: #714 (comment)

If we just add such public helper methods, then the user can plug it where-ever they want.

[daalfox]

So maybe we keep to_linear that is for any decibel value, an internal api and we add a public to_linear_normalized interface to change a linear value from [0.0, 1.0] to a logarithmic one?
Not sure if I'm understanding this currectly

[daalfox]

The main issue I'm having here is that I don't understand how I fit the mapping that @roderickvd suggested into the Amplify interface
To my little knowledge I got from this PR, this Amplify struct just multiplies each sample in the source with the given factor and we don't deal with volume here at all.
@roderickvd Am I missing something?

[roderickvd]

Maybe we need to sketch out & agree on the API first. To me, Sink::set_volume and Source::amplify are the same in that both modify signal amplitude.

// Direct linear gain
sink.set_volume(1.5);  // amplify by 150%
source.amplify(0.5);   // attenuate to 50%

// Using decibels
sink.set_volume(db_to_gain(6.0));  // +6 dB
source.amplify(db_to_gain(-3.0));  // -3 dB

// Using normalized with logarithmic scaling
sink.set_volume(norm_to_log_gain(0.5));   // 50% perceived volume
source.amplify(norm_to_log_gain(0.75));   // 75% perceived volume

[daalfox]

The thing I like about the Amplify is that it just amplifies the samples and doesn't care about the human interpretation of loudness and changing that feels wrong. I also think that just adding some public helper functions feel a lot nicer.
@dvdsk What do you think?

[dvdsk]

Terms

Maybe we need to sketch out & agree on the API first. To me, Sink::set_volume and Source::amplify are the same in that both modify signal amplitude.

Funny, to me setting volume and amplifying are different :)

Lets see what kind of terms we got:

• Volume: absolute signal strength at output
• Gain: change to signal strength before output
• Amplification: unit of gain?

The name of Sink::set_volume follows that definition. Since Sink is connected directly into the output mixer it is the last part of a rodio pipeline so you could say amplifying their is modifying the volume.

amplifies the samples and doesn't care about the human interpretation of loudness and changing that feels wrong

Yes I get that. That makes sense from a programming standpoint. On the other hand human hearing is logarithmic so linear amplification is generally not the best from a UI perspective and thus should not be the default.

Helper vs UI

adding some public helper functions feel a lot nicer

That would make it clear that decibels is just another way of expressing that amplification factor. On the other hand its not super discoverable and using a factor would still be the default.

this:

some_source
  .pausable()
  .fade_in(Duration::from_secs(60))
  .amplify(to_linear(10));

other_source
  .pausable()
  .fade_in(Duration::from_secs(60))
  .amplify(1.1);

is to me less clear then:

some_source
  .pausable()
  .fade_in(Duration::from_secs(60))
  .amplify_by_db(10);

some_source
  .pausable()
  .fade_in(Duration::from_secs(60))
  .amplify_by_factor(1.1);

Its a difficult UI to get right....

[dvdsk]

sink.set_volume(norm_to_log_gain(0.5));   // 50% perceived volume
source.amplify(norm_to_log_gain(0.75));   // 75% perceived volume

I like the use of perceived there, maybe perceived_gain would work somewhere. amplify_with_perceived_increase (using increase since is an easier word then gain).

[dvdsk]

I agree with @roderickvd btw and I think we should keep going back and forth till we all agree we have the best possible API.

[daalfox]

Maybe it's best to close this and revisit it at some point later wdyt? I plan to help with refactoring on this crate. Is there any particular section you want me to work on? Feel like amplify can also be used in place of set_volumes

[dvdsk]

Maybe it's best to close this and revisit it at some point later wdyt?

I see no reason to close this, we need a little more input to figure out the best API though. I'll mark it as help wanted and hope someone else chimes in.

[dvdsk]

Feel like amplify can also be used in place of set_volumes

on sink? yes though that would be a breaking change affecting a lot of users. Since we already want to redo sink entirely I rather keep such a change till then. You could start doing that, but that will be quite difficult. A good starting point on it would be figuring out how to do proper cross-fade (see the user stories issue for what a proper cross-fade is).

Maybe @roderickvd or @PetrGlad know whats a good issue to work on while we wait for more input on this.

[roderickvd]

Maybe @roderickvd or @PetrGlad know whats a good issue to work on while we wait for more input on this.

You know, I'd actually like to get agreement on the API and have @daalfox continue work on it. It's useful, not too hard, and has already begun, so let's also complete it. We just need agreement.

Feel like amplify can also be used in place of set_volumes

on sink? yes though that would be a breaking change affecting a lot of users. Since we already want to redo sink entirely I rather keep such a change till then.

I also do not see the point of breaking the current API, although:

• it'd be OK to deprecate one if we'd really decide that would be the way to go (not convinced)
• I do agree that they currently do the same thing

Because w.r.t. to your @dvdsk stating:

• Volume: absolute signal strength at output

I don't disagree with that definition, but that's currently not what it does. set_volume just multiplies the sample value by some number, without taking into account any gain that became before. As far as I can see, Sink even delegates to Amplify to do it.

They are the same today, and I don't recognize a need for set_volume to know about & compensate for any prior gain. In the future, I do think it's be great if it could (optionally?) gradually ramp volume up/down when changing volume, in order to prevent pops and clicks.

So back to the API... I'm for not breaking the current API if we don't need to, and we don't need to. So it comes down to the question: do we want helper functions (less discoverable, easy to plug as arguments to the existing functions) or do we want new functions? Let's take it from there and then we can decide on naming.

[daalfox]

I have one more question, do we panic outside of [0.0, 1.0] range for normalized?

[dvdsk]

I have one more question, do we panic outside of [0.0, 1.0] range for normalized?

We could also return a Result<(), OutOfRangeError>. That might be better since it would trigger users to read the docs :). Which would note the required range and have an Errors section pointing that out.

[dvdsk]

You know, I'd actually like to get agreement on the API

agreed

So it comes down to the question: do we want helper functions (less discoverable, easy to plug as arguments to the existing functions) or do we want new functions?

I prefer new functions

[roderickvd]

I have one more question, do we panic outside of [0.0, 1.0] range for normalized?

We’re talking about the 0-100% “volume slider” right? For ease of use we could also simply clamp and document that.

[dvdsk]

I have one more question, do we panic outside of [0.0, 1.0] range for normalized?

We’re talking about the 0-100% “volume slider” right? For ease of use we could also simply clamp and document that.

yeah lets do that 👍 We should highlight the clamping behavior in the documentations example.

[daalfox]

Feel free to rephrase the documentations as you like.

[roderickvd]

Thanks a lot picking this up again.

[dvdsk]

This looks great to me! Ended up being quite a nuanced feature. Thank you for getting this through!

[daalfox]

Yeah that was something. Thank you everyone

[dvdsk]

missed something (no worries its the tiniest thing). This deserves a change-log entry! :)

[daalfox]

missed something (no worries its the tiniest thing). This deserves a change-log entry! :)

Done

[roderickvd]

Awesome thanks for hanging out with us 👍

As one last improvement may I request that you reword the changelog statement of "better volume control" to be more specific like "perceptually linear"?

[dvdsk]

🥳