Bug 46299 – Add HRTFPanner files

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

Add HRTFPanner files

[Chris Rogers]

Created attachment 68431 [details]
Patch

[Chris Rogers]

Spatialized panning algorithm.  See more details in the audio specification:
http://chromium.googlecode.com/svn/trunk/samples/audio/specification/specification.html#Spatialization-section

[Chris Rogers]

Created attachment 68786 [details]
Patch

[Chris Rogers]

Uploaded new patch to address small change in HRTFDatabaseLoader API

[Chris Rogers]

Created attachment 68967 [details]
Patch

[Chris Rogers]

Created attachment 69002 [details]
Patch

[James Robinson]

Comment on attachment 69002 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69002&action=review

> WebCore/platform/audio/HRTFPanner.cpp:47
> +const double HRTFPanner::MaxDelayTime = 0.012; // 12ms

why 12?

add the unit to the name - looks like it's seconds here so perhaps MaxDelayTimeSeconds

> WebCore/platform/audio/HRTFPanner.cpp:170
> +        // If we have a stereo input, implement a strange kind of panning with left source processed by left HRTF, and right source by right HRTF.

Should it be an error at a higher level to connect a stereo input to this sort of node?  That is an odd sounding pan.

> WebCore/platform/audio/HRTFPanner.h:43
> +    HRTFPanner(double sampleRate)

one-arg constructor needs to be explicit

could the constructor definition be moved to the .cpp? I'm a little concerned about inlining the c'tor for FFTConvolver and DelayDSPKernel without know how big they are

please declare a virtual d'tor and define it in the .cpp

> WebCore/platform/audio/HRTFPanner.h:71
> +    // For de-zippering
> +    bool m_isFirstRender;
> +    int m_azimuthIndex;

What's de-zippering?

Comments should be sentences

[Chris Rogers]

Created attachment 69117 [details]
Patch

[Chris Rogers]

Comment on attachment 69002 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69002&action=review

>> WebCore/platform/audio/HRTFPanner.cpp:47
>> +const double HRTFPanner::MaxDelayTime = 0.012; // 12ms
> 
> why 12?
> 
> add the unit to the name - looks like it's seconds here so perhaps MaxDelayTimeSeconds

FIXED: I actually measured the largest delay time in all the HRTFKernels and picked a good round value higher than this.  I added a comment here.

>> WebCore/platform/audio/HRTFPanner.cpp:170
>> +        // If we have a stereo input, implement a strange kind of panning with left source processed by left HRTF, and right source by right HRTF.
> 
> Should it be an error at a higher level to connect a stereo input to this sort of node?  That is an odd sounding pan.

No, I don't think so.  Although it is more unusual than panning a mono source, it is expected to be able to pan a stereo source to stereo as well, and it results in a reasonable effect.

>> WebCore/platform/audio/HRTFPanner.h:43
>> +    HRTFPanner(double sampleRate)
> 
> one-arg constructor needs to be explicit
> 
> could the constructor definition be moved to the .cpp? I'm a little concerned about inlining the c'tor for FFTConvolver and DelayDSPKernel without know how big they are
> 
> please declare a virtual d'tor and define it in the .cpp

FIXED: I don't think the virtual destructor is necessary since it's empty and will be implicitly generated, but I added it anyway.

>> WebCore/platform/audio/HRTFPanner.h:71
>> +    int m_azimuthIndex;
> 
> What's de-zippering?
> 
> Comments should be sentences

FIXED

[James Robinson]

Comment on attachment 69117 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69117&action=review

Nearly there, few more small things.  Where is DelayDSPKernel defined? I don't see it in my tree.

> WebCore/platform/audio/HRTFPanner.cpp:117
> +    // IRCAM HRTF azimuths which we're loading are reversed from the panner's notion...

Still not technically a sentence.  If this is normalization for the way our data files are formatted, why is it being done here?

> WebCore/platform/audio/HRTFPanner.cpp:121
> +    if (azimuth < 0)
> +        azimuth += 360.0; // make positive

Not a sentence.  Why is this needed?

> WebCore/platform/audio/HRTFPanner.cpp:141
> +    bool isAzimuthGood = azimuth >= 0.0 && azimuth <= 360.0;
> +    ASSERT(isAzimuthGood);
> +    if (!isAzimuthGood) {
> +        outputBus->zero();
> +        return;
> +    }
> +
> +    int numberOfAzimuths = database->numberOfAzimuths();
> +    const double angleBetweenAzimuths = 360.0 / numberOfAzimuths;
> +
> +    // Calculate the azimuth index and the blend (0 -> 1) for interpolation.
> +    double desiredAzimuthIndexF = azimuth / angleBetweenAzimuths;
> +    int desiredAzimuthIndex = static_cast<int>(desiredAzimuthIndexF);
> +    double azimuthBlend = desiredAzimuthIndexF - static_cast<double>(desiredAzimuthIndex);
> +    
> +    // We don't immediately start using this azimuth index, but instead approach this index from the last index we rendered at.
> +    // This minimizes the clicks and graininess for moving sources which occur otherwise.
> +    desiredAzimuthIndex = max(0, desiredAzimuthIndex);
> +    desiredAzimuthIndex = min(numberOfAzimuths - 1, desiredAzimuthIndex);

We don't typically use suffixes like F for floating point values in WebKit.  This segment feels like it should be in a helper function (since this function is a bit on the long side).  Are the min/max needed given that we've already checked that azimuth is in the range [0.0,360.0] ?

> WebCore/platform/audio/HRTFPanner.cpp:174
> +        // Get the HRTFKernels and interpolated delays.
> +    

nit: don't need the extra newline here

> WebCore/platform/audio/HRTFPanner.cpp:198
> +        // Normally, we'll just be dealing with mono sources.
> +        // If we have a stereo input, implement a strange kind of panning with left source processed by left HRTF, and right source by right HRTF.
> +        AudioChannel* channelL = inputBus->channelByType(AudioBus::ChannelLeft);
> +        AudioChannel* channelR = numInputChannels > 1 ? inputBus->channelByType(AudioBus::ChannelRight) : 0;
> +
> +        // Get source and destination pointers.
> +        float* sourceL = channelL->data();
> +        float* sourceR = numInputChannels > 1 ? channelR->data() : sourceL;
> +        float* destinationL = outputBus->channelByType(AudioBus::ChannelLeft)->data();
> +        float* destinationR = outputBus->channelByType(AudioBus::ChannelRight)->data();

Should some of this be outside the loop?

> WebCore/platform/audio/HRTFPanner.h:32
> +#include "DelayProcessor.h"

I don't see this header in my checkout.  Does it define DelayDSPKernel?  Should you include DelayDSPKernel.h directly (once it exists)?

> WebCore/platform/audio/HRTFPanner.h:39
> +class FFTFrame;
> +class HRTFDatabase;

This forward declarations aren't needed.

[Chris Rogers]

Created attachment 69145 [details]
Patch

[Chris Rogers]

Comment on attachment 69117 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69117&action=review

>> WebCore/platform/audio/HRTFPanner.cpp:117
>> +    // IRCAM HRTF azimuths which we're loading are reversed from the panner's notion...
> 
> Still not technically a sentence.  If this is normalization for the way our data files are formatted, why is it being done here?

I can suggest two ways to attack this problem and I'll leave it up to you:
1) improve the comment with a FIXME to re-bake the HRTF data files with the azimuths not reversed.  We have to re-bake the files anyway for the file-size reduction, bass boost, and maybe other things.
2) remove this code from here, but do the reversal in HRTFElevation and add a similar comment as proposed in (1) there.

>> WebCore/platform/audio/HRTFPanner.cpp:121
>> +        azimuth += 360.0; // make positive
> 
> Not a sentence.  Why is this needed?

I improved the comment.

>> WebCore/platform/audio/HRTFPanner.cpp:141
>> +    desiredAzimuthIndex = min(numberOfAzimuths - 1, desiredAzimuthIndex);
> 
> We don't typically use suffixes like F for floating point values in WebKit.  This segment feels like it should be in a helper function (since this function is a bit on the long side).  Are the min/max needed given that we've already checked that azimuth is in the range [0.0,360.0] ?

I created a new method called calculateDesiredAzimuthIndexAndBlend().
I renamed desiredAzimuthIndexF to desiredAzimuthIndexFloat
I'd rather still do the min/max check since this is now moved into a separate method and is not inefficient.

>> WebCore/platform/audio/HRTFPanner.cpp:174
>> +    
> 
> nit: don't need the extra newline here

FIXED

>> WebCore/platform/audio/HRTFPanner.cpp:198
>> +        float* destinationR = outputBus->channelByType(AudioBus::ChannelRight)->data();
> 
> Should some of this be outside the loop?

FIXED

>> WebCore/platform/audio/HRTFPanner.h:32
>> +#include "DelayProcessor.h"
> 
> I don't see this header in my checkout.  Does it define DelayDSPKernel?  Should you include DelayDSPKernel.h directly (once it exists)?

I changed the code to include the header file directly.
It is not yet landed in trunk, but you can view the interface here:
https://svn.webkit.org/repository/webkit/branches/audio/WebCore/webaudio/DelayDSPKernel.h
The DelayDSPKernel implementation is not yet ready for code review, but the interface used by HRTFPanner is simple.

>> WebCore/platform/audio/HRTFPanner.h:39
>> +class HRTFDatabase;
> 
> This forward declarations aren't needed.

FIXED

[Chris Rogers]

Created attachment 69146 [details]
Patch

[James Robinson]

Comment on attachment 69146 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69146&action=review

Sorry I took so long to get back to this.  Code looks good, R- just for the license headers

> WebCore/platform/audio/HRTFPanner.cpp:15
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.

wrong license header

> WebCore/platform/audio/HRTFPanner.h:15
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.

wrong license header

> WebCore/platform/audio/HRTFPanner.h:57
> +    static const double MaxDelayTimeSeconds;

nit: better to put this in .cpp and not mention it at all in the header since it's logically file static

[Chris Rogers]

Created attachment 70321 [details]
Patch

[Chris Rogers]

Thanks James, I updated the LICENSE and made the constant local to the file.

[James Robinson]

Comment on attachment 70321 [details]
Patch

R=me

[WebKit Commit Bot]

Comment on attachment 70321 [details]
Patch

Clearing flags on attachment: 70321

Committed r69440: <http://trac.webkit.org/changeset/69440>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.