Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support "requiring" audio files via RN assets system (iOS only) #47

Merged
merged 3 commits into from
Jun 9, 2016
Merged

Support "requiring" audio files via RN assets system (iOS only) #47

merged 3 commits into from
Jun 9, 2016

Conversation

lostintangent
Copy link
Contributor

@lostintangent lostintangent commented Jun 8, 2016

This addresses #14 for iOS by adding support for using the RN assets system to require audio files that are placed alongside your JS components. I also fixed a bug where the iOS implementation wasn't correctly handling file paths that included URL encoded values (e.g. %20).

Additionally, I modified the Sound constructor signature to allow omitting the basepath argument if an asset value was specified, so that the following syntax can be achieved.

var sound = new Sound(require("./foo.mp3"), (error) => {
   if (error) {
      // Handle error
   } else {
      sound.play();
   }
});

This PR currently only supports iOS, since the RN packager places the audio files into the drawable-mpi directory, as opposed to raw (where this plugin expects to find them) when generating an Android bundle. In general, the asset system on Android doesn't seem to really accommodate non-image assets at the moment. I'm going to investigate this and likely send a PR to RN itself, but I wanted to get this PR out to start the conversation.

I've got a local fork of RN that correctly writes audio files to the raw directory, and with this PR, react-native-sound works perfectly, so this PR should accommodate both iOS and Android, yet I'll need to submit a fix to RN to actually do the right thing for Android.

NOTE: As an aside, this PR also allows you to ship updates to your audio files via OTA update services (e.g. CodePush), which is pretty awesome.

@@ -64,25 +64,6 @@ -(NSDictionary *)constantsToExport {
[session setActive: enabled error: nil];
}

RCT_EXPORT_METHOD(setCategory:(nonnull NSNumber*)key withValue:(NSString*)categoryName) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't remove this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't meant to remove this. I added it back in the 2nd commit.

@zmxv zmxv merged commit feb1e2b into zmxv:master Jun 9, 2016
@zmxv
Copy link
Owner

zmxv commented Jun 9, 2016

Thanks for the patch!

@chetstone
Copy link

chetstone commented Jun 21, 2016

I'm unable to get this to work. It didn't work in my project, so I tried in react-native-sound-demo. I used:

var s = new Sound(require('./advertising.mp3'), (e) => {

I'm using master (530a5d3)
and I get the following error message in the Chrome debugger:

Object {code: "EUNSPECIFIED", message: "The operation couldn’t be completed. (OSStatus error 2003334207.)", nativeStackIOS: Array[17], domain: "NSOSStatusErrorDomain"}
code
:
"EUNSPECIFIED"
domain
:
"NSOSStatusErrorDomain"
message
:
"The operation couldn’t be completed. (OSStatus error 2003334207.)"
nativeStackIOS
:
Array[17]
0
:
"0   RNSoundDemo                         0x000000010b9fc416 RCTJSErrorFromCodeMessageAndNSError + 134"
1
:
"1   RNSoundDemo                         0x000000010b9fc356 RCTJSErrorFromNSError + 102"
2
:
"2   RNSoundDemo                         0x000000010b972bac -[RNSound prepare:withKey:withCallback:] + 1020"
3
:
"3   CoreFoundation                      0x000000010d0e35cc __invoking___ + 140"
4
:
"4   CoreFoundation                      0x000000010d0e341e -[NSInvocation invoke] + 286"
5
:
"5   CoreFoundation                      0x000000010d172d26 -[NSInvocation invokeWithTarget:] + 54"
6
:
"6   RNSoundDemo                         0x000000010b99d05f -[RCTModuleMethod invokeWithBridge:module:arguments:] + 1887"
7
:
"7   RNSoundDemo                         0x000000010b9e45e8 -[RCTBatchedBridge _handleRequestNumber:moduleID:methodID:params:] + 680"
8
:
"8   RNSoundDemo                         0x000000010b9e346a __33-[RCTBatchedBridge handleBuffer:]_block_invoke.494 + 1258"
9
:
"9   libdispatch.dylib                   0x0000000110ccbd9d _dispatch_call_block_and_release + 12"
10
:
"10  libdispatch.dylib                   0x0000000110cec3eb _dispatch_client_callout + 8"
11
:
"11  libdispatch.dylib                   0x0000000110cd282c _dispatch_queue_drain + 2215"
12
:
"12  libdispatch.dylib                   0x0000000110cd1d4d _dispatch_queue_invoke + 601"
13
:
"13  libdispatch.dylib                   0x0000000110cd4996 _dispatch_root_queue_drain + 1420"
14
:
"14  libdispatch.dylib                   0x0000000110cd4405 _dispatch_worker_thread3 + 111"
15
:
"15  libsystem_pthread.dylib             0x00000001110294de _pthread_wqthread + 1129"
16
:
"16  libsystem_pthread.dylib             0x0000000111027341 start_wqthread + 13"
length
:
17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants