HRTFDatabaseLoader is not an absolute condition to run audioContext

HRTFDatabaseLoader is not an absolute condition to run audioContext

WebAudio feature uses HRTFDatabase(Head-releated transfer function) for pannerNode.
However, In constructor of AudioContext::AudioContext, HRTFDatabaseLoader has been created always.
It costs approximatively 30MB depends on samplerate even if there is no node to use HRTFDatabase.
As according to W3C spec and implementation of current source code, It is used only for PannerNode.
HRTFDatabaseLoader should be relocated to pannerNode.
Move to check HRTFDatabase on pannerNode::process

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

[KhNo, 2014-02-24 10:14:33]

Please review the patch set #2.

Actually, previous my chromium account has been reset unfortunately. So, I had
to raise up the patch again with new issue ID 176683003(this). All comment
history has been deleted. Sorry :( 
I had to move this issue from ID 145543008 to ID 176683003.

All layout-test has been passed.

[Ken Russell (switch to Gerrit), 2014-02-24 18:47:54]

Raymond, could you please review this first?

[Raymond Toy (Google), 2014-02-24 23:01:22]

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

https://codereview.chromium.org/176683003/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/PannerNode.cpp:117: // HRTFDatabaseLoader and
HRTFDatabase should be loaded before proceed.
Comment seems incorrect.  Change "before proceed" to "before proceeding for an
offline audio context. "

Also, is it possible for the offline audio context to check for completion
instead of here?

https://codereview.chromium.org/176683003/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/RealtimeAnalyser.cpp (left):

https://codereview.chromium.org/176683003/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/RealtimeAnalyser.cpp:33: #include
"platform/audio/FFTFrame.h"
Why is this deleted?  RealtimeAnalyser calls new FFTFrame(...)

https://codereview.chromium.org/176683003/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/RealtimeAnalyser.h (right):

https://codereview.chromium.org/176683003/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/RealtimeAnalyser.h:29: #include
"platform/audio/FFTFrame.h"
Why is  this added here and the class FFTFrame deleted near line 36 below?

It seems the original code was correct.

[KhNo, 2014-02-25 02:00:24]

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

https://codereview.chromium.org/176683003/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/PannerNode.cpp:117: // HRTFDatabaseLoader and
HRTFDatabase should be loaded before proceed.
On 2014/02/24 23:01:22, rtoy wrote:
> Comment seems incorrect.  Change "before proceed" to "before proceeding for an
> offline audio context. "
> 
> Also, is it possible for the offline audio context to check for completion
> instead of here?

Yes. It seems to be not correct comment. I will fix it.

OfflineAudioDestination::offlinerender() had a wait loader completion logic, I
have moved it to here. This change means that checking routine of HRTFDatabase
on pannerNode itself be located before transferring processed buffers to next
node or destination on audio thread as previous code did. It just moves checking
time early.

https://codereview.chromium.org/176683003/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/RealtimeAnalyser.cpp (left):

https://codereview.chromium.org/176683003/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/RealtimeAnalyser.cpp:33: #include
"platform/audio/FFTFrame.h"
On 2014/02/24 23:01:22, rtoy wrote:
> Why is this deleted?  RealtimeAnalyser calls new FFTFrame(...)

I have move to it in header

https://codereview.chromium.org/176683003/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/RealtimeAnalyser.h (right):

https://codereview.chromium.org/176683003/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/RealtimeAnalyser.h:29: #include
"platform/audio/FFTFrame.h"
On 2014/02/24 23:01:22, rtoy wrote:
> Why is  this added here and the class FFTFrame deleted near line 36 below?
> 
> It seems the original code was correct.

There was an compilation error from removing "HRFTDatabaseLoader.h" on
AudioContext.h.

In file included from ../../third_party/WebKit/Source/wtf/OwnPtr.h:27:0,
                 from ../../third_party/WebKit/Source/heap/ThreadState.h:37,
                 from ../../third_party/WebKit/Source/heap/Heap.h:36,
                 from ../../third_party/WebKit/Source/heap/Handle.h:34,
                 from
../../third_party/WebKit/Source/bindings/v8/WrapperTypeInfo.h:35,
                 from
../../third_party/WebKit/Source/bindings/v8/UnsafePersistent.h:34,
                 from
../../third_party/WebKit/Source/bindings/v8/ScriptWrappable.h:34,
                 from
../../third_party/WebKit/Source/modules/webaudio/AudioNode.h:28,
                 from
../../third_party/WebKit/Source/modules/webaudio/AudioBasicInspectorNode.h:28,
                 from
../../third_party/WebKit/Source/modules/webaudio/AnalyserNode.h:28,
                 from
../../third_party/WebKit/Source/modules/webaudio/AnalyserNode.cpp:29:
../../third_party/WebKit/Source/wtf/OwnPtrCommon.h: In static member function
‘static void WTF::OwnedPtrDeleter<T>::deletePtr(T*) [with T =
WebCore::FFTFrame]’:
../../third_party/WebKit/Source/wtf/OwnPtr.h:62:13:   instantiated from
‘WTF::OwnPtr<T>::~OwnPtr() [with T = WebCore::FFTFrame]’
../../third_party/WebKit/Source/modules/webaudio/RealtimeAnalyser.h:38:7:  
instantiated from here
../../third_party/WebKit/Source/wtf/OwnPtrCommon.h:51:1: error: invalid
application of ‘sizeof’ to incomplete type ‘WebCore::FFTFrame’ 
../../third_party/WebKit/Source/wtf/OwnPtrCommon.h:52:9: error: possible problem
detected in invocation of delete operator: [-Werror]
../../third_party/WebKit/Source/wtf/OwnPtrCommon.h:48:17: error: ‘ptr’ has
incomplete type [-Werror]
../../third_party/WebKit/Source/modules/webaudio/RealtimeAnalyser.h:36:7: error:
forward declaration of ‘struct WebCore::FFTFrame’ [-Werror]
../../third_party/WebKit/Source/wtf/OwnPtrCommon.h:52:9: note: neither the
destructor nor the class-specific operator delete will be called, even if they
are declared when the class is defined

It needs to using 'sizeof' about FFTFrame on delete operator in
RealtimeAnalyser.h

Could you recommand the way to avoid this compilation error if you knows ?

[KhNo, 2014-02-25 02:04:17]

Please review the patch set #3 which is included about modification of comment.

[Raymond Toy (Google), 2014-02-25 02:55:49]

A few more comments. 

And yes, I see that RealtimeAnalyser has a new issue with FFTFrame. Your change
is ok with me.

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

https://codereview.chromium.org/176683003/diff/100001/Source/modules/webaudio...
Source/modules/webaudio/PannerNode.cpp:55: // HRTFDatabaseLoader should be
created and load database for pannerNode asynchronously.
Sorry.  Another comment fix.  I would say something like

// Load the HRTF database asynchronously so we don't block the Javascript thread
while create the HRTF database.  The HRTF panner will return zeroes until the
database is loaded.

https://codereview.chromium.org/176683003/diff/100001/Source/modules/webaudio...
Source/modules/webaudio/PannerNode.cpp:118: if
(!m_hrtfDatabaseLoader->isLoaded()) {
This unnecessarily returns 0 even if we're not using the HRTF panner.  Say the
user set the panner to EQUALPOWER. We don't need to wait for the database to be
loaded anymore and can process audio normally.

Can you fix that so the equalpower panner doesn't wait for the database?

[KhNo, 2014-02-25 04:42:06]

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

https://codereview.chromium.org/176683003/diff/100001/Source/modules/webaudio...
Source/modules/webaudio/PannerNode.cpp:55: // HRTFDatabaseLoader should be
created and load database for pannerNode asynchronously.
It should be fixed as your recommand that is much better than mine.

https://codereview.chromium.org/176683003/diff/100001/Source/modules/webaudio...
Source/modules/webaudio/PannerNode.cpp:118: if
(!m_hrtfDatabaseLoader->isLoaded()) {
Yes. sure. I will patch soon.

[KhNo, 2014-02-25 05:35:32]

Please review the patch set #4

[Raymond Toy (Google), 2014-02-25 17:13:53]

LGTM, with one minor nit.

Also, have you run a some webaudio demos comparing the previous version with
this? Do they sound the same?

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

https://codereview.chromium.org/176683003/diff/130001/Source/modules/webaudio...
Source/modules/webaudio/PannerNode.cpp:55: // Load the HRTF database
asynchronously so we don't block the Javascript thread while create the HRTF
database.
Nit: "create" -> "creating"

[KhNo, 2014-02-26 01:52:34]

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

[KhNo, 2014-02-26 01:52:34]

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

[KhNo, 2014-02-26 01:52:42]

On 2014/02/25 17:13:53, rtoy wrote:
> LGTM, with one minor nit.
> 
> Also, have you run a some webaudio demos comparing the previous version with
> this? Do they sound the same?
> 
>
https://codereview.chromium.org/176683003/diff/130001/Source/modules/webaudio...
> File Source/modules/webaudio/PannerNode.cpp (right):
> 
>
https://codereview.chromium.org/176683003/diff/130001/Source/modules/webaudio...
> Source/modules/webaudio/PannerNode.cpp:55: // Load the HRTF database
> asynchronously so we don't block the Javascript thread while create the HRTF
> database.
> Nit: "create" -> "creating"

[KhNo, 2014-02-26 01:53:06]

On 2014/02/26 01:52:42, KhNo wrote:
> On 2014/02/25 17:13:53, rtoy wrote:
> > LGTM, with one minor nit.
> > 
> > Also, have you run a some webaudio demos comparing the previous version with
> > this? Do they sound the same?
> > 
> >
>
https://codereview.chromium.org/176683003/diff/130001/Source/modules/webaudio...
> > File Source/modules/webaudio/PannerNode.cpp (right):
> > 
> >
>
https://codereview.chromium.org/176683003/diff/130001/Source/modules/webaudio...
> > Source/modules/webaudio/PannerNode.cpp:55: // Load the HRTF database
> > asynchronously so we don't block the Javascript thread while create the HRTF
> > database.
> > Nit: "create" -> "creating"

Could you review again the patch set #6. Sorry to bother you?
I have checked some demos that is using a pannerNode in
https://chromium.googlecode.com/svn/trunk/samples/audio/samples.html
https://chromium.googlecode.com/svn/trunk/samples/audio/index.html

There was no difference and failure from layoutTest.

[KhNo, 2014-02-26 02:03:35]

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

[KhNo, 2014-02-26 02:07:17]

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

[KhNo, 2014-02-26 02:08:30]

Could you review again the patch set #6. Sorry to bother you?
I have checked some demos that is using a pannerNode in
https://chromium.googlecode.com/svn/trunk/samples/audio/samples.html
https://chromium.googlecode.com/svn/trunk/samples/audio/index.html

There was no difference and failure from layoutTest.

[Raymond Toy (Google), 2014-02-26 03:12:22]

On 2014/02/26 02:08:30, KhNo wrote:
> Could you review again the patch set #6. Sorry to bother you?
> I have checked some demos that is using a pannerNode in
> https://chromium.googlecode.com/svn/trunk/samples/audio/samples.html
> https://chromium.googlecode.com/svn/trunk/samples/audio/index.html
> 
> There was no difference and failure from layoutTest.

Still LGTM.

For your information, in case you didn't know, an LGTM with nits means you just
need to fix the nits; you don't need another review before committing.

[KhNo, 2014-02-26 03:24:25]

On 2014/02/26 03:12:22, rtoy wrote:
> On 2014/02/26 02:08:30, KhNo wrote:
> > Could you review again the patch set #6. Sorry to bother you?
> > I have checked some demos that is using a pannerNode in
> > https://chromium.googlecode.com/svn/trunk/samples/audio/samples.html
> > https://chromium.googlecode.com/svn/trunk/samples/audio/index.html
> > 
> > There was no difference and failure from layoutTest.
> 
> Still LGTM.
> 
> For your information, in case you didn't know, an LGTM with nits means you
just
> need to fix the nits; you don't need another review before committing.

Thanks a lot! Getting knowlegde about ooensource. :)

[KhNo, 2014-02-26 03:24:28]

On 2014/02/26 03:12:22, rtoy wrote:
> On 2014/02/26 02:08:30, KhNo wrote:
> > Could you review again the patch set #6. Sorry to bother you?
> > I have checked some demos that is using a pannerNode in
> > https://chromium.googlecode.com/svn/trunk/samples/audio/samples.html
> > https://chromium.googlecode.com/svn/trunk/samples/audio/index.html
> > 
> > There was no difference and failure from layoutTest.
> 
> Still LGTM.
> 
> For your information, in case you didn't know, an LGTM with nits means you
just
> need to fix the nits; you don't need another review before committing.

Thanks a lot! Getting knowlegde about ooensource. :)

[KhNo, 2014-02-26 03:24:37]

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

[Paweł Hajdan Jr., 2014-02-26 04:39:32]

The CQ bit was unchecked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-26 04:39:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keonho07.kim@samsung.com/176683003/150001

[KhNo, 2014-02-26 04:56:26]

Ken Russell, Could you check OWNER LGTM for this ?

[Paweł Hajdan Jr., 2014-02-26 05:29:52]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-26 05:31:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keonho07.kim@samsung.com/176683003/150001

[commit-bot: I haz the power, 2014-02-26 05:49:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-26 05:49:12]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[KhNo, 2014-02-26 06:38:31]

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

[KhNo, 2014-02-26 06:38:31]

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

[commit-bot: I haz the power, 2014-02-26 06:38:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keonho07.kim@samsung.com/176683003/150001

[Ken Russell (switch to Gerrit), 2014-02-26 07:18:08]

Rubber stamp LGTM

[KhNo, 2014-02-26 08:26:01]

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

[commit-bot: I haz the power, 2014-02-26 08:26:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keonho07.kim@samsung.com/176683003/150001

[commit-bot: I haz the power, 2014-02-26 10:25:33]

Change committed as 167887