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] Bump com.google.android.exoplayer:exoplayer from 2.9.2 to 2.13.3 #16123

Merged
merged 9 commits into from
Mar 2, 2022

Conversation

bbarthec
Copy link
Contributor

@bbarthec bbarthec commented Jan 28, 2022

Why

Resolves ENG-2858
Continues #16122

How

  • Removed https://github.com/yqritc/Android-ScalableVideoView by copying the parts into the expo-av codebase as it's only available on jcenter()
  • Bumped the exoplayer to the smallest version available on google maven.
  • Tweaked the codebase to work with the new version of exoplayer.

Test Plan

I've succeeded in compiling bare-expo and running the ncl#video screen with both remote and local videos playing nicely.

TODO

  • Ensure Expo Go works
  • Backport changes if necessary

@bbarthec bbarthec changed the title [av][Android] Bump 'com.google.android.exoplayer:exoplayer from 2.9.2 to 2.13.3' [av][Android] Bump com.google.android.exoplayer:exoplayer from 2.9.2 to 2.13.3 Jan 28, 2022
@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Jan 28, 2022
android/expoview/build.gradle Outdated Show resolved Hide resolved
Copy link
Contributor

@Kudo Kudo left a comment

Choose a reason for hiding this comment

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

LGTM. i think android client test error is from expo go, maybe you will update in this pr or another pr.

@mnightingale
Copy link
Contributor

I've not checked but looking at the release notes for ExoPlayer this possibly resolves a few open av issues (#16139 and maybe #16439).

Is there any reason not to go for a more recent ExoPlayer version (latest is currently 2.17.0, about 10 months more recent than 2.13.3), this PR already makes pretty much all of the required API changes from the last few years so it's not much more work.

A few changes would be required:

  • buildMediaSource handle TYPE_RTSP
  • A few renamed classes or constants
  • Likely upgrade okhttp + okhttp-urlconnection so it uses the same version as ExoPlayer
  • A few changes to SharedCookiesDataSourceFactory and CustomHeadersOkHttpDataSourceFactory - might also be able to simplify those using okhttp interceptor, else I think apis are exposed so setDefaultRequestProperties can be used

There are also usages of a few deprecated methods that could be cleaned up.

@bbarthec
Copy link
Contributor Author

bbarthec commented Mar 1, 2022

Is there any reason not to go for a more recent ExoPlayer version (latest is currently 2.17.0, about 10 months more recent than 2.13.3), this PR already makes pretty much all of the required API changes from the last few years so it's not much more work.

Hey @mnightingale, I tried to perform as small version bump as possible to minimise the scope of possible regressions 📉. If everything goes well with this version bump, we'll be bumping again to reach the most up-to-date version 💪
I'm resuming work on this one right now, so I hope it'll land very soon 🤓

…on `2.13.3`

- Bump `com.google.android.exoplayer:exoplayer` and `com.google.android.exoplayer:exoplayer-okhttp` from `2.9.2` to `2.13.3`
- Remove unused `com.android.support:support-annotations` dependecy
- Remove unused `supportLibVersion` variable from gradle
- Remove `jcenter` repository from `expoview` project
…view`

- `com.yqritc:android-scalablevideoview` is only available on `jcenter` repository and we're removing it, so I've copy-pasted classes that we rely on in `expo-av`
- Replace `ExtractorMediaSource.EventListener` with `MediaSourceEventListener`
- Replace `SimpleExoPlayer.VideoListener` with `VideoListener`
- Replace `AdaptiveMediaSourceEventListener` with `MediaSourceEventListener`
- Refactor `mSimpleExoPlayer` construction
- Adapt creating `MediaSource`
- Cleanup the codebase
@bbarthec bbarthec force-pushed the @bbarthec/av/migrate-from-jcenter branch from 06996e6 to ebe3a97 Compare March 1, 2022 21:27
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Mar 1, 2022
@bbarthec bbarthec merged commit b33900c into main Mar 2, 2022
@bbarthec bbarthec deleted the @bbarthec/av/migrate-from-jcenter branch March 2, 2022 19:09
@dantxal
Copy link

dantxal commented Mar 2, 2022

Hello bbarthec!!

I've been trying to test your solution, I tried to build by linking the dependencies of my repo to your branch on github, but that didn't work.

Now I see that you merged it, could you give me the instructions to get it running on my repo?

This is the repo I've been trying to test it on: dantxal/ExpoAvIssue.

This is the issue I talked about, for reference: #16323

Thanks for the good work you've been doing on expo!

EDIT: This is the error I am getting when linking to your commit:

A problem occurred evaluating project ':expo-av'.
> Project :ReactAndroid not found.

@bbarthec
Copy link
Contributor Author

bbarthec commented Mar 3, 2022

@dantxal, this problem you're reporting is very generic and I have no way to help you with it 😞
I'm going to publish this change to the npm with the next tag and you'll be able to test it that way 👀

@bbarthec
Copy link
Contributor Author

bbarthec commented Mar 3, 2022

I've just published expo-av@11.0.0 with the next tag on npm containing this PR and many more (see CHANGELOG) 🎉
Please try it and feel free to provide any kind of feedback you want (including new issues if something is broken in this new version out of the blue) 😁

@dantxal
Copy link

dantxal commented Mar 3, 2022

@bbarthec You're right, it didn't solve my problem. Thank you for the quick response btw! 😄

To anyone else trying to run expo-av@11.0.0:

You might run into this error on expo package:

Module was compiled with an incompatible version of Kotlin.

How to solve it

  1. Navigate into expo package within node_modules (relative path: node_modules/expo/android/build.gradle);
  2. Change line 24 from
    classpath("org.jetbrains.kotlin:kotlin-gradle-plugin:${safeExtGet('kotlinVersion', '1.4.1')}")

into

    classpath("org.jetbrains.kotlin:kotlin-gradle-plugin:${safeExtGet('kotlinVersion', '1.6.0')}")
  1. Run yarn android again. ( I ran cd android && ./gradlew clean && cd .. before that, just to be safe) .

@eeshankeni
Copy link

Please consider implementing #16455 to expo-av 11 as well

@hirbod
Copy link
Contributor

hirbod commented Mar 7, 2022

So far, expo-av 11 is working fine for me on Android. What still is so extremely annoying is the massive pauseAsync delay on iOS and the framedrops caused by it (from 60 to 20). Android does not have this issues.

@KrisLau
Copy link

KrisLau commented Sep 2, 2022

@bbarthec @Kudo Thanks for all the awesome work! I've been working on a fatal error I've been getting on my project and realized that the exoplayer version being too low is causing a fatal error when used with react-native-track-player which uses v2.18.1. Are there any plans to upgrade to that version any time soon or ado you guys have a workaround for that?

Issue with repro: #18937

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

9 participants