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

Implement ios bufferConfig #1353

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

FrikkieSnyman
Copy link
Contributor

This PR implements a bufferConfig for iOS, similar to what already exists for android-exoplayer. In fact, it uses the same bufferConfig property. The only difference currently, is that this implementation does not support the minBufferMs and maxBufferMs settings. Further, the default behaviour when bufferConfig is not specified, is to fallback to AVPlayer's playbackLikelytoKeepUp buffering.

An effective way to test this, is to set the source of the video to a remote file, and then limit network connectivity with the Network Link Conditioner to 3G. paused should be set to false, so that playback can start automatically. With no buffer config specified, the default behaviour would be to download roughly half of the video before playback will start. With a bufferConfig specified, playback should start after the specified duration has been buffered.

@dkoo761
Copy link

dkoo761 commented Jan 23, 2019

I gave this PR a try. The solution works great however I noticed that the onBuffer callback is still getting called with the final "isBuffering": false payload at the original time (ie. when playbackLikelytoKeepUp is true) rather than as soon as playback is ready. Is this by design?

@FrikkieSnyman
Copy link
Contributor Author

@dkoo761 Yeah, I noticed this as well, but I couldn't find a way to get past it. Do you have any suggestions on how we could get that callback to fire earlier?

@dkoo761
Copy link

dkoo761 commented Jan 23, 2019

@FrikkieSnyman I tried wrapping the whole block where we handle the playbackLikelytoKeepUp keypath event with an if (!_bufferConfig){...} so that we essentially ignore that block if we've set a bufferConfig. Then at the end of the bufferConfig block I added self.onVideoBuffer(@{@"isBuffering": @(NO), @"target": self.reactTag});. However this was triggering the onBuffer callback many times instead of just once so didn't seem like the right solution. Perhaps we need to set a flag when buffering starts and then check that flag before we trigger the onBuffer callback so that we only trigger it once? I'm brand new to Objective-C so I'm probably not the right guy to solve this :)

Also, it's not clear to me if the player does in fact keep buffering further after playback is ready. If it does, then maybe it's technically accurate to leave it up to the playbackLikelytoKeepUp block to specify when buffering is done and to use some other callback to specify something like "playback ready".

The confusing thing to me was that I was expecting the "onLoad" callback to have already completed whatever buffering was required in order for play to be ready since the docs for onLoad say: "Callback function that is called when the media is loaded and ready to play." For Android that seems to be the case and the callback pattern is what I would expect:

  1. onBuffer called with payload of "isBuffering"=false
  2. onLoad called - ready to play!

However, for iOS I'm seeing this pattern instead:

  1. onBuffer called with payload of "isBuffering"=false
  2. onLoad called (I would expect to be able to play here, however.....)
  3. onBuffer called again with payload of "isBuffering"=true
  4. onBuffer called again with payload of "isBuffering"=false (in my case 10 seconds later when around half of a 45MB MP3 has been buffered)

Since I'm using your bufferConfig code, somewhere between (2) and (4) above the MP3 is actually playable, but because of the strange callback timing it's not clear when exactly that is. For now, I'm just assuming playback is ready after the onLoad callback because I'm using bufferForPlaybackMs=2500 which takes < 100 milliseconds to buffer such a small amount of content and that's good enough accuracy for my use case.

@gazedash
Copy link

Is there any progress? Really need this feature.

@amit-prabhakar
Copy link

Any Updates?

@FrikkieSnyman
Copy link
Contributor Author

Haven't had a chance yet to look at it again, sorry! But please feel free to play around with it and seeing what you can find!

@dkoo761
Copy link

dkoo761 commented Mar 18, 2019

A couple months ago I created a repo based on the 4.2.0 release (since I needed MP3 support which broke in a later release) and merged this PR into it. I've been using it since then and it's been working fine. As long as you're aware of the discussion above and implement your code accordingly, you should be OK. Feel free to clone and use my repo if you like: https://github.com/dkoo761/react-native-video-4.2.0.

@FrikkieSnyman
Copy link
Contributor Author

Nice one @dkoo761 , thanks!

@SuhairZain
Copy link

@FrikkieSnyman Would it be possible to merge this branch? With this, we're able to cut down the playback time from 8s to 4s on our app

@FrikkieSnyman
Copy link
Contributor Author

@SuhairZain I would love to, but I don't have that kind of power!

In the meanwhile, I'd recommend forking the repo and merging this branch then using the forked repo in your app

@SuhairZain
Copy link

Yeah. I was thinking of doing the same. But I also think that your ios-buffer-config branch is fairly upto date with upstream. I was thinking I would pull it and use it until it's merged.

@gazedash
Copy link

@cobarx is it possible to merge this? We really need this feature

@jenshandersson
Copy link
Contributor

Any reason for not using the built-in configuration for this?https://developer.apple.com/documentation/avfoundation/avplayeritem/1643630-preferredforwardbufferduration

@FrikkieSnyman
Copy link
Contributor Author

@jenshandersson if I remember correctly, it was because the built-in config still waited for canPlayThrough regardless of setting the preferredforwardbufferduration.

But if you have a better/simpler implementation, I'd love to see it! 😄

@gazedash
Copy link

@CHaNGeTe can you check this one? It would be awesome to see this PR merged in next release

@CHaNGeTe CHaNGeTe mentioned this pull request Jul 29, 2019
@jeanffc
Copy link

jeanffc commented Sep 17, 2019

It would be great to see this PR merged in next release

@lucasrocali
Copy link

I've also forked the repo and implemented the same changes of the PR on top of version 4.4.1

https://github.com/lucasrocali/react-native-video/tree/4.4.1/ios-buffer-config

@sysless
Copy link

sysless commented Dec 17, 2019

This is really helping with loading time on iOS

@mordonez-me
Copy link

Hi guys, do you have plans for this feature? Please let us know when you can, thanks!

@GeoffreyPlitt
Copy link

GeoffreyPlitt commented Jun 18, 2020

I see merge conflicts here; does that mean the master implemented this in a different way and this PR should be closed?

@PankajChaturvediDtechies

Facing the same issue it takes up to 15 seconds to start a acverage 5 mb video from s3.

@FrikkieSnyman
Copy link
Contributor Author

@cobarx maybe we can get your input on this? Why can this not be merged? It's been hanging around for more than 2 years without any meaningful feedback on why it's not being merged.

@stale
Copy link

stale bot commented Apr 19, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. If you are having a similar problem, please open a new issue and reference this one instead of commenting on a stale or closed issue.

@stale stale bot added the stale Closed due to inactivity or lack or resources label Apr 19, 2022
@hueniverse hueniverse added stale Closed due to inactivity or lack or resources and removed stale Closed due to inactivity or lack or resources labels Apr 20, 2022
@hueniverse hueniverse closed this Apr 22, 2022
@Michota
Copy link

Michota commented Mar 30, 2024

God I wish this was fully implemented in 6.0.0 with option to edit maxBufferMs...

@Michota
Copy link

Michota commented Apr 13, 2024

Bump. I need this so badly. I would even move from Expo-AV and donate some cash if iOS buffer config will be working as intended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Closed due to inactivity or lack or resources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet