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

Fix Android audio crash fixes Issue #1417 #1529

Merged
merged 2 commits into from
Apr 4, 2019

Conversation

Mrjaco12
Copy link
Contributor

@Mrjaco12 Mrjaco12 commented Mar 19, 2019

Fix Issue #1417

Why:

It appears that in this commit the check added to ensure group lengths are greater than 0 still allows for a case in which the groupIndex is -1 but the group.length == 0 so we are never breaking for the invalid groupIndex.

This change addresses the need by:

  • Don't chain check for INDEX_UNSET to previous conditional

Why:

* There is a case where groupIndex may be unset

This change addresses the need by:

* Don't chain check for INDEX_UNSET to previous conditional
@Mrjaco12 Mrjaco12 changed the title Fix Android audio crash Fixes Issue #1417 Fix Android audio crash fixes Issue #1417 Mar 19, 2019
@rborn
Copy link

rborn commented Mar 19, 2019

@Mrjaco12 ❤️

I can confirm the fix works, tested on android 9 (nokia 7+) with audioOnly and an mp3 file
It doesn't work without the fix

@wangliang1124
Copy link

Good job.

@rborn
Copy link

rborn commented Apr 1, 2019

@cobarx hey, any chance to have a look at this?

Also any chance to reply to @kelset related to the React Native Core Contributors org we're trying to setup (well, he's doing all the work)? 🤗 https://github.com/react-native-community/meta/blob/master/MAINTAINERS.md

@kesha-antonov
Copy link

Nice!

@cobarx
Copy link
Contributor

cobarx commented Apr 4, 2019

Ok sorry for the delay on this, I haven't been as active lately.

Yes, this will work. It took me a bit of looking but you were right on the money. The nested check whether there are video groups or not through me for a loop on why unchaining worked. So I moved that into the main if check to make that more readable and added some comments.

I'll reach out to @kelset tonight.

@cobarx cobarx merged commit c12e9d8 into TheWidlarzGroup:master Apr 4, 2019
kaibarnes added a commit to TacchiStudios/react-native-video that referenced this pull request May 23, 2019
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.

5 participants