Support multiple HRTFDatabases for different sample-rates

Support multiple HRTFDatabases for different sample-rates

This removes the limitation which required OfflineAudioContexts to be created at native sample-rate

BUG=none
TEST=layout tests pass
R=kbr@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150347

[Ken Russell (switch to Gerrit), 2013-05-11 01:35:16]

https://codereview.chromium.org/14636011/diff/1/Source/core/platform/audio/HR...
File Source/core/platform/audio/HRTFDatabaseLoader.cpp (right):

https://codereview.chromium.org/14636011/diff/1/Source/core/platform/audio/HR...
Source/core/platform/audio/HRTFDatabaseLoader.cpp:54: if (loader.get()) {
RefPtr has a conversion operator to bool, so you should be able to write this as
"if (loader)".

https://codereview.chromium.org/14636011/diff/1/Source/modules/webaudio/Panne...
File Source/modules/webaudio/PannerNode.cpp (right):

https://codereview.chromium.org/14636011/diff/1/Source/modules/webaudio/Panne...
Source/modules/webaudio/PannerNode.cpp:148: HRTFDatabase* database =
context()->hrtfDatabaseLoader()->database();
There's a race condition in the fetching of the HRTFDatabase from the
HRTFDatabaseLoader. Since the HRTFDatabaseLoader creates the HRTFDatabase from
its database loader thread, there's a possibility that the database() method
call here will return NULL. To guard against this, it seems to me that the
Panner should probably take an HRTFDatabaseLoader as argument rather than an
HRTFDatabase. What do you think?

[Chris Rogers, 2013-05-13 20:05:22]

Thanks Ken, PTAL, I've also addressed the memory leak issue from the failing
test "nmi-webaudio-leak-test.html"

https://codereview.chromium.org/14636011/diff/1/Source/core/platform/audio/HR...
File Source/core/platform/audio/HRTFDatabaseLoader.cpp (right):

https://codereview.chromium.org/14636011/diff/1/Source/core/platform/audio/HR...
Source/core/platform/audio/HRTFDatabaseLoader.cpp:54: if (loader.get()) {
On 2013/05/11 01:35:16, kbr wrote:
> RefPtr has a conversion operator to bool, so you should be able to write this
as
> "if (loader)".

Done.

https://codereview.chromium.org/14636011/diff/1/Source/modules/webaudio/Panne...
File Source/modules/webaudio/PannerNode.cpp (right):

https://codereview.chromium.org/14636011/diff/1/Source/modules/webaudio/Panne...
Source/modules/webaudio/PannerNode.cpp:148: HRTFDatabase* database =
context()->hrtfDatabaseLoader()->database();
On 2013/05/11 01:35:16, kbr wrote:
> There's a race condition in the fetching of the HRTFDatabase from the
> HRTFDatabaseLoader. Since the HRTFDatabaseLoader creates the HRTFDatabase from
> its database loader thread, there's a possibility that the database() method
> call here will return NULL. To guard against this, it seems to me that the
> Panner should probably take an HRTFDatabaseLoader as argument rather than an
> HRTFDatabase. What do you think?

Good point, I'll switch this back to the previous behavior where the HRTFPanner
deals with the loader itself...

Fixed.

[Ken Russell (switch to Gerrit), 2013-05-13 21:44:18]

LGTM with one comment.

https://codereview.chromium.org/14636011/diff/7012/Source/core/platform/audio...
File Source/core/platform/audio/HRTFDatabaseLoader.cpp (right):

https://codereview.chromium.org/14636011/diff/7012/Source/core/platform/audio...
Source/core/platform/audio/HRTFDatabaseLoader.cpp:138: if (s_loaderMap) {
This method should also ASSERT(isMainThread()).

https://codereview.chromium.org/14636011/diff/7012/Source/core/platform/audio...
File Source/core/platform/audio/HRTFDatabaseLoader.h (right):

https://codereview.chromium.org/14636011/diff/7012/Source/core/platform/audio...
Source/core/platform/audio/HRTFDatabaseLoader.h:75: static
HRTFDatabaseLoader::LoaderMap* loaderMap() { return s_loaderMap; }
It seems a bad idea to expose the LoaderMap directly. The methods in this class
which access it assert that they're called on the main thread, but by exposing
the LoaderMap to the outside world, constraints could be violated.

[Chris Rogers, 2013-05-14 20:22:15]

Committed patchset #4 manually as r150347.

[Chris Rogers, 2013-05-14 20:23:10]

https://codereview.chromium.org/14636011/diff/7012/Source/core/platform/audio...
File Source/core/platform/audio/HRTFDatabaseLoader.cpp (right):

https://codereview.chromium.org/14636011/diff/7012/Source/core/platform/audio...
Source/core/platform/audio/HRTFDatabaseLoader.cpp:138: if (s_loaderMap) {
On 2013/05/13 21:44:18, kbr wrote:
> This method should also ASSERT(isMainThread()).

Done.

https://codereview.chromium.org/14636011/diff/7012/Source/core/platform/audio...
File Source/core/platform/audio/HRTFDatabaseLoader.h (right):

https://codereview.chromium.org/14636011/diff/7012/Source/core/platform/audio...
Source/core/platform/audio/HRTFDatabaseLoader.h:75: static
HRTFDatabaseLoader::LoaderMap* loaderMap() { return s_loaderMap; }
On 2013/05/13 21:44:18, kbr wrote:
> It seems a bad idea to expose the LoaderMap directly. The methods in this
class
> which access it assert that they're called on the main thread, but by exposing
> the LoaderMap to the outside world, constraints could be violated.

Agreed that it's not ideal, but PlatformMemoryInstrumentation.cpp needs it...