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.

Sign up for GitHub

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

Jump to bottom

PeriodicWave constructor behavior is not defined #1120

Closed
bzbarsky opened this issue Dec 19, 2016 · 2 comments
Closed

PeriodicWave constructor behavior is not defined #1120

bzbarsky opened this issue Dec 19, 2016 · 2 comments
Assignees
@padenot
Labels
status: needs discussion
Milestone
Web Audio V1

Comments

@bzbarsky
Copy link

There is no clear definition of how this constructor should behave. This leads to problems like https://bugzilla.mozilla.org/show_bug.cgi?id=1324181 where it's totally unclear what should actually happen for various inputs.
@bzbarsky bzbarsky mentioned this issue Dec 19, 2016
Closed
@bzbarsky bzbarsky changed the title PeriodicWave constructor behavior is not defined PeriodicWave constructor behavior is not defined Dec 19, 2016
@bzbarsky bzbarsky mentioned this issue Dec 19, 2016
Closed
@padenot padenot self-assigned this Dec 20, 2016
@padenot padenot added this to the Web Audio V1 milestone Dec 20, 2016
@rtoy
Copy link
Member

rtoy commented Mar 20, 2017

To resolve this issue, I think we need to say what happens if both real and imag are given but the lengths are different. I think we have two options here:

  1. It's an error, like for createPeriodicWave; both must have the same length
  2. The shorter is zero-padded to have the same length as the longer. This is a convenience.

I prefer the first, to make the behaviour consistent with the factor method. The convenience factor isn't really compelling to me because if you need to specify both components, they very likely have to be the same length anyway to get all of the harmonics and phase that you want.

@padenot
Copy link
Member

padenot commented Mar 21, 2017

I agree with @rtoy.

rtoy added a commit to rtoy/web-audio-api that referenced this issue Mar 21, 2017
@rtoy
Fix WebAudio#1120: Specify PeriodicWave constructor better
4a8d0bb 
If both the `real` and `imag` members of the `PeriodicWaveOptions`
dictionary are specified, they must have the same length.  Otherwise a
NotSupportedError is thrown.

(Not sure if NotSupportedError is the right one, though.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@padenot padenot

Labels
status: needs discussion
Projects
None yet
Milestone
Web Audio V1
Development

No branches or pull requests
4 participants
@padenot @rtoy @mdjp @bzbarsky