815643 - Implement ConvolverNode

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[(no longer active)]

ConvolverNode is defined here: <https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#ConvolverNode-section>.  The implementation would be very similar to the other node types that we have implemented.

Comment 1

[Andrew Quartey [:drexler]]

Attachment: WIP

Incomplete patch - missing a test and correct implementation of how it carries out a linear convolution. 

Couple of questions. In the specification, 'impulse response' and 'audio data' are mentioned. Am i to assume they're the same? 

In the attributes section (4.16.1), under buffer it states:
" At the time when this attribute is set, the buffer and the state of the normalize attribute will be used to configure the ConvolverNode with this impulse response having the given normalization". What's does |given normalization| refer to?

Similarly:
"If the normalize attribute is false when the buffer attribute is set then the ConvolverNode will perform a linear convolution given the exact impulse response contained within the buffer. "  
I'll need help in figuring out how it carries out the linear convolution and where it directs the output. In the this WIP, i assume it's carried out in conjunction with the result of |calculateNormalizationScale| when the |buffer| attribute is set.

Comment 2

[(no longer active)]

Comment on attachment 730905 [details] [diff] [review]
WIP

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

This is a great start!

::: content/media/webaudio/ConvolverNode.cpp
@@ +51,5 @@
> +    */
> +}
> +
> +/*
> + * Iimplementation of normalization scale algorithm as detailed in :

Nit: typo.

@@ +55,5 @@
> + * Iimplementation of normalization scale algorithm as detailed in :
> + * <https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#ConvolverNode-section>
> + */
> +float
> +ConvolverNode::ComputeNormalizationScale()

So the function inside the spec has been copied from the WebKit source code, and that function has diverged from the spec since then.  I have raised a spec issue about this.

@@ +72,5 @@
> +  {
> +    JSObject* channelObj = mBuffer->GetChannelData(i);
> +    JSObject* arrayBuffer = JS_GetArrayBufferViewBuffer(channelObj);
> +    uint8_t* data =  JS_GetArrayBufferData(arrayBuffer);
> +    float* channelData = reinterpret_cast<float*>(data);

static_cast

::: content/media/webaudio/ConvolverNode.h
@@ +42,5 @@
> +
> +private:
> +  nsRefPtr<AudioBuffer> mBuffer;
> +  bool mNormalize;
> +  float ComputeNormalizationScale();

Nit: please separate the private methods and fields like this:

private:
  float method();

private:
  nsRefPtr field;

Comment 3

[(no longer active)]

(In reply to Andrew Quartey [:drexler] from comment #1)
> Created attachment 730905 [details] [diff] [review]
> WIP
> 
> Incomplete patch - missing a test and correct implementation of how it
> carries out a linear convolution. 
> 
> Couple of questions. In the specification, 'impulse response' and 'audio
> data' are mentioned. Am i to assume they're the same? 

I think the buffer attribute of ConvolverNode is the impulse response, and the incoming data flowing through it is the audio data.

> In the attributes section (4.16.1), under buffer it states:
> " At the time when this attribute is set, the buffer and the state of the
> normalize attribute will be used to configure the ConvolverNode with this
> impulse response having the given normalization". What's does |given
> normalization| refer to?

this.normalize.

> Similarly:
> "If the normalize attribute is false when the buffer attribute is set then
> the ConvolverNode will perform a linear convolution given the exact impulse
> response contained within the buffer. "  

This means that in this case the impulse response will not be normalized and will be used as is.

> I'll need help in figuring out how it carries out the linear convolution and
> where it directs the output. In the this WIP, i assume it's carried out in
> conjunction with the result of |calculateNormalizationScale| when the
> |buffer| attribute is set.

Implementing linear convolution is apparently quite challenging, see <https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/convolution.html>.  I talked to Chris Rogers about this last week, and he convinced me to consider borrowing this code from WebKit.  This code seems pretty self-contained, so I might end up doing that.

Are you interested in continuing to work on this beyond the DOM bindings?  If yes, the first step is to create an engine class and deliver arguments to it and make sure that you pass through the input buffer unmodified so that putting a ConvolverNode inside a graph doesn't mute playback.  Then I'd like to review the WebKit code in the mean time to decide what to do in order to implement the actual convolution processing.

Comment 4

[Andrew Quartey [:drexler]]

> > +  {
> > +    JSObject* channelObj = mBuffer->GetChannelData(i);
> > +    JSObject* arrayBuffer = JS_GetArrayBufferViewBuffer(channelObj);
> > +    uint8_t* data =  JS_GetArrayBufferData(arrayBuffer);
> > +    float* channelData = reinterpret_cast<float*>(data);
> 
> static_cast

Used reinterpret_cast to force the type conversion otherwise i get error C2440. 

> Are you interested in continuing to work on this beyond the DOM bindings?
Yes. :)

> If yes, the first step is to create an engine class and deliver arguments to
> it and make sure that you pass through the input buffer unmodified so that
> putting a ConvolverNode inside a graph doesn't mute playback.

For this, we're deep-copying the contents of the input buffer to AudioChunk via:
ProduceAudioBlock(AudioNodeStream* aStream,const AudioChunk& aInput,
                  AudioChunk* aOutput, bool* aFinished)

since all prior processing on the input buffer will take place when the |buffer| attribute is set. Correct?

Comment 5

[(no longer active)]

Sorry for the late reply.  I somehow missed this bugmail.

(In reply to Andrew Quartey [:drexler] from comment #4)
> > > +  {
> > > +    JSObject* channelObj = mBuffer->GetChannelData(i);
> > > +    JSObject* arrayBuffer = JS_GetArrayBufferViewBuffer(channelObj);
> > > +    uint8_t* data =  JS_GetArrayBufferData(arrayBuffer);
> > > +    float* channelData = reinterpret_cast<float*>(data);
> > 
> > static_cast
> 
> Used reinterpret_cast to force the type conversion otherwise i get error
> C2440. 

Ah, ok, yeah you're right. :-)

> > Are you interested in continuing to work on this beyond the DOM bindings?
> Yes. :)
> 
> > If yes, the first step is to create an engine class and deliver arguments to
> > it and make sure that you pass through the input buffer unmodified so that
> > putting a ConvolverNode inside a graph doesn't mute playback.
> 
> For this, we're deep-copying the contents of the input buffer to AudioChunk
> via:
> ProduceAudioBlock(AudioNodeStream* aStream,const AudioChunk& aInput,
>                   AudioChunk* aOutput, bool* aFinished)
> 
> since all prior processing on the input buffer will take place when the
> |buffer| attribute is set. Correct?

Hmm, not sure what you mean by deep copying.  You just need to do:

*aOutput = aInput;

which actually doesn't copy the buffer since AudioChunk is smart enough to handle that efficiently, but you're right about the processing model of ProduceAudioBlock.  Here's the code which ensures that: <http://mxr.mozilla.org/mozilla-central/source/content/media/MediaStreamGraph.cpp#919>.  We basically come up with WEBAUDIO_BLOCK_SIZE time slices, and for each one we call into the engines for each node in order one by one, passing the output of one engine (stored in nsAudioNodeStream::mLastChunk) to the next one (see ObtainInputBlock to see how that works if you're interested.)

Please ping me if you need help making progress.  And thanks!

Comment 6

[(no longer active)]

Andrew, do you think you can work on this soon? I'd like to drive this to the finish line soon...

Comment 7

[Andrew Quartey [:drexler]]

Ooops sorry - your reply got buried in a deluge of emails. I'll devote this weekend to this.

Comment 8

[(no longer active)]

(In reply to comment #7)
> Ooops sorry - your reply got buried in a deluge of emails. I'll devote this
> weekend to this.

Thanks a lot!  :-)

BTW, I can probably help in stealing the Blink code for the processing.

Comment 9

[(no longer active)]

With your permission, I would like to steal this bug, as this is one of the most important node implementations that we're currently missing.

Comment 10

[(no longer active)]

Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.

Comment 11

[(no longer active)]

Attachment: Part 1: Implement the DOM bindings for ConvolverNode

Comment 12

[(no longer active)]

Attachment: Part 2: Refactor our Web Audio FFT code into the FFTFrame class

Comment 13

[(no longer active)]

Attachment: Part 3: Import the Convolution processing implementation from Blink

Comment 14

[(no longer active)]

Attachment: Part 4: Add the Convolution processing implementation to the build system

Comment 15

[(no longer active)]

Attachment: Part 5: Implement ConvolverNode's processing based on the Blink implementation

Comment 16

[(no longer active)]

Attachment: Part 6: Optimize FFTFrame to avoid recreating kiss_fftr_cfg objects all the time

Comment 17

[(no longer active)]

Attachment: Part 7: Add a basic API test for ConvolverNode

Comment 18

[(no longer active)]

Attachment: Part 8: Import convolution tests from Blink

Comment 19

[(no longer active)]

Attachment: Part 9: Port Blink's LayoutTest for ConvolverNode to mochitest-plain

Comment 20

[(no longer active)]

Attachment: Part 4: Add the Convolution processing implementation to the build system

Sigh, forgot that MSVC doesn't have std::isinf and std::isnan...

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 760245 [details] [diff] [review]
Part 2: Refactor our Web Audio FFT code into the FFTFrame class

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

::: content/media/webaudio/FFTFrame.h
@@ +14,5 @@
> +namespace mozilla {
> +
> +// This class defines an FFT frame, loosely modeled after Blink's FFTFrame
> +// class to make sharing code with Blink easy.
> +// Currently it's implemented on top of KissFFT on all platforms.

Do we have to call this "Frame"? That is a vastly overloaded term already.

Comment 22

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=de61f3106969
https://tbpl.mozilla.org/?tree=Try&rev=2ad8312f8856
https://tbpl.mozilla.org/?tree=Try&rev=d9aad1ee5cf5

Comment 23

[(no longer active)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> Comment on attachment 760245 [details] [diff] [review]
> Part 2: Refactor our Web Audio FFT code into the FFTFrame class
> 
> Review of attachment 760245 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webaudio/FFTFrame.h
> @@ +14,5 @@
> > +namespace mozilla {
> > +
> > +// This class defines an FFT frame, loosely modeled after Blink's FFTFrame
> > +// class to make sharing code with Blink easy.
> > +// Currently it's implemented on top of KissFFT on all platforms.
> 
> Do we have to call this "Frame"? That is a vastly overloaded term already.

I adopted this because "FFT frame" is a common terminology...  I believe this code is far enough from layout/.  :-)

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We've got, at least
-- nsIFrame
-- nsHTMLIFrameElement etc
-- audio frames (e.g. AudioSegment::AppendFrames) (1 sample per channel)
-- Opus frames (5-20ms of data)

Comment 25

[(no longer active)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> We've got, at least
> -- nsIFrame
> -- nsHTMLIFrameElement etc
> -- audio frames (e.g. AudioSegment::AppendFrames) (1 sample per channel)
> -- Opus frames (5-20ms of data)

OK, I can rename it to FFTBlock before landing...

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 760245 [details] [diff] [review]
Part 2: Refactor our Web Audio FFT code into the FFTFrame class

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

::: content/media/AudioNodeEngine.h
@@ +110,5 @@
>                                      const float aScale[WEBAUDIO_BLOCK_SIZE],
>                                      float aOutput[WEBAUDIO_BLOCK_SIZE]);
>  
>  /**
> + * Vectory copy-scaled operation on arbitrary sized buffers.

this comment seems wrong

::: content/media/webaudio/AnalyserNode.cpp
@@ +221,4 @@
>  
>    for (uint32_t i = 0; i < mOutputBuffer.Length(); ++i) {
> +    double scalarMagnitude = sqrt(mAnalysisFrame.RealData(i) * mAnalysisFrame.RealData(i) +
> +                                  mAnalysisFrame.ImagData(i) * mAnalysisFrame.ImagData(i)) *

you should use NS_hypot here

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 760256 [details] [diff] [review]
Part 4: Add the Convolution processing implementation to the build system

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

The checkin comment for this patch seems wrong.

::: content/media/AudioNodeEngine.cpp
@@ +116,5 @@
> +void
> +AudioBlockInPlaceScale(float* aBlock,
> +                       uint32_t aChannelCount,
> +                       float aScale,
> +                       uint32_t aSize)

Shouldn't this be called AudioBufferInPlaceScale?

::: content/media/AudioNodeEngine.h
@@ +166,5 @@
>  /**
> + * Return the sum of squares of all of the samples in the input.
> + */
> +float
> +AudioBlockSumOfSquares(const float* aInput, uint32_t aLength);

And shouldn't this be AudioBufferSumOfSquares?

Comment 28

[(no longer active)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> Comment on attachment 760256 [details] [diff] [review]
> Part 4: Add the Convolution processing implementation to the build system
> 
> Review of attachment 760256 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The checkin comment for this patch seems wrong.

I don't think so...  That's precisely what this commit is doing.  See <http://mxr.mozilla.org/mozilla-central/source/content/media/webaudio/blink/README>.

Comment 29

[(no longer active)]

Attachment: Part 4: Add the Convolution processing implementation to the build system

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 760537 [details] [diff] [review]
Part 4: Add the Convolution processing implementation to the build system

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

You're replacing a tryLock with an unconditional lock, and the comment there says it'll cause glitching. Have you looked into that at all? I think we currently don't support trylock, so I don't mind this landing, but maybe we should.

Comment 31

[(no longer active)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> Comment on attachment 760537 [details] [diff] [review]
> Part 4: Add the Convolution processing implementation to the build system
> 
> Review of attachment 760537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You're replacing a tryLock with an unconditional lock, and the comment there
> says it'll cause glitching. Have you looked into that at all? I think we
> currently don't support trylock, so I don't mind this landing, but maybe we
> should.

Good point.  I actually did look into this, and I think the possibility of this code trying to wait on the lock does exist in theory, but so far I've not seen any glitching in my testing.  That being said, perhaps we need to use <http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/lock.h> in order to reintroduce trylocking here (NSPR locking primitives do not support trylock.)

Filed bug 881558 about that.

Comment 32

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6058da103d45
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc80f47a7123
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bed30223d8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7efc129d2b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/90c849ba5baf
https://hg.mozilla.org/integration/mozilla-inbound/rev/22d7a1784228
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb2712f0237
https://hg.mozilla.org/integration/mozilla-inbound/rev/c78350f78192
https://hg.mozilla.org/integration/mozilla-inbound/rev/63386b71d1b5

Comment 33

[(no longer active)]

BTW, here are some demos:

http://people.mozilla.org/~eakhgari/webaudio/convolver.html
http://people.mozilla.org/~eakhgari/webaudio/convolver-sine.html
http://ehsan.github.io/chromium-audio-samples/convolution-effects.html

The first two are crappy demos that I made myself.  The third one is actually nice!  :-)

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6058da103d45
https://hg.mozilla.org/mozilla-central/rev/dc80f47a7123
https://hg.mozilla.org/mozilla-central/rev/6bed30223d8f
https://hg.mozilla.org/mozilla-central/rev/b7efc129d2b1
https://hg.mozilla.org/mozilla-central/rev/90c849ba5baf
https://hg.mozilla.org/mozilla-central/rev/22d7a1784228
https://hg.mozilla.org/mozilla-central/rev/7cb2712f0237
https://hg.mozilla.org/mozilla-central/rev/c78350f78192
https://hg.mozilla.org/mozilla-central/rev/63386b71d1b5

Comment 35

[Jeremie Patonnier :Jeremie]

Follow up for documentation: https://bugzilla.mozilla.org/show_bug.cgi?id=885284

Comment 36

[Karl Tomlinson (:karlt)]

Sorry, I landed these changesets with this bug number instead of what should have been bug 865241. 

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e796e343ef8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ddd533fc54c
https://hg.mozilla.org/integration/mozilla-inbound/rev/729de18aacef
https://hg.mozilla.org/integration/mozilla-inbound/rev/214033794f62
https://hg.mozilla.org/integration/mozilla-inbound/rev/c43863ae9218
https://hg.mozilla.org/integration/mozilla-inbound/rev/c56ecdd125ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b01d3f61a1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a05c2215091
https://hg.mozilla.org/integration/mozilla-inbound/rev/395fc5b9593c
https://hg.mozilla.org/integration/mozilla-inbound/rev/316df2d96b24
https://hg.mozilla.org/integration/mozilla-inbound/rev/d92240f69c48
https://hg.mozilla.org/integration/mozilla-inbound/rev/746b2ba6cf30
https://hg.mozilla.org/integration/mozilla-inbound/rev/62ad090a94a4

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e796e343ef8
https://hg.mozilla.org/mozilla-central/rev/5ddd533fc54c
https://hg.mozilla.org/mozilla-central/rev/729de18aacef
https://hg.mozilla.org/mozilla-central/rev/214033794f62
https://hg.mozilla.org/mozilla-central/rev/c43863ae9218
https://hg.mozilla.org/mozilla-central/rev/c56ecdd125ec
https://hg.mozilla.org/mozilla-central/rev/9b01d3f61a1b
https://hg.mozilla.org/mozilla-central/rev/5a05c2215091
https://hg.mozilla.org/mozilla-central/rev/395fc5b9593c
https://hg.mozilla.org/mozilla-central/rev/316df2d96b24
https://hg.mozilla.org/mozilla-central/rev/d92240f69c48
https://hg.mozilla.org/mozilla-central/rev/746b2ba6cf30
https://hg.mozilla.org/mozilla-central/rev/62ad090a94a4