864091 - Implement DynamicsCompressorNode's processing

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[(no longer active)]

I'm planning to borrow code from Blink for this.

Comment 1

[(no longer active)]

Attachment: Part 1: Import the Dynamics Compressor implementation from Blink

Comment 2

[(no longer active)]

Attachment: Part 2: Add the Blink Dynamics Compressor implementation to the build system

Comment 3

[(no longer active)]

Attachment: Part 3: Implement DynamicsCompressorNode's processing based on the Blink's implementation

Comment 4

[(no longer active)]

Demo/testcase: http://people.mozilla.org/~eakhgari/webaudio/compressor.html

Comment 5

[(no longer active)]

Attachment: Part 2: Add the Blink Dynamics Compressor implementation to the build system

With some build fixes to make MSVC happy.

Comment 6

[(no longer active)]

Attachment: Part 2: Add the Blink Dynamics Compressor implementation to the build system

More Windows build fixes

Comment 7

[Paul Adenot (:padenot)]

Comment on attachment 740115 [details] [diff] [review]
Part 1: Import the Dynamics Compressor implementation from Blink

Review of attachment 740115 [details] [diff] [review]:
-----------------------------------------------------------------

Are we doing something like reverse-specing this node? I can see a non-trivial number of things in their code that is not in the spec. Kind of expected considering the relevant paragraph in the spec is like 20 lines tops.

Comment 8

[Paul Adenot (:padenot)]

Comment on attachment 740249 [details] [diff] [review]
Part 2: Add the Blink Dynamics Compressor implementation to the build system

Review of attachment 740249 [details] [diff] [review]:
-----------------------------------------------------------------

Glandium, maybe you can make sure the moz.build and Makefile bits are right?

::: content/media/webaudio/blink/DynamicsCompressorKernel.cpp
@@ +38,5 @@
>  
> +#ifdef XP_WIN
> +bool isnan(float value) { return _isnan(value); }
> +bool isinf(float value) { return !_finite(value); }
> +#endif

Should the comment at the top of FloatingPoint.h saying that terrible things can happen if we use the compiler isnan/isinf, etc. applies in this case (talking about the first paragraph of the first comment block in mfbt/FloatingPoint.h)?

Comment 9

[(no longer active)]

(In reply to comment #7)
> Are we doing something like reverse-specing this node? I can see a non-trivial
> number of things in their code that is not in the spec. Kind of expected
> considering the relevant paragraph in the spec is like 20 lines tops.

Well, this definitely needs to be spec'ed but honestly I would expect the spec to match Blink's implementation anyways.  We will change this code if the spec written will require different processing, of course.  I don't think that it's practical for us to be blocked on the spec getting improved for things like this any more.  :(

Comment 10

[(no longer active)]

(In reply to Paul Adenot (:padenot) from comment #8)
> ::: content/media/webaudio/blink/DynamicsCompressorKernel.cpp
> @@ +38,5 @@
> >  
> > +#ifdef XP_WIN
> > +bool isnan(float value) { return _isnan(value); }
> > +bool isinf(float value) { return !_finite(value); }
> > +#endif
> 
> Should the comment at the top of FloatingPoint.h saying that terrible things
> can happen if we use the compiler isnan/isinf, etc. applies in this case
> (talking about the first paragraph of the first comment block in
> mfbt/FloatingPoint.h)?

Hmm, maybe.  Jeff, do you know if we should be afraid of this here?

The problem is that FloatingPoint.h doesn't support floats, and I'm not sure if the float->double implicit conversions will screw up inf/nans (but I have no reason to believe that they do either.)

Anyways, using the FloatingPoint.h stuff here is very easy if we decide that's the way to go.

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 740117 [details] [diff] [review]
Part 3: Implement DynamicsCompressorNode's processing based on the Blink's implementation

Review of attachment 740117 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webaudio/DynamicsCompressorNode.cpp
@@ +142,5 @@
> +      {
> +        nsRefPtr<DynamicsCompressorNode> node = static_cast<DynamicsCompressorNode*>(mStream->Engine()->Node());
> +        if (node) {
> +          AudioParam* reduction = node->Reduction();
> +          reduction->CancelAllEvents();

There seem to be no way in the spec to say that |reduction.value| is readonly, so authors can set it to various things without effect. Any technical reason this |reduction| member is an AudioParam at all and not a double, or even a function à la AnalyzerNode to make clear that this may return a different value each time it is called?

And how can we do side channel compression [1] with this interface? I'm going to raise that to the mailing list, because having a compressor without side channel capabilities is a serious problem for all sorts of application ranging from music production to radio broadcast. Works great, though.

[1]: https://en.wikipedia.org/wiki/Side_chain_%28sound%29#Side-chaining

Comment 12

[(no longer active)]

(In reply to Paul Adenot (:padenot) from comment #11)
> Comment on attachment 740117 [details] [diff] [review]
> Part 3: Implement DynamicsCompressorNode's processing based on the Blink's
> implementation
> 
> Review of attachment 740117 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webaudio/DynamicsCompressorNode.cpp
> @@ +142,5 @@
> > +      {
> > +        nsRefPtr<DynamicsCompressorNode> node = static_cast<DynamicsCompressorNode*>(mStream->Engine()->Node());
> > +        if (node) {
> > +          AudioParam* reduction = node->Reduction();
> > +          reduction->CancelAllEvents();
> 
> There seem to be no way in the spec to say that |reduction.value| is
> readonly, so authors can set it to various things without effect. Any
> technical reason this |reduction| member is an AudioParam at all and not a
> double, or even a function à la AnalyzerNode to make clear that this may
> return a different value each time it is called?

Yeah, this is basically really stupid, it should just be a read-only double :(  Or perhaps some kind of dummy object which provides a read-only value property.  I'll bring it up on the list.

> And how can we do side channel compression [1] with this interface? I'm
> going to raise that to the mailing list, because having a compressor without
> side channel capabilities is a serious problem for all sorts of application
> ranging from music production to radio broadcast. Works great, though.
> 
> [1]: https://en.wikipedia.org/wiki/Side_chain_%28sound%29#Side-chaining

No idea about side channel compression, please raise a spec issue about it.  Thanks!

Comment 13

[Jeff Walden [:Waldo]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #10)
> > Should the comment at the top of FloatingPoint.h saying that terrible things
> > can happen if we use the compiler isnan/isinf, etc. applies in this case
> > (talking about the first paragraph of the first comment block in
> > mfbt/FloatingPoint.h)?
> 
> Hmm, maybe.  Jeff, do you know if we should be afraid of this here?

The failures that have shown up using the compiler stuff are very finicky -- think PGO builds when the compiler methods are used in very particular places, with no obvious explanation for when/why a failure does/doesn't happen.  I would be no more or less afraid about this than at any other place.  Which is to say, moderately scared without a particularized reason to be scared.

Consistency and One Way To Do It would say do it the same way everywhere, though, so that there's no chance of having to laboriously investigate some bizarre failure.

> The problem is that FloatingPoint.h doesn't support floats, and I'm not sure
> if the float->double implicit conversions will screw up inf/nans (but I have
> no reason to believe that they do either.)

Float->double conversion in C++ are exact, when the input value can be represented in the output type.  That's the case for infinities and NaN (although obviously this breaks down with respect to there being multiple NaN values, but I assume you're not distinguishing different NaN bit patterns here).  So it's perfectly fine to pass a float into these methods and depend on double conversion being exact.  I haven't heard of any failures popping up from a mis-conversion.

It'd be mostly trivial to add float versions of all these methods in addition to double versions, if you wanted to avoid even this concern.  Just a matter of doubling up the methods and adjusting a few constants, mostly.

Comment 14

[(no longer active)]

I'll use the FloatingPoint.h stuff then, I guess.  Thanks, Jeff.

Comment 15

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50ad07904584
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2ead35253b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2f43cd4a753

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/50ad07904584
https://hg.mozilla.org/mozilla-central/rev/c2ead35253b0
https://hg.mozilla.org/mozilla-central/rev/a2f43cd4a753

Comment 17

[Oleg Romashin (:romaxa)]

linux arm build failing on 
media/webaudio/blink/DenormalDisabler.h:116: error: 'FLT_MIN' was not declared in this scope

Comment 18

[Oleg Romashin (:romaxa)]

try with float.h moved out of WIN ifdef
https://hg.mozilla.org/try/rev/b887be2fddf8

Comment 19

[(no longer active)]

Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.