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

Mandate a useful accepted sampling rate ranges for buffers created through AudioContext.createBuffer #119

Closed
olivierthereaux opened this issue Sep 11, 2013 · 14 comments

Comments

@olivierthereaux
Copy link
Contributor

Originally reported on W3C Bugzilla ISSUE-21223 Fri, 08 Mar 2013 16:55:21 GMT
Reported by Ehsan Akhgari [:ehsan]
Assigned to

Currently, the spec just suggests that implementations should at least accept sampling rate ranges of 22050-96000. I think it would be better to mandate this range as opposed to just suggest it, since otherwise it would be possible to write code which works in one UA and not the other.

Also, sampling rates outside of this range probably don't make much sense anyway.

@olivierthereaux
Copy link
Contributor Author

Original comment by Ehsan Akhgari [:ehsan] on W3C Bugzilla. Fri, 08 Mar 2013 16:57:11 GMT

Paul Adenot brought up a very good point in https://bugzilla.mozilla.org/show_bug.cgi?id=849230#c1. We should consider setting the minimum sample rate value to 8000Hz.

@olivierthereaux
Copy link
Contributor Author

Original comment by Ehsan Akhgari [:ehsan] on W3C Bugzilla. Fri, 08 Mar 2013 16:59:45 GMT

Also, the spec doesn't specify what needs to happen if the sample rate is not accepted, as usual. ;) WebKit throws DOM_SYNTAX_ERR, I'm going to modify Gecko to do the same. We should spec this.

@olivierthereaux
Copy link
Contributor Author

Original comment by Ralph Giles on W3C Bugzilla. Fri, 08 Mar 2013 21:20:20 GMT

96 kHz is an odd limit, considering the fad for material at 192 kHz. I think the allowed range should be either 8-48 kHz or 8-192 kHz.

@olivierthereaux
Copy link
Contributor Author

Original comment by Chris Rogers on W3C Bugzilla. Fri, 08 Mar 2013 21:33:37 GMT

(In reply to comment #3)

96 kHz is an odd limit, considering the fad for material at 192 kHz. I think
the allowed range should be either 8-48 kHz or 8-192 kHz.

8-192 KHz sounds good to me

@olivierthereaux
Copy link
Contributor Author

Original comment by Ehsan Akhgari [:ehsan] on W3C Bugzilla. Fri, 08 Mar 2013 21:39:09 GMT

(In reply to comment #4)

(In reply to comment #3)

96 kHz is an odd limit, considering the fad for material at 192 kHz. I think
the allowed range should be either 8-48 kHz or 8-192 kHz.

8-192 KHz sounds good to me

Sounds good to me too!

@olivierthereaux
Copy link
Contributor Author

Original comment by Marat Tanalin | tanalin.com on W3C Bugzilla. Sun, 10 Mar 2013 16:37:32 GMT

"At least" should not be confused with "limited to". These are totally different things.

Current spec says "must", not "should":

"An implementation must support sample-rates in at least the range 22050 to 96000."
(https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#dfn-sampleRate_2)

That does require that the range of sample rates must be supported, but does not prevent implementations from supporting more wide range if they want / need / are able to.

There is no sense to limit sample-rate range unless there are corresponding technical limitations in a specific implementation. This is completely out of the spec scope.

@olivierthereaux
Copy link
Contributor Author

Original comment by Olivier Thereaux on W3C Bugzilla. Mon, 11 Mar 2013 11:43:28 GMT

The spec currently says two things:

  • An implementation must support sample-rates in at least the range 22050 to 96000.
  • An exception will be thrown if the numberOfChannels or sampleRate are out-of-bounds

Looking at the discussion so far, I think the spec should say something like:

  • implementations MUST support sampling rate ranges of 8-192 KHz. sampling rates beyond this range MAY be supported.
  • [in The createBuffer method ] An exception MUST be thrown if sampleRate is out of bounds.

is DOM_SYNTAX_ERR the best exception though?

@olivierthereaux
Copy link
Contributor Author

Original comment by Ehsan Akhgari [:ehsan] on W3C Bugzilla. Mon, 11 Mar 2013 15:12:34 GMT

(In reply to comment #7)

The spec currently says two things:

  • An implementation must support sample-rates in at least the range 22050 to
    96000.
  • An exception will be thrown if the numberOfChannels or sampleRate are
    out-of-bounds

Looking at the discussion so far, I think the spec should say something like:

  • implementations MUST support sampling rate ranges of 8-192 KHz. sampling
    rates beyond this range MAY be supported.
  • [in The createBuffer method ] An exception MUST be thrown if sampleRate is
    out of bounds.

That sounds fine.

is DOM_SYNTAX_ERR the best exception though?

Not really. The best exception for this kind of error would be INDEX_SIZE_ERR, I think, since that is supposed to be used for out of range arguments http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-17189187.

@olivierthereaux
Copy link
Contributor Author

Original comment by Olivier Thereaux on W3C Bugzilla. Wed, 13 Mar 2013 14:50:31 GMT

Seeing as we seem to have consensus on the solution, I am setting the issue as ASSIGNED (to CRogers - although others are welcome to send a patch to the source of the spec in dvcs.w3.org).

@olivierthereaux
Copy link
Contributor Author

Original comment by Ehsan Akhgari [:ehsan] on W3C Bugzilla. Wed, 13 Mar 2013 15:07:04 GMT

Created attachment 1336 [details]
Patch

Does this look good?

@olivierthereaux
Copy link
Contributor Author

Original comment by Ehsan Akhgari [:ehsan] on W3C Bugzilla. Wed, 13 Mar 2013 15:08:44 GMT

Created attachment 1337 [details]
Patch

Actually this is probably better!

@olivierthereaux
Copy link
Contributor Author

Original comment by Olivier Thereaux on W3C Bugzilla. Thu, 14 Mar 2013 16:45:19 GMT

(In reply to comment #11)

Created attachment 1337 [details]
Patch

Actually this is probably better!

Ehsan, we discussed this - good to go, please push to the repo.

@padenot
Copy link
Member

padenot commented Dec 12, 2013

Fix pushed to the refactored branch, here: padenot@efcd0e0, this had never been pushed to the spec.

@olivierthereaux
Copy link
Contributor Author

Looks good. Closing.

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

2 participants