Bug 69703 – HRTF Database consolidation

• Component: Web Audio
• Status: RESOLVED FIXED

[Philippe Normand]

The current Composite HRTF database consists of 240 very small wav files. Depending on the underlying implementation used to load them the whole process can take some relevant amount of time!

One improvement would be to concatenate those small files into one and at runtime access it by segmented chunks.

Attaching a proposed patch but I have tested it only in GTK so far, hence the PLATFORM guards.

[Philippe Normand]

Created attachment 110272 [details]
Consolidate Composite azimuth/elevations in a single wav file

[Philippe Normand]

Not marking for review yet. Chris, can you have a look please?

[Philippe Normand]

In GTK I added a new build step for the concatenation, using sox. The generated file is stored in a DerivedResources/audio/ directory, installed during "make install" and for uninstalled case the AudioBus looks in that directory.

[Philippe Normand]

Hi Chris, just a little reminder about this patch ;) Any feedback welcome!

[Chris Rogers]

Comment on attachment 110272 [details]
Consolidate Composite azimuth/elevations in a single wav file

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

> Source/WebCore/platform/audio/HRTFElevation.cpp:55
> +static AudioBusMap audioBusMap;

I think it's against WebKit style to allow statically initialized C++ objects.  I think we'll need to make audioBusMap be a pointer to AudioBusMap, and lazily initialize it at first use.

> Source/WebCore/platform/audio/HRTFElevation.cpp:108
> +    AudioBus* bus;

I would suggest breaking out the code from lines 108 - 125 into a separate function/method which returns the AudioBus*

> Source/WebCore/platform/audio/HRTFElevation.cpp:119
> +    size_t expectedLength = static_cast<size_t>(240 * 256 * (sampleRate / 44100.0));

Please don't use magic constant 240. instead define a constant value at the top of the file, such as:

const size_t totalNumberOfResponses = 240;

We should define similar constants for 256, and 44100.0   --  also need to get rid of .0 so   44100.0 -> 44100

> Source/WebCore/platform/audio/HRTFElevation.cpp:127
> +    int elevationsPerAzimuth = 10;

Can we use the already defined constant HRTFDatabase::NumberOfRawElevations instead of defining a new one?

> Source/WebCore/platform/audio/HRTFElevation.cpp:132
> +    int index = ((azimuth / AzimuthSpacing) * elevationsPerAzimuth) + elevationIndex;

Can we add a sanity check on the "index" value?

> Source/WebCore/platform/audio/HRTFElevation.cpp:133
> +    int length = 256;

Use same constant for 256 that we define as on line 119.

> Source/WebCore/platform/audio/HRTFElevation.cpp:139
> +    AudioChannel* rightEarImpulseResponse = new AudioChannel(length);

To avoid a memory leak and to follow WebKit style, I believe you'll need to use OwnPtr<AudioChannel> here, something like:

OwnPtr<AudioChannel> leftEarImpulseResponse = adoptPtr(new AudioChannel(length));
OwnPtr<AudioChannel> rightEarImpulseResponse = adoptPtr(new AudioChannel(length));

[Chris Rogers]

Hi Philippe, thanks for the patch.  Could you also add the actual concatenated WAV file to this patch (in platform/audio/resources)

I know you're generating this file during the build process (using the sox utility) for the GTK port, but I think it would be useful to have the file already made and living in the resources directory for ports which don't want to create it at build time and just want to use it.

[Philippe Normand]

(In reply to comment #6)
> Hi Philippe, thanks for the patch.  Could you also add the actual concatenated WAV file to this patch (in platform/audio/resources)
> 
> I know you're generating this file during the build process (using the sox utility) for the GTK port, but I think it would be useful to have the file already made and living in the resources directory for ports which don't want to create it at build time and just want to use it.

Ok! I'm wondering anyway, how often are the single WAV files of each component supposed to change? If they'll remain static or almost never change we can add a README.txt in the resources directory explaining how to produce the concatenated WAV file.

That way we can avoid all together the extra build step... IMHO it all depends on how often the resources will be updated.

[Philippe Normand]

(In reply to comment #5)

> > Source/WebCore/platform/audio/HRTFElevation.cpp:127
> > +    int elevationsPerAzimuth = 10;
> 
> Can we use the already defined constant HRTFDatabase::NumberOfRawElevations instead of defining a new one?
> 

I don't think so, HRTFDatabase::NumberOfRawElevations is private to HRTFDatabase.

Thanks for the review, I'm preparing an updated patch!

[Philippe Normand]

Created attachment 112857 [details]
Consolidate Composite azimuth/elevations in a single wav file

Ah well, I made HRTFDatabase::NumberOfRawElevations public, I hope
it's ok!

[Chris Rogers]

Comment on attachment 112857 [details]
Consolidate Composite azimuth/elevations in a single wav file

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

Hi Philippe - thanks for doing this - it's looking better now.  I think we're going to need another round or two of changes after this one, but we're getting closer.

> Source/WebCore/platform/audio/HRTFElevation.cpp:56
> +typedef HashMap<String, AudioBus*> AudioBusMap;

Can we move the typedef to inside ensureCachedAudioBusForSubject() -- right before the DEFINE_STATIC_LOCAL?

> Source/WebCore/platform/audio/HRTFElevation.cpp:85
> +AudioBus* HRTFElevation::ensureCachedAudioBusForSubject(const String& subjectName, float sampleRate)

thanks for creating a function here :)

Best to make this a simple static function (not class method) and wrap the whole function in #if PLATFORM(GTK)

Can we change the name to something more descriptive like:

getConcatenatedImpulseResponsesForSubject()

> Source/WebCore/platform/audio/HRTFElevation.cpp:99
> +    size_t expectedLength = static_cast<size_t>(TotalNumberOfResponses * ResponseByteSize * (sampleRate / SampleRate));

The sampleRate stuff is going to be slightly trickier than this -- let's handle that part in the next round of revisions :)

> Source/WebCore/platform/audio/HRTFElevation.cpp:142
> +    unsigned index = ((azimuth / AzimuthSpacing) * HRTFDatabase::NumberOfRawElevations) + elevationIndex;

Maybe good to have a comment before this line describing the "layout" of the concatenated impulse responses.
For example, describing they are arranged with all elevation for a given azimuth next to each other...

> Source/WebCore/platform/audio/HRTFElevation.cpp:176
> +    OwnPtr<AudioChannel> rightEarImpulseResponse = adoptPtr(impulseResponse->channelByType(AudioBus::ChannelRight));

We don't want to use OwnPtr<AudioChannel> here (line 175,176) because in this case the individual AudioChannel objects are owned by the AudioBus, which is itself already managed by an OwnPtr

> Source/WebCore/platform/audio/HRTFElevation.cpp:182
> +    kernelR = HRTFKernel::create(rightEarImpulseResponse.get(), fftSize, sampleRate, true);

Don't want to use .get() here since line 175:176 should not be OwnPtr

to make 181:182 work consistently
For line 152:153 I would do something like this:

OwnPtr<AudioChannel> r1 = adoptPtr(new AudioChannel(ResponseByteSize));
OwnPtr<AudioChannel> r2 = adoptPtr(new AudioChannel(ResponseByteSize));
AudioChannel* leftEarImpulseResponse = r1.get();
AudioChannel* rightEarImpulseResponse = r2.get();

> Source/WebCore/platform/audio/HRTFElevation.h:88
> +    static const float SampleRate;

can we be more specific and call "SampleRate" -> "ResponseSampleRate" and make the comment say:
// Sample-rate of the spatialization impulse responses.

> Source/WebCore/platform/audio/HRTFElevation.h:92
> +

I'd put the three constants as simple constants (not class variables) directly in the .cpp file since we don't want them to be part of the public interface for this class

> Source/WebCore/platform/audio/HRTFElevation.h:95
> +    static AudioBus* ensureCachedAudioBusForSubject(const String& subjectName, float sampleRate);

can we make this either a "private" method, or just have it be a simple static function in the .cpp file (not a member function)

[Philippe Normand]

Created attachment 113299 [details]
Consolidate Composite azimuth/elevations in a single wav file

[Philippe Normand]

(In reply to comment #11)
> Created an attachment (id=113299) [details]
> Consolidate Composite azimuth/elevations in a single wav file

Hi Chris, I think I applied all your comments in this patch, it'd be great if you can have a look.

BTW have you tried this patch in Chromium?

[Chris Rogers]

Hi Philippe, sorry I've taken so long on this one.  I've been distracted by some other things (TPAC, etc.).
I'll do my best to make time for it today or tomorrow.

[Chris Rogers]

Comment on attachment 113299 [details]
Consolidate Composite azimuth/elevations in a single wav file

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

Hi Philippe, looks like we're getting really close.  The latest comments fix a problem where we were sample-rate converting the entire concatenated buffer.  Instead, it's more correct to extract the individual impulse-response at the same sample-rate as the audio file, then finally sample-rate convert that to the sample-rate we use for the audio hardware...

> Source/WebCore/platform/audio/HRTFElevation.cpp:54
> +static const size_t TotalNumberOfResponses = 240;

nit: you can remove the "static" keyword since it's implicit

> Source/WebCore/platform/audio/HRTFElevation.cpp:57
> +static const size_t ResponseByteSize = 256;

nit: remove the "static" keyword

Also, it appears that this should be called "ResponseFrameSize" instead of "ResponseByteSize" shouldn't it?

> Source/WebCore/platform/audio/HRTFElevation.cpp:60
> +static const float ResponseSampleRate = 44100;

nit: remove the "static" keyword

> Source/WebCore/platform/audio/HRTFElevation.cpp:92
> +static AudioBus* getConcatenatedImpulseResponsesForSubject(const String& subjectName, float sampleRate)

We don't need to pass in a "sampleRate" argument here since we need to load at "ResponseSampleRate"

> Source/WebCore/platform/audio/HRTFElevation.cpp:100
> +        OwnPtr<AudioBus> impulseResponse = AudioBus::loadPlatformResource(subjectName.utf8().data(), sampleRate);

I think a better name for "impulseResponse" would be "concatenatedImpulseResponses"

Instead of using "sampleRate" here, we need to load the concatenated impulse responses at the exact rate as stored in the audio resource -- which is "ResponseSampleRate"

> Source/WebCore/platform/audio/HRTFElevation.cpp:107
> +    size_t expectedLength = static_cast<size_t>(TotalNumberOfResponses * ResponseByteSize * (sampleRate / ResponseSampleRate));

Since we shouldn't be resampling here, this simplifies to:

TotalNumberOfResponses * ResponseFrameSize

in other words we shouldn't scale by (sampleRate / ResponseSampleRate)

> Source/WebCore/platform/audio/HRTFElevation.cpp:162
> +    AudioChannel* totalRightEarImpulseResponse = bus->channelByType(AudioBus::ChannelRight);

We can remove lines 161:162

> Source/WebCore/platform/audio/HRTFElevation.cpp:164
> +    unsigned stopFrame = startFrame + ResponseByteSize;

Keep lines 163:164

(except change ResponseByteSize -> ResponseFrameSize

> Source/WebCore/platform/audio/HRTFElevation.cpp:171
> +    rightEarImpulseResponse->copyFromRange(totalRightEarImpulseResponse, startFrame, stopFrame);

Replace lines 165:171 with:

OwnPtr<AudioBus> preSampleRateConvertedResponse = createBufferFromRange(bus, startFrame, stopFrame);
OwnPtr<AudioBus> response = createBySampleRateConverting(preSampleRateConvertedResponse.get(), false, sampleRate);
AudioChannel* leftEarImpulseResponse = response->channel(AudioBus::ChannelLeft);
AudioChannel* rightEarImpulseResponse = response->channel(AudioBus::ChannelRight);

[Philippe Normand]

Created attachment 114220 [details]
Consolidate Composite azimuth/elevations in a single wav file

[Chris Rogers]

Philippe, thanks for the latest patch.  Is this still working for you guys after the latest changes dealing with the sample-rate, etc.?  Just wanted to make sure

[Philippe Normand]

(In reply to comment #16)
> Philippe, thanks for the latest patch.  Is this still working for you guys after the latest changes dealing with the sample-rate, etc.?  Just wanted to make sure

Yes!
Would you try to enable this patch in Chromium at some point? :)

[Chris Rogers]

Comment on attachment 114220 [details]
Consolidate Composite azimuth/elevations in a single wav file

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

just comment nits at this point.

I'm going to ask Ken Russell to have a look at this for final review.

Thanks so much for this patch - I think the mac and chromium ports will eventually be interested in using this too!

> Source/WebCore/platform/audio/HRTFElevation.cpp:56
> +// Byte size of a single azimuth audio channel.

nit: the comment should really read:

// Number of frames in an individual impulse response.

> Source/WebCore/platform/audio/HRTFElevation.cpp:59
> +// Sample-rate of the spatialization impulse responses.

Might want to be more specific with the comment, something like:

// Sample-rate of the spatialization impulse responses as stored in the resource file.
// The impulse responses may be resampled to a different sample-rate (depending on the audio hardware) when they are loaded.

> Source/WebCore/platform/audio/HRTFElevation.cpp:151
> +    // The concatenated impulse response is a bus containing the all

Extra "the"  -- should be "is a bus containing all"

> Source/WebCore/platform/audio/HRTFElevation.cpp:160
> +

It would be great to have a comment here like:

// Extract the individual impulse response from the concatenated responses and potentially
// sample-rate convert it to the desired (hardware) sample-rate.

[Philippe Normand]

Created attachment 114344 [details]
Consolidate Composite azimuth/elevations in a single wav file

[Philippe Normand]

(In reply to comment #19)
> Created an attachment (id=114344) [details]
> Consolidate Composite azimuth/elevations in a single wav file

Chris, thanks for the patient reviews on the various iterations of this patch :)

[Chris Rogers]

This looks fine to me.  Thanks for your work, Philippe.

[Kenneth Russell]

Comment on attachment 114344 [details]
Consolidate Composite azimuth/elevations in a single wav file

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

Based on Chris's review, r=me, but one comment. Also, it's concerning that none of the EWS bots handle this patch because of the wav file. I assume you'll handle this somehow. Perhaps you could regenerate the patch via Subversion and check it via EWS before committing it.

> Source/WebCore/platform/audio/HRTFElevation.cpp:90
> +#if PLATFORM(GTK)

Perhaps this and the PLATFORM(GTK) below could be hoisted to the top of the file, and define a file-local symbol like USE_CONCATENATED_IMPULSE_RESPONSES?

[Philippe Normand]

Comment on attachment 114344 [details]
Consolidate Composite azimuth/elevations in a single wav file

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

>> Source/WebCore/platform/audio/HRTFElevation.cpp:90
>> +#if PLATFORM(GTK)
> 
> Perhaps this and the PLATFORM(GTK) below could be hoisted to the top of the file, and define a file-local symbol like USE_CONCATENATED_IMPULSE_RESPONSES?

Sounds like a good idea!

[Philippe Normand]

Created attachment 114485 [details]
Consolidate Composite azimuth/elevations in a single wav file

Same patch with Kenneth comments and an attempt to make EWS happy.

[Philippe Normand]

(In reply to comment #22)
> (From update of attachment 114344 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114344&action=review
> 
> Based on Chris's review, r=me, but one comment. Also, it's concerning that none of the EWS bots handle this patch because of the wav file. I assume you'll handle this somehow. Perhaps you could regenerate the patch via Subversion and check it via EWS before committing it.

Looks like a bug in VCSUtils.pm :( Having a look now.

[Kenneth Russell]

Comment on attachment 114485 [details]
Consolidate Composite azimuth/elevations in a single wav file

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

Looks fine. Let's see if the commit queue will take it.

> Source/WebCore/ChangeLog:3
> +        Consolidate Composite azimuth/elevations in a single wav file

The first line of the ChangeLog entry should generally be the bug synopsis.

[Philippe Normand]

(In reply to comment #26)
> (From update of attachment 114485 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114485&action=review
> 
> Looks fine. Let's see if the commit queue will take it.
> 

I could have landed it myself. Anyway, no big deal about that!

> > Source/WebCore/ChangeLog:3
> > +        Consolidate Composite azimuth/elevations in a single wav file
> 
> The first line of the ChangeLog entry should generally be the bug synopsis.

Right, I generated the ChangeLog entry from a local commit and this was the original commit message which I forgot to move away.

[Kenneth Russell]

(In reply to comment #27)
> (In reply to comment #26)
> > (From update of attachment 114485 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=114485&action=review
> > 
> > Looks fine. Let's see if the commit queue will take it.
> > 
> 
> I could have landed it myself. Anyway, no big deal about that!

Ah, sorry. I've gotten too used to cq+'ing patches for people who forgot cq?.

[Philippe Normand]

Comment on attachment 114485 [details]
Consolidate Composite azimuth/elevations in a single wav file

I'll land it, the cq seems to be stuck.

[Philippe Normand]

Committed r99934: <http://trac.webkit.org/changeset/99934>