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

Refactor exoplayer deprecations #1653

Conversation

CHaNGeTe
Copy link
Contributor

@CHaNGeTe CHaNGeTe commented Jul 6, 2019

@CHaNGeTe CHaNGeTe added feature help wanted The issue has been reviewed and is valid, and is waiting for someone to work on it. Platform: Android ExoPlayer labels Jul 6, 2019
@CHaNGeTe CHaNGeTe self-assigned this Jul 6, 2019
@cobarx
Copy link
Contributor

cobarx commented Jul 8, 2019

@danielmarino24i Man, it's really nice to see these deprecated functions going away. Nice work.

I'm not sure what the easiest way to send you the code recommendations, so I'm pasting them inline:

SingleSampleMediaSource

        return new SingleSampleMediaSource.Factory(mediaDataSourceFactory)
            .createMediaSource(uri, textFormat, C.TIME_UNSET);

I tested this with some sideloaded subs and this code is working fine.

AdaptiveTrackSelection.Factory

TrackSelection.Factory videoTrackSelectionFactory = new AdaptiveTrackSelection.Factory();

I don't believe the bandwidth meter is doing anything particularly important.

@CHaNGeTe
Copy link
Contributor Author

CHaNGeTe commented Jul 9, 2019

Added your suggestions, bandwidth meter is now configured on the exoplayer instance

@CHaNGeTe
Copy link
Contributor Author

CHaNGeTe commented Jul 9, 2019

Seems, if I didn't miss any, that all deprecations are gone now

@CHaNGeTe CHaNGeTe changed the title [WIP] Refactor exoplayer deprecations Refactor exoplayer deprecations Jul 9, 2019
Daniel Mariño added 2 commits July 12, 2019 10:17
# Conflicts:
#	android-exoplayer/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java
@CHaNGeTe CHaNGeTe added needs review and removed help wanted The issue has been reviewed and is valid, and is waiting for someone to work on it. labels Jul 19, 2019
@kristerkari
Copy link

@CHaNGeTe Are you planning on updating to a newer version of ExoPlayer, and if so, is there going to be another 4.x release?

I'm just thinking that updating to a newer ExoPlayer version might fix the crash that I have reported earlier: #1633

@CHaNGeTe
Copy link
Contributor Author

CHaNGeTe commented Aug 1, 2019

@kristerkari I am doing tests with com.google.android.exoplayer:exoplayer:2.10.3

But as it has AndroidX support, and they have migrated to androidx.compat, seems it will be released as 5.x

@kristerkari
Copy link

kristerkari commented Aug 1, 2019

Ok, thanks for the info! Then we have to get React Native updated to the latest version first before being able to fix the crash.

@CHaNGeTe
Copy link
Contributor Author

CHaNGeTe commented Aug 1, 2019

I think update of RN is not required to test it.

I mean, for real AndroidX support I think you have to, but to compile the app with libraries with androidX support you don't need to migrate to RN >=0.60

@kristerkari
Copy link

I think update of RN is not required to test it.

I mean, for real AndroidX support I think you have to, but to compile the app with libraries with androidX support you don't need to migrate to RN >=0.60

Oh that's great to hear, I was under the impression that updating to 5.x requires RN 0.60

@CHaNGeTe
Copy link
Contributor Author

CHaNGeTe commented Aug 1, 2019

I think it is recommended, but I am testing with "react-native": "0.59.5",

DefaultLoadControl defaultLoadControl = defaultLoadControlBuilder.createDefaultLoadControl();
DefaultRenderersFactory renderersFactory = new DefaultRenderersFactory(getContext(), DefaultRenderersFactory.EXTENSION_RENDERER_MODE_OFF);
// TODO: Add drmSessionManager to 5th param from: https://github.com/react-native-community/react-native-video/pull/1445
player = ExoPlayerFactory.newSimpleInstance(getContext(), renderersFactory, trackSelector, defaultLoadControl, null, BANDWIDTH_METER);
Copy link
Collaborator

@benoitdion benoitdion Aug 6, 2019

Choose a reason for hiding this comment

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

we should also update how the bandwidth meter is created. Looks like new DefaultBandwidthMeter() misses a lot of the smart defaults we get when using the builder that takes a context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://medium.com/google-exoplayer/https-medium-com-google-exoplayer-simplified-bandwidth-meter-usage-17d8189f978b

Sth like this?

SimpleExoPlayer player = ExoPlayerFactory.newSimpleInstance(context);

DataSource.Factory dataSourceFactory = 
    new DefaultDataSourceFactory(context, userAgent);
MediaSource mediaSource = 
    new DashMediaSource.Factory(dataSourceFactory)
        .createMediaSource(uri);

player.prepare(mediaSource);

Copy link
Collaborator

Choose a reason for hiding this comment

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

That looks right although it'd be nice to reuse the bandwidth meter between plays. I believe that's why it's currently a static variable.

Maybe outside the scope of this PR but what do you think about creating a ReactVideoPackageConfig class that exposes the exoplayer configuration. There would be a default implementation that uses the exo defaults and each application could override the config by passing a config object when instantiating ReactVideoPackage.

@benoitdion
Copy link
Collaborator

This was done in #1753! We'll release the change in a 5.1-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants