Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(18)

Issue 14636011: Support multiple HRTFDatabases for different sample-rates (Closed)

Created:
6 years, 7 months ago by Chris Rogers
Modified:
6 years, 6 months ago
CC:
blink-reviews, eae+blinkwatch, jeez
Visibility:
Public.

Description

Support multiple HRTFDatabases for different sample-rates This removes the limitation which required OfflineAudioContexts to be created at native sample-rate BUG=none TEST=layout tests pass R=kbr@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150347

Patch Set 1 #

Total comments: 4

Patch Set 2 : Pass in loader instead of database #

Patch Set 3 : Fix bad ASSERT #

Total comments: 4

Patch Set 4 : add ASSERT(isMainThread()) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+70 lines, -57 lines) Patch
M Source/core/platform/PlatformMemoryInstrumentation.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/platform/audio/HRTFDatabaseLoader.h View 1 4 chunks +19 lines, -15 lines 0 comments Download
M Source/core/platform/audio/HRTFDatabaseLoader.cpp View 1 2 3 4 chunks +29 lines, -22 lines 0 comments Download
M Source/core/platform/audio/HRTFPanner.h View 1 3 chunks +4 lines, -1 line 0 comments Download
M Source/core/platform/audio/HRTFPanner.cpp View 1 2 5 chunks +5 lines, -5 lines 0 comments Download
M Source/core/platform/audio/Panner.h View 1 2 chunks +2 lines, -1 line 0 comments Download
M Source/core/platform/audio/Panner.cpp View 1 2 chunks +2 lines, -2 lines 0 comments Download
M Source/modules/webaudio/AudioContext.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M Source/modules/webaudio/OfflineAudioContext.cpp View 2 chunks +1 line, -5 lines 0 comments Download
M Source/modules/webaudio/OfflineAudioDestinationNode.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/modules/webaudio/PannerNode.cpp View 1 2 chunks +4 lines, -4 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
Chris Rogers
6 years, 7 months ago (2013-05-10 23:56:05 UTC) #1
Ken Russell (switch to Gerrit)
https://codereview.chromium.org/14636011/diff/1/Source/core/platform/audio/HRTFDatabaseLoader.cpp File Source/core/platform/audio/HRTFDatabaseLoader.cpp (right): https://codereview.chromium.org/14636011/diff/1/Source/core/platform/audio/HRTFDatabaseLoader.cpp#newcode54 Source/core/platform/audio/HRTFDatabaseLoader.cpp:54: if (loader.get()) { RefPtr has a conversion operator to ...
6 years, 7 months ago (2013-05-11 01:35:16 UTC) #2
Chris Rogers
Thanks Ken, PTAL, I've also addressed the memory leak issue from the failing test "nmi-webaudio-leak-test.html" ...
6 years, 7 months ago (2013-05-13 20:05:22 UTC) #3
Ken Russell (switch to Gerrit)
LGTM with one comment. https://codereview.chromium.org/14636011/diff/7012/Source/core/platform/audio/HRTFDatabaseLoader.cpp File Source/core/platform/audio/HRTFDatabaseLoader.cpp (right): https://codereview.chromium.org/14636011/diff/7012/Source/core/platform/audio/HRTFDatabaseLoader.cpp#newcode138 Source/core/platform/audio/HRTFDatabaseLoader.cpp:138: if (s_loaderMap) { This method ...
6 years, 6 months ago (2013-05-13 21:44:18 UTC) #4
Chris Rogers
Committed patchset #4 manually as r150347.
6 years, 6 months ago (2013-05-14 20:22:15 UTC) #5
Chris Rogers
6 years, 6 months ago (2013-05-14 20:23:10 UTC) #6
Message was sent while issue was closed.
https://codereview.chromium.org/14636011/diff/7012/Source/core/platform/audio...
File Source/core/platform/audio/HRTFDatabaseLoader.cpp (right):

https://codereview.chromium.org/14636011/diff/7012/Source/core/platform/audio...
Source/core/platform/audio/HRTFDatabaseLoader.cpp:138: if (s_loaderMap) {
On 2013/05/13 21:44:18, kbr wrote:
> This method should also ASSERT(isMainThread()).

Done.

https://codereview.chromium.org/14636011/diff/7012/Source/core/platform/audio...
File Source/core/platform/audio/HRTFDatabaseLoader.h (right):

https://codereview.chromium.org/14636011/diff/7012/Source/core/platform/audio...
Source/core/platform/audio/HRTFDatabaseLoader.h:75: static
HRTFDatabaseLoader::LoaderMap* loaderMap() { return s_loaderMap; }
On 2013/05/13 21:44:18, kbr wrote:
> It seems a bad idea to expose the LoaderMap directly. The methods in this
class
> which access it assert that they're called on the main thread, but by exposing
> the LoaderMap to the outside world, constraints could be violated.

Agreed that it's not ideal, but PlatformMemoryInstrumentation.cpp needs it...

Powered by Google App Engine
This is Rietveld 408576698