Bug 46234 – Add HRTFDatabaseLoader files

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Rogers]

Add HRTFDatabaseLoader files

[Chris Rogers]

Created attachment 68315 [details]
Patch

[James Robinson]

Comment on attachment 68315 [details]
Patch

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

How is this class supposed to be used?  It seems that a user would first call loadAsynchronously(), but then have nothing better to do but poll isLoaded() until it returns true, which doesn't seem very useful.  Should this expose a callback for users?

r- for the unsafe use of RefPtr across threads.

> WebCore/platform/audio/HRTFDatabaseLoader.cpp:76
> +        m_hrtfDatabase = HRTFDatabase::createForSubject("Composite", m_databaseSampleRate);

It is not safe to increment a reference on a RefCounted object from one thread and then increment/decrement references from another thread.

[Chris Rogers]

(In reply to comment #2)
> (From update of attachment 68315 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68315&action=review
> 
> How is this class supposed to be used?  It seems that a user would first call loadAsynchronously(), but then have nothing better to do but poll isLoaded() until it returns true, which doesn't seem very useful.  Should this expose a callback for users?

Sorry, I didn't suppy enough context about this patch.  Please see WebCore/webaudio/AudioContext.cpp for the call to loadAsynchronously().
Also there is a method AudioContext::isRunnable() which calls the isLoaded() method.  Then the audio thread calls context()->isRunnable() at the start of each render cycle.  Please see patch:

https://bugs.webkit.org/attachment.cgi?id=68416&action=prettypatch


> 
> r- for the unsafe use of RefPtr across threads.
> 
> > WebCore/platform/audio/HRTFDatabaseLoader.cpp:76
> > +        m_hrtfDatabase = HRTFDatabase::createForSubject("Composite", m_databaseSampleRate);
> 
> It is not safe to increment a reference on a RefCounted object from one thread and then increment/decrement references from another thread.

I believe that it is safe to call m_hrtfDatabase.clear() in the unload() method because waitForThreadCompletion() has already returned.  Also, in the calling
sequences actually used for this code, it should be safe for HRTFPanner::pan() to make the reference.  But in re-reading this I think it's unnecessary.
I think I could clean this up by having the database() method return just a simple HRTFDatabase* pointer to avoid any possible problems here and also simplify the API.

I'll re-post a patch with that change and the corresponding change to the HRTFPanner patch.

[Chris Rogers]

Created attachment 68784 [details]
Patch

[Chris Rogers]

Created attachment 68961 [details]
Patch

[Chris Rogers]

Hi James, as we discussed offline, I've reworked this class to use an OwnPtr<HRTFDatabase> instead of RefPtr.

[Chris Rogers]

Created attachment 68981 [details]
Patch

[Chris Rogers]

Sorry, another patch to fix order of ASSERT around line 54 of HRTFDatabaseLoader.cpp

[James Robinson]

Comment on attachment 68981 [details]
Patch

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

Looking pretty good!  Thanks for the threading ASSERT()s, they make it much easier to reason about.

r- for nits

> WebCore/platform/audio/HRTFDatabaseLoader.cpp:65
> +    , m_databaseSampleRate(44100.0) // proper value gets set later

the constructor should just take the sample rate as a parameter since it's always set immediately after construction

> WebCore/platform/audio/HRTFDatabaseLoader.h:60
> +    static HRTFDatabaseLoader* s_loader; // singleton

should this be public?

> WebCore/platform/audio/HRTFDatabaseLoader.h:72
> +    mutable OwnPtr<HRTFDatabase> m_hrtfDatabase;

nit: it doesn't appear mutable is needed here

> WebCore/platform/audio/HRTFDatabaseLoader.h:81
> +// defaultHRTFDatabase() gives access to the loaded database.
> +// This can be called from any thread, but it is the callers responsibilty to call this while the context (and thus HRTFDatabaseLoader)
> +// is still alive.  Otherwise this will return 0.
> +extern HRTFDatabase* defaultHRTFDatabase();

This should be a static function on HRTFDatabaseLoader, no reason to put this directly in the WebCore namespace.

[Chris Rogers]

Created attachment 69001 [details]
Patch

[Chris Rogers]

James, I think I've addressed the rest of your comments.

[James Robinson]

Comment on attachment 69001 [details]
Patch

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

R=me

> WebCore/platform/audio/HRTFDatabaseLoader.h:67
> +    HRTFDatabaseLoader(double sampleRate);    

nit: explicit

[Chris Rogers]

Created attachment 69003 [details]
Patch

[Chris Rogers]

Thanks James, I addressed the last nit - was R+ before.

[WebKit Commit Bot]

Comment on attachment 69003 [details]
Patch

Clearing flags on attachment: 69003

Committed r68470: <http://trac.webkit.org/changeset/68470>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.