Better player error handling #7142

litetex · 2021-09-21T19:34:06Z

What is it?

Bugfix (user facing)
Feature (user facing)
Codebase improvement (dev facing)
Meta improvement to the project (dev facing)

Description of the changes in your PR

This PR massively improves the handling of player errors. It consist of 2 parts:

1. Improving the player error handling

Up to this PR player errors are always just reported as (or similar)
This PR introduces an option (default=off) to report the Error directly (via acra):

NPBetterErrorHandlingDemo.mp4

2. Added an option to simulate player errors

To simulate errors a debug option was introduced to crash the player.

Notes:

This option is only available when using a Debug version of the app (the same as the DebugSettingsFragment)
To make this work in all player variants (main, mini, background, popup) it was added toVideoDetailFragment - otherwise at least sometimes there is no UI where the error can be triggered from.
This is not a true Exoplayer error as it's not possible to crash the player internally (we would have to mess up the streaming data or something similar) therefore Player#onPlayerError is simply called with an exception
When no player is available (because e.g. it was crashed) a toast shows up

Fixes the following issue(s)

Fixes Better handling of ExoPlayer/unrecoverable player errors #7008

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

I read the contribution guidelines.

Stypox · 2021-09-27T05:49:45Z

I was thinking about a different approach for exceptions to report in services. I would create a new static method in the ErrorActivity that creates a notification (iirc two methods already exist for the two other ways to report an error, i.e. by opening the activity directly and by showing the bottom bar). Then when the user taps on the notification the error activity is opened via a PendingIntent.
That method could then be used in more places, e.g. in the downloader service

Stypox · 2021-09-27T05:52:49Z

Having an option to change error reporting mode and showing it to the user seems complicated for them, and also introduces a new setting the user has to know about

litetex · 2021-10-01T16:44:37Z

@Stypox

Should I change anything now here in this PR? Or do you want to do a followup?
I was using what existed and tried to stick with it 😄 (KISS-principle 4tw)

litetex · 2021-10-24T18:44:28Z

Exoplayer sometimes throws exception that are not serializable, e.g.:
#7302 (comment)

W/PlayerErrorHandler: Unable to report error:
    java.lang.RuntimeException: Parcelable encountered IOException writing serializable object (name = com.google.android.exoplayer2.ExoPlaybackException)
        at android.os.Parcel.writeSerializable(Parcel.java:2113)
        at org.schabi.newpipe.error.ErrorInfo.writeToParcel(Unknown Source:38)
        at android.os.Parcel.writeParcelable(Parcel.java:1904)
        at android.os.Parcel.writeValue(Parcel.java:1810)
        at android.os.Parcel.writeArrayMapInternal(Parcel.java:975)
        at android.os.BaseBundle.writeToParcelInner(BaseBundle.java:1620)
        at android.os.Bundle.writeToParcel(Bundle.java:1303)
        at android.os.Parcel.writeBundle(Parcel.java:1044)
        at android.content.Intent.writeToParcel(Intent.java:10855)
        at android.app.IActivityTaskManager$Stub$Proxy.startActivity(IActivityTaskManager.java:3668)
        at android.app.Instrumentation.execStartActivity(Instrumentation.java:1723)
        at android.app.ContextImpl.startActivity(ContextImpl.java:1023)
        at android.app.ContextImpl.startActivity(ContextImpl.java:994)
        at android.content.ContextWrapper.startActivity(ContextWrapper.java:403)
        at android.content.ContextWrapper.startActivity(ContextWrapper.java:403)
        at org.schabi.newpipe.error.ErrorActivity.reportError(ErrorActivity.java:84)
        at org.schabi.newpipe.player.playererror.PlayerErrorHandler.reportError(PlayerErrorHandler.java:69)
        at org.schabi.newpipe.player.playererror.PlayerErrorHandler.showPlayerError(PlayerErrorHandler.java:53)
        at org.schabi.newpipe.player.Player.onPlayerError(Player.java:2533)
        at com.google.android.exoplayer2.ExoPlayerImpl$PlaybackInfoUpdate.lambda$run$3$com-google-android-exoplayer2-ExoPlayerImpl$PlaybackInfoUpdate(ExoPlayerImpl.java:1425)
        at com.google.android.exoplayer2.ExoPlayerImpl$PlaybackInfoUpdate$$ExternalSyntheticLambda10.invokeListener(Unknown Source:2)
        at com.google.android.exoplayer2.BasePlayer$ListenerHolder.invoke(BasePlayer.java:279)
        at com.google.android.exoplayer2.ExoPlayerImpl.invokeAll(ExoPlayerImpl.java:1498)
        at com.google.android.exoplayer2.ExoPlayerImpl.access$100(ExoPlayerImpl.java:56)
        at com.google.android.exoplayer2.ExoPlayerImpl$PlaybackInfoUpdate.run(ExoPlayerImpl.java:1425)
        at com.google.android.exoplayer2.ExoPlayerImpl.notifyListeners(ExoPlayerImpl.java:1328)
        at com.google.android.exoplayer2.ExoPlayerImpl.updatePlaybackInfo(ExoPlayerImpl.java:956)
        at com.google.android.exoplayer2.ExoPlayerImpl.handlePlaybackInfo(ExoPlayerImpl.java:920)
        at com.google.android.exoplayer2.ExoPlayerImpl.lambda$new$0$com-google-android-exoplayer2-ExoPlayerImpl(ExoPlayerImpl.java:162)
        at com.google.android.exoplayer2.ExoPlayerImpl$$ExternalSyntheticLambda5.run(Unknown Source:4)
        at android.os.Handler.handleCallback(Handler.java:938)
        at android.os.Handler.dispatchMessage(Handler.java:99)
        at android.os.Looper.loop(Looper.java:223)
        at android.app.ActivityThread.main(ActivityThread.java:7656)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)
     Caused by: java.io.NotSerializableException: com.google.android.exoplayer2.mediacodec.MediaCodecInfo
        at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1240)
        at java.io.ObjectOutputStream.defaultWriteFields(ObjectOutputStream.java:1604)
        at java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1565)
        at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1488)
        at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1234)
        at java.io.ObjectOutputStream.defaultWriteFields(ObjectOutputStream.java:1604)
        at java.io.ObjectOutputStream.defaultWriteObject(ObjectOutputStream.java:463)
        at java.lang.Throwable.writeObject(Throwable.java:1027)
        at java.lang.reflect.Method.invoke(Native Method)
        at java.io.ObjectStreamClass.invokeWriteObject(ObjectStreamClass.java:1036)
        at java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1552)
        at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1488)
        at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1234)
        at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:354)
        at android.os.Parcel.writeSerializable(Parcel.java:2108)
        	... 36 more

So I guess we have to do some kind of workaround here...

litetex · 2021-10-24T19:10:43Z

Added a workaround by converting the affected exceptions into a serializable ones. Can be tested using #7302

Stypox

Or do you want to do a followup?

Yes, I'll do a followup

app/src/main/res/xml/video_audio_settings.xml

app/src/main/java/org/schabi/newpipe/player/Player.java

app/src/main/java/org/schabi/newpipe/player/playererror/PlayerErrorHandler.java

app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailPlayerCrasher.java

* Added a new setting so that player errors are reported (under Video and Audio > Player) * Moved the player error logic to separate class specially created for this purpose

opusforlife2 · 2021-11-26T17:27:59Z

Again, please don't introduce new settings.

@Stypox I think it would be better to have this kind of discussion here in the main PR comments instead of code review comments. You're not really reviewing "code". You're commenting on a general design/logic decision taken by the author, which can get lost among all the other actual code review comments. (Plus Github notifications can't properly link to the exact comment. You're just taken to the PR page and have to find it yourself, unlike a main PR comment, where you're taken to the actual comment.)

Also, you were kicked by the IRC bot for being idle. Pls rejoin.

litetex · 2021-11-27T13:16:48Z

@Stypox
Everything should now be refactored accordingly.
Could you please review again? :)

Stypox

I think it would be better to have this kind of discussion here in the main PR comments instead of code review comments.

You are right, sorry

I am unable to test whether the crasher works since I keep getting this unrelated exception when I click on the crash button, but before the selection menu shows up:

Exception

User Action: ui error
Request: ACRA report
Content Language: en-GB
Service: none
Version: 0.21.13
OS: Linux Android 10 - 29

Crash log

android.view.WindowManager$BadTokenException: Unable to add window -- token null is not valid; is your activity running?
	at android.view.ViewRootImpl.setView(ViewRootImpl.java:928)
	at android.view.WindowManagerGlobal.addView(WindowManagerGlobal.java:387)
	at android.view.WindowManagerImpl.addView(WindowManagerImpl.java:95)
	at android.app.Dialog.show(Dialog.java:342)
	at org.schabi.newpipe.fragments.detail.VideoDetailPlayerCrasher.onCrashThePlayer(VideoDetailPlayerCrasher.java:133)
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment.lambda$initListeners$1$org-schabi-newpipe-fragments-detail-VideoDetailFragment(VideoDetailFragment.java:660)
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment$$ExternalSyntheticLambda18.onClick(Unknown Source:2)
	at android.view.View.performClick(View.java:7259)
	at android.view.View.performClickInternal(View.java:7236)
	at android.view.View.access$3600(View.java:801)
	at android.view.View$PerformClick.run(View.java:27892)
	at android.os.Handler.handleCallback(Handler.java:883)
	at android.os.Handler.dispatchMessage(Handler.java:100)
	at android.os.Looper.loop(Looper.java:214)
	at android.app.ActivityThread.main(ActivityThread.java:7356)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:491)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:940)

app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailPlayerCrasher.java

litetex · 2021-11-28T12:43:18Z

@Stypox
Should be fixed now: The exception was causes by using the wrong context.

Stypox

Looks good to me, thank you! I will start the followup PR this week :-)

litetex force-pushed the better-player-error-handling branch from 89a9573 to 0accbb4 Compare September 21, 2021 19:39

litetex marked this pull request as ready for review September 21, 2021 19:40

litetex added the player Issues related to any player (main, popup and background) label Sep 21, 2021

litetex mentioned this pull request Sep 24, 2021

Some 4k vertical videos don't play #4175

Closed

AudricV added the feature request Issue is related to a feature in the app label Sep 24, 2021

litetex force-pushed the better-player-error-handling branch from 9fbda16 to 79152e3 Compare October 1, 2021 16:45

TobiGr requested a review from Stypox October 15, 2021 19:50

litetex mentioned this pull request Oct 23, 2021

"Start in full screen" bug: Switching audio -> main player causes "unrecoverable error" on rotation #7302

Closed

4 tasks

litetex force-pushed the better-player-error-handling branch from 79152e3 to e34b2eb Compare October 24, 2021 19:09

Stypox requested changes Nov 23, 2021

View reviewed changes

litetex added 8 commits November 23, 2021 20:12

Added option to report player errors

e632fab

* Added a new setting so that player errors are reported (under Video and Audio > Player) * Moved the player error logic to separate class specially created for this purpose

Added a "Crash the player" debug option

769791a

Added some doc

e5c00a7

Support for debug option "Crash the player" on TVs

c3f1478

PlayerErrorHandler refactor + docs

fb58967

Added a bit more documentation

f18ee8e

Added a workaround for not serializable exceptions

aa28a85

Refactoring

0d51eef

litetex force-pushed the better-player-error-handling branch from beeaba6 to 0d51eef Compare November 23, 2021 19:12

litetex added 2 commits November 23, 2021 20:16

Moved report_player_errors to debug

6992b2c

Fixed build

2f99a21

Stypox requested changes Nov 28, 2021

View reviewed changes

app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailPlayerCrasher.java Outdated Show resolved Hide resolved

Fixed NPE + Problems with context

4c8dca5

Stypox approved these changes Nov 28, 2021

View reviewed changes

Stypox merged commit c7daf32 into TeamNewPipe:dev Nov 28, 2021

litetex deleted the better-player-error-handling branch November 28, 2021 16:04

This was referenced Nov 30, 2021

Release/0.21.14 #7472

Closed

Release 0.21.14 #7473

Closed

Release 0.21.14 #7475

Merged

Stypox mentioned this pull request Dec 2, 2021

Unify error reporting and add error notification #7482

Merged

5 tasks

Uh oh!

Better player error handling #7142

Better player error handling #7142

Uh oh!

Conversation

litetex commented Sep 21, 2021

What is it?

Description of the changes in your PR

1. Improving the player error handling

2. Added an option to simulate player errors

Fixes the following issue(s)

APK testing

Due diligence

Uh oh!

Stypox commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Stypox commented Sep 27, 2021

Uh oh!

litetex commented Oct 1, 2021

Uh oh!

litetex commented Oct 24, 2021

Uh oh!

litetex commented Oct 24, 2021

Uh oh!

Stypox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

opusforlife2 commented Nov 26, 2021

Uh oh!

litetex commented Nov 27, 2021

Uh oh!

Stypox left a comment

Choose a reason for hiding this comment

Exception

Uh oh!

Uh oh!

litetex commented Nov 28, 2021

Uh oh!

Stypox left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Stypox commented Sep 27, 2021 •

edited

Loading