Implement WaveShaperNode

RESOLVED FIXED in mozilla24

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla24
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
Created attachment 745768 [details] [diff] [review]
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.
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.
(Assignee)

Comment 3

5 years ago
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)
(Assignee)

Comment 4

5 years ago
I'm going to steal this bug and finish your patch Sankha, hope that's OK.  :-)
Assignee: sankha93 → ehsan
(Assignee)

Comment 5

5 years ago
Created attachment 748579 [details] [diff] [review]
Patch (v1)
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.
(Assignee)

Comment 7

5 years ago
Created attachment 748946 [details] [diff] [review]
Patch (v2)
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+
(Assignee)

Comment 9

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(Assignee)

Comment 12

5 years ago
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.