From 62cd391686aca3d5abab2f411470d84c921bb977 Mon Sep 17 00:00:00 2001 From: Daniel Chalmers Date: Sun, 30 Aug 2026 09:04:42 -0500 Subject: [PATCH] Stop reporting playback on a clip that ended during the play call PlayAsync sets IsPlaying once its play operation completes, with nothing checking whether the clip is still running by then. A clip that reaches its end while that operation is in flight raises Ended, whose handler clears IsPlaying, and then the play call sets it straight back. The transport is left claiming to play a clip parked on its last frame, and nothing clears it until the user presses something else. Traced on the real stack, with a clip paused just before its end, seeked, and played straight away: play-enter +28ms Flyleaf PlaybackStopped, status Ended (the clip finishes inside the play call) +30ms controller IsPlaying=True (the play call then asserts playback) play-return The Ended handler did run; its IsPlaying=false simply raised nothing, because the value was already false. That sequence failed 3 of 40 iterations on current main. With a human-paced gap of 300ms between the seek and the play it failed 0 of 40, because the window is only as long as the play call, and a play call is only slow while Flyleaf still has a seek in flight. It is rare by hand, but the invariant is plainly wrong, and anything that makes play wait on a pending seek widens exactly this window. A counter bumped by the Ended handler lets the play call tell that the clip finished underneath it and leave the settled state alone. FakeCameraPlayer gains a PlayCallback hook, mirroring the SeekCallback it already had for interleaving an action mid-call. With the guard, the same sequence passed 80 of 80 across two runs. Two unit tests cover it: one ends the clip from inside the play call and asserts the controller does not report playback, one asserts an ordinary resume still does. Verified the first fails without the guard. This does not fix pressing play on a clip that has already finished, which can also leave the video frozen while the transport reports playback; that is a separate race inside Flyleaf's seek and play handling. 374 tests green. --- SentryDeck.Tests/Fixtures/FakeCameraPlayer.cs | 7 +++ .../VideoPlayerControllerTests.cs | 53 +++++++++++++++++++ SentryDeck/Playback/VideoPlayerController.cs | 14 ++++- 3 files changed, 73 insertions(+), 1 deletion(-) diff --git a/SentryDeck.Tests/Fixtures/FakeCameraPlayer.cs b/SentryDeck.Tests/Fixtures/FakeCameraPlayer.cs index 6e96554..3eb1d1f 100644 --- a/SentryDeck.Tests/Fixtures/FakeCameraPlayer.cs +++ b/SentryDeck.Tests/Fixtures/FakeCameraPlayer.cs @@ -67,10 +67,17 @@ public async Task OpenAsync(string path) return OpenResult; } + /// + /// Optional hook invoked synchronously inside , mirroring . + /// Lets a test make the clip end while a play operation is still in flight, which is the ordering that used to leave the controller reporting playback on a finished clip. + /// + public Action PlayCallback { get; set; } + public Task PlayAsync() { PlayCount++; CallLog.Add("play"); + PlayCallback?.Invoke(); return Task.CompletedTask; } diff --git a/SentryDeck.Tests/VideoPlayerControllerTests.cs b/SentryDeck.Tests/VideoPlayerControllerTests.cs index 8769848..000dc2a 100644 --- a/SentryDeck.Tests/VideoPlayerControllerTests.cs +++ b/SentryDeck.Tests/VideoPlayerControllerTests.cs @@ -435,6 +435,59 @@ public async Task FrontMediaEnded_WithinTolerance_CompletesNormallyWithoutRebuil controller.IsMediaOpen.ShouldBeTrue(); } + [Fact] + public async Task PlayAsync_WhenTheClipEndsDuringTheCall_DoesNotReportPlaying() + { + using var clipFiles = TestClipFiles.Create(chunkCount: 2); + var front = new FakeCameraPlayer(); + using var controller = CreateController(front); + + controller.LoadClips([clipFiles.Clip]); + controller.Playlist.MoveTo(0); + await WaitUntilClipOpenedAsync(controller, front); + + await controller.PauseAsync(); + var duration = controller.Duration; + + // Play pressed near the end, or queued behind a slow open: the clip runs out while the play operation is still in flight. + // The Ended handler clears IsPlaying, and the play call must not then set it back, or the transport claims to be playing a clip parked on its last frame until the user presses something else. + front.PlayCallback = () => + { + front.PlayCallback = null; + front.RaisePositionChanged(duration); + front.RaiseEnded(); + }; + + await controller.PlayAsync(); + + await Wait.UntilAsync(() => controller.Position == duration); + controller.IsPlaying.ShouldBeFalse(); + controller.Position.ShouldBe(duration); + // The clip is finished, not broken: the media stays open so the scrubber and frame-step still work. + controller.IsMediaOpen.ShouldBeTrue(); + controller.ErrorMessage.ShouldBeNull(); + } + + [Fact] + public async Task PlayAsync_WhenTheClipKeepsPlaying_ReportsPlaying() + { + using var clipFiles = TestClipFiles.Create(chunkCount: 2); + var front = new FakeCameraPlayer(); + using var controller = CreateController(front); + + controller.LoadClips([clipFiles.Clip]); + controller.Playlist.MoveTo(0); + await WaitUntilClipOpenedAsync(controller, front); + + await controller.PauseAsync(); + controller.IsPlaying.ShouldBeFalse(); + + // The guard above must only suppress the state a finished clip already settled; an ordinary resume still reports playback. + await controller.PlayAsync(); + + controller.IsPlaying.ShouldBeTrue(); + } + [Fact] public async Task FrontMediaEnded_WithNextClip_StaysOnCurrentClipWithoutAdvancing() { diff --git a/SentryDeck/Playback/VideoPlayerController.cs b/SentryDeck/Playback/VideoPlayerController.cs index 4267e1e..e6f19fa 100644 --- a/SentryDeck/Playback/VideoPlayerController.cs +++ b/SentryDeck/Playback/VideoPlayerController.cs @@ -60,6 +60,9 @@ public sealed partial class VideoPlayerController : ObservableObject, IDisposabl private ClipMediaSource _openedMediaSource; private int _recoveryAttempts; + // Bumped every time playback ends on its own, so a play operation that is already in flight can tell that the clip finished underneath it. + private long _playbackEndedCount; + [ObservableProperty] [NotifyPropertyChangedFor(nameof(CanPlayPause))] private bool _isLoading; @@ -182,8 +185,16 @@ await RunSerializedPlaybackOperationAsync(async _ => { if (IsMediaOpen && _openedClip == clip) { + // Snapshot before starting the players: a clip can reach its end while this operation is in flight, either because play was pressed close to the end or because the operation queued behind a slow open, and by the time it returns the Ended handler has already cleared IsPlaying. + // Re-asserting it here would leave the transport claiming to play a clip parked on its last frame, and nothing clears that until the user presses something else. + var endedCountBeforePlay = Interlocked.Read(ref _playbackEndedCount); + await PlayOpenPlayersAsync(); - IsPlaying = true; + + if (Interlocked.Read(ref _playbackEndedCount) == endedCountBeforePlay) + { + IsPlaying = true; + } } }); @@ -1013,6 +1024,7 @@ private async void OnPlayerEnded(object sender, EventArgs e) var wasPlaying = IsPlaying; IsPlaying = false; + Interlocked.Increment(ref _playbackEndedCount); // Deliberately NOT gated on IsLoading: the front plays (and can end or die on a corrupt first chunk) while the secondary-camera joins are still in flight, and IsLoading stays true until that whole open completes. // The request-id check above already filters the transitions IsLoading used to guard (clip changes zero _currentMediaRequestId first).