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

iOS: RCTVideo setSrc: will block mainthread for long due to RCT setFilter: #1348

Closed
niklassaers opened this issue Nov 27, 2018 · 5 comments · Fixed by #1360
Closed

iOS: RCTVideo setSrc: will block mainthread for long due to RCT setFilter: #1348

niklassaers opened this issue Nov 27, 2018 · 5 comments · Fixed by #1360

Comments

@niklassaers
Copy link
Contributor

niklassaers commented Nov 27, 2018

Current behavior

When I instantiate a RCTVideo, setSrc is called. It in turn calls
[self playerItemPrepareText:asset assetOptions:assetOptions withCallback:handler];
which calls
handler([AVPlayerItem playerItemWithAsset:asset]); (because I have no text tracks)
which in turn calls
[self setFilter:_filterName];
setFilter takes between 0.3 and 2.8 seconds on my iPhone 7 Plus.

This all happens on the main thread, and thus I have a main thread block. In my case, since I'm loading multiple videos, I have a real bad hang.

Reproduction steps

Load a video

Expected behavior

Immediate return, anything that takes long will be done async

Platform

iOS 12.1
react-native-video/Video 4.0.1

@cobarx
Copy link
Contributor

cobarx commented Nov 28, 2018

@nickgzzjr Can you take a look into this and see about making it so we don't create a filter unless the filter prop is set? I'd create a PR, but without doing some reading I'm not sure if it's even possible to change the composition on the fly.

Let me know if you can tackle this in the next few days as this is a pretty significant performance hit and it needs to get fixed as soon as we can.

@Jazznight
Copy link

Jazznight commented Nov 30, 2018

Not sure whether the case is same or not. For my case it will block whole app if I set src with remote m3u8 resource. It will block only when init Player and requesting remote m3u8 file. This is problem confused me a while after I upgrade from 3.2.1 to 4.0.1.

  • iOS 12.1
  • react-native-video/Video 4.0.1
  • react-native 0.57.5

I'm handling network connection exception and test with this Network link conditioner tool. After I upgrade to 4.0.1 will hang a very long time when requesting remote m3u8 resource with bad network condition. I thought it block the main thread so that all my spin/loading animation hang as well.

FYR, I found if network condition is good it will also block for a very short time(I thought only block when requesting remote resource).

screen shot 2018-11-30 at 10 58 47 pm

@nickgzzjr
Copy link
Contributor

@cobarx Not adding a filter at start is how I had originally done it but it didn't work for me. For some reason when you do apply the filter, the video transformation gets all messed up. On a side note: you can change the filter on the fly, as long as you start with a "blank" filter. I use this feature by swiping through different filters while the video is playing.

@cobarx
Copy link
Contributor

cobarx commented Dec 6, 2018

I looked at your PR and that approach seems reasonable, thanks for taking the initiative on creating a fix. I'm wondering if it would make more sense to add a second prop filterEnabled that could control whether to create a filter compatible composition. That would make it possible to support the filter switching feature that you're describing, which is going to be useful to a lot of people who are using filters.

We have other props like textTracks can only be applied when the video source is loaded, so I'm not concerned about the limitation.

What do you think?

Also, can you make sure to remove the extra newlines from the PR, the code style should match the rest of the project.

@niklassaers
Copy link
Contributor Author

@nickgzzjr If also testing for an empty string, the PR works for me too. That's effectively what I've done in the meanwhile

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 a pull request may close this issue.

4 participants