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

Prevent errors on iOS when using text tracks with caching #1172

Merged
merged 8 commits into from
Aug 28, 2018

Conversation

cobarx
Copy link
Contributor

@cobarx cobarx commented Aug 8, 2018

This PR is intended to fix a crash that happens when you attempt to mix in text tracks. Also fixes issues where the right error code wasn't returned when the file couldn't be cached.

@n1ru4l I did some refactors to make it a bit easier to read. IMO, putting all of the caching code into a separate function so you don't have to parse all the ifdefs is a lot easier to digest. For future PRs, I'd suggest breaking it into pieces. Giant PRs are make it more challenging to figure out what changed and also are a bit harder to merge since everything needs to be tested and evaluated all at once.

I did some investigation into the text track issue and have it mostly isolated. In the text track code https://github.com/react-native-community/react-native-video/blob/86d655c3d1c8a81f6b841d7379ba624903440b32/ios/Video/RCTVideo.m#L393 it is bombing at the line for:

AVAssetTrack *videoAsset = [asset tracksWithMediaType:AVMediaTypeVideo].firstObject;

Which throws an error in the debugger of:

error: warning: got name from symbols: AVMediaTypeVideo
error: cannot initialize a parameter of type 'id' with an lvalue of type 'void *'

I was thinking maybe DVURLAsset didn't subclass AVURLAsset and was missing the tracksWithMediaType method and manually created the AVURLAsset. However, that didn't fix it and it turns out it does subclass it.

I ran out of time today to finish investigating but will see if I can figure out that error this evening or tomorrow.

@n1ru4l A few other questions for you. Can we also cache the audio & text tracks? Also, I'm assuming that if you watch a video for a second time and part of the video is in the cache, it will pull any other parts that aren't cached from the network, right? Just wanted to be sure.

@cobarx
Copy link
Contributor Author

cobarx commented Aug 11, 2018

@n1ru4l I set some breakpoints in the code and what it looks like is happening is when it goes to get the video tracks there aren't any tracks in the array. What I'm guessing is that they haven't been loaded yet. Any idea how we might be able to wait until they are loaded before we try and query them?

Also, I wanted to say great job on this. The code is very easy to follow and is especially impressive considering how many advance Objective-C features you're using for someone new to the language.

@n1ru4l
Copy link
Contributor

n1ru4l commented Aug 15, 2018

@cobarx I will try to take a look when I got some spare time, currently having text tracks is unfortunately not a requirement of our app 😕.

@cobarx
Copy link
Contributor Author

cobarx commented Aug 15, 2018 via email

@cobarx
Copy link
Contributor Author

cobarx commented Aug 27, 2018

I have no idea how to fix this so I'm going to disable caching if text tracks are specified and get this released. Glad we're finally getting to ship this!

Hopefully some day your company will need to subtitles and we can get them fixed :)

cobarx and others added 5 commits August 27, 2018 17:55
Fixes a crash when using the cache & sideloaded text tracks together due to the tracks on the asset not being available. Re-visit when someone with more expertise on the cache can look at it.
@cobarx cobarx merged commit 03734bb into develop Aug 28, 2018
@cobarx cobarx changed the title [WIP] Prevent errors on iOS when using text tracks with caching Prevent errors on iOS when using text tracks with caching Aug 28, 2018
@cobarx cobarx deleted the bugfix/ios-cache-texttracks branch August 28, 2018 01:19
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