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

Added stereoPan property to allow audio panning #1051

Merged
merged 9 commits into from
Jun 8, 2018

Conversation

rafaelrpinto
Copy link
Contributor

@rafaelrpinto rafaelrpinto commented Jun 5, 2018

This PR adds the channel property to Video that allows playing the audio on the left channel, right channel or both (default).

I only added the behavior for Android. Since I use linux I'm not able to implement the IOS part but should be a similar logic using the pan property

Related issue: #1050

Update: see the comments below to check the final solution

@rafaelrpinto
Copy link
Contributor Author

I'm not very acquainted with media jargon so channel might not be the best name. Instead I could change to just pan or panPosition and instead of both use center.

@cobarx
Copy link
Contributor

cobarx commented Jun 5, 2018

Very neat concept, what are you using this for you if you don't mind my asking.

I would call it joint or stereo, that's the term they use in mp3s.

Rather than making this a binary choice of left or right, what about allowing you to choose a balance:
leftChannelVolume and rightChannelVolume (default to 1.0 for both)
That way you can do an actual pan, shifting from left to right and back again.

@rafaelrpinto
Copy link
Contributor Author

@cobarx
Thanks for checking the code.

I'm planning on using in on an app that performs hearing tests. So we need to identify some level of hearing loss on each ear.

Maybe instead of leftChannelVolume and rightChannelVolume we could use the same argument as IOS pan attribute, an integer from -1.0 to 1.0 where -1.0 is full left, 0.0 is center and 1.0 is full right.

I think this way its easier to use <Video ... volume={1} pan={-0.5} /> and for IOS the usage would be seamless.

What do you think?

@cobarx
Copy link
Contributor

cobarx commented Jun 5, 2018

That's a neat application.

Yeah I like your idea, one prop is definitely better than two. I wonder if we should call it stereoPan to distinguish from other pan related terms like the PanResponder.

@rafaelrpinto
Copy link
Contributor Author

Cool. stereoPan is fine by me.

This weekend I will setup an osx vm and make the change for both platforms to make the feature consistent.

@rafaelrpinto rafaelrpinto changed the title Added channel property to allow audio panning Added stereoPan property to allow audio panning Jun 5, 2018
@rafaelrpinto
Copy link
Contributor Author

rafaelrpinto commented Jun 5, 2018

@cobarx
I updated the code with the behavior we discussed. Now stereoPan optionally receives a value between -1.0 and 1.0 and works like the IOS pan attribute.

I will limit this PR just for Android. Setting up an osx vm will be a pain and anyone with a mac can easily implement this part on IOS because there is no volume calculation required.

@cobarx
Copy link
Contributor

cobarx commented Jun 5, 2018

@rafaelrpinto That's great!

I checked and ExoPlayer doesn't support changing the volume per channel yet:
google/ExoPlayer#2659
So unless I'm missing something, I don't believe the ExoPlayer code you added is valid, can you remove that.

I have the iOS dev environment setup, but that property only exists on the AVAudioPlayer, but not a generic AVPlayer that plays video. So I think we'll have to push that down the road.

@rafaelrpinto
Copy link
Contributor Author

rafaelrpinto commented Jun 6, 2018

@cobarx
You are right, I misunderstood the ExoPlayer part. I reverted the relevant changes.

@cobarx
Copy link
Contributor

cobarx commented Jun 8, 2018

Tested and it's working great! Merging. Thanks for making the changes and adding the docs.

@cobarx cobarx merged commit 8534ec0 into TheWidlarzGroup:master Jun 8, 2018
@rafaelrpinto rafaelrpinto deleted the feature/audio-panning branch June 8, 2018 06:39
@rafaelrpinto rafaelrpinto restored the feature/audio-panning branch June 8, 2018 06:39
@rafaelrpinto
Copy link
Contributor Author

Thanks!

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.

2 participants