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.

Sign up for GitHub

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

Jump to bottom

[video_player_android] Migrate ExoPlayer to ExoPlayer-Media3 1.3.1 #6535

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

[video_player_android] Migrate ExoPlayer to ExoPlayer-Media3 1.3.1 #6535

wants to merge 6 commits into from

Conversation

emakar
Copy link

@emakar emakar commented Apr 15, 2024 •
edited

Resolves #130272

Migrated with https://developer.android.com/media/media3/exoplayer/migration-guide#usingscript

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the [CLA].
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I [linked to at least one issue that this PR fixes] in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].
  • I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style].
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@emakar emakar marked this pull request as ready for review April 15, 2024 11:06
@emakar emakar requested a review from camsim99 as a code owner April 15, 2024 11:06
@emakar emakar force-pushed the video_player_android-media3 branch from e33e5a9 to fc005d7 Compare April 15, 2024 11:43
@emakar emakar marked this pull request as draft April 15, 2024 11:44
@emakar emakar force-pushed the video_player_android-media3 branch from fc005d7 to 76a2837 Compare April 15, 2024 12:21
emakar
emakar commented Apr 15, 2024
View reviewed changes
...r/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java Outdated
@@ -40,6 +42,7 @@
import java.util.List;
import java.util.Map;

@OptIn(markerClass = UnstableApi.class)
Copy link
Author

Choose a reason for hiding this comment

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

androidx/media#503

essentially this just opts in into supporting future breaking changes

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you leave a comment with context from that issue here?

Copy link
Author

Choose a reason for hiding this comment

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

updated, is it okay?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is unclear to me how these APIs are safe to use in the context of the plugin. According to https://developer.android.com/media/media3/exoplayer/migration-guide#unstableapi (emphasis added):

This library follows semantic versioning and the stable API forms the 'public' API for the purposes of the versioning rules. Therefore an API bearing this annotation is exempt from any compatibility guarantees implied by the semantic versioning rules.

If unstable APIs can change in non-major-version updates to media3, then it is definitely not safe to use them in the plugin, because plugin clients would be subject to compilation failure due to minor updates to the underlying library.

Copy link
Author

@emakar emakar Apr 15, 2024 •
edited

Choose a reason for hiding this comment

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

so, no migration to media3 then (at least until there is a stable alternative for such apis?)
or am I missing something?

Copy link
Author

@emakar emakar Apr 16, 2024 •
edited

Choose a reason for hiding this comment

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

Gradle will consider all requested versions, wherever they appear in the dependency graph. Out of these versions, it will select the highest one. More information on version ordering here.

https://docs.gradle.org/current/userguide/dependency_resolution.html

In case of conflict, Gradle by default uses the newest of conflicting versions

https://docs.gradle.org/current/dsl/org.gradle.api.artifacts.ResolutionStrategy.html

Copy link
Contributor

@reidbaker reidbaker Apr 16, 2024 •
edited

Choose a reason for hiding this comment

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

@stuartmorgan I think this comment is saying that gradle considers all dependencies in the scope of the app which means that versions specified in the plugin could be overridden, if there was a higher version, by dependencies the app (or another plugin) has. Which both feels like a gotcha that might exist and also not how build systems should work and is not something I was aware of before.

Copy link
Author

@emakar emakar Apr 18, 2024 •
edited

Choose a reason for hiding this comment

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

@stuartmorgan should I drop the commit with highlighting unstable apis?
what is a path forward?

to me it seems we aren't making thing worse, and this pr migrates from deprecated lib (it also contains workaround for guava 32.1.+ on gradle 6.x, which I can move into separate issue)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's much better to clearly scope the use of unstable APIs as the current commit does, if (as sounds like is the case) we are stuck with using them for now. I would also comment the annotations with a TODO pointing to an issue discussing the issue and the need to track a long term solution.

Based on the discussion above it sounds like:

  • Short term, we are no worse off than before by moving forward with the migration (but @reidbaker and @camsim99 should weigh in as well) in terms of potential for the plugin to stop compiling for clients due to upstream changes.
  • Long term, we need to revisit how we express dependencies in our Android plugins, to see if we can prevent optimistic updates across major versions. I'll file an issue for that.
  • Long term, we need to revisit the use of unstable APIs in this plugin (the TODO).

Copy link
Author

Choose a reason for hiding this comment

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

added TODOs and created flutter/flutter#147039

@emakar emakar marked this pull request as ready for review April 15, 2024 12:45
@emakar emakar mentioned this pull request Apr 15, 2024
Open
camsim99
camsim99 reviewed Apr 15, 2024
View reviewed changes
Copy link
Contributor

@camsim99 camsim99 left a comment

Choose a reason for hiding this comment

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

Looks good!! Left a couple of thoughts.

...r/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java Outdated
@@ -40,6 +42,7 @@
import java.util.List;
import java.util.Map;

@OptIn(markerClass = UnstableApi.class)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you leave a comment with context from that issue here?

packages/video_player/video_player_android/CHANGELOG.md Show resolved Hide resolved
packages/video_player/video_player_android/android/build.gradle Outdated Show resolved Hide resolved
packages/video_player/video_player_android/android/build.gradle Outdated Show resolved Hide resolved
@emakar emakar force-pushed the video_player_android-media3 branch from 76a2837 to 5b0a1cf Compare April 15, 2024 18:19
@emakar emakar changed the title [video_player_android] Migrate ExoPlayer to ExoPlayer-Media3 1.1.1 [video_player_android] Migrate ExoPlayer to ExoPlayer-Media3 1.3.1 Apr 15, 2024
@emakar emakar marked this pull request as draft April 15, 2024 19:05
@emakar emakar force-pushed the video_player_android-media3 branch 5 times, most recently from 222c1a3 to a26491d Compare April 16, 2024 12:30
@emakar emakar force-pushed the video_player_android-media3 branch 4 times, most recently from a6952b6 to eb4fd61 Compare April 16, 2024 17:54
stuartmorgan
stuartmorgan reviewed Apr 18, 2024
View reviewed changes
packages/espresso/android/build.gradle Outdated Show resolved Hide resolved
...r/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java Outdated
@@ -40,6 +42,7 @@
import java.util.List;
import java.util.Map;

@OptIn(markerClass = UnstableApi.class)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's much better to clearly scope the use of unstable APIs as the current commit does, if (as sounds like is the case) we are stuck with using them for now. I would also comment the annotations with a TODO pointing to an issue discussing the issue and the need to track a long term solution.

Based on the discussion above it sounds like:

  • Short term, we are no worse off than before by moving forward with the migration (but @reidbaker and @camsim99 should weigh in as well) in terms of potential for the plugin to stop compiling for clients due to upstream changes.
  • Long term, we need to revisit how we express dependencies in our Android plugins, to see if we can prevent optimistic updates across major versions. I'll file an issue for that.
  • Long term, we need to revisit the use of unstable APIs in this plugin (the TODO).

@stuartmorgan stuartmorgan mentioned this pull request Apr 18, 2024
Open
@emakar emakar mentioned this pull request Apr 19, 2024
Open
@emakar emakar force-pushed the video_player_android-media3 branch from eb4fd61 to 6981edd Compare April 19, 2024 08:27
@emakar emakar marked this pull request as ready for review April 19, 2024 09:46
@emakar emakar force-pushed the video_player_android-media3 branch from 6981edd to 1f6bcc5 Compare April 24, 2024 11:40
@emakar
Copy link
Author

emakar commented Apr 24, 2024

I've dropped controversial workarounds since they aren't needed anymore (all_packages uses gradle 7 now)
This is ready for review, please let me know if anything else is needed

@stuartmorgan
Copy link
Contributor

I've dropped controversial workarounds since they aren't needed anymore (all_packages uses gradle 7 now)

Per these comments I think we should probably hold this until the next stable release if it relies on those changes.

@stuartmorgan stuartmorgan added the waiting for stable update Can't be landed until functionality reaches the stable channel label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuartmorgan stuartmorgan stuartmorgan left review comments

@reidbaker reidbaker reidbaker left review comments

@camsim99 camsim99 camsim99 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
p: video_player platform-android waiting for stable update Can't be landed until functionality reaches the stable channel
Projects
None yet
Milestone
No milestone
4 participants
@emakar @stuartmorgan @reidbaker @camsim99