Re: Proposal for fixing race conditions

On Tue, Jun 25, 2013 at 10:06 AM, Chris Rogers <crogers@google.com> wrote:

> On Thu, Jun 20, 2013 at 7:32 PM, Robert O'Callahan <robert@ocallahan.org>wrote:
>
>> 1) AudioContext.createPeriodicWave(Float32Array real, Float32Array imag)
>> I propose copying the data during the createPeriodicWave call.
>>
>
> The PeriodicWave objects have their own internal representation, and don't
> need to be affected by any subsequent changes to the Float32Arrays.  The
> good news is that these PeriodicWave objects, once created can be shared
> across many many different instances of OscillatorNode, so are
> memory-efficient once created.  In other words, it's not necessary to
> create a unique PeriodicWave for each new OscillatorNode that is created
> and used.
>

Yep, so you're OK with this. Good.


>
>
>> 2) AudioParam.setValueCurveAtTime(Float32Array values, double startTime,
>> double duration)
>> I propose copying the data during the setValueCurveAtTime call.
>>
>
> This would be extremely inefficient, because these curves need to be
> shared across many different "instances".  One example of this is where the
> curve is used as a grain envelope, amplitude envelope, or custom filter
> envelope.  In granular synthesis, a large number of grain instances can be
> created per second, so it's important to be able to share these curves
> without incurring the copying cost.  Similarly with a custom filter
> envelope using curves, there can be many different synthesis "note"
> instances playing, so I can't agree with this change because of the large
> performance cost.
>

The status quo in Webkit, as I understand it, allows changes to the array
after the setValueCurveAtTime call to affect the AudioParam's behavior on
the audio thread, so those changes may or may not be observed depending on
all sorts of implementation details, including details of the memory
subsystem of the underlying hardware. This is unprecedented in existing
specs and I will object to it all the way through the W3C process if
necessary.

For this particular method, I see three options:
1) Copying the data during the setValueCurveAtTime call.
2) Neutering the array during the setValueCurveAtTime call.
3) Using the VM system to trap modifications to the Float32Array after the
setValueCurveAtTime call, and lazily making a copy of the data in that case
(the observable behavior of 1, but potentially better performance).

#1 and #2 are unacceptable to you. #3 is complex to implement, may require
JS engine changes, and might have its own performance costs, so I'm not
keen on pursuing it.

So, I propose sticking with #1 for this method and enable authors to avoid
performance issues by introducing a new method:
setValueCurveAtTime(AudioBuffer values, double startTime, double duration)
This would use the sample data in the first channel as the curve values.
This would let you use the same data repeatedly without copying.


>
>> 3) WaveShaperNode.curve
>> I propose copying the data when the attribute is assigned to.
>>
>
> Similar to the AudioParam curves, it would also be inefficient to copy
> here, although the problem isn't quite as bad (compared with the grain
> envelope case).
>

I would address the performance issues here in a similar way as above:
allow "curve" to be a "Float32Array or AudioBuffer".


>
>> 4) AudioBuffer.getChannelData(unsigned long channel)
>> This is the tricky one. Proposal:
>> Define a spec-level operation "acquire AudioBuffer contents" which
>> delivers the current contents of the AudioBuffer to whatever operation
>> needs them, replaces the AudioBuffer's internal Float32Arrays with new
>> Float32Array objects containing copies of the data, and neuters the
>> previous Float32Arrays.
>> [IMPORTANT: Implementations can and should optimize this operation so
>> that a) multiple "acquire contents" operations on the same AudioBuffer
>> (with no intervening calls to getChannelData) return the same shared data;
>> b) replacing the internal Float32Arrays with new Float32Arrays happens
>> lazily at the next getChannelData (if any); and thus c) no data copying
>> actually happens during an "acquire contents" operation. Let me know if
>> this is unclear; it's terrifically important.]
>> Then:
>> -- Every assignment to AudioBufferSourceNode.buffer "acquires the
>> contents" of that buffer and the result is what gets used by the
>> AudioBufferSourceNode.
>>
>
> One of the most important features of the Web Audio API is to be able to
> efficiently trigger many overlapping short sounds.  This is very common in
> games, interactive applications, and musical applications.  In many cases,
> these individual "sound instances" are based on the same underlying audio
> sample data.  So it's very important to not require copying PCM data.
>  These PCM buffers can also be quite large (multiple megabytes), and
> portions of a larger buffer can be scheduled as "sound grains" (or "audio
> sprites").  So I can't agree to requiring any kind of copying here.
>

Please re-read what I wrote above. The "acquires the contents" operation
does not need to actually copy. In Gecko it does not copy.

Let me try in explain what we do in more detail. In Gecko, an AudioBuffer
is in one of two internal states:
1) Its data is stored in JS Float32Arrays, one per channel.
2) Its data is stored in immutable data buffers, one per channel.
An AudioBuffer starts in state 1.
When the "acquire AudioBuffer contents" operation happens on an AudioBuffer
in state 1, we change the AudioBuffer to state 2. This detaches the data
buffers from the Float32Arrays (neutering those Float32Arrays), attaches
them into a refcounted, thread-shareable object which is the new state of
the AudioBuffer, and the refcounted object is passed to the audio thread to
be used there.
When the "acquire AudioBuffer contents" operation happens on an AudioBuffer
in state 2, we just addref the refcounted object and pass it to the audio
thread.
When getChannelData() is called on an AudioBuffer in state 1, there's
nothing to do, we just return the internal Float32Array.
When getChannelData() is called on an AudioBuffer in state 2, we change the
AudioBuffer to state 1. We create new Float32Arrays and copy from state 2's
immutable data buffers into the Float32Arrays. This is the only case where
data copying is required --- when an application calls getChannelData()
*after* the AudioBuffer has already been used by some AudioNode. (If
getChannelData() is called after all AudioNodes have stopped using the
AudioBuffer, we could still avoid copying in that case, with a bit more
work.)

Rob
-- 
Jtehsauts  tshaei dS,o n" Wohfy  Mdaon  yhoaus  eanuttehrotraiitny  eovni
le atrhtohu gthot sf oirng iyvoeu rs ihnesa.r"t sS?o  Whhei csha iids  teoa
stiheer :p atroa lsyazye,d  'mYaonu,r  "sGients  uapr,e  tfaokreg iyvoeunr,
'm aotr  atnod  sgaoy ,h o'mGee.t"  uTph eann dt hwea lmka'n?  gBoutt  uIp
waanndt  wyeonut  thoo mken.o w  *
*