AudioBuffer createBuffer(ArrayBuffer buffer, boolean mixToMono);

I briefly looked at this, since most of the webaudio demos in the wild seem to require it. It seems like most of the work lies in adding a synchronous path to MediaBufferDecoder. Maybe a little too much for the two-hour hack I had planned :)

(In reply to comment #1) > I briefly looked at this, since most of the webaudio demos in the wild seem to > require it. It seems like most of the work lies in adding a synchronous path to > MediaBufferDecoder. Maybe a little too much for the two-hour hack I had planned > :) Well, what synchronous paths for the MediaBufferDecoder is going to be very easy. I think the way to go here is to basically do what DecodeAudioData does until it finished. Also, I liked to avoid having to implement this, but I'm beginning to think that ship has sailed. :( Do you still wanna take this? I'd say it should not take more than 2 hours. ;-)

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2) > (In reply to comment #1) > > I briefly looked at this, since most of the webaudio demos in the wild seem to > > require it. It seems like most of the work lies in adding a synchronous path to > > MediaBufferDecoder. Maybe a little too much for the two-hour hack I had planned > > :) > > Well, what synchronous paths for the MediaBufferDecoder is going to be very > easy. I think the way to go here is to basically do what DecodeAudioData > does until it finished. So just call DecodeAudioData from CreateBuffer and wait for the callback? I guess that should work. > > Also, I liked to avoid having to implement this, but I'm beginning to think > that ship has sailed. :( It looks like it's in the spec, why would you want to avoid it? Just because it's an awful synchronous API? > > Do you still wanna take this? I'd say it should not take more than 2 hours. > ;-) I can give it a shot, but can't make any promises as I have other stuff I'm supposed to be doing :)

(In reply to comment #3) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #2) > > (In reply to comment #1) > > > I briefly looked at this, since most of the webaudio demos in the wild seem to > > > require it. It seems like most of the work lies in adding a synchronous path to > > > MediaBufferDecoder. Maybe a little too much for the two-hour hack I had planned > > > :) > > > > Well, what synchronous paths for the MediaBufferDecoder is going to be very > > easy. I think the way to go here is to basically do what DecodeAudioData > > does until it finished. > > So just call DecodeAudioData from CreateBuffer and wait for the callback? I > guess that should work. And also handle mixToMono, but yeah that should be basically it. > > Also, I liked to avoid having to implement this, but I'm beginning to think > > that ship has sailed. :( > > It looks like it's in the spec, why would you want to avoid it? Just because > it's an awful synchronous API? Yes! > > Do you still wanna take this? I'd say it should not take more than 2 hours. > > ;-) > > I can give it a shot, but can't make any promises as I have other stuff I'm > supposed to be doing :) Cool, that's good enough for me! Please ping me if you need help.

First pass, probably needs some more work

Comment on attachment 741498 [details] [diff] [review] Implement ArrayBuffer version of AudioContext.createBuffer Review of attachment 741498 [details] [diff] [review]: ----------------------------------------------------------------- Looks fantastic! Please let me know if the comments below make sense. This of course needs tests! But I have good news, we already have some extensive tests for decodeAudioData which you can piggy back on! I suggest refactoring the internals of this success callback <http://mxr.mozilla.org/mozilla-central/source/content/media/webaudio/test/test_decodeAudioData.html?force=1#218> into a function which takes an AudioBuffer and a test object and performs the checks, and then for each file we download, once test it using decodeAudioData and once using createBuffer. This will make sure that if we add more test cases to this file in the future, they will automatically get tested using both functions. (Also, you may want to rename that file to test_mediaDecoding.html or something.) I can deal with the mixToMono business in a follow-up bug. ::: content/media/webaudio/AudioContext.cpp @@ +117,5 @@ > + aBuffer.Data(), aBuffer.Length(), > + contentType); > + > + nsAutoPtr<WebAudioDecodeJob> job( > + new WebAudioDecodeJob(contentType, aBuffer, this)); You can just create this object on the stack. @@ +125,5 @@ > + job->mOutput) { > + return job->mOutput.forget(); > + } > + > + aRv.Throw(NS_ERROR_FAILURE); This violates the spec. It says that you should return null if something goes wrong when decoding the buffer, so you should not need to throw anything here, and should be able to get rid of aRv. ::: content/media/webaudio/MediaBufferDecoder.cpp @@ +416,5 @@ > } > > void > +MediaDecodeTask::NextPhase() > +{ Please add some comment here saying that this hides the logic of whether we're running on the main thread or talking to the thread pool. @@ +430,5 @@ > + break; > + case PhaseEnum::Decode: > + case PhaseEnum::CopyBuffer: > + mThreadPool->Dispatch(this, nsIThreadPool::DISPATCH_NORMAL); > + break; Please MOZ_ASSERT the expected thread in both cases in the switch. @@ -488,5 @@ > > void > MediaDecodeTask::CopyBuffer() > { > - MOZ_ASSERT(!NS_IsMainThread()); Removing these threading assertions is extremely dangerous. Instead, please modify it to make the decision of what to assert based on whether mThreadPool is null or not. @@ +707,5 @@ > bool > +MediaBufferDecoder::SyncDecodeMedia(const char* aContentType, uint8_t* aBuffer, > + uint32_t aLength, > + WebAudioDecodeJob& aDecodeJob) > +{ Here, please MOZ_ASSERT that the thread pool, and the callbacks are both null. Please also MOZ_ASSERT the reverse in AsyncDecodeMedia. @@ +785,5 @@ > +{ > + MOZ_ASSERT(aContext); > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_COUNT_CTOR(WebAudioDecodeJob); > +} I think you need to remove this ctor. ::: content/media/webaudio/MediaBufferDecoder.h @@ +34,5 @@ > dom::DecodeSuccessCallback* aSuccessCallback, > dom::DecodeErrorCallback* aFailureCallback); > + WebAudioDecodeJob(const nsACString& aContentType, > + const dom::ArrayBuffer& aBuffer, > + dom::AudioContext* aContext); Why do you need a new ctor? Can't you just make aSuccessCallback and aFailureCallback be nullptr by default, and MOZ_ASSERT that you either make them both null or neither? ::: dom/webidl/AudioContext.webidl @@ +23,5 @@ > > [Creator, Throws] > AudioBuffer createBuffer(unsigned long numberOfChannels, unsigned long length, float sampleRate); > > + [Creator, Throws] Drop Throws please. @@ +24,5 @@ > [Creator, Throws] > AudioBuffer createBuffer(unsigned long numberOfChannels, unsigned long length, float sampleRate); > > + [Creator, Throws] > + AudioBuffer createBuffer(ArrayBuffer buffer, boolean mixToMono); Hrm, this should be AudioBuffer? I think. I'll update the spec accordingly.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #6) > Comment on attachment 741498 [details] [diff] [review] > Implement ArrayBuffer version of AudioContext.createBuffer > > Review of attachment 741498 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks fantastic! Please let me know if the comments below make sense. > It's a great review, thanks! Some comments below > @@ +707,5 @@ > > bool > > +MediaBufferDecoder::SyncDecodeMedia(const char* aContentType, uint8_t* aBuffer, > > + uint32_t aLength, > > + WebAudioDecodeJob& aDecodeJob) > > +{ > > Here, please MOZ_ASSERT that the thread pool, and the callbacks are both > null. The callbacks should be asserted in the WebAudioDecodeJob constructor as you noted above. Making any assertions about the thread pool is kind of tricky, because you could in theory call SyncDecodeMedia and AsyncDecodeMedia on the same instance. This also means we really should to have some flag in the job that says whether it's sync or not instead of relying on the existence of the thread pool. > > @@ +24,5 @@ > > [Creator, Throws] > > AudioBuffer createBuffer(unsigned long numberOfChannels, unsigned long length, float sampleRate); > > > > + [Creator, Throws] > > + AudioBuffer createBuffer(ArrayBuffer buffer, boolean mixToMono); > > Hrm, this should be AudioBuffer? I think. I'll update the spec accordingly. Not following. Did you mean ArrayBuffer?

Ugh. It looks like the WebIDL generator is unhappy about the extended attributes differing between two overloads of the same method. Suggestions?

(In reply to comment #7) > > @@ +707,5 @@ > > > bool > > > +MediaBufferDecoder::SyncDecodeMedia(const char* aContentType, uint8_t* aBuffer, > > > + uint32_t aLength, > > > + WebAudioDecodeJob& aDecodeJob) > > > +{ > > > > Here, please MOZ_ASSERT that the thread pool, and the callbacks are both > > null. > > The callbacks should be asserted in the WebAudioDecodeJob constructor as you > noted above. Making any assertions about the thread pool is kind of tricky, > because you could in theory call SyncDecodeMedia and AsyncDecodeMedia on the > same instance. That is what I would like to disallow. :-) > This also means we really should to have some flag in the job that says whether > it's sync or not instead of relying on the existence of the thread pool. We sort of do, it's mThreadpool, we just don't waste space to store it! > > @@ +24,5 @@ > > > [Creator, Throws] > > > AudioBuffer createBuffer(unsigned long numberOfChannels, unsigned long length, float sampleRate); > > > > > > + [Creator, Throws] > > > + AudioBuffer createBuffer(ArrayBuffer buffer, boolean mixToMono); > > > > Hrm, this should be AudioBuffer? I think. I'll update the spec accordingly. > > Not following. Did you mean ArrayBuffer? Sorry, I meant: AudioBuffer? createBuffer(ArrayBuffer buffer, boolean mixToMono); Putting a question mark after a type name in Web IDL makes that nullable.

(In reply to comment #8) > Ugh. It looks like the WebIDL generator is unhappy about the extended > attributes differing between two overloads of the same method. Suggestions? Unhappy how?

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #10) > (In reply to comment #8) > > Ugh. It looks like the WebIDL generator is unhappy about the extended > > attributes differing between two overloads of the same method. Suggestions? > > Unhappy how? WebIDL.WebIDLError: error: Extended attributes differ on different overloads of <unresolved scope>::createBuffer, AudioContext.webidl line 25:4 That's if I remove 'Throws'. I guess we should be able to keep Throws and return null if we have 'AudioBuffer?'. That seems to be the important part I was missing.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #11) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #10) > > (In reply to comment #8) > > > Ugh. It looks like the WebIDL generator is unhappy about the extended > > > attributes differing between two overloads of the same method. Suggestions? > > > > Unhappy how? > > WebIDL.WebIDLError: error: Extended attributes differ on different overloads > of <unresolved scope>::createBuffer, AudioContext.webidl line 25:4 Hmm, perhaps Boris knows what's going on with that? I think it's insane to expect the extended attributes to be the same on overloads! > That's if I remove 'Throws'. I guess we should be able to keep Throws and > return null if we have 'AudioBuffer?'. That seems to be the important part I > was missing. Hmm, in order to make progress here, let's keep [Throws] but never touch the ErrorResult& argument that we receive, use AudioBuffer? and just return null if something goes wrong.

> I think it's insane to expect the extended attributes to be the same on overloads! Right now the WebIDL parser just has a single object representing "all of the overloads of a method" and associates extended attributes with that object as a whole. So we can't keep track of per-overload extended attributes. We could probably rejigger the data model to allow it, if we had to, but it would take a bit of work. For now, I recommend exactly what comment 12 said: you can always say something is [Throws] and simply never throw. All it costs you is a but of ugly in the function signature an a runtime check in the caller...

Comment on attachment 744656 [details] [diff] [review] Implement ArrayBuffer version of AudioContext.createBuffer Review of attachment 744656 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! ::: content/media/webaudio/MediaBufferDecoder.cpp @@ +418,5 @@ > void > +MediaDecodeTask::RunNextPhase() > +{ > + // This takes care of handling the logic of where to run the next phase. > + // If we were invoked synchronously, we do not have a thread pool and OMG TRAILING SPACE ;-) @@ +439,5 @@ > + mThreadPool->Dispatch(this, nsIThreadPool::DISPATCH_NORMAL); > + break; > + case PhaseEnum::Decode: > + MOZ_NOT_REACHED("Invalid phase Decode"); > + break; Nit: please indent the case statements on the same level of the switch statement to adhere with the current style of this code. @@ +522,5 @@ > { > + if (mThreadPool) > + MOZ_ASSERT(!NS_IsMainThread()); > + else > + MOZ_ASSERT(NS_IsMainThread()); This is probably better: MOZ_ASSERT(!mThreadPool == NS_IsMainThread(), "We should be on the main thread only if we don't have a thread pool"); @@ +783,5 @@ > MOZ_COUNT_CTOR(WebAudioDecodeJob); > + > + if (aSuccessCallback || aFailureCallback) { > + MOZ_ASSERT(aSuccessCallback && aFailureCallback); > + } MOZ_ASSERT((aSuccessCallback && aFailureCallback) || (!aSuccessCallback && !aFailureCallback), "You cannot pass only one of the success and failure callbacks"); ::: content/media/webaudio/MediaBufferDecoder.h @@ +31,5 @@ > WebAudioDecodeJob(const nsACString& aContentType, > const dom::ArrayBuffer& aBuffer, > dom::AudioContext* aContext, > + dom::DecodeSuccessCallback* aSuccessCallback = nullptr, > + dom::DecodeErrorCallback* aFailureCallback = nullptr); Please add a comment saying when these arguments can be null. ::: content/media/webaudio/test/test_mediaDecoding.html @@ +238,5 @@ > + > + checkAudioBuffer(result, test, function() { > + result = cx.createBuffer(xhr.response, false); > + checkAudioBuffer(result, test, callback); > + }); Reviewing this test was a pain because you have not preserved the file move. I think this is the main change and it looks fine, but please make sure that you're preserving the file move in the history before landing.

Final patch with nits fixed

Sorry, backed out on inbound because of assertion failures in debug build tests: https://hg.mozilla.org/integration/mozilla-inbound/rev/6d7fbee951a1 14:48:09 INFO - Assertion failure: (aSuccessCallback && aFailureCallback) || (!aSuccessCallback && !aFailureCallback) (You cannot pass only one of the success and failure callbacks), at ../../../../content/media/webaudio/MediaBufferDecoder.cpp:785 14:48:10 WARNING - TEST-UNEXPECTED-FAIL | /tests/content/media/webaudio/test/test_bug845960.html | Exited with code 11 during test run 14:48:22 WARNING - PROCESS-CRASH | /tests/content/media/webaudio/test/test_bug845960.html | application crashed [@ mozilla::WebAudioDecodeJob::WebAudioDecodeJob] https://tbpl.mozilla.org/php/getParsedLog.php?id=22566774&tree=Mozilla-Inbound

Sorry, this is my fault. If content doesn't specify a failure callback function, aFailureCallback will be null. I'll fix the assertion and reland.

Comment on attachment 745322 [details] [diff] [review] Implement ArrayBuffer version of AudioContext.createBuffer Review of attachment 745322 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/MediaBufferDecoder.cpp @@ +448,3 @@ > MediaDecodeTask::Decode() > { > MOZ_ASSERT(!NS_IsMainThread()); This assertion also needs to be adjusted similar to the one in CopyBuffer. @@ +724,5 @@ > + strcmp(aContentType, APPLICATION_OCTET_STREAM) == 0) { > + return false; > + } > + > + MOZ_ASSERT(!mThreadPool); You were right about this assertion, you can and do call both SyncDecodeMedia and AsyncDecodeMedia on the same instance. In fact, each AudioContext has a single MediaBufferDecoder instance which it uses for all decoding tasks. Man am I rusty on this code! :(

Fixed my review mess and relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/8e05ccbcb09b

Mass moving Web Audio bugs to the Web Audio component. Filter on duckityduck.