Fixed resuming streams at the last playback position

[absurdlylongusername]

What is it?

• Bugfix (user facing)
• Feature (user facing) ⚠️ Your PR must target the refactor branch
• Codebase improvement (dev facing)
• Meta improvement to the project (dev facing)

Description of the changes in your PR

Reason for the bug is that 1d8ea01 adds a call to useVideoAndSubtitles in the initPlayback overrides in the UI classes. This causes the recovery position for the queue item to be overridden due to the following flow:

• Player.handleIntent is called
• The if (intent.getBooleanExtra(RESUME_PLAYBACK, false) branch is reached
• It gets the playback position of the current queue item from the database via loadStreamState
• newQueue.setRecovery(newQueue.getIndex(), state.getProgressMillis()); sets the recovery position to the value from DB
• Then calls initPlayback(newQueue, playWhenReady);
• initPlayback then calls UIs.call(PlayerUi::initPlayback);, which in turn calls initPlayback on one of BackgroundPlayerUi, PopupPlayerUi, MainPlayerUi
• initPlayback override in these classes calls Player.useVideoAndSubtitles
• useVideoAndSubtitles does setRecovery()
• This setRecovery() however gets the recovery position from simpleExoPlayer.getCurrentPosition() (which is effectively zero since we just started to resume playback), and this overwrites the previous recovery position we just set setRecovery in handleIntent
• onPlaybackSynchronize gets called at some point and seeks to the recovery position (which we just set to 0)
• And thus, it seeks to 0 instead of seeking to the saved playback position

The if conditions I added are supposed to ensure setRecovery is only called when there is no pending recovery (it won't be overwriting a recovery position that's already been set), or when exoplayer is far along in it's setup so that there's stuff to play (timeline is not empty).

I considered doing a proper refactor of this, because I don't really think calling useVideoAndSubtitles from the UIs is the best way to achieve the goal of minimizing bandwidth when playing in the background as it breaks separation of concerns, but I decided it's not worth it and can be tackled in NewPlayer at a later date.

Before/After Screenshots/Screen Record

Maybe later

Fixes the following issue(s)

• Fixes [Regression] Resume playback from last position no longer working #13139

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. You can find more info and a video demonstration on this wiki page.

Due diligence

• I read the contribution guidelines.
• The proposed changes follow the AI policy.
• I tested the changes using an emulator or a physical device.

[TobiGr]

Thank you for looking into it. I just don't quite understand the changes on why the recovery position is set twice in the code.

[TobiGr]

The if conditions I added are supposed to ensure setRecovery is only called when there is no pending recovery (it won't be overwriting a recovery position that's already been set), or when exoplayer is far along in it's setup so that there's stuff to play (timeline is not empty).

Hm, setRecovery() is called anyway here, independently from whether a recovery position was already set or not.. Don't you need to remove the first call? This is at least what I understood from your detailed explanation.

[absurdlylongusername]

Lmao my bad, this a git issue, seems the rebasing I did onto dev didn't actually end up as it showed in Android Studio. Above setRecovery is not supposed to be there x).

I've confirmed this doesn't actually work with the above setRecovery still in place. I've removed it now

[TobiGr]

thanks. I think we should create a hotfix 0.28.4 out of this.

[TobiGr]

/backport release-0.28.3

[github-actions]

Successfully created backport PR for release-0.28.3:

• [Backport release-0.28.3] Fixed resuming streams at the last playback position #13197