865241 - Implement HRTF panning for PannerNode

Status: VERIFIED FIXED

(Core :: Web Audio, defect)

Comment 1

[(no longer active)]

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

Comment 2

[(no longer active)]

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

Comment 3

[Karl Tomlinson (:karlt)]

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

Comment 4

[(no longer active)]

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!

Comment 5

[Karl Tomlinson (:karlt)]

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.

Comment 6

[(no longer active)]

(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.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: Part 1: Import the HRTF panner implementation from Blink

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: part 2: removed unused HRTF code

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: part 3: Add Blink's FFTFrame to the build

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.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: part 4: Use Blink's FFTFrame

Comment 11

[Karl Tomlinson (:karlt)]

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 12

[(no longer active)]

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.

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: Part 1: Import the HRTF panner implementation from Blink

This time no FFTFrame.h.

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: part 2: remove unused HRTF code

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: part 3: Add Blink's frequency interpolation and group delay methods from FFTFrame to FFTBlock

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.

Comment 16

[Karl Tomlinson (:karlt)]

Attachment: part 4: Add Blink's HRTFKernel to the build

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.

Comment 17

[Karl Tomlinson (:karlt)]

Attachment: 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.

[1] http://recherche.ircam.fr/equipes/salles/listen/
[2]
http://trac.webkit.org/browser/branches/old/audio/WebCore/platform/audio/HRTFUtilities.cpp

Comment 18

[Karl Tomlinson (:karlt)]

Attachment: part 6: Add HRTF impulse response data

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.

Comment 19

[Karl Tomlinson (:karlt)]

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

Comment 20

[Karl Tomlinson (:karlt)]

Attachment: Script to convert the first channel of 256 sample 2 channel little-endian 16 bit wave files to a C array

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

Comment 21

[Karl Tomlinson (:karlt)]

Attachment: part 7: Add code to interpret and resample impulse response data

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.

Comment 22

[Karl Tomlinson (:karlt)]

Attachment: part 8: Reduce convolver fft size for low sample rates

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.

Comment 23

[Karl Tomlinson (:karlt)]

Attachment: part 9: Add Blink's HRTFElevation to the build

Comment 24

[Karl Tomlinson (:karlt)]

Attachment: part 10: Add Blink's HRTFDatabase and HRTFDatabaseLoader to the build

Comment 25

[Karl Tomlinson (:karlt)]

Attachment: part 11: Refactor DelayNodeEngine delay processing into a shareable class

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.

Comment 26

[Karl Tomlinson (:karlt)]

Attachment: part 12: Add Blink's HRTFPanner to the build

Comment 27

[Karl Tomlinson (:karlt)]

Attachment: 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

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 28

[(no longer active)]

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.)

Comment 29

[(no longer active)]

(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 30

[(no longer active)]

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.

Comment 31

[(no longer active)]

(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 32

[(no longer active)]

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 33

[(no longer active)]

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 34

[(no longer active)]

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 35

[(no longer active)]

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 36

[(no longer active)]

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 37

[(no longer active)]

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>

?

Comment 38

[(no longer active)]

(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?

Comment 39

[Karl Tomlinson (:karlt)]

(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.

Comment 40

[Karl Tomlinson (:karlt)]

Attachment: part 3.0: don't try to compute a phase for group delay from the DC and nyquist components of the DFT

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.

Comment 41

[Karl Tomlinson (:karlt)]

Attachment: part 3.1: Add Blink's frequency interpolation and group delay methods from FFTFrame to FFTBlock

(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.

Comment 42

[Karl Tomlinson (:karlt)]

Attachment: part 3.2: always leave 20 samples of headroom when extracting group delay, removing the discontinuity at delay = 20

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.

Comment 43

[Karl Tomlinson (:karlt)]

(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 44

[Karl Tomlinson (:karlt)]

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.

Comment 45

[Karl Tomlinson (:karlt)]

Attachment: 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.

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.

Comment 46

[Karl Tomlinson (:karlt)]

(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 47

[(no longer active)]

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.  :-)

Comment 48

[(no longer active)]

(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 49

[(no longer active)]

Comment on attachment 781531 [details] [diff] [review]
part 13: Implement HRTF panner processing based on Blink implementation

r- based on comment 48.

Comment 50

[(no longer active)]

(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 51

[Gregory Szorc [:gps]]

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.

Comment 52

[Karl Tomlinson (:karlt)]

(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?

Comment 53

[(no longer active)]

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 54

[Karl Tomlinson (:karlt)]

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!!!

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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).

Comment 56

[Karl Tomlinson (:karlt)]

Attachment: part 10.0: Add Blink's HRTFDatabase and HRTFDatabaseLoader to the build

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

Comment 57

[Karl Tomlinson (:karlt)]

Attachment: part 10 fold-in 1: add Blink's HRTFDatabaseLoader to the build without an event queue on the thread

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 58

[Karl Tomlinson (:karlt)]

Attachment: part 10.2: make HRTFDatabaseLoader ref-counting thread-safe

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.

Comment 59

[Karl Tomlinson (:karlt)]

Attachment: part 10.3: don't join HRTFDatabaseLoader thread until it has finished

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.

Comment 60

[Karl Tomlinson (:karlt)]

Attachment: part 13 v2: Implement HRTF panner processing based on Blink implementation

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 61

[Karl Tomlinson (:karlt)]

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 62

[Karl Tomlinson (:karlt)]

https://tbpl.mozilla.org/?tree=Try&rev=c7cd682b91ed

Comment 63

[(no longer active)]

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 64

[(no longer active)]

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 65

[(no longer active)]

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.

Comment 66

[(no longer active)]

(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 67

[(no longer active)]

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.

Comment 68

[Karl Tomlinson (:karlt)]

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

Comment 69

[(no longer active)]

(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! :-)

Comment 70

[Karl Tomlinson (:karlt)]

(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 71

[(no longer active)]

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...

Comment 72

[Karl Tomlinson (:karlt)]

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.

Comment 73

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 74

[Carsten Book [:Tomcat]]

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

Comment 75

[(no longer active)]

(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.

Comment 76

[Karl Tomlinson (:karlt)]

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

Comment 77

[Manuela Muntean [Away]]

Attachment: 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)

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!

Comment 78

[Paul Adenot (:padenot)]

(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?

Comment 79

[Karl Tomlinson (:karlt)]

(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.

Comment 80

[(no longer active)]

(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!

Comment 81

[Manuela Muntean [Away]]

 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.

Comment 82

[Karl Tomlinson (:karlt)]

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

Comment 83

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(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.

Comment 84

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 85

[Karl Tomlinson (:karlt)]

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

Comment 86

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/757aca9bd569