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

[av][android] upgrade exoplayer to 2.18.1 #19332

Merged
merged 3 commits into from Oct 5, 2022

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Sep 30, 2022

Why

close ENG-6246

How

  • upgrade exoplayer to 2.18.1
  • migrate breaking changes
  • kotlinize packages/expo-av/android/src/main/java/expo/modules/av/player/datasource/CustomHeadersOkHttpDataSourceFactory.java
  • remove android/expoview/src/main/java/versioned/host/exp/exponent/modules/universal/av/CustomHeadersOkHttpDataSourceFactory.kt which is exactly the same to the CustomHeadersOkHttpDataSourceFactory inside expo-av package.

Test Plan

  • android unversioned expo go + NCL Video

Checklist

@linear
Copy link

linear bot commented Sep 30, 2022

ENG-6246 Upgrade exoplayer in expo-av

There is an incompatible exoplayer issue with react-native-track-player where it bump the exoplayer to 2.18.1 internally. The exoplayer 2.18 also introduced some breaking changes. So we should upgrade the exoplayer version as well as some code in expo-av.

Context: #18937

@expo-bot expo-bot added bot: suggestions ExpoBot has some suggestions bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Sep 30, 2022
@Kudo Kudo marked this pull request as ready for review October 3, 2022 12:15
@Kudo Kudo requested a review from barthap as a code owner October 3, 2022 12:15
@Kudo Kudo requested a review from lukmccall October 3, 2022 12:15
@mnightingale
Copy link
Contributor

Not sure if its worth doing but the C.TYPE_* => C.CONTENT_TYPE_* would clear some deprecation warnings.

Also should that switch handle C.CONTENT_TYPE_RTSP it was added in 2.14.0, I'm thinking no since it would be a new feature and I don't know what iOS/web would do.

Unrelated to this but is there any disadvantage that this package uses an older version of okhttp compared to the few other packages that use it? Does it increase binary size or anything?

@Kudo
Copy link
Contributor Author

Kudo commented Oct 5, 2022

Not sure if its worth doing but the C.TYPE_* => C.CONTENT_TYPE_* would clear some deprecation warnings.

Also should that switch handle C.CONTENT_TYPE_RTSP it was added in 2.14.0, I'm thinking no since it would be a new feature and I don't know what iOS/web would do.

Unrelated to this but is there any disadvantage that this package uses an older version of okhttp compared to the few other packages that use it? Does it increase binary size or anything?

i'm more to have these as a new task. there're many deprecation warnings. i would prefer to do it together when i get more familiar with exoplayer and expo-av code. in the meantime, welcome for contributions. if you could send us a pr, that's much appreciated.

for okhttp, gradle will only have single okhttp version, so it will use the newer compatible version by default. if i get a chance, i could create a pr to remove the okhttp deprecations first.

@Kudo Kudo merged commit 26276ba into main Oct 5, 2022
@Kudo Kudo deleted the @kudo/eng-6246-upgrade-exoplayer-in-expo-av branch October 5, 2022 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants