Closed Bug 1276483 Opened 4 years ago Closed 4 years ago

WaveShaper doesn't work, when connected from a silent GainNode

Categories

(Core :: Web Audio, defect, P2)

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- affected
firefox48 --- affected
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: mohayonao, Assigned: dminor)

References

Details

(Whiteboard: [parity-Chrome][parity-Edge])

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160502172042

Steps to reproduce:

WaveShaper doesn't work, when connected from a silent GainNode. It makes all output 0.0.

```js
const audioContext = new OfflineAudioContext(1, 100, 44100);
const oscillator = audioContext.createOscillator();
const gain = audioContext.createGain();
const waveShaper = audioContext.createWaveShaper();

oscillator.start(0);
oscillator.connect(gain);

// to silence
gain.gain.value = 0;
gain.connect(waveShaper);

// convert all signal into 1.0
waveShaper.curve = new Float32Array([ 1, 1 ]);
waveShaper.connect(audioContext.destination);

startRenderting().then((audioBuffer) => {
  var result = audioBuffer.getChannelData(0);

  console.assert( result.every(x => x === 1) );
});
```


Actual results:

All values are 0. Perhaps not processing.


Expected results:

All values should be 1.
Could you attach minimum testcase or public url for testing the bug?
Flags: needinfo?(mohayonao)
test is here

https://jsfiddle.net/mohayonao/dob2yrvp/

if use setValueAtTime instead of .value, I got expected results (1.0).
Component: Untriaged → Web Audio
Product: Firefox → Core
Whiteboard: [parity-Chrome][parity-Edge]
Flags: needinfo?(mohayonao)
Thanks for the report.

Creating a DC waveform is an important use case IMO.

Note that Chromium (45.0.2454.15) behavior is quite broken too.
Not connecting gain to waveShaper or not starting the oscillator results in the same broken behavior as Firefox.
Blocks: 865251
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Version: 46 Branch → 25 Branch
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Rank: 25
Version: 25 Branch → 50 Branch
Comment on attachment 8762102 [details]
Bug 1276483 - Fix WaveShaper when connected from a silent GainNode;

https://reviewboard.mozilla.org/r/58978/#review56388

Cool, can we have a quick test for this one ?
Attachment #8762102 - Flags: review?(padenot)
https://reviewboard.mozilla.org/r/58978/#review56442

::: dom/media/webaudio/GainNode.cpp:68
(Diff revision 1)
>        float gain = mGain.GetValue();
> -      if (gain == 0.0f) {
> -        aOutput->SetNull(WEBAUDIO_BLOCK_SIZE);
> -      } else {
> -        *aOutput = aInput;
> +      *aOutput = aInput;
> -        aOutput->mVolume *= gain;
> +      aOutput->mVolume *= gain;
> -      }

The web audio nodes depend on being able to detect silence for efficiency of processing.
This change would defeat that.  I doubt that cost is really worth the benefit of just partially fixing a bug in WaveShaper.

It would be much better to address this in WaveShaper.  Although a full fix in WaveShaper would be more complicated, I expect a partial fix at least as effective as this change would not be difficult.
(In reply to Karl Tomlinson (ni?:karlt) from comment #6)

> It would be much better to address this in WaveShaper.  Although a full fix
> in WaveShaper would be more complicated, I expect a partial fix at least as
> effective as this change would not be difficult.

Sure thing. Out of curiosity, what would you consider a full fix to be?
Hrm that's embarrassing, I was convinced I was reviewing code in WaveShaperNode.cpp.
(In reply to Dan Minor [:dminor] from comment #7)
> Sure thing. Out of curiosity, what would you consider a full fix to be?

A partial fix would be to make WaveShaper produce non-silent output even when input is silent, iff the middle value of the curve is non-zero.
A more complete fix would use MarkActive()/MarkInactive() to keep such a node alive to keep producing output even when it has no inputs.
A full fix would need a fix for bug 897796 to ensure such a WaveShaper is still collected when not observable.
It's hard to say whether the "more complete" fix should wait for bug 897796 or not.
It's a choice between spontaneously disappearing signals or a leak.  Given chromium has disappearing signals, that may not be as bad as it sounds.

Bug 961932 is related.  PlayingRefChangeHandler would be required for that.
See Also: → 961932
Calling SetNull on an AudioBlock clears mChannelData and ChannelCount() returns the size of mChannelData so it seems like there isn't a tidy way to recover the channel count in the WaveShaper after SetNull has been called in the GainNode.

One way around this would be to keep track of the channel count separately from the size of mChannelData and use IsNull() to determine if the block is null or not. I'm not sure if that would be sufficient in the MarkActive()/MarkInactive() version of the fix.

Is there a better way I'm missing?
(In reply to Dan Minor [:dminor] from comment #10)
> Calling SetNull on an AudioBlock clears mChannelData and ChannelCount()
> returns the size of mChannelData so it seems like there isn't a tidy way to
> recover the channel count in the WaveShaper after SetNull has been called in
> the GainNode.

There is an existing issue that channel counts are not tracked correctly, yes.

> One way around this would be to keep track of the channel count separately
> from the size of mChannelData and use IsNull() to determine if the block is
> null or not.

I've wondered about that before, but modifying AudioChunk would only be a
partial solution to the channel count problem.

There is a similar problem when inactive nodes are garbage collected.
Their output channel count should remain on the downstream node inputs if they have not been explicitly disconnected.

This suggests that downstream nodes should track the output channel counts of
their upstream nodes, at least after they are garbage collected.  Whether this
is best limited to collected nodes (when only the maximum channel count need
be remembered) and combined with AudioChunk channel count, or whether it is
better to track channel counts even from living nodes, I don't know.

However, I don't think this needs to be solved to address this bug.

I assume you are asking about this due to
"The number of channels of the output always equals the number of channels of
the input."

Note that Web Audio almost always has a minimum channel count of 1.  "Each
output has one or more channels."  "If the input has no connections then it
has one channel which is silent."  The WaveShaper will still produce one
channel of output, even when there are no input connections.

When channelInterpretation = "speakers" is used on all nodes, there is little
opportunity to observe a difference between one channel or multiple channels
equivalent to upmixing a single channel.

> I'm not sure if that would be sufficient in the
> MarkActive()/MarkInactive() version of the fix.

For this bug, MarkActive()/MarkInactive() decisions can be based on main
thread information only.  The node needs to be marked active as soon as the
non-zero-middle curve is set (if there may be an observable output); it can't let the node be GCed while waiting for a signal from the graph thread.  When bug 961932 is also considered, it gets much more complicated, and I suspect PlayingRefChangeHandler would not simply fit in with the main thread decisions.
Thanks Karl!
I've filed Bug 1281001 (with references back to this bug) to cover the better fixes that Karl mentioned.
Attachment #8762102 - Attachment is obsolete: true
Comment on attachment 8763636 [details]
Bug 1276483 - Fix WaveShaper when connected from a silent GainNode;

https://reviewboard.mozilla.org/r/59812/#review56874
Attachment #8763636 - Flags: review?(padenot) → review+
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a73cc327229
Fix WaveShaper when connected from a silent GainNode; r=padenot
https://hg.mozilla.org/mozilla-central/rev/0a73cc327229
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1283910
You need to log in before you can comment on or make changes to this bug.