Limit speed at live edge #8134

KosOrKosm · 2022-04-02T23:26:33Z

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

Upon reaching the live edge of a livestream, if playback speed is greater than 100%, set it to 100%.
The maximum playback speed that can be set in the PlaybackParameterDialog is reduced to 100% when on the live edge.
The PlaybackParameterDialog skip silence button is also hidden on the live edge.

Note it is still possible to use playbacks speeds <= 100% at all times on livestreams, and playback speeds > 100% when not on the live edge.

Changes tested on emulator (Pixel 3a w/ API 28).

Before/After Screenshots

Before:	After:

Fixes the following issue(s)

Fixes Livestreams should not have playback parameters (speed adjustment or skip silence) #7667

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.

AudricV · 2022-04-03T09:13:52Z

Thank you for the pull request!

I know I am bit against these changes, but what do you think of setting only the playback speed to 1x by default only on livestreams? This will allow users to watch streams from the start of the live window up to the maximum speed supported in the app (see what I wrote in #7667 (comment) for more details).

So if users increased playback speed and they have issues when using it at the live edge, then they can understand the issue is from them, not from us.

KosOrKosm · 2022-04-03T15:52:31Z

@TiA4f8R Thank you for the feedback!

I was considering setting the default playback speed to 1x for livestreams, but I thought this solution might be less intrusive. This approach allows the user to speed up in the live window, just not in the live edge. In my testing, Youtube livestreams had an ~30 second live window, and only ~5 seconds of that window was the live edge. So only those last ~5 seconds cannot be sped up, and the rest of the window can still be freely controlled. I think this allows the user the most freedom without causing an unpleasant experience due to buffering at the live edge.

In terms of notifying the user, I wasn't sure if any notification was necessary. It could be surprising for the user that the speed resets at the live edge, but I also think it might be clear why it occurs. Perhaps a popup message could appear when the speed is reset to explain why it is happening?

AudricV · 2022-04-03T16:29:42Z

In my testing, Youtube livestreams had an ~30 second live window, and only ~5 seconds of that window was the live edge.

This will be changed with my PR, depending of the streams, up to one hour (for livestreams with DVR of course).

This approach allows the user to speed up in the live window, just not in the live edge

Why not using what YouTube does? When coming to the live edge or being behind the live window, YouTube player(s) reset(s) the playback speed to 1x and seek(s) to the default position. This would solve all the issues without preventing users to select the playback speed. In this case, of course, 1x should be not saved as the default playback speed for videos.

KosOrKosm · 2022-04-03T17:09:37Z

Why not using what YouTube does?

I had also considered this method. I preferred locking to the default seek position (the current implementation), rather than allowing the user to briefly accelerate past it only to send them back moments later once buffering occurs.

I can use the same strategy as Youtube simply by moving the new logic from onUpdateProgress to onBuffering + adding a seek to default. Would this be the best / preferred approach?

KosOrKosm · 2022-04-05T21:06:40Z

Hello @litetex, can you review my pull request?

litetex · 2022-04-09T20:06:56Z

Blocked by #7989

KosOrKosm · 2022-04-09T23:16:25Z

Ahh, I see. @litetex Could I close this PR and open a new one without the speed dialogue changes? I can just use the same strategy as Youtube and reset playback speed when buffering at the live edge. That will solve the underlying issue without interfering with any incoming changes to the dialogue.

Sorry, I am relatively new to Github, so I'm not sure if this would be appropriate.

Stypox · 2022-04-30T15:49:12Z

@KosOrKosm no, you PR is perfectly fine, don't worry. What litetex was saying is probably that he wants to merge #7989 (which I will do now) before this PR since they both edit the same file and that causes merge conflicts. By merging #7989 I will create some merge conflicts for you, though, let me know if you need help with solving them ;-)

Stypox · 2022-04-30T15:56:26Z

@KosOrKosm you might have to rewrite from scratch your changes to PlaybackParameterDialog since #7989 basically rewrote it from scratch, too.

…changes

…SpeedAtLiveEdge # Conflicts: # app/src/main/java/org/schabi/newpipe/player/helper/PlaybackParameterDialog.java

KosOrKosm · 2022-04-30T17:27:58Z

I reimplemented my tempo limit logic for PlaybackParameterDialog on the #7989 changes. I also merged with the latest version of dev.

Stypox

Code looks almost good. Thank you!

app/src/main/java/org/schabi/newpipe/player/helper/PlaybackParameterDialog.java

KosOrKosm · 2022-05-03T03:04:09Z

I pushed the requested changes.

sonarcloud · 2022-05-03T06:22:22Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

app/src/main/java/org/schabi/newpipe/player/helper/PlaybackParameterDialog.java

litetex

Just scrolled down a bit, noticed problem, block catastrophe.

I will review/test this in more detail once I have time and higher priority PRs are done.

litetex · 2022-05-05T19:29:02Z

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

@@ -1717,6 +1733,10 @@ private void onUpdateProgress(final int currentProgress,
        }
        binding.playbackLiveSync.setClickable(!isLiveEdge());

+        if (isLiveEdge() && getPlaybackSpeed() > 1.0f) {


Did you notice that this is run constantly in a loop?

I did realize this is a function that is executing very frequently, but I wanted the playback speed to reset the moment the live edge was reached and wasn't sure if there was a better place to put the logic.

This logic could be moved to onBuffering(), which would match Youtube's livestreaming behavior and avoid calling the check in a loop. The downside is that the speed reset won't occur immediately after hitting the live edge.

The more resource-heavy part here is probably calculating isLiveEdge, seeing it is not a trivial function.

Stypox · 2022-05-06T08:53:53Z

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

        binding.playbackLiveSync.setClickable(!isLiveEdge());

+        if (isLiveEdge() && getPlaybackSpeed() > 1.0f) {


Then probably we can just do this:

Suggested change

binding.playbackLiveSync.setClickable(!isLiveEdge());

if (isLiveEdge() && getPlaybackSpeed() > 1.0f) {

// if live edge of a livestream -> no need for sync button; playback speed must be <= 1.0

final boolean liveEdge = isLiveEdge();

binding.playbackLiveSync.setClickable(!liveEdge);

if (liveEdge && getPlaybackSpeed() > 1.0f) {

Stypox · 2024-03-30T14:20:30Z

I'm closing this due to inactivity and due to the fact that we're soon starting the rewrite and we don't want to touch the player and possibly introduce regressions. Thank you anyway for your effort!

KosOrKosm added 2 commits April 2, 2022 15:03

Player: Limit speed to 1.0 at live edge

105b882

PlaybackParamDialog: limit options at live edge

80ed98c

Player: make live edge speed adjustments temporary

878bdd2

KosOrKosm added 5 commits April 30, 2022 09:07

Player: Limit speed to 1.0 at live edge

12d8800

PlaybackParamDialog: limit options at live edge

4379266

Player: make live edge speed adjustments temporary

c618e0b

PlaybackParameterDialog: merge live tempo limit with TeamNewPipe#7989 …

ed2641e

…changes

Merge remote-tracking branch 'origin/limitSpeedAtLiveEdge' into limit…

0e18c91

…SpeedAtLiveEdge # Conflicts: # app/src/main/java/org/schabi/newpipe/player/helper/PlaybackParameterDialog.java

KosOrKosm changed the base branch from dev to master April 30, 2022 17:12

KosOrKosm changed the base branch from master to dev April 30, 2022 17:12

Stypox requested changes May 2, 2022

View reviewed changes

PlaybackParameterDialog: update livestream related variable names

541d1c9

AudricV added feature request Issue is related to a feature in the app player Issues related to any player (main, popup and background) labels May 3, 2022

litetex self-requested a review May 3, 2022 19:37

Stypox requested changes May 5, 2022

View reviewed changes

app/src/main/java/org/schabi/newpipe/player/helper/PlaybackParameterDialog.java Outdated Show resolved Hide resolved

PlaybackParameterDialog: remove unneeded initialLiveEdge tracking

81dd93e

litetex requested changes May 5, 2022

View reviewed changes

Stypox requested changes May 6, 2022

View reviewed changes

Stypox force-pushed the dev branch from a81dbe2 to dc7fce8 Compare June 24, 2022 16:53

Stypox closed this Mar 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit speed at live edge #8134

Limit speed at live edge #8134

KosOrKosm commented Apr 2, 2022

AudricV commented Apr 3, 2022

KosOrKosm commented Apr 3, 2022

AudricV commented Apr 3, 2022

KosOrKosm commented Apr 3, 2022 •

edited

Loading

KosOrKosm commented Apr 5, 2022

litetex commented Apr 9, 2022

KosOrKosm commented Apr 9, 2022

Stypox commented Apr 30, 2022

Stypox commented Apr 30, 2022

KosOrKosm commented Apr 30, 2022

Stypox left a comment

KosOrKosm commented May 3, 2022

sonarcloud bot commented May 3, 2022

litetex left a comment

litetex May 5, 2022

KosOrKosm May 5, 2022

Stypox May 6, 2022

Stypox May 6, 2022

Stypox commented Mar 30, 2024

		binding.playbackLiveSync.setClickable(!isLiveEdge());

		if (isLiveEdge() && getPlaybackSpeed() > 1.0f) {

-        binding.playbackLiveSync.setClickable(!isLiveEdge());
-        if (isLiveEdge() && getPlaybackSpeed() > 1.0f) {
+        // if live edge of a livestream -> no need for sync button; playback speed must be <= 1.0
+        final boolean liveEdge = isLiveEdge();
+        binding.playbackLiveSync.setClickable(!liveEdge);
+        if (liveEdge && getPlaybackSpeed() > 1.0f) {

Limit speed at live edge #8134

Limit speed at live edge #8134

Conversation

KosOrKosm commented Apr 2, 2022

What is it?

Description of the changes in your PR

Before/After Screenshots

Fixes the following issue(s)

APK testing

Due diligence

AudricV commented Apr 3, 2022

KosOrKosm commented Apr 3, 2022

AudricV commented Apr 3, 2022

KosOrKosm commented Apr 3, 2022 • edited Loading

KosOrKosm commented Apr 5, 2022

litetex commented Apr 9, 2022

KosOrKosm commented Apr 9, 2022

Stypox commented Apr 30, 2022

Stypox commented Apr 30, 2022

KosOrKosm commented Apr 30, 2022

Stypox left a comment

Choose a reason for hiding this comment

KosOrKosm commented May 3, 2022

sonarcloud bot commented May 3, 2022

litetex left a comment

Choose a reason for hiding this comment

litetex May 5, 2022

Choose a reason for hiding this comment

KosOrKosm May 5, 2022

Choose a reason for hiding this comment

Stypox May 6, 2022

Choose a reason for hiding this comment

Stypox May 6, 2022

Choose a reason for hiding this comment

Stypox commented Mar 30, 2024

KosOrKosm commented Apr 3, 2022 •

edited

Loading