Add HRTFDatabaseLoader files
Created attachment 68315 [details] Patch
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.
(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.
Created attachment 68784 [details] Patch
Created attachment 68961 [details] Patch
Hi James, as we discussed offline, I've reworked this class to use an OwnPtr<HRTFDatabase> instead of RefPtr.
Created attachment 68981 [details] Patch
Sorry, another patch to fix order of ASSERT around line 54 of HRTFDatabaseLoader.cpp
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.
Created attachment 69001 [details] Patch
James, I think I've addressed the rest of your comments.
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
Created attachment 69003 [details] Patch
Thanks James, I addressed the last nit - was R+ before.
Comment on attachment 69003 [details] Patch Clearing flags on attachment: 69003 Committed r68470: <http://trac.webkit.org/changeset/68470>
All reviewed patches have been landed. Closing bug.