Move handle of HRTFDatabaseLoader to AudioListener.

Move handle of HRTFDatabaseLoader to AudioListener.

Fix regression on specific application.

http://googlechrome.github.io/web-audio-samples/samples/audio/box2d-js/box2d-audio.html

Although a HRTFDatabaseLoader should keep the handle until AudioContext is destroyed,
a panner node tries to create the loader again since being removed the handle
by deref() of AudioNode.

The handle has been moved to AudioListener that is related mostly to keep the handle.

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

[KhNo, 2014-07-16 16:14:56]

PTAL. :)

[Raymond Toy, 2014-07-16 16:35:39]

Looks good overall.

Just a few questions.

https://codereview.chromium.org/393363002/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioListener.h (right):

https://codereview.chromium.org/393363002/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioListener.h:109: // HRTF DB loader for panner node.
It should be allocated once on constructor of panner node,
How do you enforce that it is allocated only once when the PannerNode is
constructed?  Based on the constructor for the panner node below, this handle is
set for every new panner that is created.

https://codereview.chromium.org/393363002/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioListener.h:110: // then keep handle for efficiant
loading.
Typo: efficiant -> efficient

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

https://codereview.chromium.org/393363002/diff/1/Source/modules/webaudio/Pann...
Source/modules/webaudio/PannerNode.cpp:70:
listener()->setHRTFDatabaseLoader(loader.release());
Per the comment above, should we check that listener hasn't already set the
handle?

Maybe we should just do something like

listener()->createAndLoadAsynchronouslyIfNecessary(context->sampleRate())

and let listener take care of setting the handle only once? 

But it seems that HRTFDatabaseLoader::createAsynchronouslyIfNecessary returns
the same loader object if it's already been loaded.

[tkent, 2014-07-16 23:32:25]

https://codereview.chromium.org/393363002/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioListener.h (right):

https://codereview.chromium.org/393363002/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioListener.h:83: void
setHRTFDatabaseLoader(PassRefPtrWillBeRawPtr<HRTFDatabaseLoader>);
PassRefPtrWIllBeRawPtr should be PassRefPtr because HRTFDatabaseLoader is not a
garbage-collected class.

https://codereview.chromium.org/393363002/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioListener.h:111:
RefPtrWillBeRawPtr<HRTFDatabaseLoader> m_hrtfDatabaseLoader;
RefPtrWillBeRawPtr should be RefPtr.

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

https://codereview.chromium.org/393363002/diff/1/Source/modules/webaudio/Pann...
Source/modules/webaudio/PannerNode.cpp:69:
RefPtrWillBeRawPtr<HRTFDatabaseLoader> loader =
HRTFDatabaseLoader::createAndLoadAsynchronouslyIfNecessary(context->sampleRate());
RefPtrWillBeRawPtr should be RefPtr.

[KhNo, 2014-07-17 16:23:17]

PTAL. My try bot is broken. Need to notice to sheriffs. I will.

https://codereview.chromium.org/393363002/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioListener.h (right):

https://codereview.chromium.org/393363002/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioListener.h:83: void
setHRTFDatabaseLoader(PassRefPtrWillBeRawPtr<HRTFDatabaseLoader>);
On 2014/07/16 23:32:25, tkent wrote:
> PassRefPtrWIllBeRawPtr should be PassRefPtr because HRTFDatabaseLoader is not
a
> garbage-collected class.

Done.

https://codereview.chromium.org/393363002/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioListener.h:109: // HRTF DB loader for panner node.
It should be allocated once on constructor of panner node,
On 2014/07/16 16:35:38, Raymond Toy wrote:
> How do you enforce that it is allocated only once when the PannerNode is
> constructed?  Based on the constructor for the panner node below, this handle
is
> set for every new panner that is created.

Yes, you right. I missed it. Basically it should be set only once.
I will add condition skip it if it is already existed.
Thanks for review.

https://codereview.chromium.org/393363002/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioListener.h:110: // then keep handle for efficiant
loading.
On 2014/07/16 16:35:38, Raymond Toy wrote:
> Typo: efficiant -> efficient
> 

Done.

https://codereview.chromium.org/393363002/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioListener.h:111:
RefPtrWillBeRawPtr<HRTFDatabaseLoader> m_hrtfDatabaseLoader;
On 2014/07/16 23:32:25, tkent wrote:
> RefPtrWillBeRawPtr should be RefPtr.
Thanks for comment. :)
Done.

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

https://codereview.chromium.org/393363002/diff/1/Source/modules/webaudio/Pann...
Source/modules/webaudio/PannerNode.cpp:69:
RefPtrWillBeRawPtr<HRTFDatabaseLoader> loader =
HRTFDatabaseLoader::createAndLoadAsynchronouslyIfNecessary(context->sampleRate());
On 2014/07/16 23:32:25, tkent wrote:
> RefPtrWillBeRawPtr should be RefPtr.

Done.

https://codereview.chromium.org/393363002/diff/1/Source/modules/webaudio/Pann...
Source/modules/webaudio/PannerNode.cpp:70:
listener()->setHRTFDatabaseLoader(loader.release());
On 2014/07/16 16:35:38, Raymond Toy wrote:
> Per the comment above, should we check that listener hasn't already set the
> handle?

Yes, we must. Thanks.

> 
> Maybe we should just do something like
> 
> listener()->createAndLoadAsynchronouslyIfNecessary(context->sampleRate())
> 
> and let listener take care of setting the handle only once? 
> 

I think your proposal is much better handing on listener.

> But it seems that HRTFDatabaseLoader::createAsynchronouslyIfNecessary returns
> the same loader object if it's already been loaded.

Inside listener, it will be care of it.

[Raymond Toy, 2014-07-17 17:25:46]

Looks good, but I have a few more comments. Mostly, can we remove the
HRTFDatabaseLoader class from PannerNode and just call into appropriate
listener() methods to do what we want? I think this makes it clearer and moves
knowledge the HRTF loading into the listener where it now happens.

WDYT?

https://codereview.chromium.org/393363002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioListener.h (right):

https://codereview.chromium.org/393363002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioListener.h:110: // then keep handle for efficient
loading.
This comment is no longer valid.  Remove this?

https://codereview.chromium.org/393363002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/PannerNode.cpp (right):

https://codereview.chromium.org/393363002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/PannerNode.cpp:127: if (m_panningModel ==
Panner::PanningModelHRTF && !hrtfDatabaseLoader()->isLoaded()) {
Since the HRTF database is loaded in the listener, should we get rid of
hrftDatabaseLoader() and use listener()->isHRTFDatabaseLoaded()? (Or some other
suitable name?)

https://codereview.chromium.org/393363002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/PannerNode.cpp:129:
hrtfDatabaseLoader()->waitForLoaderThreadCompletion();
Same as above.  Maybe use listener()->waitForHRTFLoaded()? (Or some other
suitable name.)

[KhNo, 2014-07-17 17:35:09]

On 2014/07/17 17:25:46, Raymond Toy wrote:
> Looks good, but I have a few more comments. Mostly, can we remove the
> HRTFDatabaseLoader class from PannerNode and just call into appropriate
> listener() methods to do what we want? I think this makes it clearer and moves
> knowledge the HRTF loading into the listener where it now happens.
> 
> WDYT?
> 
>
https://codereview.chromium.org/393363002/diff/80001/Source/modules/webaudio/...
> File Source/modules/webaudio/AudioListener.h (right):
> 
>
https://codereview.chromium.org/393363002/diff/80001/Source/modules/webaudio/...
> Source/modules/webaudio/AudioListener.h:110: // then keep handle for efficient
> loading.
> This comment is no longer valid.  Remove this?
> 
>
https://codereview.chromium.org/393363002/diff/80001/Source/modules/webaudio/...
> File Source/modules/webaudio/PannerNode.cpp (right):
> 
>
https://codereview.chromium.org/393363002/diff/80001/Source/modules/webaudio/...
> Source/modules/webaudio/PannerNode.cpp:127: if (m_panningModel ==
> Panner::PanningModelHRTF && !hrtfDatabaseLoader()->isLoaded()) {
> Since the HRTF database is loaded in the listener, should we get rid of
> hrftDatabaseLoader() and use listener()->isHRTFDatabaseLoaded()? (Or some
other
> suitable name?)
> 
>
https://codereview.chromium.org/393363002/diff/80001/Source/modules/webaudio/...
> Source/modules/webaudio/PannerNode.cpp:129:
> hrtfDatabaseLoader()->waitForLoaderThreadCompletion();
> Same as above.  Maybe use listener()->waitForHRTFLoaded()? (Or some other
> suitable name.)

I think that is better either, I missed it. Coming soon the next patch set. :)

[KhNo, 2014-07-20 07:42:07]

PTAL. :)

[Raymond Toy, 2014-07-21 17:43:22]

LGTM.

A question: if you create two AudioContext's, will there now be two copies of
the HRTF database?  It seems like there will be, and, if so, we probably want to
avoid that.

You don't need to fix that in this CL, however.

[KhNo, 2014-07-21 17:48:41]

On 2014/07/21 17:43:22, Raymond Toy wrote:
> LGTM.
> 
> A question: if you create two AudioContext's, will there now be two copies of
> the HRTF database?  It seems like there will be, and, if so, we probably want
to
> avoid that.
> 
> You don't need to fix that in this CL, however.

If there are several contexts, multiple loaders are created, but only case that
has different sample rate is loading another database set. Logically, those have
same sample rate, two loader share same database.

[KhNo, 2014-07-21 18:02:52]

On 2014/07/21 17:48:41, KhNo wrote:
> On 2014/07/21 17:43:22, Raymond Toy wrote:
> > LGTM.
> > 
> > A question: if you create two AudioContext's, will there now be two copies
of
> > the HRTF database?  It seems like there will be, and, if so, we probably
want
> to
> > avoid that.
> > 
> > You don't need to fix that in this CL, however.
> 
> If there are several contexts, multiple loaders are created, but only case
that
> has different sample rate is loading another database set. Logically, those
have
> same sample rate, two loader share same database.

Sorry my comment need to modify. If there are contexts those have same sample
rate, the first one, create a loader, the second one, it is just assinged
created loader already by the first one. Then database loading is not happened
since db is already ready by the first.

If there are contexts, those have different sample rate, the first one create
loader and load database. The second is just assigned created loader by the
first one. Then load database set with new sample rate for the second.

[KhNo, 2014-07-21 18:03:27]

The CQ bit was checked by keonho07.kim@samsung.com

[commit-bot: I haz the power, 2014-07-21 18:03:42]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/keonho07.kim@samsung.com/393363002/140001

[Raymond Toy, 2014-07-21 18:04:05]

On 2014/07/21 17:48:41, KhNo wrote:
> On 2014/07/21 17:43:22, Raymond Toy wrote:
> > LGTM.
> > 
> > A question: if you create two AudioContext's, will there now be two copies
of
> > the HRTF database?  It seems like there will be, and, if so, we probably
want
> to
> > avoid that.
> > 
> > You don't need to fix that in this CL, however.
> 
> If there are several contexts, multiple loaders are created, but only case
that
> has different sample rate is loading another database set. Logically, those
have
> same sample rate, two loader share same database.

Great. Thanks for fixing this issue!

[commit-bot: I haz the power, 2014-07-21 19:04:29]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/35503)

[commit-bot: I haz the power, 2014-07-21 20:02:55]

Change committed as 178583