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

Fixes Playing video from asset on Android #3123

Merged
merged 4 commits into from
Oct 28, 2020
Merged

Fixes Playing video from asset on Android #3123

merged 4 commits into from
Oct 28, 2020

Conversation

ponnamkarthik
Copy link
Contributor

Description

Fixes issue with video_player plugins throws error when playing an asset file on android.

Related Issues

flutter/flutter#66627

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@google-cla google-cla bot added the cla: yes label Oct 8, 2020
Ehesp
Ehesp suggested changes Oct 9, 2020
View reviewed changes
Copy link
Member

@Ehesp Ehesp left a comment

Choose a reason for hiding this comment

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

LGTM - please can you run pub global run flutter_plugin_tools format --plugins video_player (pub global activate flutter_plugin_tools).

@Ehesp
Copy link
Member

Ehesp commented Oct 9, 2020

Can confirm this fix works:

image

@ponnamkarthik ponnamkarthik mentioned this pull request Oct 10, 2020
Closed
@Ehesp
Copy link
Member

Ehesp commented Oct 13, 2020

@amirh Would you be able to take a look at this one? It's currently blocking flutter/flutter#66627

@cyanglaz
Copy link
Contributor

@ponnamkarthik Thanks for the fix! This is great! Can we add tests for this?

@cyanglaz
Copy link
Contributor

I notice that the plugin has another issue that the platform exception reporting is broken. Meaning that the error code is always null, which broke the exception reporting. See https://github.com/flutter/plugins/blob/master/packages/video_player/video_player/android/src/main/java/io/flutter/plugins/videoplayer/Messages.java#L600 This is an automatically generated code by pigeon. @gaaclarke do you think we can fix it from pigeon? Or we should just manually fix it for the plugin too?

@gaaclarke
Copy link
Member

I notice that the plugin has another issue that the platform exception reporting is broken. Meaning that the error code is always null, which broke the exception reporting. See https://github.com/flutter/plugins/blob/master/packages/video_player/video_player/android/src/main/java/io/flutter/plugins/videoplayer/Messages.java#L600 This is an automatically generated code by pigeon. @gaaclarke do you think we can fix it from pigeon? Or we should just manually fix it for the plugin too?

We have an issue related to this already here: flutter/flutter#67586 If you could fix it in pigeon that would be helpful. I'll get around to it eventually. Any manual fix will probably get overwritten on accident.

@gaaclarke
Copy link
Member

@cyanglaz I've landed an external contributors fix for this: flutter/flutter#67586 It will publish in 0.1.12 if you can LGTM my PR flutter/packages#224

@cyanglaz
Copy link
Contributor

cyanglaz commented Oct 15, 2020 •
edited
Loading

@ponnamkarthik looks like this PR is missing tests, could you add tests before we can land it?

It's also missing pubspec updates and version updates.

@Ehesp
Copy link
Member

Ehesp commented Oct 15, 2020

@cyanglaz The current tests suggest that it isn't possible: https://github.com/flutter/plugins/blob/master/packages/video_player/video_player/example/test_driver/video_player_test.dart#L15

Can you advise on what's needed to test this PR? Without it working on an emulator I'm not sure what can be done.

xster
xster approved these changes Oct 20, 2020
View reviewed changes
Copy link
Member

@xster xster left a comment

Choose a reason for hiding this comment

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

This is the correct fix.

#3072 didn't refactor correctly to match flutter/engine#20789. LGTM.

...yer/video_player/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayerPlugin.java
@@ -60,6 +60,7 @@ public static void registerWith(io.flutter.plugin.common.PluginRegistry.Registra

@Override
public void onAttachedToEngine(FlutterPluginBinding binding) {

Copy link
Member

Choose a reason for hiding this comment

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

remove extra line

@cyanglaz
Copy link
Contributor

@Ehesp For testing, is it possible to repro the exception with a test like https://github.com/flutter/plugins/blob/master/packages/video_player/video_player/example/integration_test/video_player_test.dart#L32-L41? I also wonder why this particular test didn't fail because of this bug. Looking at the issue description, I felt the test should throw the same exception as it is loading a video from assets.

@Ehesp
Copy link
Member

Ehesp commented Oct 21, 2020

Yeah I'm not sure either. I'll have to investigate it more.

@OmerDital
Copy link

@ponnamkarthik Thanks for the fix!!

@cyanglaz cyanglaz mentioned this pull request Oct 28, 2020
Closed
@cyanglaz cyanglaz merged commit edfaa43 into flutter:master Oct 28, 2020
@cyanglaz cyanglaz mentioned this pull request Oct 28, 2020
Closed
yasargil added a commit to yasargil/plugins that referenced this pull request Oct 30, 2020
@yasargil
Merge branch 'master' into gmain
52dac7c 
* master: (48 commits)
  [video_player] Add toString() to Caption (flutter#3233)
  [google_maps_flutter_web] Show one InfoWindow at a time. (flutter#3224)
  [in_app_purchase] Bump version (flutter#3227)
  [google_maps_flutter] Overhaul lifecycle management in GoogleMapsPlugin (flutter#3213)
  [in_app_purchase] Remove the custom analysis options, fix failing lints. (flutter#3220)
  [video_player]Fixes Playing video from asset on Android (flutter#3123)
  [camera] Added documentation about video not working correctly on Android emulators (flutter#3180)
  Revert "update api"
  update api
  [wifi_info_flutter] Method channel name fixed for android (flutter#3207)
  [share] Fix bug on iPad where `origin` is null and enable XCUITests in the repo (flutter#3210)
  [google_maps_flutter] Clean up google_maps_flutter plugin (flutter#3206)
  Exclude generated_plugin_registrant.cc (flutter#3198)
  broaden the constraint on package:vm_service (flutter#3208)
  Remove unnecessary work around from test in prep for vm_service migration (flutter#3209)
  Add windows directory to examples (flutter#3149)
  [video_player] Upgrade ExoPlayer (flutter#3204)
  [android_alarm_manager] Removed deprecated display1 (flutter#3200)
  [Connectivity] wifi removal (flutter#3173)
  [wifi_info_flutter] make it ready for initial 1.0.0 release  (flutter#3191)
  ...
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
@ponnamkarthik
[video_player]Fixes Playing video from asset on Android (flutter#3123)
7762805
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xster xster xster approved these changes

@Ehesp Ehesp Ehesp approved these changes

@cyanglaz cyanglaz Awaiting requested review from cyanglaz

@iskakaushik iskakaushik Awaiting requested review from iskakaushik

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ponnamkarthik @Ehesp @cyanglaz @gaaclarke @OmerDital @xster