Make reduction value of dynamics compressor zero when no sources are connected

This fixes the issue of 'internal state' in DynamicsCompressorNode.

When a compressor node is in the active signal chain. the reduction
(.reduction) value returns the reduced amount of gain from
the compression. However, when user requests the reduction value after
the signal chain is deactivated (i.e. a source node stops), the returned
value is the last calculated in the compressor. This value should be
zero since there is no incoming audio signal.

Repro case:
http://jsfiddle.net/hoch/k5e3meuL/1/

Related StackOverflow post:
http://stackoverflow.com/questions/26413675/how-to-fix-frozen-div-when-using-compressor-reduction-value-to-monitor-compressi

BUG=425616
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184208

[hongchan, 2014-10-21 18:35:47]

hongchan@chromium.org changed reviewers:
+ rtoy@chromium.org

[Raymond Toy, 2014-10-21 19:28:56]

https://codereview.chromium.org/645853010/diff/1/LayoutTests/webaudio/dynamic...
File LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html (right):

https://codereview.chromium.org/645853010/diff/1/LayoutTests/webaudio/dynamic...
LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html:32: }
I would simplify this by removing the variable success, and just add an else
statement with line 34.

https://codereview.chromium.org/645853010/diff/1/LayoutTests/webaudio/dynamic...
LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html:45: // Create
the constant sample buffer of 0.5 sec.
Correct the comment. It's not 0.5 sec, right?

https://codereview.chromium.org/645853010/diff/1/LayoutTests/webaudio/dynamic...
LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html:49: compressor
= context.createDynamicsCompressor();
The compressor doesn't seem to have anything connected to it or connects to
anything. Is that right?

https://codereview.chromium.org/645853010/diff/1/LayoutTests/webaudio/dynamic...
LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html:51:
compressor.ratio.value = -8.0;
Why these values? Does it really matter for the test?

https://codereview.chromium.org/645853010/diff/1/LayoutTests/webaudio/dynamic...
LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html:58:
source.stop(0.5);
Why 0.5 sec? The value of testDurationSamples is much less than 0.5 sec, so
maybe you don't need stop here.

https://codereview.chromium.org/645853010/diff/1/Source/modules/webaudio/Dyna...
File Source/modules/webaudio/DynamicsCompressorNode.cpp (right):

https://codereview.chromium.org/645853010/diff/1/Source/modules/webaudio/Dyna...
Source/modules/webaudio/DynamicsCompressorNode.cpp:113:
m_reduction->setValue(0);
Any other state we want to clear here?

[Raymond Toy, 2014-10-21 19:32:37]

Use a better description of the issue. Maybe something like "Make reduction
value of dynamics compressor zero when no sources are connected" Or something.

[hongchan, 2014-10-21 20:20:37]

https://codereview.chromium.org/645853010/diff/1/LayoutTests/webaudio/dynamic...
File LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html (right):

https://codereview.chromium.org/645853010/diff/1/LayoutTests/webaudio/dynamic...
LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html:32: }
On 2014/10/21 19:28:56, Raymond Toy wrote:
> I would simplify this by removing the variable success, and just add an else
> statement with line 34.

I tried to keep the code untouched as much as possible. Let me do the general
clean up and refactor the test code.

https://codereview.chromium.org/645853010/diff/1/LayoutTests/webaudio/dynamic...
LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html:45: // Create
the constant sample buffer of 0.5 sec.
On 2014/10/21 19:28:56, Raymond Toy wrote:
> Correct the comment. It's not 0.5 sec, right?

I have to figure out this problem - because the line here is different to the
code on my local branch.

https://codereview.chromium.org/645853010/diff/1/LayoutTests/webaudio/dynamic...
LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html:49: compressor
= context.createDynamicsCompressor();
On 2014/10/21 19:28:56, Raymond Toy wrote:
> The compressor doesn't seem to have anything connected to it or connects to
> anything. Is that right?

The same problem, the changes were not applied to here.

https://codereview.chromium.org/645853010/diff/1/LayoutTests/webaudio/dynamic...
LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html:51:
compressor.ratio.value = -8.0;
On 2014/10/21 19:28:56, Raymond Toy wrote:
> Why these values? Does it really matter for the test?

This is to make sure that the compression to actually happen.

https://codereview.chromium.org/645853010/diff/1/LayoutTests/webaudio/dynamic...
LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html:58:
source.stop(0.5);
On 2014/10/21 19:28:56, Raymond Toy wrote:
> Why 0.5 sec? The value of testDurationSamples is much less than 0.5 sec, so
> maybe you don't need stop here.

The reason behind this is that we play the buffer 0.5 sec and stop, and the
audio context will continue for another 0.5sec rendering the zero audio signal.
In that way, we can verify the reduction value when there is no active
connection and incoming signal.

https://codereview.chromium.org/645853010/diff/1/Source/modules/webaudio/Dyna...
File Source/modules/webaudio/DynamicsCompressorNode.cpp (right):

https://codereview.chromium.org/645853010/diff/1/Source/modules/webaudio/Dyna...
Source/modules/webaudio/DynamicsCompressorNode.cpp:113:
m_reduction->setValue(0);
On 2014/10/21 19:28:56, Raymond Toy wrote:
> Any other state we want to clear here? 

The other parameters are user-modifiable. I believe the reduction value is the
only value that needs to be zeroed.

[Raymond Toy, 2014-10-21 20:47:57]

Almost there. A few nits and a couple more comments.

https://codereview.chromium.org/645853010/diff/20001/LayoutTests/webaudio/dyn...
File LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html (right):

https://codereview.chromium.org/645853010/diff/20001/LayoutTests/webaudio/dyn...
LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html:48: // Create
comporessor and set parameters accordingly.
Nit: Typo: "comporessor"

https://codereview.chromium.org/645853010/diff/20001/LayoutTests/webaudio/dyn...
LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html:59:
source.stop(0.5);
Nit: this seems redundant. The source is 0.5 sec long so it will stop after 0.5
sec. Calling stop here is not needed.

https://codereview.chromium.org/645853010/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioNode.cpp (right):

https://codereview.chromium.org/645853010/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.cpp:482: // Resets internal states for future
external data request. (i.e. Gain reduction)
I think I would remove this comment.  The comment for the clearInternalState
method should explain what is happening and why.

https://codereview.chromium.org/645853010/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.cpp:483: if (nodeType() ==
NodeTypeDynamicsCompressor) {
This if test isn't needed.  Every node now has a clearInternalState method, and
the DynamicsCompressorNode has the right implementation to clear the internal
state.

https://codereview.chromium.org/645853010/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioNode.h (right):

https://codereview.chromium.org/645853010/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.h:124: // from providing the incorrect data
based on the last-known data inside.
"deactivated" -> "disabled". I think this comment is not quite right. Perhaps
something like:

Clear internal state when the node is disabled.  When a node is disabled, it is
no longer pulled so any internal state is never updated. But some nodes
(DynamicsCompressorNode) have internal state that is still accessible by the
user.  Update the internal state as if the node were still connected but
processing all zeroes. This gives a consistent view to the user.

https://codereview.chromium.org/645853010/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.h:125: virtual void clearInternalState();
If the comment above is used, perhaps this function should be called
updateInternalStateWhenDisabled instead of clearInternalState?

[hongchan, 2014-10-21 20:58:54]

https://codereview.chromium.org/645853010/diff/20001/LayoutTests/webaudio/dyn...
File LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html (right):

https://codereview.chromium.org/645853010/diff/20001/LayoutTests/webaudio/dyn...
LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html:48: // Create
comporessor and set parameters accordingly.
On 2014/10/21 20:47:57, Raymond Toy wrote:
> Nit: Typo: "comporessor"

Acknowledged.

https://codereview.chromium.org/645853010/diff/20001/LayoutTests/webaudio/dyn...
LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html:59:
source.stop(0.5);
On 2014/10/21 20:47:57, Raymond Toy wrote:
> Nit: this seems redundant. The source is 0.5 sec long so it will stop after
0.5
> sec. Calling stop here is not needed.

Acknowledged.

https://codereview.chromium.org/645853010/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioNode.cpp (right):

https://codereview.chromium.org/645853010/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.cpp:482: // Resets internal states for future
external data request. (i.e. Gain reduction)
On 2014/10/21 20:47:57, Raymond Toy wrote:
> I think I would remove this comment.  The comment for the clearInternalState
> method should explain what is happening and why.

Acknowledged.

https://codereview.chromium.org/645853010/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.cpp:483: if (nodeType() ==
NodeTypeDynamicsCompressor) {
On 2014/10/21 20:47:57, Raymond Toy wrote:
> This if test isn't needed.  Every node now has a clearInternalState method,
and
> the DynamicsCompressorNode has the right implementation to clear the internal
> state.

I agree, but this was what we talked about it before so I left it there.
Removing.

https://codereview.chromium.org/645853010/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioNode.h (right):

https://codereview.chromium.org/645853010/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.h:124: // from providing the incorrect data
based on the last-known data inside.
On 2014/10/21 20:47:57, Raymond Toy wrote:
> "deactivated" -> "disabled". I think this comment is not quite right. Perhaps
> something like:
> 
> Clear internal state when the node is disabled.  When a node is disabled, it
is
> no longer pulled so any internal state is never updated. But some nodes
> (DynamicsCompressorNode) have internal state that is still accessible by the
> user.  Update the internal state as if the node were still connected but
> processing all zeroes. This gives a consistent view to the user.

Acknowledged and replaced.

https://codereview.chromium.org/645853010/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.h:125: virtual void clearInternalState();
On 2014/10/21 20:47:57, Raymond Toy wrote:
> If the comment above is used, perhaps this function should be called
> updateInternalStateWhenDisabled instead of clearInternalState?

Good to be more descriptive. But I think this method can be used in other places
where require the reset the internal state. updateInternalState() should be
okay. What do you think?

[Raymond Toy, 2014-10-21 21:05:54]

https://codereview.chromium.org/645853010/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioNode.h (right):

https://codereview.chromium.org/645853010/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.h:125: virtual void clearInternalState();
On 2014/10/21 20:58:54, hoch wrote:
> On 2014/10/21 20:47:57, Raymond Toy wrote:
> > If the comment above is used, perhaps this function should be called
> > updateInternalStateWhenDisabled instead of clearInternalState?
> 
> Good to be more descriptive. But I think this method can be used in other
places
> where require the reset the internal state. updateInternalState() should be
> okay. What do you think?

Hard to say what the future holds. Ok for now, as long as we are careful not to
overuse this to clear internal state.  The comment, however, makes it clear what
this method is supposed to do and when.

[hongchan, 2014-10-21 21:16:48]

On 2014/10/21 21:05:54, Raymond Toy wrote:
>
https://codereview.chromium.org/645853010/diff/20001/Source/modules/webaudio/...
> File Source/modules/webaudio/AudioNode.h (right):
> 
>
https://codereview.chromium.org/645853010/diff/20001/Source/modules/webaudio/...
> Source/modules/webaudio/AudioNode.h:125: virtual void clearInternalState();
> On 2014/10/21 20:58:54, hoch wrote:
> > On 2014/10/21 20:47:57, Raymond Toy wrote:
> > > If the comment above is used, perhaps this function should be called
> > > updateInternalStateWhenDisabled instead of clearInternalState?
> > 
> > Good to be more descriptive. But I think this method can be used in other
> places
> > where require the reset the internal state. updateInternalState() should be
> > okay. What do you think?
> 
> Hard to say what the future holds. Ok for now, as long as we are careful not
to
> overuse this to clear internal state.  The comment, however, makes it clear
what
> this method is supposed to do and when.

Okay, then I think we want to name it as clearInternalStateWhenDisabled(). It is
more of clearing than updating (calculating) in a meaningful way.

[Raymond Toy, 2014-10-21 23:25:51]

lgtm with nits.

https://codereview.chromium.org/645853010/diff/40001/LayoutTests/webaudio/dyn...
File LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html (right):

https://codereview.chromium.org/645853010/diff/40001/LayoutTests/webaudio/dyn...
LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html:51:
compressor.ratio.value = -8.0;
Nit: Add comment that the threshold and ratio values are selected to make sure
compression happens.  These are fairly arbitrary, right?

https://codereview.chromium.org/645853010/diff/40001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioNode.h (right):

https://codereview.chromium.org/645853010/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.h:128: // to the user.
Nit: I think blink style is to use one space after a period. (I have a long
habit of using two.) This is no longer enforced but you're supposed to use a
style consistent with the file.

[hongchan, 2014-10-21 23:43:21]

https://codereview.chromium.org/645853010/diff/40001/LayoutTests/webaudio/dyn...
File LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html (right):

https://codereview.chromium.org/645853010/diff/40001/LayoutTests/webaudio/dyn...
LayoutTests/webaudio/dynamicscompressor-clear-internal-state.html:51:
compressor.ratio.value = -8.0;
On 2014/10/21 23:25:51, Raymond Toy wrote:
> Nit: Add comment that the threshold and ratio values are selected to make sure
> compression happens.  These are fairly arbitrary, right?

Actually the default parameter values for threshold and ratio is almost same
with the setting above. It was my habit to set parameters explicitly, but these
should be removed since it is redundant.

https://codereview.chromium.org/645853010/diff/40001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioNode.h (right):

https://codereview.chromium.org/645853010/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.h:128: // to the user.
On 2014/10/21 23:25:51, Raymond Toy wrote:
> Nit: I think blink style is to use one space after a period. (I have a long
> habit of using two.) This is no longer enforced but you're supposed to use a
> style consistent with the file.

Acknowledged.

[hongchan, 2014-10-22 17:28:19]

The CQ bit was checked by hongchan@chromium.org

[commit-bot: I haz the power, 2014-10-22 17:29:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/645853010/30007

[commit-bot: I haz the power, 2014-10-22 18:34:03]

Committed patchset #4 (id:30007) as 184208