BiquadFilterNode should continue producing output a short time after input stops

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

Trunk
mozilla27
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Consider passing an impulse into a low pass filter.
Assignee: nobody → karlt
Status: NEW → ASSIGNED
Some clean-up.
Attachment #814653 - Flags: review?(ehsan)
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".
Attachment #814658 - Flags: review?(ehsan)
Comment on attachment 814653 [details] [diff] [review]
no need to set filter parameters after every process()

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

Haha! :-)
Attachment #814653 - Flags: review?(ehsan) → review+
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!  :-)
Attachment #814658 - Flags: review?(ehsan) → review+
(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 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.
Landed just the cleanup part.  The rest is dependent on changes in bug 923301.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4599a83b871
Depends on: 923301
Whiteboard: [leave open]
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
Flags: in-testsuite+
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/d8fe38e7b4f7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 939491
You need to log in before you can comment on or make changes to this bug.