Implement CronetEngine and HttpEngine support #7
Conversation
new-player/build.gradle.kts
Outdated
There was a problem hiding this comment.
The detailed info on the libs (group, name, version) are placed in the version catalog
There was a problem hiding this comment.
The detailed info on the libs (group, name, version) are placed in the version catalog
now I need to remove them and just keep the important context change as NewPipeTeam will override the get data source obviously.
new-player/build.gradle.kts
Outdated
| implementation(libs.coil.compose) | ||
| implementation(libs.reorderable) | ||
| implementation(libs.androidx.media3.session) | ||
| implementation(libs.androidx.media3.exoplayer.dash) | ||
| implementation(libs.androidx.adaptive.android) | ||
|
|
||
| implementation("org.chromium.net:cronet-embedded:119.6045.31") |
There was a problem hiding this comment.
why cronet-embedded and not cronet?
There was a problem hiding this comment.
Because said by Audric and Stypox but now I need to remove them and just keep the important context change as they will override the get data source obviously
| } else { | ||
| if (!CronetProvider.getAllProviders(context).filter { it.isEnabled }.toList().isEmpty()) { | ||
| val cronetEngine = CronetEngine.Builder(context) | ||
| .enableHttpCache(CronetEngine.Builder.HTTP_CACHE_IN_MEMORY, 10 * 1024 * 1024) |
There was a problem hiding this comment.
Why did you choose exactly this value?
There was a problem hiding this comment.
10 x 1024 x 1024 = 10485760 bytes
which means 10MB so highest 10MB wil be cached in memory
There was a problem hiding this comment.
@uzu09811 can you please put this value into a named constant so its no MagicNumber.
There was a problem hiding this comment.
Ah, after the PR description was added I understood the intention behind the PR.
I like it. @uzu09811 I think you could maybe still add CornetEngine support by creating a meta MediaRepository implementation that only implements getDataSourceFactory(). What do you think?
| } else { | ||
| if (!CronetProvider.getAllProviders(context).filter { it.isEnabled }.toList().isEmpty()) { | ||
| val cronetEngine = CronetEngine.Builder(context) | ||
| .enableHttpCache(CronetEngine.Builder.HTTP_CACHE_IN_MEMORY, 10 * 1024 * 1024) |
There was a problem hiding this comment.
@uzu09811 can you please put this value into a named constant so its no MagicNumber.
There was a problem hiding this comment.
I am taking care of this PR since @uzu09811 contacted me on Matrix. I would say it's better to let apps choose whether to use cronet or not (because e.g. cronet-embedded adds 9mb). So in the end the PR just modifies the API to allow the usage of cronet in apps. @theScrabi you reviewed an old version of the code for some reason (?).
new-player/src/main/java/net/newpipe/newplayer/repository/MediaRepository.kt
Show resolved
Hide resolved
| @@ -114,8 +118,9 @@ interface MediaRepository { | |||
| /** | |||
| * Supply a custom [HttpDataSource.Factory]. This is important for Youtube. | |||
| */ | |||
| fun getHttpDataSourceFactory(item: String): HttpDataSource.Factory = | |||
| DefaultHttpDataSource.Factory() | |||
| fun getHttpDataSourceFactory(item: String, context: Context): DataSource.Factory { | |||
There was a problem hiding this comment.
Is an Android Context now really needed? If so, some doc should be added to explain why that's the case.
There was a problem hiding this comment.
@AudricV
Yes an Android Context is needed when building CronetDataSource, OkhttpDataSource or HttpEngineDataSource
new-player/src/main/java/net/newpipe/newplayer/logic/MediaSourceBuilder.kt
Outdated
Show resolved
Hide resolved
new-player/src/main/java/net/newpipe/newplayer/logic/MediaSourceBuilder.kt
Outdated
Show resolved
Hide resolved
|
As a general point of view, except maybe the context change, for a pure HTTP implementation, I don't think changing anything else is needed, as all ExoPlayer data sources for network stacks implement However, this may allow to use data source with non HTTP origins (like local files), but as I don't really know the NewPlayer structure, I am not certain of this. |
NewPlayer should support playing local files, so does ExoPlayer. However I don't think HttpDataSource is required for playing local files. |
|
DefaultDataSource is needed for local video files because DefaultHttpDataSource will not work for local files |
AudricV
dismissed
their stale review
Outdated + I don't have the capacity to review properly the code
ShareASmile
added
dependency
This modification is just made to allow people people to override other data sources like Okhttp3, Cronet, HttpEngine.