WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
46234
Add HRTFDatabaseLoader files
https://bugs.webkit.org/show_bug.cgi?id=46234
Summary
Add HRTFDatabaseLoader files
Chris Rogers
Reported
2010-09-21 17:21:39 PDT
Add HRTFDatabaseLoader files
Attachments
Patch
(8.05 KB, patch)
2010-09-21 17:22 PDT
,
Chris Rogers
no flags
Details
Formatted Diff
Diff
Patch
(8.03 KB, patch)
2010-09-24 16:25 PDT
,
Chris Rogers
no flags
Details
Formatted Diff
Diff
Patch
(9.35 KB, patch)
2010-09-27 14:46 PDT
,
Chris Rogers
no flags
Details
Formatted Diff
Diff
Patch
(9.34 KB, patch)
2010-09-27 15:53 PDT
,
Chris Rogers
no flags
Details
Formatted Diff
Diff
Patch
(9.29 KB, patch)
2010-09-27 17:50 PDT
,
Chris Rogers
no flags
Details
Formatted Diff
Diff
Patch
(9.30 KB, patch)
2010-09-27 17:58 PDT
,
Chris Rogers
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Chris Rogers
Comment 1
2010-09-21 17:22:43 PDT
Created
attachment 68315
[details]
Patch
James Robinson
Comment 2
2010-09-24 15:34:11 PDT
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
Comment 3
2010-09-24 16:19:17 PDT
(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
Comment 4
2010-09-24 16:25:05 PDT
Created
attachment 68784
[details]
Patch
Chris Rogers
Comment 5
2010-09-27 14:46:56 PDT
Created
attachment 68961
[details]
Patch
Chris Rogers
Comment 6
2010-09-27 14:48:19 PDT
Hi James, as we discussed offline, I've reworked this class to use an OwnPtr<HRTFDatabase> instead of RefPtr.
Chris Rogers
Comment 7
2010-09-27 15:53:41 PDT
Created
attachment 68981
[details]
Patch
Chris Rogers
Comment 8
2010-09-27 15:54:30 PDT
Sorry, another patch to fix order of ASSERT around line 54 of HRTFDatabaseLoader.cpp
James Robinson
Comment 9
2010-09-27 16:50:05 PDT
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
Comment 10
2010-09-27 17:50:44 PDT
Created
attachment 69001
[details]
Patch
Chris Rogers
Comment 11
2010-09-27 17:51:18 PDT
James, I think I've addressed the rest of your comments.
James Robinson
Comment 12
2010-09-27 17:54:03 PDT
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
Comment 13
2010-09-27 17:58:02 PDT
Created
attachment 69003
[details]
Patch
Chris Rogers
Comment 14
2010-09-27 17:58:43 PDT
Thanks James, I addressed the last nit - was R+ before.
WebKit Commit Bot
Comment 15
2010-09-27 19:08:41 PDT
Comment on
attachment 69003
[details]
Patch Clearing flags on attachment: 69003 Committed
r68470
: <
http://trac.webkit.org/changeset/68470
>
WebKit Commit Bot
Comment 16
2010-09-27 19:08:48 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug