Bug 34660 – audio engine: add audio resources abstraction

• Component: Platform
• Status: RESOLVED FIXED

[Chris Rogers]

In order to perform spatialized stereo panning, a number of data files representing the recorded impulse responses (HRTFs) from real human subjects are needed.  These files need to be packaged in an appropriate "Resources" directory depending on platform.  For the "mac" port, perhaps they would go in a sub-directory of the Resources directory of WebCore.framework.

This patch, creates an abstraction for getting the path to this directory by defining the function:

const char* GetAudioSpatializationResourcePath()

An implementation is provided for the mac port in AudioResourcesMac.mm

[Chris Rogers]

Created attachment 48257 [details]
patch for spatialization resources abstraction

[Darin Adler]

Comment on attachment 48257 [details]
patch for spatialization resources abstraction

I have no idea what a spatialization resource is, so please take the feedback with a grain of salt.

> +const char* GetAudioSpatializationResourcePath();

WebKit function names begin with a lower case letter.

WebKit function names use "get" in their names only if the use out arguments. Functions with return values are named after their return values.

> +    // First look in Resources directory for application's main bundle 
> +    NSString* resourcePath = [[NSBundle mainBundle] resourcePath];
> +
> +    NSString* spatializationPath = [resourcePath stringByAppendingPathComponent:@"AudioSpatialization"];
> +
> +    if ([[NSFileManager defaultManager] fileExistsAtPath:spatializationPath]) {
> +        return [spatializationPath UTF8String];
> +    }
> +    
> +    // Impulse responses are not found in browser application's resources directory
> +    // fallback to looking in WebCore.framework
> +
> +    NSString* webCorePath = [[NSBundle bundleWithIdentifier:@"com.apple.WebCore"] resourcePath];
> +
> +    spatializationPath = [webCorePath stringByAppendingPathComponent:@"AudioSpatialization"];
> +
> +    if ([[NSFileManager defaultManager] fileExistsAtPath:spatializationPath]) {
> +        return [spatializationPath UTF8String];
> +    }

WebKit project uses a space between NSString and *, although we are talking about changing that guideline, because it's inconsistent with what we do for C++ types. Please follow our current guideline.

WebKit project uses no braces around a single line if statement body.

The correct method to use for a path is fileSystemRepresentation rather than UTF8String.

> +    return NULL; // not found anywhere

WebKit project uses 0 for this.

In general this code seems to have so little to do with audio that I think it should be calling some shared functions that can get at resources. It shouldn’t be doing all the bundle stuff directly

[Chris Rogers]

Hi Darin, thanks for the quick response

I'll address all of the style issues, but wanted to wait until we decided if this is the correct approach.

I'm more than happy to switch over to using a more general approach to handling resources.  Do we have any utility functions or helper classes for this already?

Basicallly, I just need some way to get the path to the directory containing these files in a cross-platform manner.

I'm more than happy to explain more technical details about what these files are about if you're interested.

[Simon Fraser (smfr)]

Is an GetAudioSpatializationResource something you may wish to load remotely via URL at some point?

[Chris Rogers]

Hi Simon, that's a very good question.  I believe we should have a "default" data-set built-in to the browser for the normal use case.  The datasets I believe are  somewhere around 750K, so there may be some issues with download speed, plus we don't want every javascript developer to have to host these datasets server-side.  That said, it might be interesting to allow javascript developers to load up "custom" spatialization data-sets via URL.  But, since they're fairly difficult to create, it would not be generally used this way.

So, this patch represents an abstraction to get the "default" data-set, which I imagine would be in the application's Resources directory somewhere...

[Simon Fraser (smfr)]

If you ever think you'll load them remotely then you should always load via URLs.

It's also hard to know if spatialized stereo panning even fits into the first cut of the audio APIs, so maybe this patch is getting ahead of itself.

[Chris Rogers]

I've been talking with Dimitri Glazkov and Darin Fisher about this, and they're helping me work out a better way to abstract this (dealing with Chrome sandboxing, etc.) -- still working on it...

[Sam Weinig]

In general, I think we should be working toward making any code that hits the file system (and of course network) async.  Can this be made async?

[Chris Rogers]

After speaking with Darin Fisher, he suggests we use an approach similar to what we've done for images:

platform/graphics/Image.h:90:    static PassRefPtr<Image> loadPlatformResource(const char* name);

except it would return a SharedBuffer for the data

We should be able to make it async if we have a completion callback instead.

[Chris Rogers]

Created attachment 71796 [details]
Patch

[Chris Rogers]

This new patch follows very closely what we're doing in Image::loadPlatformResource().  The place where it's currently called is in a special loader thread (not the main thread), which should address Sam's concern about async loading.

[Chris Rogers]

Created attachment 73144 [details]
Patch

[Kenneth Russell]

Comment on attachment 73144 [details]
Patch

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

The code basically looks fine. r- for the license header issue as well as a couple of other minor issues.

> WebCore/platform/audio/mac/AudioBusMac.mm:15
> + *     from this software without specific prior written permission.

Wrong license header.

> WebCore/platform/audio/mac/AudioBusMac.mm:40
> +@interface WebCoreBundleClass : NSObject

I think it would be safer to name this temporary class something audio-specific like WebCoreAudioBundleClass.

> WebCore/platform/audio/mac/AudioBusMac.mm:68
> +} // WebCore

// namespace WebCore

[Chris Rogers]

Created attachment 73317 [details]
Patch

[Kenneth Russell]

Comment on attachment 73317 [details]
Patch

Looks fine.

[WebKit Commit Bot]

Comment on attachment 73317 [details]
Patch

Rejecting patch 73317 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', 73317]" exit_code: 1
Traceback (most recent call last):
  File "./WebKitTools/Scripts/webkit-patch", line 70, in <module>
    main()
  File "./WebKitTools/Scripts/webkit-patch", line 63, in main
    from webkitpy.tool.main import WebKitPatch
  File "/Users/abarth/git/webkit-queue/WebKitTools/Scripts/webkitpy/tool/main.py", line 42, in <module>
    from webkitpy.common.net.rietveld import Rietveld
ImportError: No module named rietveld

Full output: http://queues.webkit.org/results/5577008

[WebKit Commit Bot]

Comment on attachment 73317 [details]
Patch

Clearing flags on attachment: 73317

Committed r71613: <http://trac.webkit.org/changeset/71613>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.