865251 - Implement WaveShaperNode

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Comment 1

[Sankha Narayan Guria [:sankha]]

Attachment: WIP patch v1

This is a work in progress patch. Can you tell me if I am proceeding in the right direction?

I not sure if I have written line 29 in WaveShaperNode.cpp correctly.

Comment 2

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

Float32Arrays aren't refcounted. They're JS objects. You'll want to deal with them like BiquadFilterNode does. Although actually I just realized that making the Float32Array a settable attribute has some problems. I'll write to the public-audio list about that.

Comment 3

[(no longer active)]

Comment on attachment 745768 [details] [diff] [review]
WIP patch v1

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

This patch is a good start.  Note that you also need to setup and AudioNodeEngine for this, create the audio node stream, pass in the curve array to the engine, add tests for this, etc.  I suggest we wait here until the spec issue is resolved.

::: content/media/webaudio/WaveShaperNode.cpp
@@ +15,5 @@
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mCurve)
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(WaveShaperNode, AudioNode)
> + NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_PTR(tmp->mCurve, Float32Array, "shaping curve")
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

When the issue of the curve attribute is resolved on the public-audio list, you may need to use NS_HOLD_JS_OBJECTS and NS_DROP_JS_OBJECTS here similar to the AudioBuffer class.

::: content/media/webaudio/WaveShaperNode.h
@@ +23,5 @@
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(WaveShaperNode, AudioNode)
> +
> +  virtual JSObject* WrapObject(JSContext *aCx, JSObject* aScope,
> +  	                           bool* aTriedToWrap);

Nit: indentation.

@@ +32,5 @@
> +  }
> +  virtual uint32_t MaxNumberOfOutputs() const MOZ_FINAL MOZ_OVERRIDE
> +  {
> +    return 1;
> +  }

None of these two should be needed.

@@ +40,5 @@
> +    return mCurve;
> +  }
> +
> +private:
> +  nsRefPtr<Float32Array> mCurve;

What roc said.  Does this even compile?

Comment 4

[(no longer active)]

I'm going to steal this bug and finish your patch Sankha, hope that's OK.  :-)

Comment 5

[(no longer active)]

Attachment: Patch (v1)

Comment 6

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

Comment on attachment 748579 [details] [diff] [review]
Patch (v1)

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

::: content/media/AudioNodeStream.h
@@ +74,5 @@
>    void SetInt32Parameter(uint32_t aIndex, int32_t aValue);
>    void SetTimelineParameter(uint32_t aIndex, const dom::AudioParamTimeline& aValue);
>    void SetThreeDPointParameter(uint32_t aIndex, const dom::ThreeDPoint& aValue);
>    void SetBuffer(already_AddRefed<ThreadSharedFloatArrayBufferList> aBuffer);
> +  void SetRawArrayData(const float* aData, uint32_t aLength);

I'd prefer this to take an nsTArray<float> parameter and use a chain of SwapElements to steal the data and avoid copies.

::: content/media/webaudio/WaveShaperNode.cpp
@@ +71,5 @@
> +        // incoming signal by clamping the amplitude to [-1, 1] and
> +        // computing the index into the length of the curve.
> +        int32_t index = std::max(int32_t(0),
> +                                 std::min(int32_t(mCurveLength - 1),
> +                                          int32_t(mCurveLength * (inputBuffer[j] + 1) / 2)));

I think we should round the value here instead of truncating. So add 0.5f before casting to int32_t.

However, better still I think we should use linear interpolation. There was consensus for this on the list. I.e. consider both the "round up" and "round down" indices and use the sum weighted by the distance from the true point to the array indices.

Comment 7

[(no longer active)]

Attachment: Patch (v2)

Comment 8

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

Comment on attachment 748946 [details] [diff] [review]
Patch (v2)

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

::: content/media/AudioNodeEngine.h
@@ +187,5 @@
>    virtual void SetBuffer(already_AddRefed<ThreadSharedFloatArrayBufferList> aBuffer)
>    {
>      NS_ERROR("SetBuffer called on engine that doesn't support it");
>    }
> +  virtual void SetRawArrayData(nsTArray<float>& aData)

Document that this consumes the aData array.

::: content/media/AudioNodeStream.h
@@ +74,5 @@
>    void SetInt32Parameter(uint32_t aIndex, int32_t aValue);
>    void SetTimelineParameter(uint32_t aIndex, const dom::AudioParamTimeline& aValue);
>    void SetThreeDPointParameter(uint32_t aIndex, const dom::ThreeDPoint& aValue);
>    void SetBuffer(already_AddRefed<ThreadSharedFloatArrayBufferList> aBuffer);
> +  void SetRawArrayData(nsTArray<float>& aData);

Document that this consumes the aData array.

::: content/media/webaudio/WaveShaperNode.cpp
@@ +132,5 @@
> +  }
> +
> +  AudioNodeStream* ns = static_cast<AudioNodeStream*>(mStream.get());
> +  MOZ_ASSERT(ns, "Why don't we have a stream here?");
> +  ns->SetRawArrayData(curve);

I think we should neuter the array --- for the reason that you explained on the list.  But we can do that as a followup I guess.

Comment 9

[(no longer active)]

Let's get consensus on the neutering on the list first and do it as a follow-up.  Should be pretty easy.

Comment 10

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/813a1f7e66ef

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/813a1f7e66ef

Comment 12

[(no longer active)]

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

Comment 13

[Karl Tomlinson (:karlt)]

Could add some tests to check that the signal is processed correctly, including checking that zero input corresponds to the centre value of the curve array and checking that rounding is correct when just below zero.