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

PeriodicWave constructor examines lengths of things that might not be there #2147

Closed
bzbarsky opened this issue Feb 7, 2020 · 9 comments
Closed

Comments

@bzbarsky
Copy link

bzbarsky commented Feb 7, 2020

https://webaudio.github.io/web-audio-api/#PeriodicWave-constructors says (step 2):

If the real and imag parameters of the PeriodicWaveOptions are not of the same length, an IndexSizeError exception MUST be thrown.

but those parameters might not be present at all. Is this meaning to only compare the lengths when both are present? Or does being not present correspond to a length of 0? The text here needs to be a lot more explicit about what should happen.

Then step 4 says:

Otherwise, let [[real]] and [[imag]] be two internal slots of type Float32Array, both of length equal to the maximum of the lengths of the real and imag attributes of the PeriodicWaveOptions passed in.

but the two things passed in must be the same length per step 2. Again, unless being missing means skipping the length check. But then what does the "maximum" mean if one of the lengths does not exist? If both lengths do not exist?

There's something in step 3 about the "options has not been passed in" case, which can't happen, because options has a default value, so it's always passed in. It may or may not have real and imag present in it, of course, whether it's the default value or an explicit value passed from script.

@bzbarsky
Copy link
Author

bzbarsky commented Feb 7, 2020

Put another way: there is no way to implement this part of spec right now based on just reading the spec, without reverse-engineering another implementation. That is not a desirable property for specs...

@bzbarsky
Copy link
Author

bzbarsky commented Feb 7, 2020

I guess maybe the intent is that https://webaudio.github.io/web-audio-api/#dictdef-periodicwaveoptions describes what should happen when those members are missing? But that's a really weird comefrom-like way to write a spec, and still doesn't explain what step 4 is trying to do with taking the maximum.

@rtoy
Copy link
Member

rtoy commented Feb 13, 2020

Teleconf: We'll rewrite this. I think the intent was if both arrays are given they have to have the same length.

@rtoy
Copy link
Member

rtoy commented Feb 14, 2020

I think this is how we want this to work.

  1. If PeriodicWaveOptions is not given
    1. Set [[real]] and [[imag]] to be zero-filled arrays of length 2, and set element at index 1 of [[imag]] to be 1.
  2. If PeriodicWaveOptions is given
    1. If both real and imag members are given
      1. If the lengths are different, throw an error, and abort these steps.
      2. Create arrays [[real]] and [[imag]] of the same length as real
      3. Copy all elements of real to [[real]] and imag to [[imag]]
    2. If only real is given
      1. Throw error if length is less than 2, and abort these steps.
      2. Create arrays [[real]] and [[imag]] of the same length as real, initializing arrays to 0
      3. Copy real to [[real]]
    3. If only imag is given
      1. Throw error if length is less than 2, and abort these steps.
      2. Create arrays [[real]] and [[imag]] of the same length as imag, initializing arrays to 0
      3. Copy imag to [[imag]]
    4. Otherwise
      1. Set [[real]] and [[imag]] to be zero-filled arrays of length 2, and set element at index 1 of [[imag]] to be 1.

Probably want to remove some of the text from the description of the members of PeriodicWaveOptions dictionary. Then use the above steps as part of the algorithm in creating the actual PeriodicWave object.

@bzbarsky
Copy link
Author

If PeriodicWaveOptions is not given

For what it's worth, this can't happen. It has a default value (empty dictionary), so it's always "given".

If both real and imag members are given

Should this case allow length 0 or length 1?

Probably want to remove some of the text from the description of the members of PeriodicWaveOptions dictionary. Then use the above steps as part of the algorithm in creating the actual PeriodicWave object.

That sounds perfect, thank you!

@bzbarsky
Copy link
Author

Oh, and "present" is the spec term for dictionary members whose value was provided.

@rtoy rtoy self-assigned this Feb 18, 2020
@rtoy
Copy link
Member

rtoy commented Feb 18, 2020

Thanks for your comments.

Length 0 isn't really useful but could be spec'ed to produce 0 always. Length 1 isn't really useful because that represents the DC term, which is always ignored. So you'd always get 0 output here too. There are easier ways to produce 0-output.

I'll write up a PR for this soon.

@bzbarsky
Copy link
Author

Should this case allow length 0 or length 1?

Just to be clear, in the proposal above we disallow this if only one of real and imag is provided but allow it if both are provided; I was just checking whether that's intentional.

@rtoy
Copy link
Member

rtoy commented Feb 18, 2020

Sorry I misunderstood. I think the length must be 2 or more in all cases.

rtoy added a commit to rtoy/web-audio-api that referenced this issue Feb 18, 2020
Cleans up the constructor for PeriodicWave based on the algorithm
given in
WebAudio#2147 (comment).

A few other minor cleanups in the algorithm description were also
done:
*  Move declaration of `[[real]]`, `[[imag]]`, and `[[normalize]]` to
   the beginning of the algorithm.
* Remove extra text for the members of the `PeriodicWaveOptions` to
  the algorithm so that everthing is all in one place.
@rtoy rtoy closed this as completed in 3ff9eea Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants