-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Expose DefaultLoadControl parameters Android ExoPlayer #1160
Conversation
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. |
@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(); |
There was a problem hiding this comment.
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.
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. |
@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.
Thanks for making the updates! I tested this, made a few tweaks and everything is ready to go. Thanks again for implementing. |
This makes it possible to adjust the parameters of the DefaultLoadControl in the Android ExoPlayer: minBufferMs, maxBufferMs, bufferForPlaybackMs and bufferForPlaybackAfterRebufferMs.