Conversation
| import { VideoAdaptationData, AdaptationConfig } from './adaptationConfig'; | ||
|
|
||
| // Export config types from Adaptation module. | ||
| export { VideoAdaptationData, AdaptationConfig }; |
There was a problem hiding this comment.
Hi @saravanans-github!
To fix the documentation error, and possibly a potential integration error regarding imports, please replace this line here with the suggestion.
| export { VideoAdaptationData, AdaptationConfig }; | |
| export * from './adaptationConfig'; |
The reason is that all exported types should be re-exported on the top-level and now this is not the case.
zigavehovec
left a comment
zigavehovec
left a comment
There was a problem hiding this comment.
Nice work! Mostly nitpicks from me 👍
|
|
||
| ### Changed | ||
|
|
||
| - Android: Refactored AdaptationConfig to enable the `onVideoAdaptation` callback |
There was a problem hiding this comment.
I think this changelog isn't necessary, as the existing api's can be called the same way as before, or am I missing something?
|
|
||
| ### Added | ||
|
|
||
| - Android: Added support for the `onVideoAdaptation` callback |
There was a problem hiding this comment.
What do you think about this?
| - Android: Added support for the `onVideoAdaptation` callback | |
| - Android: Added `AdaptationConfig.videoAdaptation` to customize the player's adaptation logic |
| } | ||
| } | ||
|
|
||
| private fun initConfigBlocks(nativeId: String, config: ReadableMap) { |
There was a problem hiding this comment.
I would just directly call initVideoAdaptationData from initWithConfig. I guess this pattern comes form NetworkModule, where there were multiple init blocks to trigger.
| adaptationConfig.videoAdaptation = VideoAdaptation { data -> | ||
| val future = onVideoAdaptationFromJS(nativeId, data) | ||
| try { | ||
| val callbackValue = future.get(1, TimeUnit.SECONDS) // set timeout to mimize playback performance impact |
There was a problem hiding this comment.
Have you seen problems with the promise taking longer to resolve? Is this really needed, I don't see a reason why this could take more than a second, do we really need this?
If so, we might want to consider if we need this also in other similar APIs like preprocessHttpResponseCallback and preprocessHttpRequestCallback in the NetworkModule - not a part of this PR of course.
| withInt("initialBandwidthEstimateOverride") { initialBandwidthEstimateOverride = it.toLong(); } | ||
| } | ||
|
|
||
| fun ReadableMap.toVideoAdaptationData(): VideoAdaptationData? { |
There was a problem hiding this comment.
This is not used, do is it a leftover or are we missing something?
| * @param requestId Passed through to identify the completion handler of the request on native. | ||
| * @param data The quality according to VideoQuality.id that is returned will be downloaded next. | ||
| */ | ||
|
|
3 participants
Description
Added support for Android onVideoAdaptation callback.
Changes
In a similar design to the NetworkConfig I abstracted the AdaptationConfig to an AdaptationModule to support the callback
Checklist
CHANGELOGentry