Backport pull request #17797 from jellyfin/release-12.z

Correct SyncPlay sessions that report playback at a stale position

Original-merge: a93180d35f3528a556b1c81aac4ba4132b34391e

Merged-by: crobibero <cody@robibe.ro>

Backported-by: Cody Robibero <cody@robibe.ro>

3 files changed, 150 insertions, 1 deletions

diff --git a/MediaBrowser.Controller/SyncPlay/GroupStates/WaitingGroupState.cs b/MediaBrowser.Controller/SyncPlay/GroupStates/WaitingGroupState.cs
index f4fab29800..8f17039ae1 100644
--- a/MediaBrowser.Controller/SyncPlay/GroupStates/WaitingGroupState.cs
+++ b/MediaBrowser.Controller/SyncPlay/GroupStates/WaitingGroupState.cs
@@ -50,6 +50,11 @@ namespace MediaBrowser.Controller.SyncPlay.GroupStates
         /// </summary>
         private GroupStateType InitialState { get; set; }
 
+        /// <summary>
+        /// Gets or sets a value indicating whether the group position moved during this wait.
+        /// </summary>
+        private bool PositionJumped { get; set; }
+
         /// <inheritdoc />
         public override void SessionJoined(IGroupStateContext context, GroupStateType prevState, SessionInfo session, CancellationToken cancellationToken)
         {
@@ -136,6 +141,7 @@ namespace MediaBrowser.Controller.SyncPlay.GroupStates
             ResumePlaying = true;
 
             var setQueueStatus = context.SetPlayQueue(request.PlayingQueue, request.PlayingItemPosition, request.StartPositionTicks);
+            PositionJumped = setQueueStatus;
             if (!setQueueStatus)
             {
                 _logger.LogError("Unable to set playing queue in group {GroupId}.", context.GroupId.ToString());
@@ -175,6 +181,7 @@ namespace MediaBrowser.Controller.SyncPlay.GroupStates
             ResumePlaying = true;
 
             var result = context.SetPlayingItem(request.PlaylistItemId);
+            PositionJumped = result;
             if (result)
             {
                 var playQueueUpdate = context.GetPlayQueueUpdate(PlayQueueUpdateReason.SetCurrentItem);
@@ -214,6 +221,7 @@ namespace MediaBrowser.Controller.SyncPlay.GroupStates
             {
                 ResumePlaying = true;
                 context.RestartCurrentItem();
+                PositionJumped = true;
 
                 var playQueueUpdate = context.GetPlayQueueUpdate(PlayQueueUpdateReason.NewPlaylist);
                 var update = new SyncPlayPlayQueueUpdate(context.GroupId, playQueueUpdate);
@@ -310,6 +318,7 @@ namespace MediaBrowser.Controller.SyncPlay.GroupStates
             // Seek.
             context.PositionTicks = ticks;
             context.LastActivity = DateTime.UtcNow;
+            PositionJumped = true;
 
             var command = context.NewSyncPlayCommand(SendCommandType.Seek);
             context.SendCommand(session, SyncPlayBroadcastType.AllGroup, command, cancellationToken);
@@ -450,7 +459,13 @@ namespace MediaBrowser.Controller.SyncPlay.GroupStates
             {
                 // Handle case where session reported as ready but in reality
                 // it has no clue of the real position nor the playback state.
-                if (!request.IsPlaying && Math.Abs(delayTicks) > maxPlaybackOffsetTicks)
+                // A jump means the session has not applied the new position; without one it is
+                // catching up after buffering and is allowed to lag.
+                var maxOffsetTicks = request.IsPlaying && !PositionJumped
+                    ? TimeSpan.FromMilliseconds(context.MaxCatchUpOffset).Ticks
+                    : maxPlaybackOffsetTicks;
+
+                if (Math.Abs(delayTicks) > maxOffsetTicks)
                 {
                     // Session not ready at all.
                     context.SetBuffering(session, true);
@@ -580,6 +595,7 @@ namespace MediaBrowser.Controller.SyncPlay.GroupStates
             }
 
             var newItem = context.NextItemInQueue();
+            PositionJumped = newItem;
             if (newItem)
             {
                 // Send playing-queue update.
@@ -626,6 +642,7 @@ namespace MediaBrowser.Controller.SyncPlay.GroupStates
             }
 
             var newItem = context.PreviousItemInQueue();
+            PositionJumped = newItem;
             if (newItem)
             {
                 // Send playing-queue update.
diff --git a/MediaBrowser.Controller/SyncPlay/IGroupStateContext.cs b/MediaBrowser.Controller/SyncPlay/IGroupStateContext.cs
index ddf86be71f..e02d1bde45 100644
--- a/MediaBrowser.Controller/SyncPlay/IGroupStateContext.cs
+++ b/MediaBrowser.Controller/SyncPlay/IGroupStateContext.cs
@@ -34,6 +34,12 @@ namespace MediaBrowser.Controller.SyncPlay
         long MaxPlaybackOffset { get; }
 
         /// <summary>
+        /// Gets the maximum offset accepted for a session catching up after buffering, in milliseconds.
+        /// </summary>
+        /// <value>The maximum catch-up offset, in milliseconds.</value>
+        long MaxCatchUpOffset => 60000;
+
+        /// <summary>
         /// Gets the group identifier.
         /// </summary>
         /// <value>The group identifier.</value>
diff --git a/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs b/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs
index 0cccd5d4ca..81af12ba8c 100644
--- a/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs
+++ b/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs
@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
 using Jellyfin.Database.Implementations.Entities;
@@ -20,6 +21,128 @@ namespace Jellyfin.Server.Implementations.Tests.SyncPlay;
 public class WaitingGroupStateTests
 {
     [Fact]
+    public void Ready_PlayingSessionReportsPositionFromBeforeSeek_IsCorrected()
+    {
+        var harness = new GroupHarness();
+        var group = harness.Group;
+
+        group.PositionTicks = TimeSpan.FromMinutes(10).Ticks;
+        group.LastActivity = DateTime.UtcNow;
+
+        var state = new WaitingGroupState(NullLoggerFactory.Instance) { ResumePlaying = true };
+
+        // One member seeks half an hour in.
+        state.HandleRequest(
+            new SeekGroupRequest(TimeSpan.FromMinutes(40).Ticks),
+            group,
+            GroupStateType.Playing,
+            harness.Second,
+            CancellationToken.None);
+
+        harness.Commands.Clear();
+
+        // The other member has not applied the seek yet and reports the old position, still playing.
+        state.HandleRequest(
+            new ReadyGroupRequest(DateTime.UtcNow, TimeSpan.FromMinutes(10).Ticks, true, harness.PlaylistItemId),
+            group,
+            GroupStateType.Waiting,
+            harness.First,
+            CancellationToken.None);
+
+        // It must be seeked into position, not accepted as ready and handed a pause command
+        // scheduled the length of the seek into the future.
+        Assert.Contains(harness.Commands, c => c.Command == SendCommandType.Seek);
+        Assert.DoesNotContain(harness.Commands, c => c.Command == SendCommandType.Pause);
+        Assert.True(group.IsBuffering(), "session should still be considered buffering");
+    }
+
+    [Fact]
+    public void Ready_PlayingSessionRecoveringFromALongStall_IsNotSeeked()
+    {
+        var harness = new GroupHarness();
+        var group = harness.Group;
+
+        group.PositionTicks = TimeSpan.FromMinutes(10).Ticks;
+        group.LastActivity = DateTime.UtcNow;
+
+        var state = new WaitingGroupState(NullLoggerFactory.Instance) { ResumePlaying = true };
+
+        // The session reports it is buffering. No seek happens, so the group position stays put.
+        state.HandleRequest(
+            new BufferGroupRequest(DateTime.UtcNow, group.PositionTicks, true, harness.PlaylistItemId),
+            group,
+            GroupStateType.Playing,
+            harness.First,
+            CancellationToken.None);
+
+        harness.Commands.Clear();
+
+        // It recovers 45 seconds later, still behind, and must be waited for rather than seeked
+        // forward past content it already buffered.
+        var behind = group.PositionTicks - TimeSpan.FromSeconds(45).Ticks;
+        state.HandleRequest(
+            new ReadyGroupRequest(DateTime.UtcNow, behind, true, harness.PlaylistItemId),
+            group,
+            GroupStateType.Waiting,
+            harness.First,
+            CancellationToken.None);
+
+        Assert.DoesNotContain(harness.Commands, c => c.Command == SendCommandType.Seek);
+    }
+
+    [Fact]
+    public void Ready_PlayingSessionSlightlyBehindGroup_IsStillTreatedAsCatchingUp()
+    {
+        var harness = new GroupHarness();
+        var group = harness.Group;
+
+        // A session that is a couple of seconds behind is genuinely recovering, and the group
+        // is expected to wait for it rather than seek it around.
+        group.PositionTicks = TimeSpan.FromMinutes(30).Ticks;
+        group.LastActivity = DateTime.UtcNow;
+        group.SetBuffering(harness.First, true);
+        group.SetBuffering(harness.Second, true);
+
+        var state = new WaitingGroupState(NullLoggerFactory.Instance) { ResumePlaying = true };
+        harness.Commands.Clear();
+
+        var clientPosition = group.PositionTicks - TimeSpan.FromSeconds(2).Ticks;
+        state.HandleRequest(
+            new ReadyGroupRequest(DateTime.UtcNow, clientPosition, true, harness.PlaylistItemId),
+            group,
+            GroupStateType.Waiting,
+            harness.First,
+            CancellationToken.None);
+
+        Assert.DoesNotContain(harness.Commands, c => c.Command == SendCommandType.Seek);
+        Assert.Contains(harness.Commands, c => c.Command == SendCommandType.Pause);
+    }
+
+    [Fact]
+    public void Ready_PausedSessionOutOfPosition_IsStillCorrected()
+    {
+        var harness = new GroupHarness();
+        var group = harness.Group;
+
+        group.PositionTicks = TimeSpan.FromMinutes(30).Ticks;
+        group.LastActivity = DateTime.UtcNow;
+        group.SetBuffering(harness.First, true);
+        group.SetBuffering(harness.Second, true);
+
+        var state = new WaitingGroupState(NullLoggerFactory.Instance) { ResumePlaying = true };
+        harness.Commands.Clear();
+
+        state.HandleRequest(
+            new ReadyGroupRequest(DateTime.UtcNow, 0, false, harness.PlaylistItemId),
+            group,
+            GroupStateType.Waiting,
+            harness.First,
+            CancellationToken.None);
+
+        Assert.Contains(harness.Commands, c => c.Command == SendCommandType.Seek);
+    }
+
+    [Fact]
     public void Ready_ClientResumedWithLowPing_AppliesTheDefaultPingFloorInMilliseconds()
     {
         var harness = new GroupHarness();
@@ -99,6 +222,7 @@ public class WaitingGroupStateTests
 
             sessionManager
                 .Setup(m => m.SendSyncPlayCommand(It.IsAny<string>(), It.IsAny<SendCommand>(), It.IsAny<CancellationToken>()))
+                .Callback<string, SendCommand, CancellationToken>((_, command, _) => Commands.Add(command))
                 .Returns(Task.CompletedTask);
 
             sessionManager
@@ -137,5 +261,7 @@ public class WaitingGroupStateTests
         public SessionInfo Second { get; }
 
         public Guid PlaylistItemId { get; }
+
+        public List<SendCommand> Commands { get; } = new List<SendCommand>();
     }
 }