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

[video_player] Fixes video initialization future stall. #2134

Merged
mklim merged 3 commits into from
Jan 6, 2020
Merged

[video_player] Fixes video initialization future stall. #2134

mklim merged 3 commits into from
Jan 6, 2020

Conversation

@slightfoot
Copy link
Member

@slightfoot slightfoot commented Oct 2, 2019

Description

Fixes issue where initialize() Future stalls when failing to load source data and does not throw error.

For example if the network is offline and you try to load a video. The video will attempt to load the source on Android through ExoPlayer. This error of failing to load is reported through the event stream to the client. However, this does not complete the initialization Completer leaving the `Future stalled waiting for the completer to complete which it never will.

@slightfoot slightfoot requested a review from cyanglaz October 3, 2019 13:47
@slightfoot slightfoot mentioned this pull request Oct 6, 2019
Merged
13 tasks
@slightfoot slightfoot force-pushed the video_initialize_error branch from 25d83d4 to 432f4e3 Compare October 29, 2019 20:21
@slightfoot slightfoot force-pushed the video_initialize_error branch 2 times, most recently from 907ccaa to 4530941 Compare November 19, 2019 17:01
@slightfoot slightfoot requested review from amirh and removed request for cyanglaz November 19, 2019 17:02
@slightfoot
Copy link
Member Author

slightfoot commented Nov 19, 2019

@amirh Added tests as requested on discord sometime ago. I also have another fix to go into release on top of this. I'll link the PR to this one and update the CHANGELOG and pubspec in that PR.

@slightfoot
[video_player] Fixes issue where initialize() future stalls when fail…
45836b2 
…ing to load source data and does not throw error.

Fix formatting.
Add tests for errors during init.
@slightfoot slightfoot force-pushed the video_initialize_error branch from 4530941 to 45836b2 Compare November 29, 2019 17:03
mklim
mklim reviewed Dec 9, 2019
View reviewed changes
Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

The code itself LG, but the pubspec.yaml and CHANGELOG.md also need to be updated here.

packages/video_player/video_player/test/video_player_test.dart
Comment on lines +140 to +147
try {
dynamic error;
fakeVideoPlayerPlatform.forceInitError = true;
await controller.initialize().catchError((dynamic e) => error = e);
final PlatformException platformEx = error;
expect(platformEx.code, equals('VideoError'));
} finally {
fakeVideoPlayerPlatform.forceInitError = false;
Copy link
Contributor

@mklim mklim Dec 9, 2019

Choose a reason for hiding this comment

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

nit: Why the try/finally here? Is the catchError not really catching the error?

Copy link
Member Author

@slightfoot slightfoot Dec 30, 2019

Choose a reason for hiding this comment

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

Just used to reset the forceInitError flag to ensure if any other error is thrown during testing the flag is reset so other tests can complete without issue.

@mklim mklim self-assigned this Dec 9, 2019
@slightfoot
Copy link
Member Author

slightfoot commented Dec 30, 2019

@mklim do you want me to update the CHANGELOG and pubspec in this PR or do you want to handle that yourself?

My other work that depends on this needs test cases and other things that I do not have time to complete on. slightfoot@599b6b0

So you might want to merge this ASAP before it gets out of date.

@mklim
Copy link
Contributor

mklim commented Jan 6, 2020

@slightfoot ideally this patch would be ready to land as-is. :) Since you don't have the bandwidth to work on this though I'll get it up to date.

@mklim mklim merged commit f959157 into flutter:master Jan 6, 2020
@mklim
Copy link
Contributor

mklim commented Jan 6, 2020

Thanks again for the contribution!

hjc22 pushed a commit to hjc22/plugins that referenced this pull request Jan 8, 2020
Merge remote-tracking branch 'flutterPlugin/master' into cash-video-c…
df1df41 
…acheing-01-08

* flutterPlugin/master: (30 commits)
  Update Gradle version (flutter#2448)
  [image_picker] support android V2 embedding (flutter#2430)
  [webview_flutter] Setup XCTests (flutter#2445)
  [video_player] Fixes video initialization future stall. (flutter#2134)
  [ci] Upgrade to Xcode 11.3 (flutter#2435)
  [In_app_purchases] migrate to Play Billing Library 2.0. (flutter#2287)
  Migrate away from deprecated `BinaryMessages` (flutter#2444)
  [google_sign_in]Update google_sign_in_example name in pubspec.yaml (flutter#2335)
  [ios_platform_images] Removed android support from the pubspec. (flutter#2432)
  [google_sign_in] Expose network error (flutter#2398)
  [battery] cleanup for Android embedding post 1.12 (flutter#2400)
  [flutter_webview] Raise min Flutter SDK to stable (flutter#2425)
  re-enable stable CI (flutter#2402)
  [in_app_purchase]Change a comment. (flutter#2329)
  [google_sign_in] Pass the client id to the platform interface. (flutter#2427)
  [ios_platform_images] Made ios_platform_images set the correct image scale. (flutter#2414)
  [url_launcher_platform_interface] use non static token for platform interface (flutter#2418)
  [plugin_platform_interface] Don't use const Object as a token (flutter#2417)
  Update endorsed macos plugins readme and update others (flutter#2407)
  [webview_flutter] add gesture navigation for iOS (flutter#2339)
  ...

# Conflicts:
#	packages/video_player/video_player/CHANGELOG.md
#	packages/video_player/video_player/pubspec.yaml
WoodyGuo pushed a commit to liuwei1130/plugins that referenced this pull request Feb 10, 2020
@slightfoot @WoodyGuo
[video_player] Fixes video initialization future stall. (flutter#2134)
be07c6d 
Fixes issue where `initialize()` `Future` stalls when failing to load source data and does not throw error.

For example if the network is offline and you try to load a video. The video will attempt to load the source on Android through ExoPlayer. This error of failing to load is reported through the event stream to the client. However, this does not complete the initialization `Completer` leaving the `Future stalled waiting for the completer to complete which it never will.
WoodyGuo pushed a commit to liuwei1130/plugins that referenced this pull request Feb 10, 2020
@slightfoot @WoodyGuo
[video_player] Fixes video initialization future stall. (flutter#2134)
c72b33b 
Fixes issue where `initialize()` `Future` stalls when failing to load source data and does not throw error.

For example if the network is offline and you try to load a video. The video will attempt to load the source on Android through ExoPlayer. This error of failing to load is reported through the event stream to the client. However, this does not complete the initialization `Completer` leaving the `Future stalled waiting for the completer to complete which it never will.
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
@slightfoot
[video_player] Fixes video initialization future stall. (flutter#2134)
7873890 
Fixes issue where `initialize()` `Future` stalls when failing to load source data and does not throw error.

For example if the network is offline and you try to load a video. The video will attempt to load the source on Android through ExoPlayer. This error of failing to load is reported through the event stream to the client. However, this does not complete the initialization `Completer` leaving the `Future stalled waiting for the completer to complete which it never will.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@amirh amirh Awaiting requested review from amirh

1 more reviewer

@mklim mklim mklim left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@mklim mklim

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@slightfoot @mklim @googlebot