Address requested changes by review

Emby.Server.Implementations/SyncPlay/GroupController.cs

@@ -126,16 +126,6 @@ namespace Emby.Server.Implementations.SyncPlay
             State = new IdleGroupState(_logger);
         }
 
-        /// <summary>
-        /// Checks if a session is in this group.
-        /// </summary>
-        /// <param name="sessionId">The session identifier to check.</param>
-        /// <returns><c>true</c> if the session is in this group; <c>false</c> otherwise.</returns>
-        private bool ContainsSession(string sessionId)
-        {
-            return Participants.ContainsKey(sessionId);
-        }
-
         /// <summary>
         /// Adds the session to the group.
         /// </summary>
@@ -174,16 +164,22 @@ namespace Emby.Server.Implementations.SyncPlay
                 case SyncPlayBroadcastType.CurrentSession:
                     return new SessionInfo[] { from };
                 case SyncPlayBroadcastType.AllGroup:
-                    return Participants.Values.Select(
-                        session => session.Session).ToArray();
+                    return Participants
+                        .Values
+                        .Select(session => session.Session)
+                        .ToArray();
                 case SyncPlayBroadcastType.AllExceptCurrentSession:
-                    return Participants.Values.Select(
-                        session => session.Session).Where(
-                        session => !session.Id.Equals(from.Id)).ToArray();
+                    return Participants
+                        .Values
+                        .Select(session => session.Session)
+                        .Where(session => !session.Id.Equals(from.Id))
+                        .ToArray();
                 case SyncPlayBroadcastType.AllReady:
-                    return Participants.Values.Where(
-                        session => !session.IsBuffering).Select(
-                        session => session.Session).ToArray();
+                    return Participants
+                        .Values
+                        .Where(session => !session.IsBuffering)
+                        .Select(session => session.Session)
+                        .ToArray();
                 default:
                     return Array.Empty<SessionInfo>();
             }
@@ -236,7 +232,8 @@ namespace Emby.Server.Implementations.SyncPlay
             }
 
             // Get list of users.
-            var users = Participants.Values
+            var users = Participants
+                .Values
                 .Select(participant => _userManager.GetUserById(participant.Session.UserId));
 
             // Find problematic users.
@@ -365,7 +362,7 @@ namespace Emby.Server.Implementations.SyncPlay
         /// <inheritdoc />
         public void SetIgnoreGroupWait(SessionInfo session, bool ignoreGroupWait)
         {
-            if (!ContainsSession(session.Id))
+            if (!Participants.ContainsKey(session.Id))
             {
                 return;
             }
@@ -443,8 +440,8 @@ namespace Emby.Server.Implementations.SyncPlay
         public long SanitizePositionTicks(long? positionTicks)
         {
             var ticks = positionTicks ?? 0;
-            ticks = ticks >= 0 ? ticks : 0;
-            ticks = ticks > RunTimeTicks ? RunTimeTicks : ticks;
+            ticks = Math.Max(ticks, 0);
+            ticks = Math.Min(ticks, RunTimeTicks);
             return ticks;
         }
 
@@ -663,8 +660,13 @@ namespace Emby.Server.Implementations.SyncPlay
             {
                 var currentTime = DateTime.UtcNow;
                 var elapsedTime = currentTime - LastActivity;
-                // Event may happen during the delay added to account for latency.
-                startPositionTicks += elapsedTime.Ticks > 0 ? elapsedTime.Ticks : 0;
+                // Elapsed time is negative if event happens
+                // during the delay added to account for latency.
+                // In this phase clients haven't started the playback yet.
+                // In other words, LastActivity is in the future,
+                // when playback unpause is supposed to happen.
+                // Adjust ticks only if playback actually started.
+                startPositionTicks += Math.Max(elapsedTime.Ticks, 0);
             }
 
             return new PlayQueueUpdate()

Emby.Server.Implementations/SyncPlay/GroupStates/AbstractGroupState.cs

@@ -212,7 +212,7 @@ namespace MediaBrowser.Controller.SyncPlay
 
         private void UnhandledRequest(IPlaybackGroupRequest request)
         {
-            _logger.LogWarning("HandleRequest: unhandled {0} request for {1} state.", request.GetRequestType(), this.GetGroupState());
+            _logger.LogWarning("HandleRequest: unhandled {0} request for {1} state.", request.GetRequestType(), GetGroupState());
         }
     }
 }

Emby.Server.Implementations/SyncPlay/GroupStates/IdleGroupState.cs

@@ -16,7 +16,8 @@ namespace MediaBrowser.Controller.SyncPlay
         /// <summary>
         /// Default constructor.
         /// </summary>
-        public IdleGroupState(ILogger logger) : base(logger)
+        public IdleGroupState(ILogger logger)
+            : base(logger)
         {
             // Do nothing.
         }

Emby.Server.Implementations/SyncPlay/GroupStates/PausedGroupState.cs

@@ -17,7 +17,8 @@ namespace MediaBrowser.Controller.SyncPlay
         /// <summary>
         /// Default constructor.
         /// </summary>
-        public PausedGroupState(ILogger logger) : base(logger)
+        public PausedGroupState(ILogger logger)
+            : base(logger)
         {
             // Do nothing.
         }
@@ -70,8 +71,12 @@ namespace MediaBrowser.Controller.SyncPlay
                 var currentTime = DateTime.UtcNow;
                 var elapsedTime = currentTime - context.LastActivity;
                 context.LastActivity = currentTime;
+                // Elapsed time is negative if event happens
+                // during the delay added to account for latency.
+                // In this phase clients haven't started the playback yet.
+                // In other words, LastActivity is in the future,
+                // when playback unpause is supposed to happen.
                 // Seek only if playback actually started.
-                // Pause request may be issued during the delay added to account for latency.
                 context.PositionTicks += Math.Max(elapsedTime.Ticks, 0);
 
                 var command = context.NewSyncPlayCommand(SendCommandType.Pause);

Emby.Server.Implementations/SyncPlay/GroupStates/PlayingGroupState.cs

@@ -22,7 +22,8 @@ namespace MediaBrowser.Controller.SyncPlay
         /// <summary>
         /// Default constructor.
         /// </summary>
-        public PlayingGroupState(ILogger logger) : base(logger)
+        public PlayingGroupState(ILogger logger)
+            : base(logger)
         {
             // Do nothing.
         }

Emby.Server.Implementations/SyncPlay/GroupStates/WaitingGroupState.cs

@@ -32,7 +32,8 @@ namespace MediaBrowser.Controller.SyncPlay
         /// <summary>
         /// Default constructor.
         /// </summary>
-        public WaitingGroupState(ILogger logger) : base(logger)
+        public WaitingGroupState(ILogger logger)
+            : base(logger)
         {
             // Do nothing.
         }
@@ -59,8 +60,12 @@ namespace MediaBrowser.Controller.SyncPlay
                 var currentTime = DateTime.UtcNow;
                 var elapsedTime = currentTime - context.LastActivity;
                 context.LastActivity = currentTime;
+                // Elapsed time is negative if event happens
+                // during the delay added to account for latency.
+                // In this phase clients haven't started the playback yet.
+                // In other words, LastActivity is in the future,
+                // when playback unpause is supposed to happen.
                 // Seek only if playback actually started.
-                // Event may happen during the delay added to account for latency.
                 context.PositionTicks += Math.Max(elapsedTime.Ticks, 0);
             }
 
@@ -355,6 +360,12 @@ namespace MediaBrowser.Controller.SyncPlay
                 var currentTime = DateTime.UtcNow;
                 var elapsedTime = currentTime - context.LastActivity;
                 context.LastActivity = currentTime;
+                // Elapsed time is negative if event happens
+                // during the delay added to account for latency.
+                // In this phase clients haven't started the playback yet.
+                // In other words, LastActivity is in the future,
+                // when playback unpause is supposed to happen.
+                // Seek only if playback actually started.
                 context.PositionTicks += Math.Max(elapsedTime.Ticks, 0);
 
                 // Send pause command to all non-buffering sessions.
@@ -484,7 +495,7 @@ namespace MediaBrowser.Controller.SyncPlay
                     {
                         // Client, that was buffering, resumed playback but did not update others in time.
                         delayTicks = context.GetHighestPing() * 2 * TimeSpan.TicksPerMillisecond;
-                        delayTicks = delayTicks < context.DefaultPing ? context.DefaultPing : delayTicks;
+                        delayTicks = Math.Max(delayTicks, context.DefaultPing);
 
                         context.LastActivity = currentTime.AddTicks(delayTicks);
 

MediaBrowser.Controller/SyncPlay/Queue/PlayQueueManager.cs

@@ -25,7 +25,7 @@ namespace MediaBrowser.Controller.SyncPlay
     /// <summary>
     /// Class PlayQueueManager.
     /// </summary>
-    public class PlayQueueManager : IDisposable
+    public class PlayQueueManager
     {
         /// <summary>
         /// Gets or sets the playing item index.
@@ -83,27 +83,6 @@ namespace MediaBrowser.Controller.SyncPlay
             Reset();
         }
 
-        /// <inheritdoc />
-        public void Dispose()
-        {
-            Dispose(true);
-            GC.SuppressFinalize(this);
-        }
-
-        /// <summary>
-        /// Releases unmanaged and optionally managed resources.
-        /// </summary>
-        /// <param name="disposing"><c>true</c> to release both managed and unmanaged resources; <c>false</c> to release only unmanaged resources.</param>
-        protected virtual void Dispose(bool disposing)
-        {
-            if (_disposed)
-            {
-                return;
-            }
-
-            _disposed = true;
-        }
-
         /// <summary>
         /// Gets the next available identifier.
         /// </summary>
@@ -284,7 +263,7 @@ namespace MediaBrowser.Controller.SyncPlay
                     ShuffledPlaylist.Add(playingItem);
                 }
                 PlayingItemIndex = 0;
-                }
+            }
             else
             {
                 PlayingItemIndex = NoPlayingItemIndex;