924288 - BiquadFilterNode should continue producing output a short time after input stops

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Karl Tomlinson (:karlt)]

Consider passing an impulse into a low pass filter.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: no need to set filter parameters after every process()

Some clean-up.

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: continue producing BiquadFilter sound output after input becomes null

There are similar issues with channel count changes, like in bug 857610 but probably needing a different solution.

This at least deals with probably the most common case of input changing to null.
We need to use null blocks to save processing as in bug 923319, so they are kind of code for "silence with unknown channel count".

Comment 3

[(no longer active)]

Comment on attachment 814653 [details] [diff] [review]
no need to set filter parameters after every process()

Review of attachment 814653 [details] [diff] [review]:
-----------------------------------------------------------------

Haha! :-)

Comment 4

[(no longer active)]

Comment on attachment 814658 [details] [diff] [review]
continue producing BiquadFilter sound output after input becomes null

Review of attachment 814658 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webaudio/BiquadFilterNode.cpp
@@ +141,5 @@
>                                   const AudioChunk& aInput,
>                                   AudioChunk* aOutput,
>                                   bool* aFinished) MOZ_OVERRIDE
>    {
> +    float inputBuffer[WEBAUDIO_BLOCK_SIZE];

= {0.0f} to actually start with a silent buffer.

@@ +164,5 @@
> +        aOutput->SetNull(WEBAUDIO_BLOCK_SIZE);
> +        return;
> +      }
> +
> +      PodArrayZero(inputBuffer);

You should be able to remove this now.

@@ -147,5 @@
>      }
>  
> -    // Adjust the number of biquads based on the number of channels
> -    const uint32_t numberOfChannels = aInput.mChannelData.Length();
> -    mBiquads.SetLength(numberOfChannels);

Hmm, can you please add some debugging code which does an NS_WARNING if the number of channels change here, to ease debugging test cases which do that in the future?

::: content/media/webaudio/test/test_biquadFilterNodeWithGain.html
@@ +46,5 @@
> +    // silence.
> +    var blend = context.createGain();
> +    biquad1.connect(blend);
> +    biquad2.connect(blend);
> +    biquadWithGain.connect(blend);

This is awesome!  :-)

Comment 5

[Karl Tomlinson (:karlt)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> > +    float inputBuffer[WEBAUDIO_BLOCK_SIZE];
> 
> = {0.0f} to actually start with a silent buffer.

> > +      PodArrayZero(inputBuffer);
> 
> You should be able to remove this now.

inputBuffer is often not used at all, and the multichannel non-unit mVolume case depends on AudioBlockCopyChannelWithScale() overwriting all values for each channel, so I'd prefer to zero-initialize only when necessary.

> > -    // Adjust the number of biquads based on the number of channels
> > -    const uint32_t numberOfChannels = aInput.mChannelData.Length();
> > -    mBiquads.SetLength(numberOfChannels);
> 
> Hmm, can you please add some debugging code which does an NS_WARNING if the
> number of channels change here, to ease debugging test cases which do that
> in the future?

Good idea.  Will do.

Comment 6

[(no longer active)]

Comment on attachment 814658 [details] [diff] [review]
continue producing BiquadFilter sound output after input becomes null

Review of attachment 814658 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webaudio/BiquadFilterNode.cpp
@@ +194,1 @@
>          input = inputBuffer;

(In reply to Karl Tomlinson (:karlt) from comment #5)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> > > +    float inputBuffer[WEBAUDIO_BLOCK_SIZE];
> > 
> > = {0.0f} to actually start with a silent buffer.
> 
> > > +      PodArrayZero(inputBuffer);
> > 
> > You should be able to remove this now.
> 
> inputBuffer is often not used at all, and the multichannel non-unit mVolume
> case depends on AudioBlockCopyChannelWithScale() overwriting all values for
> each channel, so I'd prefer to zero-initialize only when necessary.

Fair enough!  So please do that here.

Comment 7

[Karl Tomlinson (:karlt)]

Landed just the cleanup part.  The rest is dependent on changes in bug 923301.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4599a83b871

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a4599a83b871

Comment 9

[Karl Tomlinson (:karlt)]

Landed with an update to use DispatchToMainThreadAfterStreamStateUpdate() for the PlayingRefChangeHandlers consistent with attachment 820836 [details] [diff] [review]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8fe38e7b4f7

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d8fe38e7b4f7