Bug 77856 – Have the DynamicsCompressorNode support multi-channel data

• Component: Web Audio
• Status: RESOLVED FIXED

[Raymond]

Hi

Current DynamicsCompressorNode only support mono and stereo audio data. need to add the support for multi channel.

And current DynamicsCompressorNode seems do not handle stereo audio data well. when measure the compressor input level, it take the average of left and right channel data as :

  compressorInput = 0.5f * (undelayedL + undelayedR);

While this is not reasonable. suppose left channel and right channel have the same sin wave but with phase difference of PI, then the compressorInput will always be 0, thus lead to wrong compressor gain.

I think it might be better to have

 compressorInput = 0.5f * (abs(undelayedL) + abs(undelayedR));

while, I still think this is not quite reasonable, since the purpose here is to figure out the common gain for both channel so that the sound won't shift position, at the same time it should also have each channel be compressed correctly. when we support more channels, say 5.1 channels, if only 1 channel has loud volume, the average of all channel will be very low thus this very channel won't get compressed correctly.

Thus I think

 compressorInput = max(abs(undelayedL), abs(undelayedR)) might be even better.


I would like to provide a patch to handle multiple channels and have this issue fix at the same time. Any comments?

[Raymond]

Created attachment 125583 [details]
compressor result

This attached img show case a stereo sound with sin wave in left and right channel with a PI phase difference.

up part is wave form before compress, down part is wave form after compress. (with audio route: source->analyser->compressor->analyser2->destination)

In the image the delay should be introduced by compressorNode's predalay, while you can see the sin wave get wrongly compressed. ( when left and right channel have the same phase, the output will be perfectly compressed)

[Raymond]

Created attachment 125584 [details]
compressor result 2

And if use compressorInput = max(abs(left), abs(right)), the compressed result will be perfectly.

[Chris Rogers]

(In reply to comment #2)
> Created an attachment (id=125584) [details]
> compressor result 2
> 
> And if use compressorInput = max(abs(left), abs(right)), the compressed result will be perfectly.

Hi Raymond, so far I've tested:
0.5f * (abs(undelayedL) + abs(undelayedR));

which appears to work well on a variety of real-world source material.  Please give me a little time to test your second approach (using max).  I understand your reasoning, but want to be very careful about how the change translates to real-world source material which I've been testing with the algorithm for a long time.  I'll get back with the results...

[Raymond]

> Hi Raymond, so far I've tested:
> 0.5f * (abs(undelayedL) + abs(undelayedR));
> 
> which appears to work well on a variety of real-world source material.  Please give me a little time to test your second approach (using max).  I understand your reasoning, but want to be very careful about how the change translates to real-world source material which I've been testing with the algorithm for a long time.  I'll get back with the results...

Hi Chris

Great! I only test a sin wave (which is too simple for real world case ) and a few music (which I can only say it sounds to me ok, but I don't know whether they are typical enough to cover the real world case). If you can verify on your source (which I believe are much more complete and typical) that will be really really wonderful ;)

[Raymond]

> 
> Hi Raymond, so far I've tested:
> 0.5f * (abs(undelayedL) + abs(undelayedR));
> 
> which appears to work well on a variety of real-world source material.  Please give me a little time to test your second approach (using max).  I understand your reasoning, but want to be very careful about how the change translates to real-world source material which I've been testing with the algorithm for a long time.  I'll get back with the results...

Hi Chris

I test the 0.5f * (abs(undelayedL) + abs(undelayedR)) approaching and max approach again today.

With 3 scenario

1: L=sin(t) R=sin(t+PI)
2: L=sin(t) R=sin(t+PI/2)
3: L=sin(t) R=0

the 0.5f * (abs(undelayedL) + abs(undelayedR)) not only works for case 1, but not works for case 2 and case 3.

While the max approach works fine for all these 3 cases.

[Raymond]

(In reply to comment #5)
> 
> the 0.5f * (abs(undelayedL) + abs(undelayedR)) not only works for case 1, but not works for case 2 and case 3.
> 

sorry, typo, not only->only

[Chris Rogers]

(In reply to comment #5)
> > 
> > Hi Raymond, so far I've tested:
> > 0.5f * (abs(undelayedL) + abs(undelayedR));
> > 
> > which appears to work well on a variety of real-world source material.  Please give me a little time to test your second approach (using max).  I understand your reasoning, but want to be very careful about how the change translates to real-world source material which I've been testing with the algorithm for a long time.  I'll get back with the results...
> 
> Hi Chris
> 
> I test the 0.5f * (abs(undelayedL) + abs(undelayedR)) approaching and max approach again today.
> 
> With 3 scenario
> 
> 1: L=sin(t) R=sin(t+PI)
> 2: L=sin(t) R=sin(t+PI/2)
> 3: L=sin(t) R=0
> 
> the 0.5f * (abs(undelayedL) + abs(undelayedR)) not only works for case 1, but not works for case 2 and case 3.
> 
> While the max approach works fine for all these 3 cases.

Hi Raymond, I've had a chance to test the max approach on a variety of source material and it seems to work well.  So, this seems fine to me.  I think it should be safe to land, but I will continue listening carefully as the changes go in...

[Raymond]

(In reply to comment #7)

> Hi Raymond, I've had a chance to test the max approach on a variety of source material and it seems to work well.  So, this seems fine to me.  I think it should be safe to land, but I will continue listening carefully as the changes go in...

Hi Chris

Thanks, then I will upload a patch to support have the DynamicsCompressorNode to support multi-channel and have the compressorInput caculation changes applied together. btw, Can you take look on Bug https://bugs.webkit.org/show_bug.cgi?id=78057 ? It's a simple quick fix on preDealy time in DynamicsCompressorNode

[Chris Rogers]

Thanks Raymond.  By the way I'm also working in this code right now, but I don't think it will interfere with your changes.  I'm working on adding the threshold, ratio, and knee parameters to adjust the static compression curve.  I have a comment mentioning at least "threshold" here:
https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#DynamicsCompressorNode-section

[Raymond]

(In reply to comment #9)
> Thanks Raymond.  By the way I'm also working in this code right now, but I don't think it will interfere with your changes.  I'm working on adding the threshold, ratio, and knee parameters to adjust the static compression curve.  I have a comment mentioning at least "threshold" here:
> https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#DynamicsCompressorNode-section

Hi chris, cool!

The patch I am uploading involve changes in DynamicsCompressorNode, DynamicsCompressor and DynamicsCompressorKernel. These changes are brought due to the fact that after enabling multi channel support these code will works within the scope of AudioBasicProcessorNode. So I rebase the class to it. If it conflict with your changes, I will rebase the code after your commit.

[Raymond]

Created attachment 126223 [details]
Patch

[Raymond]

Created attachment 126233 [details]
Patch

[Raymond]

patch updated to rebase on the latest code.

[Chris Rogers]

Comment on attachment 126233 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126233&action=review

Some very early comments here - I've just glanced at the patch, but have some concerns about the common case of mixed mono and stereo sources killing filter state in the ZeroPole filters.

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:45
> +    , m_numberOfChannels(0)

Maybe we start off with a default value of 2 for stereo, and then try to keep processing at stereo even if only mono channels are feeding it?  Often, the compressor is used right at the output stage.
I'm wondering what happens in the case of a mixed set of mono and stereo sources (notes being played non-overlapping but frequently) if setNumberOfChannels() is frequently called for values 1 and 2.
The ZeroPole filter "packs" are going to be continually re-built, destroying any filter state in them.  It would be great to avoid "thrashing" them...

> Source/WebCore/platform/audio/DynamicsCompressor.h:96
> +    } ZeroPoleFileterPack4;

spelling nit: ZeroPoleFileterPack4 -> ZeroPoleFilterPack4

[Chris Rogers]

Also, please look into mac build failure.

[Raymond]

(In reply to comment #14)
> (From update of attachment 126233 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126233&action=review
> 
> Some very early comments here - I've just glanced at the patch, but have some concerns about the common case of mixed mono and stereo sources killing filter state in the ZeroPole filters.
> 
> > Source/WebCore/platform/audio/DynamicsCompressor.cpp:45
> > +    , m_numberOfChannels(0)
> 
> Maybe we start off with a default value of 2 for stereo, and then try to keep processing at stereo even if only mono channels are feeding it?  Often, the compressor is used right at the output stage.
> I'm wondering what happens in the case of a mixed set of mono and stereo sources (notes being played non-overlapping but frequently) if setNumberOfChannels() is frequently called for values 1 and 2.
> The ZeroPole filter "packs" are going to be continually re-built, destroying any filter state in them.  It would be great to avoid "thrashing" them...
> 

Hi Chris,
Very true! we need to reserve the filter state at our best. And I am wondering, maybe, only in the case that when channels count shrink, we need to reserve the remained channels' filter state. e.g.

stereo->mono, we just remove the extra one instead of rebuilt all of them or reserve both channel untouched. For any way, when a notes is added back later, and changes mono->stereo, the right channel's filter's state is useless, since they are totally different audio.

In the way, we can handle not only stereo<->mono switching, but also apply to other 5.1 <-> stereo switching etc.

Any comments?

[WebKit Review Bot]

Comment on attachment 126233 [details]
Patch

Attachment 126233 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11447072

[Chris Rogers]

(In reply to comment #16)
> (In reply to comment #14)
> > (From update of attachment 126233 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=126233&action=review
> > 
> > Some very early comments here - I've just glanced at the patch, but have some concerns about the common case of mixed mono and stereo sources killing filter state in the ZeroPole filters.
> > 
> > > Source/WebCore/platform/audio/DynamicsCompressor.cpp:45
> > > +    , m_numberOfChannels(0)
> > 
> > Maybe we start off with a default value of 2 for stereo, and then try to keep processing at stereo even if only mono channels are feeding it?  Often, the compressor is used right at the output stage.
> > I'm wondering what happens in the case of a mixed set of mono and stereo sources (notes being played non-overlapping but frequently) if setNumberOfChannels() is frequently called for values 1 and 2.
> > The ZeroPole filter "packs" are going to be continually re-built, destroying any filter state in them.  It would be great to avoid "thrashing" them...
> > 
> 
> Hi Chris,
> Very true! we need to reserve the filter state at our best. And I am wondering, maybe, only in the case that when channels count shrink, we need to reserve the remained channels' filter state. e.g.
> 
> stereo->mono, we just remove the extra one instead of rebuilt all of them or reserve both channel untouched. For any way, when a notes is added back later, and changes mono->stereo, the right channel's filter's state is useless, since they are totally different audio.

That's not really the way it works, the right-channel filter state technically needs to maintain continuity.  Remember that the time between these transitions from 2 -> 1 -> 2 could be very very small, happening in rapid succession.

> 
> In the way, we can handle not only stereo<->mono switching, but also apply to other 5.1 <-> stereo switching etc.
> 
> Any comments?

I know you might not like this idea at first, but I propose that the compressor number-of-channels remain fixed at 2 by default, so that if there's a mixture of mono and stereo notes feeding it (or a combination of mono and stereo coming and going), then it always up-mixes to stereo to avoid the filter thrash.  Then if it's desired to have the compressor output 6 channels (6.1, etc.) then we can add a way to configure the output channels explicitly (in JavaScript API).  We're already going to have to have such an API anyway for the ConvolverNode, because its output number-of-channels and matrixing is so interlinked.

[Raymond]

(In reply to comment #18)
> 
> I know you might not like this idea at first, but I propose that the compressor number-of-channels remain fixed at 2 by default, so that if there's a mixture of mono and stereo notes feeding it (or a combination of mono and stereo coming and going), then it always up-mixes to stereo to avoid the filter thrash.  Then if it's desired to have the compressor output 6 channels (6.1, etc.) then we can add a way to configure the output channels explicitly (in JavaScript API).  We're already going to have to have such an API anyway for the ConvolverNode, because its output number-of-channels and matrixing is so interlinked.

OK, Sounds reasonable. I will update the patch later.

[Raymond]

Created attachment 126413 [details]
Patch

[Raymond]

(In reply to comment #14)

Hi Chris
Patch updated according to your comments.


> 
> Some very early comments here - I've just glanced at the patch, but have some concerns about the common case of mixed mono and stereo sources killing filter state in the ZeroPole filters.
> 
> > Source/WebCore/platform/audio/DynamicsCompressor.cpp:45
> > +    , m_numberOfChannels(0)
> 
> Maybe we start off with a default value of 2 for stereo, and then try to keep processing at stereo even if only mono channels are feeding it?  Often, the compressor is used right at the output stage.

Current patch add the multi-channel support in DynamicsCompressorKernel(full support) and DynamicsCompressor(partly support by switch/case). While limit the channel number to 2 by default in DynamicsCompressorNode. Thus we can easily extend its capability to support other channel layout and at the same time keep the current solution friendly to mono<->stereo switching case.

> 
> > Source/WebCore/platform/audio/DynamicsCompressor.h:96
> > +    } ZeroPoleFileterPack4;
> 
> spelling nit: ZeroPoleFileterPack4 -> ZeroPoleFilterPack4

Fixed.

And, The mac build issue is due to parameter mis-matching ( unsinged and size_t ) , since DynamicsCompressor no longer subclass from AudioProcessor, this issue is also addressed.

Please take a review again. Thanks!

[Chris Rogers]

Comment on attachment 126413 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126413&action=review

Raymond, I've added a few comments -- I'll try to do a more complete review tomorrow, but seems to be basically good

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:114
> +

nit: extra blank lines

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:131
> +    typedef const float* constFloatPtr;

not sure if the typedef is worth it

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:147
> +        ASSERT_NOT_REACHED();

I think we need to be safer here and call destinationBus->zero() then return early to handle the release build...

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:200
> +    constFloatPtr* inplaceSourceChannels = (constFloatPtr*)destinationChannels;

WebKit style: we can't have a "normal" C style cast -- but looking closer at this, do we even need a cast?  Normally, it should be OK to pass a non-const pointer to a const pointer argument?

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:281
> +                for (unsigned i = 0; i < numberOfChannels; ++i) {

Any idea what kind of performance penalty (if any) there is for having these inner-loops versus the old code?  Hopefully not too much :)

[Raymond]

Created attachment 126460 [details]
Patch

[Raymond]

(In reply to comment #22)

Hi Chris

Patch updated according to your comments.

> 
> Raymond, I've added a few comments -- I'll try to do a more complete review tomorrow, but seems to be basically good
> 

Thanks! 

> > Source/WebCore/platform/audio/DynamicsCompressor.cpp:114
> > +
> 
> nit: extra blank lines
> 

Fixed.

> > Source/WebCore/platform/audio/DynamicsCompressor.cpp:131
> > +    typedef const float* constFloatPtr;
> 
> not sure if the typedef is worth it
>

Just try to make the code more readable, but you are right, const float** is also ok. Fixed.
 
> > Source/WebCore/platform/audio/DynamicsCompressor.cpp:147
> > +        ASSERT_NOT_REACHED();
> 
> I think we need to be safer here and call destinationBus->zero() then return early to handle the release build...

Fixed.

> 
> > Source/WebCore/platform/audio/DynamicsCompressor.cpp:200
> > +    constFloatPtr* inplaceSourceChannels = (constFloatPtr*)destinationChannels;
> 
> WebKit style: we can't have a "normal" C style cast -- but looking closer at this, do we even need a cast?  Normally, it should be OK to pass a non-const pointer to a const pointer argument?
> 

Hmm, if it is float * -> const float *, or float** -> float const**, it can be converted implicitly. While from float** p -> const float** p, it can't, since it's not p itself need to be cast, but *p need to be cast.

I also tried static_cast and reinterpret_cast here, neither is working. only the C style cast works here.

But, you are right, we should avoid using the C style cast, and even it is working, now I doubt that is it really correct. So I changed the code to assign the value manually to solve this issue.

> > Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:281
> > +                for (unsigned i = 0; i < numberOfChannels; ++i) {
> 
> Any idea what kind of performance penalty (if any) there is for having these inner-loops versus the old code?  Hopefully not too much :)

I guess it should be ok? Since these calculation itself need to be done anyway, and the loop index overhead is very little ( I am not sure is there any other overhead versus the old code other then that we need to increase the index?) , and also compare to other part of the inner loop code, it is a relative small part.

[Raymond]

Hi Chris

Anything else I need to fix on this patch? ;)

[Chris Rogers]

Hi Raymond, sorry I've been late on reviewing this patch.  I've been very busy!
I'll try my best to have a look tomorrow

[Chris Rogers]

Comment on attachment 126460 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126460&action=review

Looking good overall!

> Source/WebCore/ChangeLog:8
> +        No new tests required.

I would just remove this line

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:103
> +        ZeroPole* preFilter = &(m_preFilterPacks[i]->filters[stageIndex]);

This is just a detail, but I think it's slightly more elegant to make "preFilter" a reference instead of pointer:

ZeroPole& preFilter = m_preFilterPacks[i]->filters[stageIndex];

It's a little less ugly looking

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:110
> +        ZeroPole* postFilter = &(m_postFilterPacks[i]->filters[stageIndex]);

see above comment about reference instead of pointer

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:126
> +    unsigned numberOfChannels = destinationBus->numberOfChannels();

Maybe for naming consistency, call this "numberOfDestinationChannels"

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:127
> +    unsigned numberOfSourceChannels = destinationBus->numberOfChannels();

Wait, shouldn't this be:

sourceBus->numberOfChannels()

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:134
> +        sourceChannels[0] = sourceBus->channel(0)->data();

What if numberOfSourceChannels (assuming this means sourceBus->numberOfChannels()) equals 0?

Maybe we need to detect this and exit early (zeroing the destinationBus)

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:138
> +        else

Maybe add simple comment here explaining that in the case of mono input and stereo output, we simply copy the mono channel

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:-183
> -                         destinationR,

I read your comment about troubles casting related to inplaceSourceChannels.
If we do keep this code (using temporary inplaceSourceChannels) at least can we add a comment here (with FIXME) to suggest looking at a more elegant solution than copying into a temporary buffer like this.
I have a feeling there must be a better way (one solution is to just remove the "const" in the DynamicsCompressorKernel::process() method)

> Source/WebCore/platform/audio/DynamicsCompressor.h:96
>      // Emphasis filters.

Maybe could change the comment to:

// Per-channel emphasis filters.

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:57
> +    , m_numberOfChannels(numberOfChannels)

I think we can get rid of m_numberOfChannels (see comments below in setNumberOfChannelsMethod()

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:63
> +        m_preDelayBuffers.append(adoptPtr(new AudioFloatArray(MaxPreDelayFrames)));

This is duplicated code in setNumberOfChannels().

How about simply calling:
setNumberOfChannels(numberOfChannels);

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:73
> +    m_preDelayBuffers.clear();

This isn't necessary, the destructor will tear down m_preDelayBuffers automatically

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:78
> +    if (numberOfChannels == m_numberOfChannels)

Can we instead check using m_preDelayBuffers.size()?  This avoids needing to keep track of m_numberOfChannels.

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:106
> +                                       float* destinationBuffers[],

When we call this method, we use the word "channel" instead of "buffer".  Also, I'd like to avoid confusion with the AudioBuffer class, so
I suggest naming these:

sourceChannels and destinationChannels (both in this file and in the header file)

> Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:124
> +    ASSERT(m_numberOfChannels == numberOfChannels);

maybe can ASSERT(m_preDelayBuffers.size() == numberOfChannels)

> Source/WebCore/platform/audio/DynamicsCompressorKernel.h:48
> +                 float* destinationBuffers[],

I think better names would be:

sourceChannels
destinationChannels

> Source/WebCore/platform/audio/DynamicsCompressorKernel.h:74
> +    unsigned m_numberOfChannels;

Can we get rid of this member variable?  See other comments in setNumberOfChannels() method

> Source/WebCore/webaudio/DynamicsCompressorNode.h:54
> +    enum { DefaultNumberOfOutputChannels = 2 };

Any reason this needs to be an enum in the header file?  Could we move this to the .cpp as a static const:

static const unsigned defaultNumberOfChannels = 2;

near the top of the file

[Raymond]

Created attachment 127494 [details]
Patch

[Raymond]

(In reply to comment #27)

Hi Chris
Thanks for your review in great detail!
Patch updated according to your review.

> > Source/WebCore/ChangeLog:8
> > +        No new tests required.
> 
> I would just remove this line

Removed.

> > Source/WebCore/platform/audio/DynamicsCompressor.cpp:103
> > +        ZeroPole* preFilter = &(m_preFilterPacks[i]->filters[stageIndex]);
> 
> This is just a detail, but I think it's slightly more elegant to make "preFilter" a reference instead of pointer:
> 
> ZeroPole& preFilter = m_preFilterPacks[i]->filters[stageIndex];
> 
> It's a little less ugly looking
> 

All related Fixed.

> 
> > Source/WebCore/platform/audio/DynamicsCompressor.cpp:126
> > +    unsigned numberOfChannels = destinationBus->numberOfChannels();
> 
> Maybe for naming consistency, call this "numberOfDestinationChannels"

It's due to the reason that later this numberOfchannels is used for both source and destination channel loops. I add some comments there to describe it.

> > Source/WebCore/platform/audio/DynamicsCompressor.cpp:127
> > +    unsigned numberOfSourceChannels = destinationBus->numberOfChannels();
> 
> Wait, shouldn't this be:
> 
> sourceBus->numberOfChannels()
> 

Oops, Fixed.

> > Source/WebCore/platform/audio/DynamicsCompressor.cpp:134
> > +        sourceChannels[0] = sourceBus->channel(0)->data();
> 
> What if numberOfSourceChannels (assuming this means sourceBus->numberOfChannels()) equals 0?
> 
> Maybe we need to detect this and exit early (zeroing the destinationBus)

It won't, the Channels number will never be 0, its mini value will be 1, this is assured by the AudioNodeInput.

> 
> > Source/WebCore/platform/audio/DynamicsCompressor.cpp:138
> > +        else
> 
> Maybe add simple comment here explaining that in the case of mono input and stereo output, we simply copy the mono channel

Done.

> 
> > Source/WebCore/platform/audio/DynamicsCompressor.cpp:-183
> > -                         destinationR,
> 
> I read your comment about troubles casting related to inplaceSourceChannels.
> If we do keep this code (using temporary inplaceSourceChannels) at least can we add a comment here (with FIXME) to suggest looking at a more elegant solution than copying into a temporary buffer like this.
> I have a feeling there must be a better way (one solution is to just remove the "const" in the DynamicsCompressorKernel::process() method)

Ok, I think to remove the const might be the best way at present. Done.

> > Source/WebCore/platform/audio/DynamicsCompressor.h:96
> >      // Emphasis filters.
> 
> Maybe could change the comment to:
> 
> // Per-channel emphasis filters.

Done.

> 
> > Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:57
> > +    , m_numberOfChannels(numberOfChannels)
> 
> I think we can get rid of m_numberOfChannels (see comments below in setNumberOfChannelsMethod()

All related fixed.

> > Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:73
> > +    m_preDelayBuffers.clear();
> 
> This isn't necessary, the destructor will tear down m_preDelayBuffers automatically

oops. leave out this one. will update patch again.

> 
> > Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp:106
> > +                                       float* destinationBuffers[],
> 
> When we call this method, we use the word "channel" instead of "buffer".  Also, I'd like to avoid confusion with the AudioBuffer class, so
> I suggest naming these:
> 
> sourceChannels and destinationChannels (both in this file and in the header file)

Quite right! Fixed.

> 
> > Source/WebCore/webaudio/DynamicsCompressorNode.h:54
> > +    enum { DefaultNumberOfOutputChannels = 2 };
> 
> Any reason this needs to be an enum in the header file?  Could we move this to the .cpp as a static const:
> 
> static const unsigned defaultNumberOfChannels = 2;
> 
> near the top of the file

Oh, this is due to that the DynamicCompressorKernel use this approaching to define many thing like : MaxPreDelayFrames,MaxPreDelayFramesMask etc. I try to keep the same approach.

If you think this should be fixed. I will fix them in another patch later? to find out is there any other more need to be fixed.

[Raymond]

Created attachment 127496 [details]
Patch

[Chris Rogers]

Comment on attachment 127496 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127496&action=review

This looks like it's almost finished - a few comments:

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:104
> +        // Set pre-filter zero and pole to create an emphasis filter.

nit: I'd move this comment up just before line 103

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:138
> +        sourceChannels[0] = sourceBus->channel(0)->data();

I understand your comment that AudioNodeInput assures that sourceBus should have at least one channel, but I don't think we should trust that and make any assumptions.  We should
still protect and be paranoid about that kind of thing.

So, I still suggest doing both an ASSERT and early return (zeroing destinationBus) if sourceBus has 0 channels.  That should probably be somewhere before the switch statement...

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:148
> +        // FIXME : support other layouts.

I wouldn't use the word "layout" here, but instead "support other number of channels"

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:-183
> -                         destinationR,

It looks like you accidentally deleted the two lines of comments.

> Source/WebCore/webaudio/DynamicsCompressorNode.h:54
> +    enum { DefaultNumberOfOutputChannels = 2 };

Although the DynamicsCompressorKernel uses the 'enum' approach, I think it's slightly against WebKit style (we don't have to go fix the DynamicsCompressorKernel stuff now).
But for any new code, we should put this in the .cpp file

[Chris Rogers]

FYI, after this patch is landed, I hope to land this one:
https://bugs.webkit.org/show_bug.cgi?id=78937

[Raymond]

(In reply to comment #32)
> FYI, after this patch is landed, I hope to land this one:
> https://bugs.webkit.org/show_bug.cgi?id=78937

Okay, Thanks. I am occupied this weekend. I will update this patch as soon as I return to office next Monday morning, Hope this won't block your patch.

[Chris Rogers]

(In reply to comment #33)
> (In reply to comment #32)
> > FYI, after this patch is landed, I hope to land this one:
> > https://bugs.webkit.org/show_bug.cgi?id=78937
> 
> Okay, Thanks. I am occupied this weekend. I will update this patch as soon as I return to office next Monday morning, Hope this won't block your patch.

No problem, I'm sure it won't block the other patch.  Have a good weekend!

[Raymond]

Created attachment 127747 [details]
Patch

[Raymond]

(In reply to comment #31)

Hi Chris
All fixed and patch updated.

[WebKit Review Bot]

Comment on attachment 127747 [details]
Patch

Clearing flags on attachment: 127747

Committed r108263: <http://trac.webkit.org/changeset/108263>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yuta Kitamura]

Your change broke Chromium builds on Windows. Please fix this ASAP, otherwise I'm going to roll out this change.

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20Builder/builds/20684/steps/compile/logs/stdio

1>------ Build started: Project: webcore_platform, Configuration: Release Win32 ------
1>Compiling...
1>DynamicsCompressor.cpp
1>..\platform\audio\DynamicsCompressor.cpp(140) : error C2057: expected constant expression
1>..\platform\audio\DynamicsCompressor.cpp(140) : error C2466: cannot allocate an array of constant size 0
1>..\platform\audio\DynamicsCompressor.cpp(140) : error C2133: 'sourceChannels' : unknown size
1>..\platform\audio\DynamicsCompressor.cpp(141) : error C2057: expected constant expression
1>..\platform\audio\DynamicsCompressor.cpp(141) : error C2466: cannot allocate an array of constant size 0
1>..\platform\audio\DynamicsCompressor.cpp(141) : error C2133: 'destinationChannels' : unknown size
1>Build log was saved at "file://c:\b\build\slave\webkit-win-latest-rel\build\src\build\Release\obj\webcore_platform\BuildLog.htm"
1>webcore_platform - 6 error(s), 0 warning(s)

[Yuta Kitamura]

Comment on attachment 127747 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127747&action=review

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:141
> +    const float* sourceChannels[numberOfChannels];
> +    float* destinationChannels[numberOfChannels];

This isn't valid C++ because numberOfChannnels is not a compile-time constant. (I'm rather surprised some compilers doesn't get angry about this.)

[Yuta Kitamura]

Reverted r108263 for reason:

Broke Chromium Windows build.

Committed r108271: <http://trac.webkit.org/changeset/108271>

[Raymond]

(In reply to comment #41)
> Reverted r108263 for reason:
> 
> Broke Chromium Windows build.
> 
> Committed r108271: <http://trac.webkit.org/changeset/108271>

oh, sorry, always working on Linux, Never try windows. Will fix this.

[Raymond]

Created attachment 127876 [details]
Patch

[Raymond]

(In reply to comment #40)
> (From update of attachment 127747 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127747&action=review
> 
> > Source/WebCore/platform/audio/DynamicsCompressor.cpp:141
> > +    const float* sourceChannels[numberOfChannels];
> > +    float* destinationChannels[numberOfChannels];
> 
> This isn't valid C++ because numberOfChannnels is not a compile-time constant. (I'm rather surprised some compilers doesn't get angry about this.)

Fixed, Hope this one works.

[Chris Rogers]

Comment on attachment 127876 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127876&action=review

> Source/WebCore/platform/audio/DynamicsCompressor.cpp:141
> +    float** destinationChannels = new float* [numberOfChannels];

Unfortunately it's very much against WebKit style to use raw "new" and "delete" like this.  Here are two solutions I can think of:

1.  Revert back to the old way using simple local arrays, but define a "maxNumberOfChannels" static constant for this class (and add ASSERTs for this constant here and in setNumberOfChannels()
2.  Use WTF::OwnArrayPtr, then the code would be something like this

OwnArrayPtr<float*> destinationChannels = adoptArrayPtr(new float* [numberOfChannels]);

Using (2) then you don't have to worry about calling delete [] as you have below, since it automatically does this.  The reason WebKit prefers this is to protect against possible leaks which can happen when using raw "new" and "delete" calls.

If using (2) then it probably would be even better to allocate the "sourceChannels" and "destinationChannels" variables as member variables which are initialized in setNumberOfChannels() to avoid the constant allocs/deallocs...

[Raymond]

Created attachment 128142 [details]
Patch

[Raymond]

(In reply to comment #45)

> 2.  Use WTF::OwnArrayPtr, then the code would be something like this
> 
> OwnArrayPtr<float*> destinationChannels = adoptArrayPtr(new float* [numberOfChannels]);
> 
> Using (2) then you don't have to worry about calling delete [] as you have below, since it automatically does this.  The reason WebKit prefers this is to protect against possible leaks which can happen when using raw "new" and "delete" calls.
> 
> If using (2) then it probably would be even better to allocate the "sourceChannels" and "destinationChannels" variables as member variables which are initialized in setNumberOfChannels() to avoid the constant allocs/deallocs...

Thanks, Chris
Patch updated.

[Chris Rogers]

Thanks Raymond, looks good.

[WebKit Review Bot]

Comment on attachment 128142 [details]
Patch

Clearing flags on attachment: 128142

Committed r108538: <http://trac.webkit.org/changeset/108538>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raymond]

(In reply to comment #48)
> Thanks Raymond, looks good.

Chris, Thanks for spending so much time on review this patch. I learn a lot during patching the patch :)