From 0c05d9d1a97ccf77571c7cffd39314c5cf3ad4e6 Mon Sep 17 00:00:00 2001 From: fmarcac <188743521+fmarcac@users.noreply.github.com> Date: Sat, 5 Sep 2026 14:20:26 +0200 Subject: Apply the SyncPlay resume delay floor in the correct unit --- .../SyncPlay/GroupStates/WaitingGroupState.cs | 2 +- .../SyncPlay/WaitingGroupStateTests.cs | 109 +++++++++++++++++++++ 2 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs diff --git a/MediaBrowser.Controller/SyncPlay/GroupStates/WaitingGroupState.cs b/MediaBrowser.Controller/SyncPlay/GroupStates/WaitingGroupState.cs index eb38eeb503..f4fab29800 100644 --- a/MediaBrowser.Controller/SyncPlay/GroupStates/WaitingGroupState.cs +++ b/MediaBrowser.Controller/SyncPlay/GroupStates/WaitingGroupState.cs @@ -501,7 +501,7 @@ namespace MediaBrowser.Controller.SyncPlay.GroupStates { // Client, that was buffering, resumed playback but did not update others in time. delayTicks = context.GetHighestPing() * 2 * TimeSpan.TicksPerMillisecond; - delayTicks = Math.Max(delayTicks, context.DefaultPing); + delayTicks = Math.Max(delayTicks, TimeSpan.FromMilliseconds(context.DefaultPing).Ticks); context.LastActivity = currentTime.AddTicks(delayTicks); diff --git a/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs b/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs new file mode 100644 index 0000000000..1e2467b3fc --- /dev/null +++ b/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs @@ -0,0 +1,109 @@ +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using Jellyfin.Database.Implementations.Entities; +using MediaBrowser.Controller.Entities; +using MediaBrowser.Controller.Library; +using MediaBrowser.Controller.Session; +using MediaBrowser.Controller.SyncPlay.GroupStates; +using MediaBrowser.Controller.SyncPlay.PlaybackRequests; +using MediaBrowser.Controller.SyncPlay.Requests; +using MediaBrowser.Model.SyncPlay; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; +using SyncPlayGroup = Emby.Server.Implementations.SyncPlay.Group; + +namespace Jellyfin.Server.Implementations.Tests.SyncPlay; + +public class WaitingGroupStateTests +{ + [Fact] + public void Ready_ClientResumedWithLowPing_AppliesTheDefaultPingFloorInMilliseconds() + { + var harness = new GroupHarness(); + var group = harness.Group; + + // Both members report a ping well under the default, so the floor is what decides the delay. + group.UpdatePing(harness.First, 10); + group.UpdatePing(harness.Second, 10); + + group.PositionTicks = TimeSpan.FromMinutes(5).Ticks; + group.LastActivity = DateTime.UtcNow; + group.SetBuffering(harness.First, true); + group.SetBuffering(harness.Second, false); + + var state = new WaitingGroupState(NullLoggerFactory.Instance) { ResumePlaying = true }; + + var before = DateTime.UtcNow; + state.HandleRequest( + new ReadyGroupRequest(DateTime.UtcNow, group.PositionTicks, true, harness.PlaylistItemId), + group, + GroupStateType.Waiting, + harness.First, + CancellationToken.None); + + // DefaultPing is expressed in milliseconds, so the floor must be converted before being + // compared against a tick count. Without the conversion the floor is 500 ticks (0.05 ms) + // and never applies. + var scheduledDelay = group.LastActivity - before; + Assert.True( + scheduledDelay >= TimeSpan.FromMilliseconds(group.DefaultPing), + $"expected a resume delay of at least {group.DefaultPing} ms, got {scheduledDelay.TotalMilliseconds} ms"); + } + + private sealed class GroupHarness + { + public GroupHarness() + { + var userManager = new Mock(); + var sessionManager = new Mock(); + var libraryManager = new Mock(); + + var user = new User("tester", "auth-provider", "pwdreset-provider"); + userManager.Setup(m => m.GetUserById(It.IsAny())).Returns(user); + + var item = new Mock(); + item.Setup(i => i.IsVisibleStandalone(It.IsAny())).Returns(true); + item.Object.RunTimeTicks = TimeSpan.FromHours(2).Ticks; + libraryManager.Setup(m => m.GetItemById(It.IsAny())).Returns(item.Object); + + sessionManager + .Setup(m => m.SendSyncPlayCommand(It.IsAny(), It.IsAny(), It.IsAny())) + .Returns(Task.CompletedTask); + + Group = new SyncPlayGroup( + NullLoggerFactory.Instance, + userManager.Object, + sessionManager.Object, + libraryManager.Object); + + First = new SessionInfo(sessionManager.Object, NullLogger.Instance) + { + Id = "first", + UserId = user.Id, + UserName = "first" + }; + Second = new SessionInfo(sessionManager.Object, NullLogger.Instance) + { + Id = "second", + UserId = user.Id, + UserName = "second" + }; + + Group.CreateGroup(First, new NewGroupRequest("group"), CancellationToken.None); + Group.SessionJoin(Second, new JoinGroupRequest(Group.GroupId), CancellationToken.None); + Group.SetPlayQueue(new List { Guid.NewGuid() }, 0, 0); + PlaylistItemId = Group.PlayQueue.GetPlayingItemPlaylistId(); + } + + public SyncPlayGroup Group { get; } + + public SessionInfo First { get; } + + public SessionInfo Second { get; } + + public Guid PlaylistItemId { get; } + } +} -- cgit v1.2.3 From e356fe9146bb4c62226e9c5108bfa9bf17171f68 Mon Sep 17 00:00:00 2001 From: fmarcac <188743521+fmarcac@users.noreply.github.com> Date: Sat, 5 Sep 2026 14:20:26 +0200 Subject: Return the default ping for an empty SyncPlay group --- Emby.Server.Implementations/SyncPlay/Group.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Emby.Server.Implementations/SyncPlay/Group.cs b/Emby.Server.Implementations/SyncPlay/Group.cs index 38a0018a70..256faffbf4 100644 --- a/Emby.Server.Implementations/SyncPlay/Group.cs +++ b/Emby.Server.Implementations/SyncPlay/Group.cs @@ -451,7 +451,9 @@ namespace Emby.Server.Implementations.SyncPlay max = Math.Max(max, session.Ping); } - return max; + // A group with no participants has no ping to report. Returning long.MinValue would + // overflow the callers that scale this value into ticks, so fall back to the default. + return max == long.MinValue ? DefaultPing : max; } /// -- cgit v1.2.3 From 7ce911a40145fce2600ba3ce042b82f7cb97d7f7 Mon Sep 17 00:00:00 2001 From: fmarcac <188743521+fmarcac@users.noreply.github.com> Date: Sat, 5 Sep 2026 15:06:37 +0200 Subject: Clamp client reported ping in SyncPlay groups --- Emby.Server.Implementations/SyncPlay/Group.cs | 14 +++++++++- .../SyncPlay/WaitingGroupStateTests.cs | 32 ++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/Emby.Server.Implementations/SyncPlay/Group.cs b/Emby.Server.Implementations/SyncPlay/Group.cs index 256faffbf4..923bfc67aa 100644 --- a/Emby.Server.Implementations/SyncPlay/Group.cs +++ b/Emby.Server.Implementations/SyncPlay/Group.cs @@ -90,6 +90,18 @@ namespace Emby.Server.Implementations.SyncPlay /// The default ping. public long DefaultPing { get; } = 500; + /// + /// Gets the maximum ping, in milliseconds, accepted from a session. + /// + /// + /// Pings are reported by clients and are scaled into the delays used to schedule playback, + /// so an unbounded value lets a single session push the whole group's resume point + /// arbitrarily far out, or overflow the arithmetic entirely. Anything above this is not a + /// usable measurement for synchronisation. + /// + /// The maximum ping. + public long MaxPing { get; } = 10000; + /// /// Gets the maximum time offset error accepted for dates reported by clients, in milliseconds. /// @@ -438,7 +450,7 @@ namespace Emby.Server.Implementations.SyncPlay { if (_participants.TryGetValue(session.Id, out GroupMember value)) { - value.Ping = ping; + value.Ping = Math.Clamp(ping, 0, MaxPing); } } diff --git a/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs b/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs index 1e2467b3fc..0cccd5d4ca 100644 --- a/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs +++ b/tests/Jellyfin.Server.Implementations.Tests/SyncPlay/WaitingGroupStateTests.cs @@ -53,6 +53,34 @@ public class WaitingGroupStateTests $"expected a resume delay of at least {group.DefaultPing} ms, got {scheduledDelay.TotalMilliseconds} ms"); } + [Theory] + [InlineData(4_000_000_000L)] + [InlineData(1_000_000_000_000_000L)] + [InlineData(long.MaxValue)] + [InlineData(-1L)] + public void UpdatePing_ClientReportsAnUnusablePing_IsClampedAndCannotStallTheGroup(long reportedPing) + { + var harness = new GroupHarness(); + var group = harness.Group; + + group.UpdatePing(harness.First, reportedPing); + + Assert.InRange(group.GetHighestPing(), 0, group.MaxPing); + + // The reported ping is scaled into the group's resume point, so an unclamped value either + // pushes playback months out or overflows the arithmetic outright. + var state = new PlayingGroupState(NullLoggerFactory.Instance); + var before = DateTime.UtcNow; + state.HandleRequest( + new UnpauseGroupRequest(), + group, + GroupStateType.Paused, + harness.First, + CancellationToken.None); + + Assert.InRange(group.LastActivity - before, TimeSpan.Zero, TimeSpan.FromMinutes(1)); + } + private sealed class GroupHarness { public GroupHarness() @@ -73,6 +101,10 @@ public class WaitingGroupStateTests .Setup(m => m.SendSyncPlayCommand(It.IsAny(), It.IsAny(), It.IsAny())) .Returns(Task.CompletedTask); + sessionManager + .Setup(m => m.SendSyncPlayGroupUpdate(It.IsAny(), It.IsAny>(), It.IsAny())) + .Returns(Task.CompletedTask); + Group = new SyncPlayGroup( NullLoggerFactory.Instance, userManager.Object, -- cgit v1.2.3