Skip to content

[video_player_android] Handle BehindLiveWindowException #5869

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

Merged

Conversation

SynSzakala
Copy link
Contributor

@SynSzakala SynSzakala commented Jan 11, 2024

This PR adds error handling for BehindLiveWindowException when playing video streams as recommended in ExoPlayer docs

Fixes flutter/flutter#100478

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 listed 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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@SynSzakala SynSzakala marked this pull request as ready for review January 11, 2024 17:34
camsim99
camsim99 previously approved these changes Jan 22, 2024
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! Just left some nits and a question about forwarding the error code to Dart.

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

* Adds error handling for `BehindLiveWindowException`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
* Adds error handling for `BehindLiveWindowException`
* Adds error handling for `BehindLiveWindowException`, which may occur upon video playback failure.

// See https://exoplayer.dev/live-streaming.html#behindlivewindowexception-and-error_code_behind_live_window
exoPlayer.seekToDefaultPosition();
exoPlayer.prepare();
} else if (eventSink != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we not still want to forward the video error to Dart?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think not, since it's basically un-actionable from Dart. It could make sense if there was an option to disable this error handling logic from Dart, but it would require a change in platform interface - this seems to be an overkill for this fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Longer term it would make more sense to have no handling here, and forward more structured errors to the clients so they could decide how to handle them, but we can start with this and see if there are requests for more control.

@@ -278,4 +281,28 @@ public void onIsPlayingChangedSendsExpectedEvent() {
assertEquals(event2.get("event"), "isPlayingStateUpdate");
assertEquals(event2.get("isPlaying"), false);
}

@Test
public void behindLiveWindowErrorResets() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can this be slightly more descriptive to describe what we expect? Something like:

Suggested change
public void behindLiveWindowErrorResets() {
public void behindLiveWindowErrorResetsPlaybackPositionToDefault() {

@camsim99 camsim99 dismissed their stale review January 22, 2024 22:31

Meant to comment

…g_exception_fix

# Conflicts:
#	packages/video_player/video_player_android/CHANGELOG.md
…_fix' into video_player_streaming_exception_fix
@SynSzakala SynSzakala requested a review from camsim99 February 1, 2024 15:53
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.

LGTM!

@SynSzakala
Copy link
Contributor Author

@camsim99 Is there any further action needed from my side? Or you'll just need to add the autosubmit label?

@camsim99
Copy link
Contributor

camsim99 commented Feb 8, 2024

@camsim99 Is there any further action needed from my side? Or you'll just need to add the autosubmit label?

Ah sorry I didn't realize I needed to add it!

@camsim99 camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 8, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 8, 2024
Copy link
Contributor

auto-submit bot commented Feb 8, 2024

auto label is removed for flutter/packages/5869, due to This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

  • Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM

// See https://exoplayer.dev/live-streaming.html#behindlivewindowexception-and-error_code_behind_live_window
exoPlayer.seekToDefaultPosition();
exoPlayer.prepare();
} else if (eventSink != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Longer term it would make more sense to have no handling here, and forward more structured errors to the clients so they could decide how to handle them, but we can start with this and see if there are requests for more control.

@stuartmorgan-g stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 12, 2024
@auto-submit auto-submit bot merged commit 2424147 into flutter:main Feb 12, 2024
@SynSzakala SynSzakala deleted the video_player_streaming_exception_fix branch February 13, 2024 14:40
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 13, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 13, 2024
flutter/packages@0a69259...9385bbb

2024-02-13 [email protected] Convert startProductRequest(), finishTransaction(), restoreTransactions(), presentCodeRedemptionSheet() to pigeon (flutter/packages#6032)
2024-02-13 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump org.json:json from 20231013 to 20240205 in /packages/in_app_purchase/in_app_purchase/example/android/app (flutter/packages#6096)
2024-02-12 [email protected] [local_auth] Rename iOS classes (flutter/packages#6108)
2024-02-12 [email protected] [video_player_android] Handle BehindLiveWindowException (flutter/packages#5869)
2024-02-12 [email protected] [in_app_purchase] Add alternative billing apis for android (flutter/packages#6056)
2024-02-12 [email protected] [webview_flutter] Update compileSdk to 34 (flutter/packages#6106)
2024-02-12 [email protected] [cupertino_icons] Add example to cupertino icons (flutter/packages#5312)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: video_player platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[video_player] BehindLiveWindowException
3 participants