Fwd: WaveShaperNode.curve

Forwarding my conversation with Kumar which intentionally got off-list.

---------- Forwarded message ----------
From: Ehsan Akhgari <ehsan.akhgari@gmail.com>
Date: Wed, May 15, 2013 at 6:50 PM
Subject: Re: WaveShaperNode.curve
To: Srikumar Subramanian <srikumarks@gmail.com>

On Tue, May 14, 2013 at 9:35 PM, Kumar <srikumarks@gmail.com> wrote:

>
>  var curve = new Float32Array(N);
>>>  // ... fill up curve with data...
>>> shaper1.curve = shaper2.curve = curve;
>>>
>>
>> If we make the getter return a new JS object pointing to an array with a
>> copy of the data passed to the setter, then this code will work fine.
>>
>
> Ehsan - I wrote that code in an abbreviated form, but it is more likely to
> be found like this (I think) -
>
> shaper1.curve = curve;
> shaper2.curve = curve;
>
> .. where the two assignments may happen at different times. For example,
> when a waveshaper is part of the processing pipeline for a "voice".
>

Sure, such code will break!


>   This is close to best, but since compatibility is of interest here, the
>>> deprecated attribute shouldn't be spec'd to neuter because it'll break code
>>> that shares an array between shaper nodes. setCurve() should then be spec'd
>>> such that modifying the array after setCurve() is called should not affect
>>> the shaper node.
>>>
>>
>> I don't think that's a good argument.  I think spec'ing neutering on set
>> and copying on get is the best we can do compatibility wise.  I don't
>> expect that to break a lot of existing content, and we should not be
>> interested in the theoretical breakages.
>>
>
> I agree that we shouldn't be considering theoretical breakages. I'm
> bringing up that case since I tend to share data tables between nodes like
> that all the time with other nodes - ex: I use a shared noise buffer
> between many AudioBufferSourceNode instances and that wasn't even my
> original idea - I borrowed it from Chris Wilson's code. So I suspect
> sharing data buffers between nodes can be an expected use case in the
> future even if we aren't considering compatibility.
>

No, that's not necessarily safe...  The current spec mandates the
implementation to neuter the ArrayBuffer argument passed to decodeAudioData
for example.  WebKit doesn't support that, but it's a bug in their
implementation.  We will probably need to do more of this for example in
the case of WaveShaperNode to close up the data race loopholes in the spec.


> Regarding compatibility related decisions, is there a bank of code that is
> examined currently?
>

Not sure, Chris Rogers has a good idea on the backwards compatibility for
some of the APIs, but he has not said anything about WaveShaperNode.curve
yet.


>
> Note that I'm not suggesting neutering as a good way to design the API,
>> I'm suggesting that as the best compromise to fix the API that is already
>> broken, because overhauling the API without changing WebKit/Blink is not
>> practical.
>>
>
> That's probably where I'm disagreeing - that neutering is the best
> compromise. From what I see, neutering isn't a common practice in the JS
> community. I was introduced to that concept only on the web audio api list
> as well. Is it possible to design the API based on a language where the
> concept of neutering doesn't exist or is not commonly practiced?
>
> If it is indeed common practice, well, "you learn something old everyday"
> as a friend of mine says :) In that case, there is probably a spec gap to
> be addressed on what happens if neutered buffers are set as data on nodes.
>

No, neutering is not common practice, and I agree that it sucks from the
viewpoint of authors.  But consider where we are in the Web Audio spec.
There are race conditions in WaveShaperNode.curve (see the code sample I
posted earlier in this thread.)  This is a *serious* problem since such
data races do not exist anywhere else in the platform.  The right way to
fix this would be to remove the curve attribute, replace it with a
setCurve(Float32Array curve) method which explicitly copies the array
buffer passed to it, and then the whole problem would be solved without the
need to resort to neutering.  However, this won't work in practice since
WebKit/Blink is not willing to implement this kind of backwards
incompatible change, and unfortunately that's currently the API most people
are targeting, so we either have the choice of spec'ing the correct thing,
and then have Gecko implement the spec and WebKit/Blink continue to ignore
the spec and hence end up with non-interoperable implementations (which
means you have to write your code once for WebKit/Blink and once for Gecko
and future spec-compliant implementations) or we have to resort to
neutering tricks to break code which is currently relying on the data race
behavior (even unknowingly) even though that makes for a less than pleasant
API.

This line of issue has come up several times on this list, and we discussed
this extensively at the Audio working group face to face as well.
Unfortunately, without WebKit/Blink willing to change their implementation
in backwards incompatible ways, given the fact that those engines are
currently the "de facto" standard, we need to resort to these kinds of
tricks to keep backwards compatibility but remove problems such as data
races.  This makes me very sad, but really there is only so much that we
can do about this on the Gecko and spec side of things.  :(

Hope this makes sense...

Cheers,
--
Ehsan
<http://ehsanakhgari.org/>