Re: New proposal for fixing race conditions

Chris,

On Fri, Jul 19, 2013 at 8:54 PM, Chris Rogers <crogers@google.com> wrote:

> Jer, thanks for sharing the proposal.
>
> I can't support a solution like this because it:
>
> 1. Forces the need for copies both for setting and reading data.  PCM data
> is large and it's inefficient to be required to copy.
>

As we discussed earlier - I don't think that the added copies are
necessarily a show stopper. Could you please provide concrete examples /
benchmark results that show that this would be a real problem?


> 2. It needlessly complicates the API, where we now have a simple solution.
>

That depends on how you see it. It's maybe one more interface level in some
cases (when you need to access the raw data of your AudioBuffers). For the
most common use cases it wouldn't matter though, since you wouldn't be
touching the raw data anyway. On the other hand I find the new proposed
interface much more clear in terms of what to expect from each operation.
To be honest it actually took me quite some time to figure out how to
interpret the current interfaces when I read the spec the first time,
simply because it does not behave in a way familiar from other similar APIs
(e.g. OpenAL, OpenGL), nor *any* of the existing Web interfaces (see below).


> 3. The "problem" it is supposed to solve is effectively a non-issue.
>

Here I simply can not agree. Unprotected mutable data sharing between
threads does not exist on the Web platform, and introducing it is something
that shouldn't be taken lightly, and pushing it onto the Web is bigger than
the Audio WG, IMO.

There's actually another problem with the current interface that seems to
have been ignored, and that is that the specification does not say anything
about what to expect (as a Web developer, I am very interested in these
things, and as a [potential] implementor, even more so). The current
approach seems to be equal to writing "undefined behavior" in a lot of
places in the spec, which I also think is something that shouldn't be taken
lightly. An alternative would be to try to spec the actual behavior (e.g.
taking the Blink implementation as a reference, and requiring that other
implementations re-implement the same behavior), but I think it will be
really difficult task to define all aspects of what must, should and must
not happen etc, and it would also prevent alternative optimizations that
would be more suitable on another engine / architecture.

To me, the easy-way-out here is to abandon the racy interfaces and replace
them with properly well defined interfaces, even if it means a potential
performance hit in *some* cases.

/Marcus



> Chris
>
>
>
> On Thu, Jul 18, 2013 at 1:02 PM, Jer Noble <jer.noble@apple.com> wrote:
>
>> Overview
>>
>> In order to solve the "data race" issue in WebAudio (bug 22725<https://www.w3.org/Bugs/Public/show_bug.cgi?id=22725>),
>> this proposal restricts access to the internal PCM data of an AudioBuffer.
>> This does not require AudioBuffers to be immutable, but that modifying
>> their contents occurs through an API which enforces no-data-race semantics.
>>
>> Since this will involve breaking API changes, this proposal incorporates
>> suggestions from Issue 48<https://www.w3.org/Bugs/Public/show_bug.cgi?id=17341> to
>> simplify the AudioBuffer interface, and will also adopt a constructor, as
>> opposed to a factory method.
>>
>> The new constructor provides an upgrade path for existing pages while not
>> breaking those pages. The factory method can continue to create the
>> "legacy" AudioBuffer in prefixed implementations.
>>
>> *Note:* I still prefer the "Immutable ArrayBuffer" proposal.
>> New interfacesAudioBufferChannel
>>
>> This interface represents a single channel of an AudioBuffer.
>>
>> interface AudioBufferChannel {
>>     readonly attribute unsigned long length;
>>
>>     void set(Float32Array array, optional unsigned long offset);
>>     Float32Array slice(long begin, optional long end);
>> };
>>
>> Attributes
>>
>> length
>>
>>    - Length of the PCM audio data in sample-frames.
>>
>> Methods
>>
>> set
>>
>>    -
>>
>>    Copies the Float32Array representing the PCM audio data.
>>    -
>>
>>    An INDEX_SIZE_ERR exception must be thrown if the length ofarray plus
>>    offset exceeds the length of the available audio data.
>>    -
>>
>>    An NO_MODIFICATION_ALLOWED_ERR exception must be thrown if the
>>    AudioBufferChannel is currently is use by a live AudioNode.
>>
>> slice
>>
>>    -
>>
>>    Returns a copy of the underlying PCM audio data. If eitherbegin or end is
>>    negative, it refers to an index from the end of the audio data.
>>    -
>>
>>    If end is unspecified, the copy will include all the data frombegin to
>>    the end of the audio data.
>>    -
>>
>>    The range specified by begin and end is clamped to the valid range
>>    for the audio data. If the computed length of the copied data would be
>>    negative, it is clamped to zero.
>>
>> AudioBuffer
>>
>> [Constructor(unsigned long numberOfChannels, unsigned long length, float sampleRate)]
>> interface AudioBuffer {
>>
>>     readonly attribute float sampleRate;
>>
>>     // in seconds
>>     readonly attribute double duration;
>>
>>     readonly attribute AudioBufferChannel[] channels;
>> };
>>
>> Attributes
>>
>> sampleRate
>>
>>    - The sample-rate for the PCM audio data in samples per second.
>>
>> duration
>>
>>    - Duration of the PCM audio data in seconds.
>>
>> channels
>>
>>    -
>>
>>    An array of AudioBufferChannel objects representing the PCM audio
>>    data for each audio channel.
>>    -
>>
>>    This array is of fixed length and is read-only.
>>
>> AlternativesImmutable ArrayBuffer
>>
>> To make ArrayBuffer immutable, the set() method would be removed from
>> ArrayBufferChannel.
>>
>> interface ArrayBufferChannel {
>>     readonly attribute unsigned long length;
>>     Float32Array slice(long begin, optional long end);
>> };
>>
>> This would require a different constructor method on ArrayBuffer.
>>
>> [Constructor(Float32Array[] arrays, float sampleRate)]
>>
>> This would also require the outputBuffer attribute on
>> AudioProcessingEvent to drop the 'readonly' declaration.
>>
>> partial interface AudioProcessingEvent {
>>     attribute AudioBuffer outputBuffer;
>> };
>>
>> Backwards Compatibility Not normative WebKitAudioBuffer
>>
>> interface WebKitAudioBuffer : AudioBuffer {
>>     readonly attribute length;
>>
>>     readonly attribute long numberOfChannels;
>>
>>     Float32Array getChannelData(unsigned long channel);
>> };
>>
>> WebKitAudioContext
>>
>> partial interface WebKitAudioContext {
>>     WebKitAudioBuffer createBuffer(unsigned long numberOfChannels, unsigned long length, float sampleRate);
>> };
>>
>> WebKit will continue to support WebKitAudioBuffer objects in the prefixed
>> version of the API for every attribute or parameter which currently
>> specifies AudioBuffer. WebKit will drop WebKitAudioBuffer support in the
>> unprefixed API.
>>
>
>