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

Karl, please ping me if you hit problem porting stuff from Blink, or, any other problems. :-)

HRTF impluse responses in blink are (by default) concatenated into one file: % svn ls -v http://src.chromium.org/blink/trunk/Source/core/platform/audio/resources/Composite.wav 132993 philn@we 245804 Oct 31 2012 Composite.wav % gzip -c Composite.wav|wc -c 142903

Karl, do you have an ETA on when you're going to have the patches for this? I'd really like to get something landed before the uplift which is this Monday. Thanks!

I'm not going to have a patch for Monday. I expect I'll need another week. I understand much of what the blink code is doing now, but am still working out how to hook it into our code.

(In reply to comment #5) > I'm not going to have a patch for Monday. I expect I'll need another week. > I understand much of what the blink code is doing now, but am still working out > how to hook it into our code. OK, thanks! Please ping me if you have any questions, such as which of their objects map to which of ours, etc.

Blink's FFTFrame cross-platform routines have some routines for extracting the group delay from the DFT and interpolating DFTs. These are used in the HRTF implementation. There are some heuristics involved in the interpolation code, so it would be hard to reimplement without effectively copying. I think the tidiest way to use this is to merge Ehsan's FFTBlock implementation into the blink implementation and switch to using that. This is asking permission from Ehsan to relicense his code from FFTBlock.

I may attach some more patches later today, but they are not tested beyond compiling. I still have some days of work to do to have the rest of implementation ready to be testing, but I can attach if Ehsan is interested in reviewing untested code. https://tbpl.mozilla.org/?tree=Try&rev=995b65f296b3 (compile errors on older gcc need to be resolved by switching to MOZ_ARRAY_LENGTH)

Comment on attachment 769819 [details] [diff] [review] part 3: Add Blink's FFTFrame to the build Review of attachment 769819 [details] [diff] [review]: ----------------------------------------------------------------- I don't think it's a good idea to borrow Blink's FFTFrame directly. Their code is a lot more complicated, and they support multiple FFT backends, which is not something that we need to do at this point. Instead, we should just extend our FFTBlock to add any additional APIs that the HRTF code relies on, and then rewrite that code to use mozilla::FFTBlock instead of WebCore::FFTFrame.

This time no FFTFrame.h.

The only real differences between the FFTFrame and FFTBlock API now are FFTBlock::SetSize(), FFTFrame::log2FFTSize(), FFTFrame has a copy constructor, FFTBlock::createInterpolatedFrame() returns a not-smart pointer, and the DFT data APIs.

The Blink extractAverageGroupDelay() did an ASSERT(isSizeGood) and then returned early if it were not. This assert would be hit when the sample rate is below 44100 Hz. This patch moves the assertion to HRTFElevation. It will be removed and dealt with in parts 7 and 8.

Blink uses impulse responses derived from IRCAM Listen Project measurements [1]. They have been averaged over multiple subjects, probably using HRTFUtilities.cpp [2], and symmetrized, probably using HRTFElevation::calculateSymmetricKernelsForAzimuthElevation(). Because of symmetry of left and right ears and azimuths around the entire circle, each right ear response is exactly the same as (at least) one of the left ear responses. [1] http://recherche.ircam.fr/equipes/salles/listen/ [2] http://trac.webkit.org/browser/branches/old/audio/WebCore/platform/audio/HRTFUtilities.cpp

Although Blink has 240 impulse response files, only 187 are unique. This is because the Listen responses were measured at fewer azimuths for elevations above 45 degrees. The duplicate files in Blink come from copying responses from lower elevations so that all elevations have the same number of azimuths. The duplicates are not included in this patch.

The Listen project data is public domain [3]. Chris Rogers has said publicly that he is "happy to share the dataset used in WebKit" [4]. I emailed Chris privately asking whether he would be willing to grant that derived data to the public domain or make them under another license. He replied saying he would be happy to make them available in the public domain. [3] e.g. http://recherche.ircam.fr/equipes/salles/listen/info_display.php?subject=IRC_1047 [4] http://lists.w3.org/Archives/Public/public-audio/2013AprJun/0069.html

Usage was: for elevation in 315 330 345 000 015 030 045 060 075 090; do ~/moz/dev/hrtfwav2c.pl IRC_Composite_C_R0195_T*_P$elevation.wav; done >> ~/moz/dev/content/media/webaudio/blink/IRC_Composite_C_R0195-incl.cpp

The fftSizeForSampleRate method is moved from HRTFPanner to HRTFElevation because the values that it returns are dependent on the impulse response sizes and use of HRTFKernel in HRTFElevation. This also makes the class dependency graph directed.

The blink version of this function asserted sampleRate >= 44100 even though Blink offline audio contexts can use rates down to 22050. Gecko audio contexts can have even lower rates. I'm concerned about rates even a little below 44100 because, as explained in the new comments, HRTFKernel is not designed for DFT sizes greater than twice the impulse response length.

This is used in HRTFPanner in the next patch. Using AudioChunk in the interface instead of the float** channel pointers may have been nicer. That's probably possible, but gets more complicated in HRTFPanner where single channels have different delays and subsegments are used for cross-fading.

This is not ready for production yet as the HRTFDatabaseLoader gets destroyed on the MSG thread, but it is enough to check that the rest is performing basic functionality. I'll be away for a week or so, so posting where I'm at. There are only minor changes between parts 1 to 12 and changesets in this try run: https://tbpl.mozilla.org/?tree=Try&rev=1a66cf489adc This sounds better than the equalpower model, much more convincing left to right. With my on-ear phones on my ears, it doesn't do a good job of placing the sound in front. I haven't got Chromium to produce sound yet, so haven't compared there. http://people.mozilla.org/~karlt/panner-horizontal.html http://people.mozilla.org/~karlt/panner-vertical.html I haven't worked out the best approach for dealing with HRTFDatabaseLoader. It is designed to have its lifetime controlled from the mail thread, so it would like to live on the PannerNode. I don't know whether taking a NodeMutex on every ProduceAudioBlock call is ideal. I haven't got a good understanding of the PlayingRefChangeHandler. It seems that there are some situations where the AudioNode can be assumed to be still alive, but I don't know what those situations are. An alternative might be to add a mutex into HRTFDatabaseLoader, as that is accessed less frequently.

Comment on attachment 773073 [details] [diff] [review] part 3: Add Blink's frequency interpolation and group delay methods from FFTFrame to FFTBlock Review of attachment 773073 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/FFTBlock.cpp @@ +48,5 @@ > + int fftSize = newBlock->FFTSize(); > + nsTArray<float> buffer; > + buffer.SetLength(fftSize); > + newBlock->PerformInverseFFT(buffer.Elements()); > + PodZero(buffer.Elements() + fftSize / 2, fftSize / 2); See below... @@ +75,5 @@ > double lastPhase1 = 0.0; > double lastPhase2 = 0.0; > > + dft[0].r = static_cast<float>(s1base * dft1[0].r + s2base * dft2[0].r); > + dft[0].i = static_cast<float>(s1base * dft1[0].i + s2base * dft2[0].i); Blink's FFTFrame uses a packed format. Basically, if you DFT into an array of size N+1, the imaginary components of the first and last elements in the array will always be 0, so in the packed format you allocate an array of N complex numbers, and store the (N+1)th element's real component in the imaginary component of the first element, so this kind of code is necessary to account for that. Our FFTBlock uses an unpacked format, which should make this kind of hack unnecessary. These algorithms should laos be modified to be aware of the fact that our array storage is N+1 complex numbers, not N (the above case, for example.)

(See <http://developer.apple.com/library/ios/#documentation/Performance/Conceptual/vDSP_Programming_Guide/UsingFourierTransforms/UsingFourierTransforms.html> for more information on the packed storage format.)

Comment on attachment 773074 [details] [diff] [review] part 4: Add Blink's HRTFKernel to the build Review of attachment 773074 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/blink/HRTFKernel.cpp @@ +101,5 @@ > { > MemoryClassInfo info(memoryObjectInfo, this, PlatformMemoryTypes::AudioSharedData); > info.addMember(m_fftFrame, "fftFrame"); > } > +#endif Not sure if keeping this is worth it... It's pretty simple, I'd personally get rid of this.

(In reply to Karl Tomlinson (:karlt, back 22 July) from comment #17) > Created attachment 773078 [details] [diff] [review] > part 5: Use symmetry to halve the number of HRTF kernels calculated and > cached > > Blink uses impulse responses derived from IRCAM Listen Project measurements > [1]. > They have been averaged over multiple subjects, probably using > HRTFUtilities.cpp [2], and symmetrized, probably using > HRTFElevation::calculateSymmetricKernelsForAzimuthElevation(). > > Because of symmetry of left and right ears and azimuths around the entire > circle, each right ear response is exactly the same as (at least) one of the > left ear responses. Do you think this is worth upstreaming?

Comment on attachment 773085 [details] [diff] [review] part 6: Add HRTF impulse response data Review of attachment 773085 [details] [diff] [review]: ----------------------------------------------------------------- I didn't actually review this... ;-)

Comment on attachment 773093 [details] [diff] [review] part 8: Reduce convolver fft size for low sample rates Review of attachment 773093 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/blink/HRTFElevation.cpp @@ +76,5 @@ > + unsigned resampledLength = > + floorf(ResponseFrameSize * sampleRate / rawSampleRate); > + // Keep things semi-sane, with max FFT size of 1024 and minimum of 4. > + unsigned size = min(resampledLength, 1023U); > + size |= 3; Can you please use std::min instead of bitwise or? :-)

Comment on attachment 773095 [details] [diff] [review] part 9: Add Blink's HRTFElevation to the build Review of attachment 773095 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/blink/HRTFElevation.cpp @@ +298,5 @@ > { > MemoryClassInfo info(memoryObjectInfo, this, PlatformMemoryTypes::AudioSharedData); > info.addMember(m_kernelListL, "kernelListL"); > } > +#endif Same comment about the other reportMemoryUsage here.

Comment on attachment 773096 [details] [diff] [review] part 10: Add Blink's HRTFDatabase and HRTFDatabaseLoader to the build Review of attachment 773096 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/blink/HRTFDatabase.cpp @@ +121,5 @@ > { > MemoryClassInfo info(memoryObjectInfo, this, PlatformMemoryTypes::AudioSharedData); > info.addMember(m_elevations, "elevations"); > } > +#endif Ditto. ::: content/media/webaudio/blink/HRTFDatabaseLoader.cpp @@ +36,5 @@ > namespace WebCore { > > // Singleton > +nsTHashtable<HRTFDatabaseLoader::LoaderByRateEntry>* > + HRTFDatabaseLoader::s_loaderMap = 0; Nit: nullptr. @@ +127,1 @@ > m_databaseLoaderThread = 0; Nit: nullptr. @@ +133,5 @@ > MemoryClassInfo info(memoryObjectInfo, this, PlatformMemoryTypes::AudioSharedData); > info.addMember(m_hrtfDatabase, "hrtfDatabase"); > info.addMember(s_loaderMap, "loaderMap", WTF::RetainingPointer); > } > +#endif Ditto.

Comment on attachment 773101 [details] [diff] [review] part 11: Refactor DelayNodeEngine delay processing into a shareable class Review of attachment 773101 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/DelayNode.cpp @@ +82,3 @@ > mLeftOverData == INT32_MIN && > aStream->AllInputsFinished()) { > + mLeftOverData = mProcessor.CurrentDelayFrames() - WEBAUDIO_BLOCK_SIZE; Please MOZ_ASSERT(aStream->SampleRate() == mDestnation->SampleRate());

Comment on attachment 773102 [details] [diff] [review] part 12: Add Blink's HRTFPanner to the build Review of attachment 773102 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/blink/HRTFPanner.cpp @@ +26,2 @@ > > +// #include <algorithm> ?

(In reply to Karl Tomlinson (:karlt, back 22 July) from comment #27) > Created attachment 773114 [details] [diff] [review] > par 13: WIP Implement HRTF panner processing based on Blink implementation > > This is not ready for production yet as the HRTFDatabaseLoader gets destroyed > on the MSG thread, but it is enough to check that the rest is performing > basic > functionality. I'll be away for a week or so, so posting where I'm at. > There are only minor changes between parts 1 to 12 and changesets in this try > run: https://tbpl.mozilla.org/?tree=Try&rev=1a66cf489adc Great work overall! > This sounds better than the equalpower model, much more convincing left to > right. With my on-ear phones on my ears, it doesn't do a good job of > placing the sound in front. I haven't got Chromium to produce sound yet, so > haven't compared there. > > http://people.mozilla.org/~karlt/panner-horizontal.html > http://people.mozilla.org/~karlt/panner-vertical.html I tried the demos in Chrome on Mac and it seemed to produce sound and the panning works fine (the only distance model which seems to produce sound is linear though, not sure why.) > I haven't worked out the best approach for dealing with HRTFDatabaseLoader. > It is designed to have its lifetime controlled from the mail thread, so it > would like to live on the PannerNode. Why does its lifetime have to be controlled by the main thread? It's refcounted, right? > I don't know whether taking a NodeMutex on every ProduceAudioBlock call is > ideal. That would be really really bad. > I haven't got a good understanding of the PlayingRefChangeHandler. It seems > that there are some situations where the AudioNode can be assumed to be still > alive, but I don't know what those situations are. Those situations will only matter if the node can produce sound after its input has been cut off. That is not the case for PannerNode, right?

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #28) > > + dft[0].r = static_cast<float>(s1base * dft1[0].r + s2base * dft2[0].r); > > + dft[0].i = static_cast<float>(s1base * dft1[0].i + s2base * dft2[0].i); > [...] in the packed format you allocate an array of N > complex numbers, and store the (N+1)th element's real component in the > imaginary component of the first element, Ah, thanks. I had wondered why the imaginary part of the DC component was labelled "nyquist" in the debug log.

I was going to leave fixing this bug to later, but it would become even more obvious when changing to N/2+1 storage, as we'd have to add more code to keep the bug. Given imagP[0] is really realP[halfSize], it doesn't make sense to calculate a phase from placing the DC and Nyquist components in different directions. The DC component doesn't have a phase offset and lastPhase is already initialized to zero. This makes extractAverageGroupDelay() consistent with addConstantGroupDelay() and to some extent interpolateFrequencyComponents(). The effect of the bug would have been small for reasonably sized FFTs.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #28) > Review of attachment 773073 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/webaudio/FFTBlock.cpp > @@ +48,5 @@ > > + int fftSize = newBlock->FFTSize(); > > + nsTArray<float> buffer; > > + buffer.SetLength(fftSize); > > + newBlock->PerformInverseFFT(buffer.Elements()); > > + PodZero(buffer.Elements() + fftSize / 2, fftSize / 2); > > See below... This buffer is in the time domain, so there is no adjustment required here to account for different frequency domain storage. > > + dft[0].r = static_cast<float>(s1base * dft1[0].r + s2base * dft2[0].r); > > + dft[0].i = static_cast<float>(s1base * dft1[0].i + s2base * dft2[0].i); > Our FFTBlock uses an unpacked format, which should make this kind of hack > unnecessary. These algorithms should laos be modified to be aware of the > fact that our array storage is N+1 complex numbers, not N (the above case, > for example.) The line you quoted was the only one that needed changing here as interpolateFrequencyComponents() was the only method that used the Nyquist component. I've retained separate treatment of the DC and Nyquist components, rather than folding into the loop, to clarify the situation that these components don't have phase and to avoid introducing a non-zero imaginary part for the Nyquist component from the deltaPhases.

At 48 kHz sample rate the minimum aveSampleDelay is 34 frames, which is always > 20. At lower sample rates, this can sometimes fall below 20 and a discontinuity develops. This is not good because realtime interpolation between DFTs is done by selecting one of the nearest DFTs and interpolating the delays returned by ExtractAverageGroupDelay. If these delays don't mean something consistent, then the interpolation doesn't make sense. Perhaps the headroom should be measured in time rather than frame count but I haven't tried to do that.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #33) > Review of attachment 773093 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/webaudio/blink/HRTFElevation.cpp > @@ +76,5 @@ > > + unsigned resampledLength = > > + floorf(ResponseFrameSize * sampleRate / rawSampleRate); > > + // Keep things semi-sane, with max FFT size of 1024 and minimum of 4. > > + unsigned size = min(resampledLength, 1023U); > > + size |= 3; > > Can you please use std::min instead of bitwise or? :-) Added a comment in my patch to explain why std::min won't do what is required: // Keep things semi-sane, with max FFT size of 1024 and minimum of 4. + // "size |= 3" ensures a minimum of 4 (with the size++ below) and sets the + // 2 least significant bits for rounding up to the next power of 2 below. unsigned size = min(resampledLength, 1023U); size |= 3; // Round up to the next power of 2, making the FFT size no more than twice // the impulse response length. This doubles size for values that are // already powers of 2. This works by filling in 7 bits to right of the // most significant bit. The most significant bit is no greater than // 1 << 9, and the least significant 2 bits were already set above. Also removed the various reportMemoryUsage methods, s/0/nullptr/ where requested, and added sample rate assertion to patches in my queue.

Comment on attachment 773102 [details] [diff] [review] part 12: Add Blink's HRTFPanner to the build Requesting build peer review of the LOCAL_INCLUDES in Makefile.in. (In reply to :Ehsan Akhgari (needinfo? me!) from comment #37) > > +// #include <algorithm> > > ? Removed in my patch queue.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #38) > (In reply to Karl Tomlinson (:karlt, back 22 July) from comment #27) > > I haven't got Chromium to produce sound yet, so > > haven't compared there. > > > > http://people.mozilla.org/~karlt/panner-horizontal.html > > http://people.mozilla.org/~karlt/panner-vertical.html > > I tried the demos in Chrome on Mac and it seemed to produce sound and the > panning works fine (the only distance model which seems to produce sound is > linear though, not sure why.) Ah, linear does work in Chromium when testing via http:. It was file: access that wasn't working for me. Restrictions on resources in the same directory, perhaps. I compared Chromium and Firefox with a couple of headphones and I'm happy that effects are as similar as I can detect. They each even share similar blips when moving the source point. > > I haven't worked out the best approach for dealing with HRTFDatabaseLoader. > > It is designed to have its lifetime controlled from the mail thread, so it > > would like to live on the PannerNode. > > Why does its lifetime have to be controlled by the main thread? It's > refcounted, right? HRTFDatabaseLoader factories and destructors access a singleton hash table so that HRTFDatabaseLoader::createAndLoadAsynchronouslyIfNecessary() may return an HRTFDatabaseLoader concurrently in use by another HRTFPanner. Adding a mutex around the hash table access and making ref-counting atomic may be another option, so long as we don't race in allocating the memory for the singleton mutex. Table access is usually fast but can be slower when the size of table changes. However, I've tried to keep things simple by using the equivalent of an nsProxyRelease (which supports only nsISupports) to unref the HRTFDatabaseLoader on the main thread. Delaying the destruction could be a good thing as it might save another PannerNode from creating a new database, which may take up to a second or two. Maybe a timer even would make sense, but delays before GC seem to usually make that unnecessary. I've made the HRTFPanner constructor take a TemporaryRef parameter so that ownership transfers, hopefully reducing the risk of someone ref-counting the (shared) HRTFDatabaseLoader on a non-main thread, should the HRTFPanner one day be constructed on a different thread. > > I haven't got a good understanding of the PlayingRefChangeHandler. It seems > > that there are some situations where the AudioNode can be assumed to be still > > alive, but I don't know what those situations are. > > Those situations will only matter if the node can produce sound after its > input has been cut off. That is not the case for PannerNode, right? Impulse responses for convolution are 256 samples at 44.1 kHz. That means there should be up to 5.8 ms extra output after input stops. It could be a noticeable click if the output shuts off quickly near the end of a block. I haven't addressed this here. https://tbpl.mozilla.org/?tree=Try&rev=8e48b8f9b668 Next step is to try to write a test. Response to an impulse at the direction of one of the measured impulse responses should be close to matching, but won't match exactly due to processing and extracting the average group delay. Perhaps they will match better in the frequency domain and AnalyzerNode can be used.

(In reply to Karl Tomlinson (:karlt) from comment #45) > https://tbpl.mozilla.org/?tree=Try&rev=8e48b8f9b668 Looks like I have a 32-bit opt Mutex lock crash to investigate. valgrind on 64-bit debug turned up only this, which doesn't immediately look related: ==17397== Thread 24: ==17397== Conditional jump or move depends on uninitialised value(s) ==17397== at 0x7C55C08: long const& std::max<long>(long const&, long const&) (stl_algobase.h:215) ==17397== by 0x91BD100: mozilla::StreamBuffer::Track::ForgetUpTo(long) (StreamBuffer.h:168) ==17397== by 0x91BD479: mozilla::StreamBuffer::ForgetUpTo(long) (StreamBuffer.cpp:82) ==17397== by 0x9155F09: mozilla::MediaStream::AdvanceTimeVaryingValuesToCurrentTime(long, long) (MediaStreamGraph.h:464) ==17397== by 0x91A7951: mozilla::MediaStreamGraphImpl::UpdateCurrentTime() (MediaStreamGraph.cpp:362) ==17397== by 0x91A9EC6: mozilla::MediaStreamGraphImpl::RunThread() (MediaStreamGraph.cpp:1008) ==17397== by 0x91AABC7: mozilla::(anonymous namespace)::MediaStreamGraphThreadRunnable::Run() (MediaStreamGraph.cpp:1228) ==17397== by 0x8529607: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:622) ==17397== by 0x84A5DBA: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:238) ==17397== by 0x852839D: nsThread::ThreadFunc(void*) (nsThread.cpp:250) ==17397== by 0x408E4CC: _pt_root (ptthread.c:204) ==17397== by 0x4E3AE13: start_thread (in /lib64/libpthread-2.15.so) ==17397== Who would run a 32-bit browser anyway? ;)

Comment on attachment 781499 [details] [diff] [review] part 3.0: don't try to compute a phase for group delay from the DC and nyquist components of the DFT Review of attachment 781499 [details] [diff] [review]: ----------------------------------------------------------------- Please add a comment here with a distilled version of comment 40. :-)

(In reply to Karl Tomlinson (:karlt) from comment #45) > Created attachment 781531 [details] [diff] [review] > part 13: Implement HRTF panner processing based on Blink implementation > > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #38) > > (In reply to Karl Tomlinson (:karlt, back 22 July) from comment #27) > > > I haven't got Chromium to produce sound yet, so > > > haven't compared there. > > > > > > http://people.mozilla.org/~karlt/panner-horizontal.html > > > http://people.mozilla.org/~karlt/panner-vertical.html > > > > I tried the demos in Chrome on Mac and it seemed to produce sound and the > > panning works fine (the only distance model which seems to produce sound is > > linear though, not sure why.) > > Ah, linear does work in Chromium when testing via http:. > It was file: access that wasn't working for me. Restrictions on resources in > the same directory, perhaps. Any idea why the other two modes don't work? > I compared Chromium and Firefox with a couple of headphones and I'm happy > that > effects are as similar as I can detect. They each even share similar blips > when moving the source point. Great! :-) > > > I haven't worked out the best approach for dealing with HRTFDatabaseLoader. > > > It is designed to have its lifetime controlled from the mail thread, so it > > > would like to live on the PannerNode. > > > > Why does its lifetime have to be controlled by the main thread? It's > > refcounted, right? > > HRTFDatabaseLoader factories and destructors access a singleton hash table so > that HRTFDatabaseLoader::createAndLoadAsynchronouslyIfNecessary() may return > an HRTFDatabaseLoader concurrently in use by another HRTFPanner. Adding a > mutex around the hash table access and making ref-counting atomic may be > another option, so long as we don't race in allocating the memory for the > singleton mutex. Table access is usually fast but can be slower when the > size > of table changes. > > However, I've tried to keep things simple by using the equivalent of an > nsProxyRelease (which supports only nsISupports) to unref the > HRTFDatabaseLoader on the main thread. Delaying the destruction could be a > good thing as it might save another PannerNode from creating a new database, > which may take up to a second or two. Maybe a timer even would make sense, > but delays before GC seem to usually make that unnecessary. > > I've made the HRTFPanner constructor take a TemporaryRef parameter so that > ownership transfers, hopefully reducing the risk of someone ref-counting the > (shared) HRTFDatabaseLoader on a non-main thread, should the HRTFPanner one > day be constructed on a different thread. Hmm, how about making AudioContext itself own HRTFDatabaseLoader, and lazily create it there? We can then use a timeout to free the DB loader let's say 5 seconds after the last PannerNode goes away to conserve memory (you can piggy-back on the UnregisterPannerNode infrastructure that is already there.) I really don't like the complexity of the lifetime management for this object in your current patch. > > > I haven't got a good understanding of the PlayingRefChangeHandler. It seems > > > that there are some situations where the AudioNode can be assumed to be still > > > alive, but I don't know what those situations are. > > > > Those situations will only matter if the node can produce sound after its > > input has been cut off. That is not the case for PannerNode, right? > > Impulse responses for convolution are 256 samples at 44.1 kHz. That means > there should be up to 5.8 ms extra output after input stops. It could be a > noticeable click if the output shuts off quickly near the end of a block. I > haven't addressed this here. Hmm, OK I see. In that case, we need a self reference, similar to the one that ConvolverNode has. > https://tbpl.mozilla.org/?tree=Try&rev=8e48b8f9b668 > > Next step is to try to write a test. > > Response to an impulse at the direction of one of the measured impulse > responses should be close to matching, but won't match exactly due to > processing and extracting the average group delay. Perhaps they will match > better in the frequency domain and AnalyzerNode can be used. Have you seen if Blink has tests covering this? If yes, we should be able to import those tests.

Comment on attachment 781531 [details] [diff] [review] part 13: Implement HRTF panner processing based on Blink implementation r- based on comment 48.

(In reply to Karl Tomlinson (:karlt) from comment #46) > valgrind on 64-bit debug turned up only this, which doesn't immediately look > related: > > ==17397== Thread 24: > ==17397== Conditional jump or move depends on uninitialised value(s) > ==17397== at 0x7C55C08: long const& std::max<long>(long const&, long > const&) (stl_algobase.h:215) > ==17397== by 0x91BD100: mozilla::StreamBuffer::Track::ForgetUpTo(long) > (StreamBuffer.h:168) > ==17397== by 0x91BD479: mozilla::StreamBuffer::ForgetUpTo(long) > (StreamBuffer.cpp:82) > ==17397== by 0x9155F09: > mozilla::MediaStream::AdvanceTimeVaryingValuesToCurrentTime(long, long) > (MediaStreamGraph.h:464) > ==17397== by 0x91A7951: > mozilla::MediaStreamGraphImpl::UpdateCurrentTime() (MediaStreamGraph.cpp:362) > ==17397== by 0x91A9EC6: mozilla::MediaStreamGraphImpl::RunThread() > (MediaStreamGraph.cpp:1008) > ==17397== by 0x91AABC7: mozilla::(anonymous > namespace)::MediaStreamGraphThreadRunnable::Run() (MediaStreamGraph.cpp:1228) > ==17397== by 0x8529607: nsThread::ProcessNextEvent(bool, bool*) > (nsThread.cpp:622) > ==17397== by 0x84A5DBA: NS_ProcessNextEvent(nsIThread*, bool) > (nsThreadUtils.cpp:238) > ==17397== by 0x852839D: nsThread::ThreadFunc(void*) (nsThread.cpp:250) > ==17397== by 0x408E4CC: _pt_root (ptthread.c:204) > ==17397== by 0x4E3AE13: start_thread (in /lib64/libpthread-2.15.so) > ==17397== This is bad! :-)

Comment on attachment 773102 [details] [diff] [review] part 12: Add Blink's HRTFPanner to the build Review of attachment 773102 [details] [diff] [review]: ----------------------------------------------------------------- Build bits look good.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #48) > Hmm, how about making AudioContext itself own HRTFDatabaseLoader, and lazily > create it there? Can we know that the AudioContext will outlive the PannerNodeEngine? What ensures that?

Every *Engine's lifetime is controlled by the AudioNodeStream corresponding to it, which is controlled by the AudioNode corresponding to it. Every AudioNode holds a strong mContext reference so AudioContext is guaranteed to outlive all of its nodes. We're already using this invariant in a number of places, and it's safe to rely on that here as well.

Comment on attachment 773096 [details] [diff] [review] part 10: Add Blink's HRTFDatabase and HRTFDatabaseLoader to the build (In reply to Karl Tomlinson (:karlt) from comment #46) > (In reply to Karl Tomlinson (:karlt) from comment #45) > > https://tbpl.mozilla.org/?tree=Try&rev=8e48b8f9b668 > > Looks like I have a 32-bit opt Mutex lock crash to investigate. > void HRTFDatabaseLoader::waitForLoaderThreadCompletion() > { >- MutexLocker locker(m_threadLock); >+ MutexAutoLock locker(m_threadLock); > > // waitForThreadCompletion() should not be called twice for the same thread. > if (m_databaseLoaderThread) >- waitForThreadCompletion(m_databaseLoaderThread); >+ m_databaseLoaderThread->Shutdown(); I must have skimmed over this statement in the documentation of nsIThread::Shutdown(): "During this method call, events for the current thread may be processed." Yikes!!!

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #53) > Every *Engine's lifetime is controlled by the AudioNodeStream corresponding > to it, which is controlled by the AudioNode corresponding to it. Every > AudioNode holds a strong mContext reference so AudioContext is guaranteed to > outlive all of its nodes. We're already using this invariant in a number of > places, and it's safe to rely on that here as well. It's possible for a MediaStream to outlive its AudioNode briefly. AudioNode's destructor calls MediaStream::Destroy, which posts a message to the MSG thread to actually destroy the MediaStream (in general; it's possible for the MediaStream to be kept alive in some other way too, since it's a threadsafe-refcounted object).

This is the parts of attachment 773096 [details] [diff] [review] already reviewed minus the thread management which had the bug in comment 54.

This is for folding into part 10.0. This time, instead of using nsIThread and events, the thread is managed directly. We didn't need the event queue anyway.

Comment 55 means that having the AudioContext own the HRTFDatabaseLoader doesn't address the key issue of having the PannerNodeEngine know that the HRTFDatabaseLoader is still alive when it wants to use it. Please have a look at this alternative. The key advantage over the previous approach is that the complexity is all contained within the HRTFDatabaseLoader class. There is no longer any need to fear a reference being changed on the wrong thread. I haven't added a timer as that is not a top priority. Adding a timer requires extra shutdown code to avoid leaks. That is doable, but is difficult to do with initing and cancelling an nsITimer on concurrent threads. Running the timer from the main thread will be simpler, and this patch is a step in the right direction for that.

Another advantage of the approach in comment 58 is that the thread-safe ref-counting provides a reasonably simple way to ensure that the main thread doesn't block waiting for the loader thread to finish. There is a bit of boiler plate here to pass the shutdown through WebAudioUtils to avoid exporting blink headers. That could also be used if each HRTFDatabaseLoader owns an nsITimer to run before delete and to cancel if pulled out of the hash table again.

Differences from v1 are: The HRTFPanner doesn't need to proxy its HRTFDatabaseLoader release, or store a raw strong reference. I'm still using a TemporaryRef for the HRTFPanner constructor to avoid unnecessary reference count changes. mPanningModelFunction is run even when the input is null so that output is not directly cut off early here. There is still the chance of output being cut off early if the AudioNode is destroyed, which I'd like to address in bug 898291.

Comment on attachment 787433 [details] [diff] [review] part 10.2: make HRTFDatabaseLoader ref-counting thread-safe >+ MOZ_COUNT_CTOR(HRTFDatabaseLoader); >+ MOZ_COUNT_DTOR(HRTFDatabaseLoader); I'll remove these, now we have the NS_LOG_ADDREF/RELEASE logging.

Comment on attachment 787433 [details] [diff] [review] part 10.2: make HRTFDatabaseLoader ref-counting thread-safe Review of attachment 787433 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/blink/HRTFDatabaseLoader.cpp @@ +91,5 @@ > } > > +class HRTFDatabaseLoader::ProxyReleaseEvent MOZ_FINAL : public nsRunnable { > +public: > + ProxyReleaseEvent(HRTFDatabaseLoader* loader) : mLoader(loader) {} Nit: explicit. @@ +114,5 @@ > + // Should be in XPCOM shutdown. > + MOZ_ASSERT(NS_IsMainThread(), > + "Main thread is not available for dispatch."); > + MainThreadRelease(); > + } Hmm, why is the else branch necessary? Can't we ensure that this reference is properly cleaned up before we enter the XPCOM shutdown?

Comment on attachment 787437 [details] [diff] [review] part 10.3: don't join HRTFDatabaseLoader thread until it has finished Review of attachment 787437 [details] [diff] [review]: ----------------------------------------------------------------- OK, let's do this.

Comment on attachment 787433 [details] [diff] [review] part 10.2: make HRTFDatabaseLoader ref-counting thread-safe Review of attachment 787433 [details] [diff] [review]: ----------------------------------------------------------------- I guess patch 10.3 answers my question.

(In reply to Karl Tomlinson (:karlt) from comment #61) > Comment on attachment 787433 [details] [diff] [review] > part 10.2: make HRTFDatabaseLoader ref-counting thread-safe > > >+ MOZ_COUNT_CTOR(HRTFDatabaseLoader); > > >+ MOZ_COUNT_DTOR(HRTFDatabaseLoader); > > I'll remove these, now we have the NS_LOG_ADDREF/RELEASE logging. I'd prefer to keep this, as the latter macros only kick in in tracerefcnt builds.

Comment on attachment 773070 [details] [diff] [review] Part 1: Import the HRTF panner implementation from Blink We need these patches for Web Audio in Firefox 25.

Thanks for looking through this (In reply to :Ehsan Akhgari (needinfo? me!) from comment #63) > > +class HRTFDatabaseLoader::ProxyReleaseEvent MOZ_FINAL : public nsRunnable { > > +public: > > + ProxyReleaseEvent(HRTFDatabaseLoader* loader) : mLoader(loader) {} > > Nit: explicit. Good call. (In reply to :Ehsan Akhgari (needinfo? me!) from comment #66) > (In reply to Karl Tomlinson (:karlt) from comment #61) > > Comment on attachment 787433 [details] [diff] [review] > > part 10.2: make HRTFDatabaseLoader ref-counting thread-safe > > > > >+ MOZ_COUNT_CTOR(HRTFDatabaseLoader); > > > > >+ MOZ_COUNT_DTOR(HRTFDatabaseLoader); > > > > I'll remove these, now we have the NS_LOG_ADDREF/RELEASE logging. > > I'd prefer to keep this, as the latter macros only kick in in tracerefcnt > builds. Both sets of macros are dependent on the same condition: NS_BUILD_REFCNT_LOGGING. http://hg.mozilla.org/mozilla-central/annotate/264e54846d4a/xpcom/glue/nsTraceRefcnt.h#l10 I don't know why we have MOZ_ and NS_. Perhaps one was added before the other. The runtime decisions look the same from a brief look at implementations, both using the same global variables. http://hg.mozilla.org/mozilla-central/annotate/264e54846d4a/xpcom/base/nsTraceRefcntImpl.cpp#l1080 http://hg.mozilla.org/mozilla-central/annotate/264e54846d4a/xpcom/base/nsTraceRefcntImpl.cpp#l959 BloatEntry::AddRef() calls Ctor(), so I think having both macros would confuse things by counting each object twice. http://hg.mozilla.org/mozilla-central/annotate/264e54846d4a/xpcom/base/nsTraceRefcntImpl.cpp#l246

(In reply to comment #68) > Thanks for looking through this > > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #63) > > > +class HRTFDatabaseLoader::ProxyReleaseEvent MOZ_FINAL : public nsRunnable { > > > +public: > > > + ProxyReleaseEvent(HRTFDatabaseLoader* loader) : mLoader(loader) {} > > > > Nit: explicit. > > Good call. > > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #66) > > (In reply to Karl Tomlinson (:karlt) from comment #61) > > > Comment on attachment 787433 [details] [diff] [review] > > > part 10.2: make HRTFDatabaseLoader ref-counting thread-safe > > > > > > >+ MOZ_COUNT_CTOR(HRTFDatabaseLoader); > > > > > > >+ MOZ_COUNT_DTOR(HRTFDatabaseLoader); > > > > > > I'll remove these, now we have the NS_LOG_ADDREF/RELEASE logging. > > > > I'd prefer to keep this, as the latter macros only kick in in tracerefcnt > > builds. > > Both sets of macros are dependent on the same condition: > NS_BUILD_REFCNT_LOGGING. > http://hg.mozilla.org/mozilla-central/annotate/264e54846d4a/xpcom/glue/nsTraceRefcnt.h#l10 > > I don't know why we have MOZ_ and NS_. Perhaps one was added before the other. > > The runtime decisions look the same from a brief look at implementations, both > using the same global variables. > http://hg.mozilla.org/mozilla-central/annotate/264e54846d4a/xpcom/base/nsTraceRefcntImpl.cpp#l1080 > http://hg.mozilla.org/mozilla-central/annotate/264e54846d4a/xpcom/base/nsTraceRefcntImpl.cpp#l959 > > BloatEntry::AddRef() calls Ctor(), so I think having both macros would confuse > things by counting each object twice. > http://hg.mozilla.org/mozilla-central/annotate/264e54846d4a/xpcom/base/nsTraceRefcntImpl.cpp#l246 OK, thanks for diffing into this! Please drop the ctor macros as you were then! :-)

(In reply to Karl Tomlinson (:karlt) from comment #62) > https://tbpl.mozilla.org/?tree=Try&rev=c7cd682b91ed The linux-32 mochitest-3 webrtc oom crash is filed as bug 903225 Also happens on m-c: https://tbpl.mozilla.org/?tree=Try&rev=6c1bdd4e2417

Comment on attachment 773070 [details] [diff] [review] Part 1: Import the HRTF panner implementation from Blink We don't need aurora approval for Web Audio any more...

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e796e343ef8 https://hg.mozilla.org/integration/mozilla-inbound/rev/5ddd533fc54c https://hg.mozilla.org/integration/mozilla-inbound/rev/07e1d8379a30 https://hg.mozilla.org/integration/mozilla-inbound/rev/729de18aacef https://hg.mozilla.org/integration/mozilla-inbound/rev/5fd2ef2d9947 https://hg.mozilla.org/integration/mozilla-inbound/rev/214033794f62 https://hg.mozilla.org/integration/mozilla-inbound/rev/c43863ae9218 https://hg.mozilla.org/integration/mozilla-inbound/rev/c56ecdd125ec https://hg.mozilla.org/integration/mozilla-inbound/rev/9b01d3f61a1b https://hg.mozilla.org/integration/mozilla-inbound/rev/5a05c2215091 https://hg.mozilla.org/integration/mozilla-inbound/rev/395fc5b9593c https://hg.mozilla.org/integration/mozilla-inbound/rev/316df2d96b24 https://hg.mozilla.org/integration/mozilla-inbound/rev/c19e2f326d15 https://hg.mozilla.org/integration/mozilla-inbound/rev/7ab5c4babe56 https://hg.mozilla.org/integration/mozilla-inbound/rev/d92240f69c48 https://hg.mozilla.org/integration/mozilla-inbound/rev/746b2ba6cf30 https://hg.mozilla.org/integration/mozilla-inbound/rev/62ad090a94a4 (In reply to :Ehsan Akhgari (needinfo? me!) from comment #48) > > > I tried the demos in Chrome on Mac and it seemed to produce sound and the > > > panning works fine (the only distance model which seems to produce sound is > > > linear though, not sure why.) > Any idea why the other two modes don't work? The sound with non-linear modes is very quiet, but it is there. The behavior is the same in both browsers. refDistance should probably be set appropriately in the testcase. > Have you seen if Blink has tests covering this? If yes, we should be able > to import those tests. The Blink tests in LayoutTests/webaudio/ that check panner output use equalpower for predictable output. I'll see if I can make a fuzzy test.

Out of interest, is there a plan for keeping the imported code up to date?

https://hg.mozilla.org/mozilla-central/rev/07e1d8379a30 https://hg.mozilla.org/mozilla-central/rev/5fd2ef2d9947 https://hg.mozilla.org/mozilla-central/rev/c19e2f326d15 https://hg.mozilla.org/mozilla-central/rev/7ab5c4babe56

(In reply to comment #73) > Out of interest, is there a plan for keeping the imported code up to date? We'll try to do that, but the upstream code isn't very churny.

https://hg.mozilla.org/releases/mozilla-aurora/rev/22e882d5e741 https://hg.mozilla.org/releases/mozilla-aurora/rev/5df778cfd65b https://hg.mozilla.org/releases/mozilla-aurora/rev/7d6134d11ba0 https://hg.mozilla.org/releases/mozilla-aurora/rev/0309702ab12d https://hg.mozilla.org/releases/mozilla-aurora/rev/f3e8d45691de https://hg.mozilla.org/releases/mozilla-aurora/rev/8ec81b82a574 https://hg.mozilla.org/releases/mozilla-aurora/rev/e91ef8824279 https://hg.mozilla.org/releases/mozilla-aurora/rev/d1dab35e44a7 https://hg.mozilla.org/releases/mozilla-aurora/rev/bf74f1f67386 https://hg.mozilla.org/releases/mozilla-aurora/rev/4839aa086dfc https://hg.mozilla.org/releases/mozilla-aurora/rev/581e1a7c6a7f https://hg.mozilla.org/releases/mozilla-aurora/rev/a867ee21d619 https://hg.mozilla.org/releases/mozilla-aurora/rev/b085cd4bd4ca https://hg.mozilla.org/releases/mozilla-aurora/rev/c02414b48056 https://hg.mozilla.org/releases/mozilla-aurora/rev/beab9c07c4e4 https://hg.mozilla.org/releases/mozilla-aurora/rev/8307386d6911 https://hg.mozilla.org/releases/mozilla-aurora/rev/34daf22f7fb2

While testing for the pre-beta sign-off of this feature, I tried to verify this bug, so here are my questions and results: 1) the link from comment 3 sounds the same with the latest Aurora (build ID: 20130902004002)and Chrome, on an Ubuntu 12.10 32bit machine and a Mac 10.9 in 32bit mode one 2) the demos from the link from comment 17 : http://recherche.ircam.fr/equipes/salles/listen/sounds.html , can't be open in Chrome (see the attached screenshot) 3) with both links from comment 27, I don't hear the same sound with the latest Aurora and Chrome, on Mac 10.9 in 32bit mode: with Aurora, after pressing the "Stop" button, I hear a long stinging beep, but with Chrome I can still hear the original beep Does anybody have any thoughts/suggestions? Thanks!

(In reply to Manuela Muntean [:Manuela] [QA] from comment #77) > Created attachment 798510 [details] > ScreenShot.png > > While testing for the pre-beta sign-off of this feature, I tried to verify > this bug, so here are my questions and results: > > 1) the link from comment 3 sounds the same with the latest Aurora (build ID: > 20130902004002)and Chrome, on an Ubuntu 12.10 32bit machine and a Mac 10.9 > in 32bit mode one > > 2) the demos from the link from comment 17 : > http://recherche.ircam.fr/equipes/salles/listen/sounds.html , can't be open > in Chrome (see the attached screenshot) Those two links are not presenting audio files that should be listened to, and are not using the feature, so it does not matter much. > 3) with both links from comment 27, I don't hear the same sound with the > latest Aurora and Chrome, on Mac 10.9 in 32bit mode: with Aurora, after > pressing the "Stop" button, I hear a long stinging beep, but with Chrome I > can still hear the original beep The panning seems similar, but I haven't tested every corner case. Chrome should stop when pressing stop, and does not, because they seem to have a bug where they reject a stop() call without arguments (the idl for the method is "void stop(optional double when = 0);"), so they should stop immediately. I could hear the beep as well, maybe it's one of the lifetime issues we have? Karl, Ehsan, does this look like something that would be expected in the current state of things? Maybe bug 898291?

(In reply to Paul Adenot (:padenot) from comment #78) > (In reply to Manuela Muntean [:Manuela] [QA] from comment #77) > > 3) with both links from comment 27, I don't hear the same sound with the > > latest Aurora and Chrome, on Mac 10.9 in 32bit mode: with Aurora, after > > pressing the "Stop" button, I hear a long stinging beep, but with Chrome I > > can still hear the original beep > I could hear the beep as well, maybe it's one of the lifetime issues we > have? Karl, Ehsan, does this look like something that would be expected in > the current state of things? Maybe bug 898291? This is bug 890528, which should be fixed in tomorrow's nightly and aurora.

(In reply to Karl Tomlinson (:karlt) from comment #79) > (In reply to Paul Adenot (:padenot) from comment #78) > > (In reply to Manuela Muntean [:Manuela] [QA] from comment #77) > > > 3) with both links from comment 27, I don't hear the same sound with the > > > latest Aurora and Chrome, on Mac 10.9 in 32bit mode: with Aurora, after > > > pressing the "Stop" button, I hear a long stinging beep, but with Chrome I > > > can still hear the original beep > > > I could hear the beep as well, maybe it's one of the lifetime issues we > > have? Karl, Ehsan, does this look like something that would be expected in > > the current state of things? Maybe bug 898291? > > This is bug 890528, which should be fixed in tomorrow's nightly and aurora. Manuela, can you please retest this when that fix gets into Nightly? Thanks!

Verified as fixed since now, with both links from comment 27, I can hear the same sound with the latest Aurora (build ID: 20130904004004), latest Nightly (build ID: 20130904030204) and Chrome, on Win 8 32bit.

Added a sanity mochitest, not as thorough as the idea in comment 45, but simpler and enough to catch a bug i had while working on bug 857610. https://hg.mozilla.org/integration/mozilla-inbound/rev/f814a1a08de6

(In reply to Karl Tomlinson (:karlt) from comment #82) > Added a sanity mochitest, not as thorough as the idea in comment 45, but > simpler and enough to catch a bug i had while working on bug 857610. > https://hg.mozilla.org/integration/mozilla-inbound/rev/f814a1a08de6 This was backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/188d170df345 for failing pretty much once per push since it landed. Failures looked like https://tbpl.mozilla.org/php/getParsedLog.php?id=35622533&tree=Mozilla-Inbound though the values were all slightly different.

Also a failure on Android, not just Linux: https://tbpl.mozilla.org/php/getParsedLog.php?id=35632800&tree=Mozilla-Inbound

The test should not have been analysing output from panner nodes that began processing during HRIR database initialization. The tail time of these nodes and the delay nodes in the test could mean that output was affected even after initialization was complete. https://hg.mozilla.org/integration/mozilla-inbound/rev/757aca9bd569