[Webkit-unassigned] [Bug 69703] HRTF Database consolidation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=69703





--- Comment #10 from Chris Rogers <crogers at google.com>  2011-10-28 19:06:19 PST ---
(From update of attachment 112857)
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)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.