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

Units & examples used in DynamicsCompressorNode are ambiguous #2393

Open
citelao opened this issue Nov 30, 2020 · 7 comments
Open

Units & examples used in DynamicsCompressorNode are ambiguous #2393

citelao opened this issue Nov 30, 2020 · 7 comments

Comments

@citelao
Copy link
Contributor

citelao commented Nov 30, 2020

In #2273, I proposed a fix to ambiguous unit usage (linear units vs decibel units) in the specification for the DynamicsCompressorNode. That change fixes a specific ambiguity in the units used for calculating the knee thresholds for the compression curve function.

However, upon working further with the API, it seems that:

  1. There is at least one other ambiguous use of units (in the description of the compression ratio).
  2. The compression curve function as described is non-continuous.

For that reason, I decided to file this bug (per @rtoy's advice).


Ambiguous units

The units in Step 5 of the compression curve are unclear:

This function is linear, based on the ratio, after the threshold and the soft knee (i.e., 𝑓(𝑥)=1 / 𝑟𝑎𝑡𝑖𝑜⋅𝑥).

Since implementers have both linear and exponential units to choose from, the compression curve may behave exponentially differently depending on unit choice. For example, choosing to apply the curve to the decibel value would be off by ~10^(1/ratio) versus using linear values.

We should be clearer here that the ratio applies to the decibel values:

The amount of dB change in input for a 1 dB change in output.

See also Chromium's code(dynamics_compressor_kernel.cc):

float y_db = yknee_threshold_db_ + slope_ * (x_db - knee_threshold_db_);

Non-continuous compression curve

The spec is unambiguous at several points that the compression curve must be continuous and piece-wise differentiable, but the example functions provided, while linear, are not continuous with the other parts of the function and may provide a false impression to implementers (I made this mistake 😄):

This function is linear, based on the ratio, after the threshold and the soft knee (i.e., 𝑓(𝑥)=1 / 𝑟𝑎𝑡𝑖𝑜⋅𝑥).

Chromium implements this as (dynamics_compressor_kernel.cc):

(threshold + knee) + (1 / ratio) * (x - threshold - knee)

While the example function in the spec may simply be to illustrate linear functions, I believe it is confusing and it should be replaced with a more applicable function. Perhaps the above or:

offset + (1 / ratio) * (x - offset)

(Or we should remind readers that the example function is just an example of a linear function).

This also affects the Processing overview:

The compression curve has three parts:

  • The first part is the identity: 𝑓(𝑥)=𝑥
  • The second part is the soft-knee portion, which MUST be a monotonically increasing function.
  • The third part is a linear function: 𝑓(𝑥)=1/𝑟𝑎𝑡𝑖𝑜⋅𝑥

This curve MUST be continuous and piece-wise differentiable, and corresponds to a target output level, based on the input level.


Ask

  • We should do a pass of the DynamicsCompressorNode writeup and ensure that we unambiguously use decibel & linear units where appropriate.
  • We should update the example functions to be more illustrative (or clarify that they are simply examples of linear functions).

Thanks!

@rtoy
Copy link
Member

rtoy commented Dec 1, 2020

Thanks for looking in to this; it's much appreciated.

About your questions, I think yes if you have the time and desire. Clarity in the units is important as well as in the example functions if they are basically wrong.

@citelao
Copy link
Contributor Author

citelao commented Dec 1, 2020

I filed this issue as a first step since I am new to the spec and because I'm worried that I will miss certain times where the units are misused.

I'm happy to take a first pass at this, though!

@rtoy
Copy link
Member

rtoy commented Dec 3, 2020

Teleconf: We wish to limit the scope of new issues on V1 to critical ones. Propose to move this issue to V2.

@rtoy
Copy link
Member

rtoy commented Jan 7, 2021

Moving to v2.

@rtoy rtoy transferred this issue from WebAudio/web-audio-api Jan 7, 2021
@padenot
Copy link
Member

padenot commented May 21, 2021

AudioWG virtual F2F:

  • This is ready for editing (in the v1 text), let us know if you want to give it a shot, in which case we'll review in due time. Otherwise, we'll get to it when we can.

@citelao
Copy link
Contributor Author

citelao commented May 22, 2021

@padenot Thanks for the update! Are you asking if I'd like to give it a shot? I'm happy to do so, but it will take a bit.

@rtoy
Copy link
Member

rtoy commented May 25, 2021

We encourage group participation, so if you can, we'd appreciate a pull request with the fixes you have in mind. If can't, we'll eventually get around to fixing these.

@mdjp mdjp transferred this issue from WebAudio/web-audio-api-v2 Sep 23, 2021
@mdjp mdjp added this to To do in v.next via automation Sep 23, 2021
@mdjp mdjp moved this from Untriaged to Ready For Editing in v.next Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v.next
Ready For Editing
Development

No branches or pull requests

4 participants