Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Visible Data Races #254

Closed
chrislo opened this issue Oct 17, 2013 · 5 comments
Closed

Visible Data Races #254

chrislo opened this issue Oct 17, 2013 · 5 comments
Assignees

Comments

@chrislo
Copy link
Member

chrislo commented Oct 17, 2013

The following issue was raised by the W3C TAG as part of their review of the Web Audio API

Visible Data Races

This topic has been debated on the public-audio mailing list and in blogs.

It's reasonable to suggest that de-sugaring into a style that transfers ownership of buffers to nodes during processing is sufficient to close much of the problem down, but whatever the solution, we wish to be on the record as saying clearly that it is impermissible for Web Audio to unilaterally add visible data races to the JavaScript execution model.

The Web Audio group is best suited to solve this issue, but we insist that no API be allowed to create visible data races from the perspective of linearly-executing JavaScript code.

@padenot
Copy link
Member

padenot commented Oct 18, 2013

Now that the most problematic race is fixed (AudioBuffer), we still have to address the remaining problems, outlined in this email.

Points (3) and (4) in the email has been solved by roc's proposal.

Those remaining races are:

  • When calling AudioContext.createPeriodicWave(Float32Array real, Float32Array imag), on both arguments ;
  • AudioParam.setValueCurveAtTime(Float32Array values, double startTime, double duration), on the values argument ;
  • AudioProcessingEvent.inputBuffer, AudioProcessingEvent.outputBuffer: clarify the current spec text further ;
  • OfflineAudioCompletionEvent.renderedBuffer.

Here is a proposal, taking into account some feedback from the thread at the time:

  • Because PeriodicWaves maintain their own, internal, representation of the real and imag argument, specify that any modifications of these argument after the call are not reflected in the PeriodicWaves.

  • setValueCurveAtTime: we could simply copy the data. Concerns have been expressed about performances on some specific use cases, however, no testcase/benchmark has been provided.

  • AudioProcessingEvent.(inputBuffer|outputbBuffer): we could simply drop the sentences:

    This AudioBuffer is only valid while in the scope of the onaudioprocess
    function. Its values will be meaningless outside of this scope.

    and

    Any script modifications to this AudioBuffer outside of this scope will not
    produce any audible effects.

    and simply state that different buffers should be used each time, making quite clear that modification of the buffer outside of the callback will not have any effect. (The outputBuffer case has been solved by adopting roc's proposal). Additionally fresh buffers should be used each time for the outputBuffer as well.

  • OfflineAudioCompletionEvent.renderedBuffer, a new AudioBuffer
    should be used each time.

Thoughts?

@ghost
Copy link

ghost commented Oct 21, 2013

On Fri, Oct 18, 2013 at 5:07 AM, Paul ADENOT notifications@github.comwrote:

Now that the most problematic race is fixed (AudioBuffer), we still have
to address the remaining problems, outlined in this emailhttp://lists.w3.org/Archives/Public/public-audio/2013AprJun/0644.html
.

Points (3) and (4) in the email has been solved by roc's proposal.

Those remaining races are:

  • When calling AudioContext.createPeriodicWave(Float32Array real,
    Float32Array imag), on both arguments ;
  • AudioParam.setValueCurveAtTime(Float32Array values, double
    startTime, double duration), on the values argument ;
  • AudioProcessingEvent.inputBuffer, AudioProcessingEvent.outputBuffer:
    clarify the current spec text further ;
  • OfflineAudioCompletionEvent.renderedBuffer.

Here is a proposal, taking into account some feedback from the thread at
the time:

  • Because PeriodicWaves maintain their own, internal, representation
    of the real and imag argument, specify that any modifications of these
    argument after the call are not reflected in the PeriodicWaves.

Seems fine with me.

  • setValueCurveAtTime: we could simply copy the data. Concernshttp://lists.w3.org/Archives/Public/public-audio/2013AprJun/0660.htmlhave been expressed about performances on some specific use cases, however,
    no testcase/benchmark has been provided.

We could instead neuter the array. Either way is okay, I think.

AudioProcessingEvent.(inputBuffer|outputbBuffer): we could simply drop
the sentences:

This AudioBuffer is only valid while in the scope of the onaudioprocess
function. Its values will be meaningless outside of this scope.

and

Any script modifications to this AudioBuffer outside of this scope
will not
produce any audible effects.

and simply state that different buffers should be used each time,
making quite clear that modification of the buffer outside of the callback
will not have any effect. (The outputBuffer case has been solved by
adopting roc's proposal). Additionally fresh buffers should be used each
time for the outputBuffer as well.

I'm not sure about that solution - it seems like it's a performance
penalty?

  • OfflineAudioCompletionEvent.renderedBuffer, a new AudioBuffer should
    be used each time.

I think that's fine.

-Chris

@ghost ghost assigned cwilso Nov 7, 2013
@cwilso
Copy link
Contributor

cwilso commented Nov 7, 2013

On the WG call today (11/7), we agreed that there's consensus we should be addressing these now. I agree with your recommendations excepting the AudioProcessingEvent, about which I'm just not sure yet. On value curve, periodic wave and offline, I think those resolutions are fine.

@cwilso
Copy link
Contributor

cwilso commented Nov 7, 2013

Olivier asked me to split the AudioProcessing event out from this issue.

@ghost ghost assigned padenot Dec 19, 2013
@padenot
Copy link
Member

padenot commented Jan 15, 2014

Pushed in padenot@545dad7

padenot added a commit that referenced this issue Feb 19, 2014
@padenot
Copy link
Member

padenot commented Feb 19, 2014

Pushed to ac15990

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants