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

Expose DefaultLoadControl parameters Android ExoPlayer #1160

Merged

Conversation

bryanvanwijk
Copy link
Contributor

This makes it possible to adjust the parameters of the DefaultLoadControl in the Android ExoPlayer: minBufferMs, maxBufferMs, bufferForPlaybackMs and bufferForPlaybackAfterRebufferMs.

@cobarx
Copy link
Contributor

cobarx commented Jul 31, 2018

This is a great feature to have!

Can these be props that you pass to the video? The only reason I can see for creating a separate config module would be there was a shared buffer for all ExoPlayer instances so that you have to configure the settings on a global basis. We want to stick to a simple props API wherever possible.

Also, this needs to be documented in the README file. Make sure it has the default values so that people have a starting point to tune from.

@bryanvanwijk
Copy link
Contributor Author

@cobarx The props can now be passed to the video instead of the separate config. The reason why I had a separate config before was because the parameters are needed during the initialization of the video player and cannot be changed like the other video properties. So now I just create a new video player when the load control parameters are set.

@@ -931,4 +939,14 @@ public void setFullscreen(boolean fullscreen) {
public void setUseTextureView(boolean useTextureView) {
exoPlayerView.setUseTextureView(useTextureView);
}

public void setLoadControl(int newMinBufferMs, int newMaxBufferMs, int newBufferForPlaybackMs, int newBufferForPlaybackAfterRebufferMs) {
player.release();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will generate a null pointer exception if setLoadControl gets called before setSrc. There is no guarantee to the order, it's somewhat random depending on what order you specify props in. I'm not sure if it's a good idea to call player.release() without doing all the cleanup that happens in releasePlayer() so I would suggest calling stopPlayback() which will do the null pointer check and cleanup.

Also, you can remove the this since all the variable names are different it doesn't achieve anything.

@cobarx
Copy link
Contributor

cobarx commented Aug 1, 2018

Thanks, yeah I was wondering how you were going to handle that, good catch. I left you a note about the implementation.

Sorry to ask for another round of changes, but I think it would be better to call the prop bufferConfig or bufferSettings. If someone is looking at the list of props, something that mentions buffer will be a lot more obvious than having to read through everything and then discover that you can change buffer settings via loadControl. I would also mention in the docs that you need to apply the prop when loading the video, since AFAIK you can't adjust it after the video is already loaded.

@bryanvanwijk
Copy link
Contributor Author

@cobarx Thanks for your feedback, I've made the requested changes.

I'm guessing this was a bug as it doesn't make sense that we would set paused to true if the video is playing, and not paused if it wasn't. I believe this is a safe change since we only release the player when the app is closing or we detach (which is going away shortly). We need this since we release the player in order to apply new bufferConfig settings.
@cobarx
Copy link
Contributor

cobarx commented Aug 3, 2018

Thanks for making the updates! I tested this, made a few tweaks and everything is ready to go. Thanks again for implementing.

@cobarx cobarx merged commit 2b99f0e into TheWidlarzGroup:master Aug 3, 2018
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