Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

video_player: use exoplayer for better video compatibility #597

Merged
merged 11 commits into from
Jun 7, 2018

Conversation

jonasbark
Copy link
Contributor

The default Android implementation using the MediaPlayer had different playback / codec problems on different devices. We mostly saw problems with a few Samsung phones.

By using the ExoPlayer library playback works fine on all devices we tested so far.

- remove unused imports
- fix formatting
Copy link
Contributor

@mravn-google mravn-google left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, much appreciated. Can I ask you to address the white space issues? That would make it much easier to review the actual code changes. Thanks!

dependencies {
implementation 'com.google.android.exoplayer:exoplayer-core:2.8.0'
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Please this remove empty line.

implementation 'com.google.android.exoplayer:exoplayer-core:2.8.0'
}

}
Copy link
Contributor

Choose a reason for hiding this comment

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

... and add one at the end of the file.

result.error("VideoError", "IOError when initializing video player " + e.toString(), null);
}
}
private static class VideoPlayer {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reduce indentation to two spaces to make the real diff stand out.

@mravn-google mravn-google self-assigned this Jun 6, 2018
@jonasbark
Copy link
Contributor Author

Done - Readability seems better now.

@mravn-google
Copy link
Contributor

@jonasbark Thanks! The formatter CI job is still complaining about white space. The CONTRIBUTING.md file details how to get the Java formatter running on your machine.

pub global run flutter_plugin_tools format --plugins video_player
Copy link
Contributor

@mravn-google mravn-google left a comment

Choose a reason for hiding this comment

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

LGTM so far. I'll do some on-device testing and see if anything shows up.

@@ -1,3 +1,7 @@
## 0.5.5

* Android: use ExoPlayer instead of MediaPlayer for better video compatibility
Copy link
Contributor

Choose a reason for hiding this comment

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

"better video compatibility" => "better video format support"?

Please end line with a period.

public void onLoadingChanged(final boolean isLoading) {

if (!isLoading) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove empty lines inside methods.

new MediaPlayer.OnCompletionListener() {
@Override
public void onCompletion(MediaPlayer mediaPlayer) {
exoPlayer.addListener(new EventListener() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe subclass DefaultEventListener to avoid the empty methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea - I implemented the changes based on your other suggestions as well and pushed the commit.

@@ -337,6 +320,9 @@ private void onMethodCall(MethodCall call, Result result, long textureId, VideoP
videoPlayers.remove(textureId);
result.success(null);
break;
case "orientation":
Copy link
Contributor

Choose a reason for hiding this comment

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

The Dart side does not call this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, that was a leftover. Will remove it.

exoPlayer.setPlayWhenReady(true);

exoPlayer.addListener(
new EventListener() {
Copy link
Contributor

Choose a reason for hiding this comment

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

In case you missed my earlier comment here (we had a mid-air collision with your white space changes): please consider subclassing DefaultEventListener instead to avoid the empty methods.

- remove setVolume(0.0) call when loading is finished
- adjust changelog
@mravn-google
Copy link
Contributor

The example video takes a long time (order of 30 seconds) to show up, and when it does, it's several seconds into the playback. Are we starting the player correctly?

For the butterfly video, I get one of these on each loop:

I/ACodec  (18171): codec does not support config priority (err -2147483648)

@sroddy
Copy link
Contributor

sroddy commented Jun 6, 2018

@mravn-google I've noticed that the actual code in the example is, I would say, "accidentally", working due to a series of possibly random conditions:

  1. The listener that is registered in the initState is only tracking for play status changed
  2. The _isPlaying property, even if set by the listener in the setState, is not used at all in the build method.
  3. As video loading is performed in the initState method, there is no guarantee that the video is loaded by the time the platform part already updated the content and status of the video to a ready status.
  4. I've noticed that if the above happens, the video is simply not shown until the very first rebuild of the the widget containing the VideoPlayer widget, which is, "accidentally" happening now only because the play status changes and triggers a rebuild due a setState that updates and unused variable

@jonasbark
Copy link
Contributor Author

Indeed - I didn't catch that during testing. I pushed the necessary changes. Initialization should be sent already when the playback state changes to Player.STATE_READY. Previously it was sent only when the video has finished being fully loaded.

Nothing wrong on the Dart side imo.

Regarding the ACodec output. As it's marked as Log.INFO I wouldn't consider this an error.

@sigurdm
Copy link
Contributor

sigurdm commented Jun 6, 2018

@sroddy are you taking about the example in the README.md file?
It is intentional that a change in play-status will trigger a setState to rebuild with the video shown.
Maybe I don't fully understand your description. What would you change to make this work less 'accidentally'?

@sroddy
Copy link
Contributor

sroddy commented Jun 6, 2018

@sigurdm

  1. first fact that sounds strange to me is that you are calling a setState setting the _isPlaying variable in the state that you are not using inside the build function (in the build function you directly use _controller.value.isPlaying instead of that variable).
  2. the second fact is that you are relying on the isPlaying changes to update the widget while I expect that the first frame of the video would be visible right after it has been loaded by the platform. In your example the video appears only when I tap on the play button, while I expected it to appear as soon as it is loaded.

@jonasbark
Copy link
Contributor Author

@sroddy regarding 2.): with the fixed commits the example videos will start right away. No need to tap on a play button.

@@ -2,8 +2,8 @@ name: video_player
description: Flutter plugin for displaying inline video with other Flutter
widgets on Android and iOS.
author: Flutter Team <flutter-dev@googlegroups.com>
version: 0.5.5
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 we should make this a 0.6.0 to allow for patches to the version of the plugin that still uses MediaPlayer.

Map<String, Object> reply = new HashMap<>();
reply.put("textureId", textureEntry.id());
result.success(reply);
}

@SuppressWarnings("deprecation")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no ExoPlayer equivalent of the code below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There actually is. I thought this was just a workaround to ensure proper audio playback on older Android devices. I readded this.

@sigurdm
Copy link
Contributor

sigurdm commented Jun 6, 2018

@sroddy I see your point now! The video will not show the first frame before it actually starts playing.

That should probably be fixed...

@jonasbark
Copy link
Contributor Author

Does anyone have an idea how to approach this?
I'm not even sure if I'm really seeing any missing frames with the example application.

Copy link
Contributor

@mravn-google mravn-google left a comment

Choose a reason for hiding this comment

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

LGTM

Map<String, Object> reply = new HashMap<>();
reply.put("textureId", textureEntry.id());
result.success(reply);
}

@SuppressWarnings("deprecation")
private static void setAudioAttributes(MediaPlayer mediaPlayer) {
private static void setAudioAttributes(SimpleExoPlayer mediaPlayer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

mediaPlayer => exoPlayer

@mravn-google
Copy link
Contributor

@jonasbark The initialization issue is not really related to your changes, and we are working to fix it in a separate PR.

@mravn-google
Copy link
Contributor

@jonasbark It appears we've accrued a merge conflict while waiting for CI to go green. Care to rebase and bump the version number?

…oplayer

# Conflicts:
#	packages/video_player/CHANGELOG.md
#	packages/video_player/pubspec.yaml
@jonasbark
Copy link
Contributor Author

Done

@mravn-google
Copy link
Contributor

@jonasbark Thanks a lot. I'll land on green.

@mravn-google mravn-google merged commit eb17154 into flutter:master Jun 7, 2018
@mravn-google
Copy link
Contributor

Published.

@pyrossh
Copy link

pyrossh commented Jun 9, 2018

@jonasbark Great work. Just FYI .m3u8 format is not supported I've raised an issue for that. Will see if I can tackle it. flutter/flutter#18328
Also it would be great if we can handle/show the error gracefully. Right now it just throws random text into the layout.

@uwejan
Copy link

uwejan commented Sep 14, 2018

Hi, dose it support RTMP playback, it seems not i get the following exception though,

E/ExoPlayerImplInternal(28021): Source error.
E/ExoPlayerImplInternal(28021): com.google.android.exoplayer2.upstream.HttpDataSource$HttpDataSourceException: Unable to connect to 'rtmp url here'
E/ExoPlayerImplInternal(28021): 	at com.google.android.exoplayer2.upstream.DefaultHttpDataSource.open(DefaultHttpDataSource.java:194)
E/ExoPlayerImplInternal(28021): 	at com.google.android.exoplayer2.source.ExtractorMediaPeriod$ExtractingLoadable.load(ExtractorMediaPeriod.java:848)
E/ExoPlayerImplInternal(28021): 	at com.google.android.exoplayer2.upstream.Loader$LoadTask.run(Loader.java:317)
E/ExoPlayerImplInternal(28021): 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1133)
E/ExoPlayerImplInternal(28021): 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:607)
E/ExoPlayerImplInternal(28021): 	at java.lang.Thread.run(Thread.java:762)
E/ExoPlayerImplInternal(28021): Caused by: java.net.MalformedURLException: unknown protocol: rtmp
E/ExoPlayerImplInternal(28021): 	at java.net.URL.<init>(URL.java:596)
E/ExoPlayerImplInternal(28021): 	at java.net.URL.<init>(URL.java:486)
E/ExoPlayerImplInternal(28021): 	at java.net.URL.<init>(URL.java:435)
E/ExoPlayerImplInternal(28021): 	at com.google.android.exoplayer2.upstream.DefaultHttpDataSource.makeConnection(DefaultHttpDataSource.java:341)
E/ExoPlayerImplInternal(28021): 	at com.google.android.exoplayer2.upstream.DefaultHttpDataSource.open(DefaultHttpDataSource.java:192)

@charafau
Copy link
Contributor

@uwejan afaik exoplayer2 supports rtmp via plugin: https://github.com/google/ExoPlayer/tree/dev-v2-r2.8.4/extensions/rtmp

so it's not supported ootb

@pyrossh
Copy link

pyrossh commented Sep 17, 2018

@uwejan I have added m3u8 and dash support in this PR. https://github.com/flutter/plugins/pull/613/files
You can add rtmp by adding this implementation 'com.google.android.exoplayer:extension-rtmp:2.8.0' in the gradle file and checking if it works.

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

Successfully merging this pull request may close these issues.

8 participants