Closed Bug 865251 Opened 7 years ago Closed 7 years ago

Implement WaveShaperNode

Categories

(Core :: Web Audio, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Attached patch WIP patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → sankha93
Attachment #745768 - Flags: feedback?(roc)
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 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?
Attachment #745768 - Flags: feedback?(roc)
I'm going to steal this bug and finish your patch Sankha, hope that's OK.  :-)
Assignee: sankha93 → ehsan
Attached patch Patch (v1) (obsolete) — Splinter Review
Attachment #745768 - Attachment is obsolete: true
Attachment #748579 - Flags: review?(roc)
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.
Attached patch Patch (v2)Splinter Review
Attachment #748579 - Attachment is obsolete: true
Attachment #748579 - Flags: review?(roc)
Attachment #748946 - Flags: review?(roc)
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.
Attachment #748946 - Flags: review?(roc) → review+
Let's get consensus on the neutering on the list first and do it as a follow-up.  Should be pretty easy.
https://hg.mozilla.org/mozilla-central/rev/813a1f7e66ef
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
Depends on: 875277
Depends on: 949680
No longer depends on: 949680
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.
Flags: in-testsuite?
Depends on: 1276483
You need to log in before you can comment on or make changes to this bug.