Implement DynamicsCompressorNode's processing

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

(Blocks 1 bug)

Trunk
mozilla23
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Assignee

Description

6 years ago
I'm planning to borrow code from Blink for this.
Assignee

Updated

6 years ago
Assignee: nobody → ehsan
Assignee

Comment 5

6 years ago
With some build fixes to make MSVC happy.
Attachment #740116 - Attachment is obsolete: true
Attachment #740116 - Flags: review?(paul)
Attachment #740160 - Flags: review?(paul)
Assignee

Comment 6

6 years ago
More Windows build fixes
Attachment #740160 - Attachment is obsolete: true
Attachment #740160 - Flags: review?(paul)
Attachment #740249 - Flags: review?(paul)
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.
Attachment #740115 - Flags: review?(paul) → review+
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)?
Attachment #740249 - Flags: review?(paul)
Attachment #740249 - Flags: review?(mh+mozilla)
Attachment #740249 - Flags: review+
Assignee

Comment 9

6 years ago
(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.  :(
Assignee

Comment 10

6 years ago
(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.
Flags: needinfo?(jwalden+bmo)
Attachment #740249 - Flags: review?(mh+mozilla) → review+
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
Attachment #740117 - Flags: review?(paul) → review+
Assignee

Comment 12

6 years ago
(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.
Flags: needinfo?(jwalden+bmo)
Assignee

Comment 14

6 years ago
I'll use the FloatingPoint.h stuff then, I guess.  Thanks, Jeff.
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
Depends on: 867964
Assignee

Comment 19

6 years ago
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
Blocks: 924289
Blocks: 924290
Blocks: 944143
You need to log in before you can comment on or make changes to this bug.