progressCallback in decodeAudioData
ouhouhsami opened this issue · 35 comments
Hello,
What do you think about having a progressCallback with decodeAudioData. In case of large audio files, it could takes time to "decode" the ArrayBuffer, and so the user could be informed about the progress of the decode function.
We then could have:
decodeAudioData(ArrayBuffer audioData,
DecodeSuccessCallback successCallback,
optional DecodeErrorCallback errorCallback,
optional ProgressCallback progressCallback);
Does it make sens?
Well, we could also use the Progress Events spec (https://dvcs.w3.org/hg/progress/raw-file/tip/Overview.html) to mesure the decode audio data progression.
Check with TAG on pattern for this.
@cwilson I recall that you did check with the TAG but don't see a resolution -- is this ready for editing?
Shared feeling that user experience of audio apps is much improved by understanding progress of long-running decode operations. @cwilso proposes subclassing ProgressEvent; we will discuss at F2F. One issue is, what object should act as the dispatcher and target of the event?
I'd suggest subclassing ProgressEvent just to add the ArrayBuffer to it, and expecting developers to use that as the key for which decode this is a ProgressEvent for, and targeting the events to the AudioContext (which is already an EventTarget).
@cwilso Ick. What benefit does the ProgressEvent have over a callback? With a callback, there's no need to figure out context; authors can use JavaScript scopes to track what decode operation is associated with what callback. With a ProgressEvent, there can only ever be one handler, on the AudioContext. With a callback, each decode operation can have a separate callback handler.
@jernoble Doesn't addEventListener allow for multiple event listeners to be added to the AudioContext?
Lets contrast dummy code written against the ProgressEvent and the callback approaches.
With ProgressEvent (AudioContext as target):
var audioContext = ...;
var arrayBuffer = ...;
var progressObserver = function(event) {
if (arrayBuffer === event.arrayBuffer) {
// Update UI with progress...
}
};
audioContext.addEventListener('decodeprogress', progressObserver);
audioContext.decodeAudioData(arrayBuffer).then(function(audioBuffer) {
audioContext.removeEventListener('decodeprogress', progressObserver);
// Do something with the audioBuffer...
}, function() {
audioContext.removeEventListener('decodeprogress', progressObserver);
// Cry...
});With callback:
var audioContext = ...;
var arrayBuffer = ...;
var progressObserver = function(progress /* or event? */) {
// Update UI with progress...
};
audioContext.decodeAudioData(arrayBuffer, null, null, progressObserver).then(function(audioBuffer) {
// Do something with the audioBuffer...
}, function() {
// Cry...
});For the callback approach, I assumed that the signature for decodeAudioData has a new optional callback following the optional legacy (pre-promise) success and failure callbacks.
@jernoble Doesn't addEventListener allow for multiple event listeners to be added to the AudioContext?
Yes, that's true. I was thinking of a single onprogress attribute. Still, they'll each get fired for every simultaneous decode progress event, so you end up with--effectively--a single callback containing a giant if-then-else-if statement (as you illustrate with your sample code).
var progressObserver = function(progress /* or event? */) {
I would suggest duration here. There are a few audio formats whose durations cannot be known until the entire file is decoded. Passing a percentage here (length decoded / total duration) would be incorrect for these formats.
@mark-buer yes, you can have multiple event listeners. I'd expect in most apps that you'd want a consistent UI for decoding progress, so you'd actually WANT to collect the progress all in one place. Also, I doubt (for that reason) that you'd want to remove the listener all the time, and the pattern would be:
var audioContext = ...;
var arrayBuffer = ...;
audioContext.addEventListener('decodeprogress', function(event) {
// Update UI with progress for given ArrayBuffer...
});
audioContext.decodeAudioData(arrayBuffer).then(function(audioBuffer) {
// Do something with the audioBuffer...
}, function() {
// Cry...
});
@jernoble It's not "Ick" - it's the benefit of having a consistent pattern of code for consistent patterns of scenario. This is precisely why we use Promises now rather than success/error callbacks in modern APIs - because it's a consistent pattern, and developers don't have to keep looking up what the right pattern is.
@cwilso It is "ick", precisely because this breaks the consistent pattern of objects which emit progress events. You don't have a global object which represents all HTTP requests, and force all users to do a giant switch statement to figure out which progress goes with which request. And that's what you're suggesting we add here.
Or, it'd be like having a single VideoContext object, and forcing clients of HTMLMediaElement to all listen for progress events emitted out that global object, rather than the media element itself.
...so you'd actually WANT to collect the progress all in one place...
If you had a per-decode callback (or had a per-decode object as the target for a progress event), you could still get that behavior by passing the same callback/listener for each decode, without handicapping other use cases.
As a developer who has sometimes had to play the game of mapping globally dispatched events back to specific handlers, I have to agree with @jernoble on the undesirability of firing all this out of the AudioContext. I don't think we should assume that progress events are all going to be collected and summarized in one place.
A per-decode object would be nicest. A per-decode callback is not as nice, but we started this method with callbacks.
Perhaps another cleaner possibility is to support a new method, .progress(), on the returned Promise (which would then be a subclass of Promise I guess).
@joeberkovitz I'd rather add a new per-decode object than try to subclass Promise. I've got a spec patch for adding streaming audio decoding, which might help illustrate what this might look like: http://jernoble.github.io/web-audio-api/#the-audiostreamdecoder-interface | https://github.com/jernoble/web-audio-api/tree/337-audiostreamdecoder
@jernoble That seems like a very elegant approach.
@jernoble At the risk of heading off-topic, a per-decode object might also provide a simple API for (potentially large/numerous) decodes to be cancelled. The current API offers no mechanism for this.
In our app, we opportunistically queue up many large decodes ahead of time so that the decoded data is ready if/when we need it. User interactions dictate the set of required decoded data. These interactions can occur in rapid succession. Ideally, our app would cancel unneeded incomplete decodes before initiating new decodes, potentially saving time and energy (especially on mobile).
Resolution: we will add a third, optional progressCallback argument to decodeAudioData() with this type:
callback ProgressCallback = void (unsigned long long decoded, unsigned long long total);
The decoded and total counts refer to the number of bytes in the input buffer -- i.e. decoded means "number of input bytes decoded so far", not "number of decoded output bytes".
Hmm. In #30, it has been suggested to add a fourth optional parameter for a dictionary to allow disabling the resampling for decodeAudioData. This now conflicts with #335 (comment) from @joeberkovitz.
How do we reconcile these two? Make the progressCallback method another entry in the dictionary?
The WG should discuss this. Because the conflict involves a comment from me (which just recorded an earlier resolution by the group) I want to clarify that I'm not opposed to changing the approach to a unified dictionary that includes the callback.
Per teleconf: Need to look into how merge these two ideas.
Any suggestions, @domenic ?
I think since callbacks and dictionaries are distinguishable, you could add an overload. Something like:
// legacy version
audioContext.decodeAudioData(arrayBuffer, successCb, errorCb);
// new hotness
audioContext.decodeAudioData(arrayBuffer, {
progress(decoded, total) { /* ... */ },
disableResampling: true
})
.then(successCb, errorCb);Thanks for the suggestion. @padenot was wondering if this was good Web style to do this kind of overloading. I assume this is ok, based on your suggestion.
I think in general it's bad style to do that kind of overloading (in particular I personally dislike overloading more than most). However, in my opinion it's fairly justified in this case.
The mitigating factors are: (1) other alternatives all seem worse; (2) this is less about providing two overloads you expect people to use both of, than it is about providing a new method and leaving the old method around without breaking back-compat.
A minor fix for the property bag:
audioContext.decodeAudioData(arrayBuffer, {
progressCallback: function (decoded, total) { /* ... */ },
disableResampling: true
})
.then(successCallback, rejectCallback);I'm not sure you need a type suffix in the name (processCallback vs. progress). It's pretty clear that progress(decoded, total) is a function. You don't say disableResamplingBoolean.
Oh, the first value in the property bag was just a function. It needs to be a key-value pair. doesn't it?
In modern JavaScript you can omit : function for methods.
Wow. Good to know!
Resolved that it's better to defer this capability until we develop a cleaner and more fully thought out decoding API?
c'mon i'm waitin' on this 4 2 years now...
any updates?
For what it's worth, we've found that, in the meantime, a good approximation of determining decode progress is to use a fake progress that lasts for a duration 2.5x as long as a FileReader.readAsArrayBuffer call. We managed to stay within a sub-50% error margin (Chrome), which, I must say, isn't that bad at all.
If you really need to display progress, I'd say this is still much better than showing no progress at all.
This is of course entirely implementation dependent, and it's likely that browser updates will eventually drift the value.
This will now be handled by https://discourse.wicg.io/t/webcodecs-proposal/3662
This will now be handled by https://discourse.wicg.io/t/webcodecs-proposal/3662
Tried to use WebCodecs today to do this with WebCodecs (unfortunately failing so far), and posted w3c/webcodecs#366 about my findings so far. Any help would be appreciated!