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

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

With some build fixes to make MSVC happy.

More Windows build fixes

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 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)?

(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. :(

(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 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

(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!

(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.

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

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

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

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

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

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