Fix surface view not resizing video correctly + other player fixes

[Stypox]

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

The surface view is the view that contains the playing video, on which exoplayer draws. Its height was never being initialized, leaving it to 0, so the view did not have enough information to respect the video aspect ratio (which instead was being set correctly), leading to #8170 (comment). The height was never being set because the code tried to set it when the surface view hadn't been initialized yet. Now the height-setup is posted to a Handler, this way the player initialization completes (creating the surface view) and then the height is initialized.

This PR should also fix #8678 (comment), since now all null checks are done inside the lambda posted to the main thread loop. See the comments.

Before/After Screenshots/Screen Record

Before	After

Fixes the following issue(s)

Fixes #8170 (comment)
Also fixes #8678 (comment)

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.

[PJansky]

This pull request definitely fixes #8170 (comment), but has introduced a regression in the screen rotation animation. This regression is new to this pull and not previously seen in #8170. As you can see in the videos below, the animation now has some artifacts while rotating and briefly fades to black before showing the video. As a result the animation looks choppy and the fade to black before showing the video is perceptible as an unpleasant flash.

Animation after #8731:

After8731.mp4

Animation before:

Before8731.mp4

[opusforlife2]

Crash. Same STR as #8678 (comment), but different error, interestingly:

Exception

• User Action: ui error
• Request: ACRA report
• Content Country: CH
• Content Language: en-IN
• App Language: en_IN
• Service: none
• Version: 0.23.1
• OS: Linux Android 12 - 32

Crash log

java.lang.IllegalArgumentException: STATE_SETTLING should not be set externally.
	at com.google.android.material.bottomsheet.BottomSheetBehavior.setState(BottomSheetBehavior.java:1200)
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment.setupBottomPlayer(VideoDetailFragment.java:2285)
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment.initListeners(VideoDetailFragment.java:694)
	at org.schabi.newpipe.BaseFragment.onViewCreated(BaseFragment.java:68)
	at org.schabi.newpipe.fragments.BaseStateFragment.onViewCreated(BaseStateFragment.java:42)
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment.onViewCreated(VideoDetailFragment.java:607)
	at androidx.fragment.app.Fragment.performViewCreated(Fragment.java:3019)
	at androidx.fragment.app.FragmentStateManager.createView(FragmentStateManager.java:551)
	at androidx.fragment.app.FragmentStateManager.moveToExpectedState(FragmentStateManager.java:261)
	at androidx.fragment.app.FragmentStore.moveToExpectedState(FragmentStore.java:113)
	at androidx.fragment.app.FragmentManager.moveToState(FragmentManager.java:1374)
	at androidx.fragment.app.FragmentManager.dispatchStateChange(FragmentManager.java:2841)
	at androidx.fragment.app.FragmentManager.dispatchActivityCreated(FragmentManager.java:2784)
	at androidx.fragment.app.FragmentController.dispatchActivityCreated(FragmentController.java:262)
	at androidx.fragment.app.FragmentActivity.onStart(FragmentActivity.java:478)
	at androidx.appcompat.app.AppCompatActivity.onStart(AppCompatActivity.java:246)
	at android.app.Instrumentation.callActivityOnStart(Instrumentation.java:1471)
	at android.app.Activity.performStart(Activity.java:8082)
	at android.app.ActivityThread.handleStartActivity(ActivityThread.java:3732)
	at android.app.servertransaction.TransactionExecutor.performLifecycleSequence(TransactionExecutor.java:221)
	at android.app.servertransaction.TransactionExecutor.cycleToPath(TransactionExecutor.java:201)
	at android.app.servertransaction.TransactionExecutor.executeLifecycleState(TransactionExecutor.java:173)
	at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:97)
	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2253)
	at android.os.Handler.dispatchMessage(Handler.java:106)
	at android.os.Looper.loopOnce(Looper.java:201)
	at android.os.Looper.loop(Looper.java:288)
	at android.app.ActivityThread.main(ActivityThread.java:7870)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003)

---

[opusforlife2]

Also seeing the choppy animation PJanksy mentions above.

[opusforlife2]

Pasting a mysterious crash report here as suggested by Stypox: Edit by @Stypox: actually it's the same as #8731 (comment) ;-)

[Stypox]

I pushed 8b7c785 to fix the choppy animation, can you confirm that it is fixed @PJansky @opusforlife2?

For comparison, here is a video showing the behavior before fixing (5e094ab), the behavior in v0.23.2 and the behavior after the fix (8b7c785). I couldn't reproduce better than in the video on the left 😅.

[Stypox]

Originally posted by @opusforlife2 in #8678 (comment)

---

Which problems does your phone have with that little piece of code? 😂

Here's possibly another. <3

Rough STR (might not work): Opened a video, it didn't load. Realised internet was off. Turned it on. Tried to close and reopen the video details screen. <- crash occurred somewhere here. Happened again. Not network-related. Actual STR:

• Have "Start in full screen" toggled on, Autoplay toggled off, and rotation locked to portrait. (Not sure if needed, but that's my setup.)
• Open a video details page.
• ASAP, before the video details can load, tap the thumbnail area. The player will go into full screen. (BTW, can it be made so that the "thumbnail area" cannot be tapped until the thumbnail itself has loaded, along with video details? I think that would fix a lot of bugs.)
• Before it can start playing (while it is still fetching the streams), tap Back ASAP to rotate back to portrait, and then close the minimised player.

Tested on 2 devices, so fully reproducible. Doesn't occur on 0.23.1.

Exception

• User Action: ui error
• Request: ACRA report
• Content Country: CH
• Content Language: en-IN
• App Language: en_IN
• Service: none
• Version: 0.23.1
• OS: Linux Android 12 - 32

Crash log

android.app.RemoteServiceException$ForegroundServiceDidNotStartInTimeException: Context.startForegroundService() did not then call Service.startForeground(): ServiceRecord{2fc9070 u0 org.schabi.newpipe.debug.mediasessionui/org.schabi.newpipe.player.PlayerService}
	at android.app.ActivityThread.generateForegroundServiceDidNotStartInTimeException(ActivityThread.java:1965)
	at android.app.ActivityThread.throwRemoteServiceException(ActivityThread.java:1936)
	at android.app.ActivityThread.access$2700(ActivityThread.java:256)
	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2190)
	at android.os.Handler.dispatchMessage(Handler.java:106)
	at android.os.Looper.loopOnce(Looper.java:201)
	at android.os.Looper.loop(Looper.java:288)
	at android.app.ActivityThread.main(ActivityThread.java:7870)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003)
Caused by: android.app.StackTrace: Last startServiceCommon() call for this service was made here
	at android.app.ContextImpl.startServiceCommon(ContextImpl.java:1877)
	at android.app.ContextImpl.startForegroundService(ContextImpl.java:1832)
	at android.content.ContextWrapper.startForegroundService(ContextWrapper.java:781)
	at androidx.core.content.ContextCompat$Api26Impl.startForegroundService(ContextCompat.java:931)
	at androidx.core.content.ContextCompat.startForegroundService(ContextCompat.java:703)
	at org.schabi.newpipe.player.helper.PlayerHolder.startService(PlayerHolder.java:126)
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment.openMainPlayer(VideoDetailFragment.java:1217)
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment.$r8$lambda$7cA0nfOW4EXxLpA03fs92UQohwM(Unknown Source:0)
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment$$ExternalSyntheticLambda13.run(Unknown Source:2)
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment.replaceQueueIfUserConfirms(VideoDetailFragment.java:2130)
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment.openVideoPlayer(VideoDetailFragment.java:1184)
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment.openVideoPlayerAutoFullscreen(VideoDetailFragment.java:1197)
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment.onClick(VideoDetailFragment.java:511)
	at android.view.View.performClick(View.java:7455)
	at android.view.View.performClickInternal(View.java:7432)
	at android.view.View.access$3700(View.java:835)
	at android.view.View$PerformClick.run(View.java:28810)
	at android.os.Handler.handleCallback(Handler.java:938)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	... 6 more

[Stypox]

@opusforlife2 actually, the bug reported in #8731 (comment) (I moved the comment here) has an even simpler STR: just open a video and tap on the thumbnail before the video details can load. Anyway, I fixed it as you said, i.e. by ignoring any click on the thumbnail when nothing has loaded yet.

I tried to go deeper and understand the causes of the problem, and actually understood them, but they depend on the value of playAfterConnect received here the second time the player tries to start: in the case reported it is false, so openVideoPlayerAutoFullscreen is not called when the service is connected, which implies no intent with a queue is ever sent to the player, so startForeground is not called. This will not be fixed by this PR but rather in #7673.

[opusforlife2]

actually it's the same as 8731 (comment) ;-)

Oh wow you found it! So my past self did report the crash in time, and is not as silly as I thought.

This will not be fixed by this PR but rather in 7673.

Okay, so then this PR is only blocked on the choppy animation problem. I'll test that just now.

Edit: I just realised this build is pre 0.23.3-hotfix code. I'll wait for an updated build, then.

[Stypox]

Triggered a new build @opusforlife2

[opusforlife2]

Triggered a new build

Eh? Don't you need to rebase to include the hotfixed code?

Edit: The APK didn't work. I've rerun the APK build manually.

Edit 2: The rebuilt APK doesn't work either. The version is still showing as 0.23.2 too. Since master has been merged back into dev, shouldn't it show 0.23.3?

Edit 3: I give up. As a last resort, I tried rerunning all the jobs, wildly guessing that some sort of cache might be in need of clearing or something. Nope. No luck.

[Stypox]

I'm stupid, sorry, now I have rebased

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

0.0% Coverage
0.0% Duplication

[opusforlife2]

Okay, so, if compared with the current release, there is a noticeable lag between the rotations. The choppiness is gone. But the black screen remains for longer than on the current release. However, since users will only be rotating roughly once per video and won't be comparing builds like I am, you could consider this a non-issue.

Edit: Oh wait, this might just be debug build vs release build jank.

[Stypox]

I am merging this PR even though it didn't receive reviews yet because it needs to be included in the release, and also because both me and opus tested and it seems to work. @litetex you can always review later ;-)

[opusforlife2]

There's still time while the RC bakes, after all.