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

It makes no sense to use EventListener for onaudioprocess if ScriptProcessorNode isn't EventTarget #116

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

Comments

@olivierthereaux
Copy link
Contributor

Originally reported on W3C Bugzilla ISSUE-20764 Thu, 24 Jan 2013 22:57:14 GMT
Reported by Olli Pettay
Assigned to

First, EventListener should not be used for onfoo handlers.
EventHandler is the right type (or in very special legacy cases OnErrorEventHandler).

But more importantly, if ScriptProcessorNode isn't EventTarget dispatching
event to it isn't really possible, and the event wouldn't have any
.target.

So, either ScriptProcessorNode needs to become EventTarget somehow, and
get normal event handling, or the callback needs to not use Event objects.

@olivierthereaux
Copy link
Contributor Author

Original comment by Dominic Cooney on W3C Bugzilla. Tue, 19 Feb 2013 05:14:30 GMT

Bug 17493 comment 1 [1] is relevant.

ScriptProcessorNode should have its own type of callback for onaudioprocess.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=17493#c1

@olivierthereaux
Copy link
Contributor Author

Original comment by Ehsan Akhgari [:ehsan] on W3C Bugzilla. Tue, 19 Feb 2013 05:16:40 GMT

That works too, as long as it's not an event target.

@olivierthereaux
Copy link
Contributor Author

Original comment by Ehsan Akhgari [:ehsan] on W3C Bugzilla. Tue, 19 Feb 2013 05:19:39 GMT

Also, in that case, we should rename onaudioprocess for consistency as well.

@olivierthereaux
Copy link
Contributor Author

Original comment by Chris Rogers on W3C Bugzilla. Tue, 19 Feb 2013 06:01:26 GMT

(In reply to comment #3)

Also, in that case, we should rename onaudioprocess for consistency as well.

This would be a breaking API change for shipping versions, so we'd have to be careful to move it to the "deprecated" and recommended section if we thought it important to change. Personally, I don't think there's any reason it can't still be called onaudioprocess even though it has nothing to do with EventListener

@olivierthereaux
Copy link
Contributor Author

Original comment by Ehsan Akhgari [:ehsan] on W3C Bugzilla. Tue, 19 Feb 2013 06:12:01 GMT

(In reply to comment #4)

(In reply to comment #3)

Also, in that case, we should rename onaudioprocess for consistency as well.

This would be a breaking API change for shipping versions, so we'd have to
be careful to move it to the "deprecated" and recommended section if we
thought it important to change. Personally, I don't think there's any
reason it can't still be called onaudioprocess even though it has nothing to
do with EventListener

webkit can still support the old API I guess, but the onXXX naming convention is used throughout the web platform for things using event targets, and using such a naming scheme will be very surprising to all web developers, so I think that is a really bad idea.

Also the usage of callbacks can enable us clean up the API by accepting the callback as an argument to the createScriptProcessor function, which makes it unnecessary to expose it on ScriptProcessorNode at all. Speaking of which, I don't think exposing bufferSize on that node type makes much sense either.

@olivierthereaux
Copy link
Contributor Author

Original comment by Srikumar Subramanian (Kumar) on W3C Bugzilla. Tue, 19 Feb 2013 08:04:40 GMT

(In reply to comment #5)

Also the usage of callbacks can enable us clean up the API by accepting the
callback as an argument to the createScriptProcessor function, which makes
it unnecessary to expose it on ScriptProcessorNode at all. Speaking of
which, I don't think exposing bufferSize on that node type makes much sense
either.

It is useful to be able to change the callback after creating the script node. Please lets not change that.

Exposing buffer size is also useful. The script node currently shares CPU with the main thread. Given this architecture, the amount of time available for audio can be different for different kinds of applications. A graphics rich application may not be able to guarantee enough callbacks just in time, in which case longer buffers would be desirable. An application that spends most of its time doing audio or being idle can use smaller buffer sizes to get lower latency. It is unnecessary to mandate an implementation to automatically determine the required buffering.

@olivierthereaux
Copy link
Contributor Author

Original comment by Robert O'Callahan (Mozilla) on W3C Bugzilla. Mon, 04 Mar 2013 22:40:05 GMT

(In reply to comment #6)

Exposing buffer size is also useful. The script node currently shares CPU
with the main thread. Given this architecture, the amount of time available
for audio can be different for different kinds of applications. A graphics
rich application may not be able to guarantee enough callbacks just in time,
in which case longer buffers would be desirable. An application that spends
most of its time doing audio or being idle can use smaller buffer sizes to
get lower latency. It is unnecessary to mandate an implementation to
automatically determine the required buffering.

The problem with the author choosing the buffer size is that they almost certainly won't be able to choose a size that will work on all kinds of platforms and devices. The UA is best placed to do that. Anyway, there has been discussion about this on the list, and further discussion should take place there, definitely not in this bug :-)

@olivierthereaux
Copy link
Contributor Author

Original comment by Olivier Thereaux on W3C Bugzilla. Thu, 14 Mar 2013 17:36:06 GMT

At the 2013-03-14 teleconference, we agreed that the best solution was probably the one suggested by Dominic in http://lists.w3.org/Archives/Public/public-audio/2013JanMar/0387.html

Ehsan, would you be able to fill in the blanks and make the change?

@olivierthereaux
Copy link
Contributor Author

Original comment by Dominic Cooney on W3C Bugzilla. Fri, 15 Mar 2013 02:05:55 GMT

I feel like the change proposed here is not in line with recent discussion on public-audio ("The issue of ScriptProcessorNode not being an EventTarget.") http://lists.w3.org/Archives/Public/public-audio/2013JanMar/0216.html

@olivierthereaux
Copy link
Contributor Author

Original comment by Dominic Cooney on W3C Bugzilla. Fri, 15 Mar 2013 02:07:10 GMT

Please disregard Comment 9--I did not see Comment 8.

@olivierthereaux
Copy link
Contributor Author

Original comment by Ehsan Akhgari [:ehsan] on W3C Bugzilla. Fri, 15 Mar 2013 22:36:02 GMT

OK, I edited the spec accordingly:

https://dvcs.w3.org/hg/audio/rev/c68c2551f6c8

Please let me know if it looks good.

Now's the time to talk about AudioProcessingEvent.node. I don't think that it makes sense for that attribute to exist, since it will be the same as AudioProcessingEvent.target. Do I need to file another bug on that?

@olivierthereaux
Copy link
Contributor Author

Original comment by Olli Pettay on W3C Bugzilla. Fri, 15 Mar 2013 22:45:17 GMT

"This interface is a type of Event which is passed to the onaudioprocess event handler used by ScriptProcessorNode. "
is still a bit odd, since event handling uses now the normal model.

And there is also
"This value controls how frequently the onaudioprocess event handler is called"

The spec also doesn't mention when audioprocess event is dispatched, only
vaguely talks about how bufferSize affects to the frequency of those events.

@olivierthereaux
Copy link
Contributor Author

Original comment by Dominic Cooney on W3C Bugzilla. Mon, 18 Mar 2013 09:12:33 GMT

Where is EventHandler defined? In the [DOM] reference, only EventListener is defined. I think the spec might be missing a reference.

@olivierthereaux
Copy link
Contributor Author

Original comment by Ehsan Akhgari [:ehsan] on W3C Bugzilla. Thu, 21 Mar 2013 03:56:45 GMT

(In reply to comment #12)

"This interface is a type of Event which is passed to the onaudioprocess
event handler used by ScriptProcessorNode. "
is still a bit odd, since event handling uses now the normal model.

And there is also
"This value controls how frequently the onaudioprocess event handler is
called"

I cleared up the wording in https://dvcs.w3.org/hg/audio/rev/e26b49247d95.

The spec also doesn't mention when audioprocess event is dispatched, only
vaguely talks about how bufferSize affects to the frequency of those events.

That is a good question. I'll leave that part to crogers.

(In reply to comment #13)

Where is EventHandler defined? In the [DOM] reference, only EventListener is
defined. I think the spec might be missing a reference.

The HTML spec. I added a reference in https://dvcs.w3.org/hg/audio/rev/2056740735fd.

@cwilso
Copy link
Contributor

cwilso commented Feb 24, 2014

Is the only remaining part of this bug more detail on when the audioprocess event is dispatched? I'm not even sure that can be detailed in great depth.

@cwilso
Copy link
Contributor

cwilso commented Oct 22, 2014

I think this can be closed as no longer relevant.

@cwilso cwilso closed this as completed Oct 30, 2014
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

3 participants