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)

Assignee

Description

6 years ago
Consider passing an impulse into a low pass filter.
Assignee

Updated

6 years ago
Assignee: nobody → karlt
Status: NEW → ASSIGNED
Assignee

Comment 1

6 years ago
Some clean-up.
Attachment #814653 - Flags: review?(ehsan)
Assignee

Comment 2

6 years ago
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 3

6 years ago
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 4

6 years ago
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+
Assignee

Comment 5

6 years ago
(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

6 years ago
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.
Assignee

Comment 7

6 years ago
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]
Assignee

Comment 9

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Updated

6 years ago
Depends on: 939491
You need to log in before you can comment on or make changes to this bug.