diff --git a/src/ui/Logic/VideoPlayers/Ffmpeg/FfmpegPlayer.cs b/src/ui/Logic/VideoPlayers/Ffmpeg/FfmpegPlayer.cs index e01e1fc6c2..07162c5123 100644 --- a/src/ui/Logic/VideoPlayers/Ffmpeg/FfmpegPlayer.cs +++ b/src/ui/Logic/VideoPlayers/Ffmpeg/FfmpegPlayer.cs @@ -304,6 +304,45 @@ public double Position return duration > 0 ? Math.Min(value, duration) : value; } + /// + /// State visible after a native seek failure. Only the request that actually failed may roll + /// back to the committed pipeline serial/position; a newer request that arrived while + /// av_seek_frame was blocked must remain pending. + /// + internal static (int RequestedSerial, double RequestedTarget) FailedSeekState( + int failedSerial, + int currentSerial, + int requestedSerial, + double requestedTarget, + double currentPosition) + { + return requestedSerial == failedSerial + ? (currentSerial, currentPosition) + : (requestedSerial, requestedTarget); + } + + internal static bool ShouldAutoRewindOnPlay( + bool hasOutstandingSeek, + bool endReached, + double duration, + double position) + { + return !hasOutstandingSeek && + (endReached || (duration > 0 && position >= duration - 0.01)); + } + + internal static bool ShouldReachAudioOnlyEnd( + bool playing, + bool hasOutstandingSeek, + double duration, + double position) + { + return playing && + !hasOutstandingSeek && + duration > 0 && + position >= duration; + } + public double Duration => _session?.Duration ?? 0; public int VolumeMaximum => 100; @@ -359,7 +398,7 @@ public void Dispose() CloseFile(); } - private void Present(VideoFrame frame, VideoFrameQueue pool) + private void PublishFrame(VideoFrame frame, VideoFrameQueue pool) { VideoFrame? previous; lock (_currentFrameLock) @@ -370,6 +409,10 @@ private void Present(VideoFrame frame, VideoFrameQueue pool) pool.Return(previous); Interlocked.Increment(ref _frameVersion); + } + + private void NotifyFrameReady() + { FrameReady?.Invoke(); } @@ -405,6 +448,11 @@ or AVSampleFormat.AV_SAMPLE_FMT_DBLP or AVSampleFormat.AV_SAMPLE_FMT_S64P; } + internal static bool CanPublishVideoFrame(int frameSerial, int currentSerial) + { + return frameSerial == currentSerial; + } + /// /// avcodec_send_packet(EAGAIN) means the decoder rejected the input. The same packet may be /// resent only after receive_frame made progress, and only while the serial is still current. @@ -679,12 +727,12 @@ public void Play() return; } - if (_endReached || (Duration > 0 && Position >= Duration - 0.01)) + var hasOutstandingSeek = HasOutstandingSeek(); + if (ShouldAutoRewindOnPlay(hasOutstandingSeek, _endReached, Duration, Position)) { Seek(0); } - _endReached = false; _playing = true; _wallClockBase = _pausedPosition; _wallClock.Restart(); @@ -736,10 +784,11 @@ public void Seek(double seconds) _requestedSerial++; _requestedTarget = seconds; _seekPending = true; - _pausedPosition = seconds; } - _endReached = false; + // Keep the committed playhead/end state unchanged until libavformat accepts the seek. + // Position reports _requestedTarget while the request is outstanding, so optimistic + // mutation here is unnecessary and would corrupt state if av_seek_frame fails. _demuxWake.Set(); } @@ -799,6 +848,14 @@ private double Clock() return _wallClockBase + _wallClock.Elapsed.TotalSeconds * _speed; } + private bool HasOutstandingSeek() + { + lock (_seekLock) + { + return _requestedSerial != _currentSerial; + } + } + /// True when a seek newer than the given serial has been requested (performed or not). private bool SeekRequestedSince(int serial) { @@ -819,8 +876,11 @@ private void DemuxLoop() { if (TryTakeSeek(out var target, out var serial)) { - PerformSeek(target, serial); - eof = false; + if (PerformSeek(target, serial)) + { + eof = false; + } + continue; } @@ -894,13 +954,15 @@ private bool TryTakeSeek(out double target, out int serial) } } - private void PerformSeek(double target, int serial) + private bool PerformSeek(double target, int serial) { var timestamp = (long)((target + _startTimeSeconds) * ffmpeg.AV_TIME_BASE); var result = ffmpeg.av_seek_frame(_format, -1, timestamp, ffmpeg.AVSEEK_FLAG_BACKWARD); if (result < 0) { - System.Diagnostics.Debug.WriteLine($"ffmpeg seek failed: {FfmpegLibraries.ErrorText(result)}"); + Se.LogError($"ffmpeg player: seek to {target:0.###} s failed ({FfmpegLibraries.ErrorText(result)})"); + RollBackFailedSeek(serial); + return false; } lock (_seekLock) @@ -920,11 +982,35 @@ private void PerformSeek(double target, int serial) } } + _endReached = false; _videoPackets.Flush(serial, target); _audioPackets.Flush(serial, target); _videoFrames.Flush(); _audioSink.Reset(); _presentWake.Set(); + return true; + } + + private void RollBackFailedSeek(int serial) + { + // Seek no longer mutates the committed paused position, so this remains the actual + // position when paused. While playing, Clock() follows the still-current pipeline. + var currentPosition = _playing ? Clock() : _pausedPosition; + + lock (_seekLock) + { + var state = FailedSeekState( + serial, + _currentSerial, + _requestedSerial, + _requestedTarget, + currentPosition); + + _requestedSerial = state.RequestedSerial; + _requestedTarget = state.RequestedTarget; + } + + _presentWake.Set(); } // ---------------------------------------------------------------- video decode @@ -940,6 +1026,7 @@ private void VideoLoop() var swsSourceFormat = AVPixelFormat.AV_PIX_FMT_NONE; var outputWidth = 0; var outputHeight = 0; + VideoFrame? lastDropped = null; // detached from the queue while seeking; return it on every exit try { @@ -959,7 +1046,6 @@ private void VideoLoop() var minimumReplaySerial = -1; var dropUntil = -1.0; var presentedForSerial = false; - VideoFrame? lastDropped = null; // kept so a target past the last picture still shows something while (!_closing) { @@ -1171,6 +1257,8 @@ private void VideoLoop() } finally { + _videoFrames.Return(lastDropped); + if (sws != null) { ffmpeg.sws_freeContext(sws); @@ -1758,6 +1846,15 @@ private void PresentLoop() continue; } + if (SeekRequestedSince(frame.Serial)) + { + // Keep the old EOS marker until the seek commits or fails. Without + // this guard a video-only stream can ReachEnd while av_seek_frame is + // still blocked and lose a Play issued for the requested destination. + _presentWake.WaitOne(20); + continue; + } + if (_hasAudio) { // Video ended first; let the audio play out before stopping. The wake @@ -1868,7 +1965,7 @@ private void PresentLoop() private void PresentAudioOnlyTick() { - if (_playing && Duration > 0 && Clock() >= Duration) + if (ShouldReachAudioOnlyEnd(_playing, HasOutstandingSeek(), Duration, Clock())) { ReachEnd(); } @@ -1885,15 +1982,25 @@ private void ReachEnd() private void ShowFrame(VideoFrame frame) { - var popped = _videoFrames.Pop(); - if (!ReferenceEquals(popped, frame)) - { - _videoFrames.Return(popped); - return; - } - + var published = false; lock (_seekLock) { + // The initial PresentLoop check is only advisory: a successful seek can advance + // _currentSerial before this frame is popped. Claim and publish the frame while + // holding the same lock that commits seek serials, so an old serial can never be + // published after a newer seek has committed. + if (!CanPublishVideoFrame(frame.Serial, _currentSerial)) + { + return; + } + + var popped = _videoFrames.Pop(); + if (!ReferenceEquals(popped, frame)) + { + _videoFrames.Return(popped); + return; + } + if (frame.Serial > _restartSerial) { _restartSerial = frame.Serial; @@ -1906,12 +2013,18 @@ private void ShowFrame(VideoFrame frame) { _pausedPosition = frame.Pts; } + + // Publish frame state before releasing the seek lock. The callback itself is + // deliberately deferred until after the lock so UI/user code never runs under it. + Interlocked.Exchange(ref _lastRestartTimestamp, Stopwatch.GetTimestamp()); + _owner.PublishFrame(frame, _videoFrames); + published = true; } - // Timestamp after the serial so HasPlaybackRestartedSince never sees a new - // timestamp with an old serial. - Interlocked.Exchange(ref _lastRestartTimestamp, Stopwatch.GetTimestamp()); - _owner.Present(frame, _videoFrames); + if (published) + { + _owner.NotifyFrameReady(); + } } // ---------------------------------------------------------------- teardown diff --git a/src/ui/Logic/VideoPlayers/Ffmpeg/VideoFrameQueue.cs b/src/ui/Logic/VideoPlayers/Ffmpeg/VideoFrameQueue.cs index f705fe3a7b..cb21829071 100644 --- a/src/ui/Logic/VideoPlayers/Ffmpeg/VideoFrameQueue.cs +++ b/src/ui/Logic/VideoPlayers/Ffmpeg/VideoFrameQueue.cs @@ -156,22 +156,19 @@ public void Return(VideoFrame? frame) } } - /// Drops all queued frames (back to the pool). + /// + /// Drops all queued frames after a seek. Queued frames are disposed rather than returned to + /// the pool: the presenter may still hold a reference obtained from Peek/PeekSecond before + /// this flush. Reusing such an object immediately would let the decoder overwrite its serial, + /// timestamp and pixel buffer while the presenter is still reading the stale reference. + /// public void Flush() { lock (_lock) { while (_frames.Count > 0) { - var frame = _frames.Dequeue(); - if (frame.Data != System.IntPtr.Zero && frame.Width == _width && frame.Height == _height) - { - _pool.Push(frame); - } - else - { - frame.Dispose(); - } + _frames.Dequeue().Dispose(); } Monitor.PulseAll(_lock); diff --git a/tests/UI/Logic/FfmpegPlayerTests.cs b/tests/UI/Logic/FfmpegPlayerTests.cs index 522c3bd496..6104063021 100644 --- a/tests/UI/Logic/FfmpegPlayerTests.cs +++ b/tests/UI/Logic/FfmpegPlayerTests.cs @@ -106,6 +106,14 @@ public void VideoFrameQueue_PeekSecond_SeesTheFrameBehindTheHead() queue.Close(); } + [Theory] + [InlineData(7, 7, true)] + [InlineData(7, 8, false)] + public void CanPublishVideoFrame_RequiresCurrentSeekSerial(int frameSerial, int currentSerial, bool expected) + { + Assert.Equal(expected, FfmpegPlayer.CanPublishVideoFrame(frameSerial, currentSerial)); + } + [Fact] public void VideoFrameQueue_SizeChange_DropsOldPool() { @@ -279,6 +287,73 @@ public void ShouldDropPacketBeforeHardwareReplay_RequiresOlderSerial( FfmpegPlayer.ShouldDropPacketBeforeHardwareReplay(packetSerial, minimumReplaySerial)); } + [Fact] + public void FailedSeekState_LatestRequestRollsBackToCommittedPipeline() + { + var state = FfmpegPlayer.FailedSeekState( + failedSerial: 4, + currentSerial: 3, + requestedSerial: 4, + requestedTarget: 42.0, + currentPosition: 12.5); + + Assert.Equal(3, state.RequestedSerial); + Assert.Equal(12.5, state.RequestedTarget); + } + + [Fact] + public void FailedSeekState_StaleFailureDoesNotEraseNewerRequest() + { + var state = FfmpegPlayer.FailedSeekState( + failedSerial: 4, + currentSerial: 3, + requestedSerial: 5, + requestedTarget: 55.0, + currentPosition: 12.5); + + Assert.Equal(5, state.RequestedSerial); + Assert.Equal(55.0, state.RequestedTarget); + } + + [Theory] + [InlineData(false, true, 60.0, 60.0, true)] + [InlineData(false, false, 60.0, 60.0, true)] + [InlineData(false, false, 0.0, 60.0, false)] + [InlineData(true, true, 60.0, 60.0, false)] + [InlineData(true, false, 60.0, 60.0, false)] + public void ShouldAutoRewindOnPlay_DoesNotOverwriteOutstandingSeek( + bool hasOutstandingSeek, + bool endReached, + double duration, + double position, + bool expected) + { + Assert.Equal(expected, FfmpegPlayer.ShouldAutoRewindOnPlay( + hasOutstandingSeek, + endReached, + duration, + position)); + } + + [Theory] + [InlineData(true, false, 60.0, 60.0, true)] + [InlineData(true, true, 60.0, 60.0, false)] + [InlineData(false, false, 60.0, 60.0, false)] + [InlineData(true, false, 0.0, 60.0, false)] + public void ShouldReachAudioOnlyEnd_WaitsForOutstandingSeek( + bool playing, + bool hasOutstandingSeek, + double duration, + double position, + bool expected) + { + Assert.Equal(expected, FfmpegPlayer.ShouldReachAudioOnlyEnd( + playing, + hasOutstandingSeek, + duration, + position)); + } + [Theory] [InlineData(AVSampleFormat.AV_SAMPLE_FMT_U8, false)] [InlineData(AVSampleFormat.AV_SAMPLE_FMT_S16, false)] diff --git a/tests/UI/Logic/FfmpegVideoFrameQueueFlushTests.cs b/tests/UI/Logic/FfmpegVideoFrameQueueFlushTests.cs new file mode 100644 index 0000000000..3930458ac2 --- /dev/null +++ b/tests/UI/Logic/FfmpegVideoFrameQueueFlushTests.cs @@ -0,0 +1,30 @@ +using Nikse.SubtitleEdit.Logic.VideoPlayers.Ffmpeg; + +namespace UITests.Logic; + +public class FfmpegVideoFrameQueueFlushTests +{ + [Fact] + public void Flush_DisposesQueuedFrameInsteadOfReusingPeekedReference() + { + var queue = new VideoFrameQueue(2); + var serial = 1; + var frame = queue.Rent(8, 8, serial, ref serial)!; + queue.Push(frame); + + var peeked = queue.Peek(); + Assert.Same(frame, peeked); + + queue.Flush(); + + Assert.Equal(IntPtr.Zero, peeked!.Data); + Assert.Equal(0, queue.Count); + + var replacement = queue.Rent(8, 8, serial, ref serial)!; + Assert.NotSame(peeked, replacement); + Assert.NotEqual(IntPtr.Zero, replacement.Data); + + queue.Return(replacement); + queue.Close(); + } +}