From 3e2971cbbdb1376d2029f2ee37878c66bb326ce9 Mon Sep 17 00:00:00 2001 From: BlackSpirits Date: Wed, 16 Sep 2026 18:48:38 +0200 Subject: [PATCH] Validate integrated FFmpeg runtime hardening stack --- .../VideoPlayer/VideoPlayerControl.cs | 58 ++- .../Logic/VideoPlayers/Ffmpeg/FfmpegPlayer.cs | 466 +++++++++++++++--- .../Ffmpeg/FfmpegSoftwareControl.cs | 16 + .../VideoPlayers/Ffmpeg/VideoFrameQueue.cs | 17 +- src/ui/Logic/VideoPlayers/IVideoPlayer.cs | 8 + .../VideoPlayerControlTeardownTests.cs | 64 ++- tests/UI/Logic/FfmpegPlayerTests.cs | 147 ++++++ .../Logic/FfmpegVideoFrameQueueFlushTests.cs | 30 ++ 8 files changed, 717 insertions(+), 89 deletions(-) create mode 100644 tests/UI/Logic/FfmpegVideoFrameQueueFlushTests.cs diff --git a/src/ui/Controls/VideoPlayer/VideoPlayerControl.cs b/src/ui/Controls/VideoPlayer/VideoPlayerControl.cs index 5077f4d34a2..d6dbbeb8437 100644 --- a/src/ui/Controls/VideoPlayer/VideoPlayerControl.cs +++ b/src/ui/Controls/VideoPlayer/VideoPlayerControl.cs @@ -13,6 +13,7 @@ using Nikse.SubtitleEdit.Logic; using Nikse.SubtitleEdit.Logic.Config; using Nikse.SubtitleEdit.Logic.VideoPlayers; +using Nikse.SubtitleEdit.Logic.VideoPlayers.Ffmpeg; using Nikse.SubtitleEdit.Logic.VideoPlayers.LibMpvDynamic; using Optris.Icons.Avalonia; using System; @@ -966,11 +967,14 @@ private void UpdateVideoFileNameMaxWidth() _textBlockVideoFileName.MaxWidth = available > 20 ? available : 20; } - internal void Close() + private void StopCloseUiActivity() { _positionTimer?.Stop(); StopAutoHideControls(); - _videoPlayerInstance.CloseFile(); + } + + private void ResetClosedUiState() + { ProgressText = string.Empty; _videoFileName = string.Empty; _textBlockVideoFileName.Text = string.Empty; @@ -978,6 +982,13 @@ internal void Close() Duration = 0; } + internal void Close() + { + StopCloseUiActivity(); + _videoPlayerInstance.CloseFile(); + ResetClosedUiState(); + } + /// /// Set once has started tearing this control down. /// Every async open/restore sequence (Open, WaitForPlayersReadyAsync, the reopen @@ -1004,17 +1015,46 @@ internal void Close() /// until restart (issue #13048). /// /// - /// Order matters: stop and unload first so the player is idle, then drop the content - /// (which destroys the embedded window), and only then destroy the core. mpv's - /// mpv_terminate_destroy blocks until every worker has exited - milliseconds when - /// idle, but many seconds if a load is stuck on a slow path - so it runs on a worker - /// thread rather than freezing the UI (same reasoning as issue #11176). + /// Order matters for native render hosts: stop/unload first, mark any GL teardown state, + /// then drop the content and destroy the core on a worker. FFmpeg is the exception during + /// definitive teardown: its CloseFile itself joins demux/decode/presenter workers + /// and can wait for the teardown timeout, while Dispose already performs that close. + /// A player can therefore opt to defer the final CloseFile to the background Dispose; + /// the normal reusable path remains synchronous. /// /// internal void CloseAndDisposePlayer() { + if (IsDisposed) + { + return; + } + IsDisposed = true; - Close(); + var disposablePlayer = _videoPlayerInstance as IDisposable; + + // FFmpeg CloseFile joins its demux/decode/presenter workers and can wait for the + // teardown timeout. This control is being discarded permanently, so let Dispose do + // that close on the worker below instead of blocking the UI before we even detach. + // The normal Close() path remains synchronous because that path reuses the player. + if (_videoPlayerInstance.DeferCloseFileToDispose && disposablePlayer != null) + { + StopCloseUiActivity(); + ResetClosedUiState(); + } + else + { + Close(); + } + + // FfmpegSoftwareControl normally owns disposal from its detach callback. Final + // VideoPlayerControl teardown already has a background Dispose owner below, so transfer + // ownership first; otherwise Content = null would schedule two concurrent disposes. + if (PlayerContent is FfmpegSoftwareControl ffmpegControl && + _videoPlayerInstance is FfmpegPlayer ffmpegPlayer) + { + ffmpegControl.RelinquishPlayerForExternalDispose(ffmpegPlayer); + } // Mark before the content goes. On the OpenGL host mpv's render context may only be // freed from the GL deinit callback (the GL context has to be current), and dropping @@ -1024,7 +1064,7 @@ internal void CloseAndDisposePlayer() Content = null; - if (_videoPlayerInstance is not IDisposable disposablePlayer) + if (disposablePlayer == null) { return; } diff --git a/src/ui/Logic/VideoPlayers/Ffmpeg/FfmpegPlayer.cs b/src/ui/Logic/VideoPlayers/Ffmpeg/FfmpegPlayer.cs index 8da2fdb1399..f3f2c232727 100644 --- a/src/ui/Logic/VideoPlayers/Ffmpeg/FfmpegPlayer.cs +++ b/src/ui/Logic/VideoPlayers/Ffmpeg/FfmpegPlayer.cs @@ -84,6 +84,10 @@ public FfmpegPreviewSubtitle PreviewSubtitle public string Name => string.IsNullOrEmpty(_decoderName) ? "ffmpeg" : $"ffmpeg ({_decoderName})"; public string FileName => _fileName; + // CloseFile joins the demux/decode/presenter workers (up to the teardown timeout). During + // definitive UI teardown Dispose already performs that close on a worker thread. + public bool DeferCloseFileToDispose => true; + public bool CanLoad() { return FfmpegLibraries.IsAvailable(); @@ -155,7 +159,7 @@ public Task LoadFile(string fileName, double startPositionSeconds = 0) Session session; try { - session = new Session(this, fileName); + session = new Session(this, fileName, generation); } catch (Exception exception) { @@ -201,25 +205,55 @@ public Task LoadFile(string fileName, double startPositionSeconds = 0) { // CloseFile took and disposed the session while it was being started. } + catch (Exception exception) + { + Se.LogError(exception, $"ffmpeg player failed to start: {fileName}"); + + // Session.Start is transactional, but remove this failed Session from the owner + // as well. If CloseFile/replacement already took it, that path owns disposal. + if (Interlocked.CompareExchange(ref _session, null, session) == session) + { + session.Dispose(); + TryClearOwnerMediaStateForGeneration(generation); + } + + if (generation == Volatile.Read(ref _loadGeneration)) + { + _fileName = string.Empty; + } + } }); } public void CloseFile() { - Interlocked.Increment(ref _loadGeneration); + var generation = Interlocked.Increment(ref _loadGeneration); var session = Interlocked.Exchange(ref _session, null); _fileName = string.Empty; session?.Dispose(); + // Dispose can wait on stubborn workers. If another load became current meanwhile, this + // older close must not clear the newer session's decoder badge or presented frame. + TryClearOwnerMediaStateForGeneration(generation); + } + + internal bool TryClearOwnerMediaStateForGeneration(int loadGeneration) + { lock (_currentFrameLock) { - // The frame belonged to the session's pool, which is gone now. + if (loadGeneration != Volatile.Read(ref _loadGeneration)) + { + return false; + } + + _decoderName = string.Empty; _currentFrame?.Dispose(); _currentFrame = null; + Interlocked.Increment(ref _frameVersion); } - Interlocked.Increment(ref _frameVersion); FrameReady?.Invoke(); + return true; } public void Play() @@ -395,6 +429,16 @@ internal static double WallClockPosition(double basePosition, double elapsedSeco return basePosition + elapsedSeconds * speed; } + internal static bool ShouldInterruptOpen(bool closing, bool ownerDisposed, int loadGeneration, int currentGeneration) + { + return closing || ownerDisposed || loadGeneration != currentGeneration; + } + + internal static bool ShouldScheduleDeferredSessionCleanup(bool cleanupDeferred, int activeWorkers) + { + return cleanupDeferred && activeWorkers == 0; + } + public double Duration => _session?.Duration ?? 0; public int VolumeMaximum => 100; @@ -450,20 +494,53 @@ public void Dispose() CloseFile(); } - private void Present(VideoFrame frame, VideoFrameQueue pool) + internal bool TryPresentFrameFromSession(int loadGeneration, VideoFrame frame, VideoFrameQueue pool) { - VideoFrame? previous; + VideoFrame? previous = null; + var accepted = false; lock (_currentFrameLock) { - previous = _currentFrame; - _currentFrame = frame; + // CloseFile increments the generation before waiting for workers. A worker that + // outlives that wait must return its frame to its own pool instead of overwriting the + // current/new session's UI frame. + if (loadGeneration == Volatile.Read(ref _loadGeneration)) + { + previous = _currentFrame; + _currentFrame = frame; + Interlocked.Increment(ref _frameVersion); + accepted = true; + } + } + + if (!accepted) + { + pool.Return(frame); + return false; } pool.Return(previous); - Interlocked.Increment(ref _frameVersion); + return true; + } + + private void NotifyFrameReady() + { FrameReady?.Invoke(); } + internal bool TrySetDecoderNameFromSession(int loadGeneration, string decoderName) + { + lock (_currentFrameLock) + { + if (loadGeneration != Volatile.Read(ref _loadGeneration)) + { + return false; + } + + _decoderName = decoderName; + return true; + } + } + private static IAudioSink CreateAudioSink() { if (OperatingSystem.IsWindows()) @@ -510,6 +587,24 @@ internal static bool ShouldResendPacket(int sendResult, bool receivedOutput, boo return sendResult == -ffmpeg.EAGAIN && receivedOutput && !interrupted; } + internal static bool ShouldReplayHardwareSendFailure(int sendResult, bool hardware) + { + return hardware && + sendResult < 0 && + sendResult != -ffmpeg.EAGAIN && + sendResult != ffmpeg.AVERROR_EOF; + } + + internal static bool ShouldDropPacketBeforeHardwareReplay(int packetSerial, int minimumReplaySerial) + { + return minimumReplaySerial >= 0 && packetSerial < minimumReplaySerial; + } + + internal static bool CanPublishVideoFrame(int frameSerial, int currentSerial) + { + return frameSerial == currentSerial; + } + internal static bool AudioWriteFailureIsDeviceFailure( bool writeAccepted, bool closing, @@ -549,6 +644,21 @@ internal static bool AudioClockReadFailureIsDeviceFailure( currentSerial == requestedSerial; } + /// + /// ffplay only publishes end-of-stream after av_read_frame reports AVERROR_EOF or the + /// underlying AVIO context reports that reading ended. Demuxer-level errors such as + /// AVERROR_INVALIDDATA can be recoverable on the next read and must not truncate playback. + /// + internal static bool IsDemuxEndOfInput(int readResult, bool avioEnded) + { + return readResult == ffmpeg.AVERROR_EOF || avioEnded; + } + + internal static bool ShouldRetryDecoderOpenInSoftware(int openResult, bool hardwareRequested, bool hardwareAttached) + { + return openResult < 0 && hardwareRequested && hardwareAttached; + } + private static double TimestampToSeconds(long timestamp, AVRational timeBase) { return timestamp == ffmpeg.AV_NOPTS_VALUE ? double.NaN : timestamp * ffmpeg.av_q2d(timeBase); @@ -619,7 +729,13 @@ private sealed class Session : IDisposable private Thread? _videoThread; private Thread? _audioThread; private Thread? _presentThread; + private readonly Lock _lifecycleLock = new(); + private readonly int _ownerLoadGeneration; private volatile bool _closing; + private bool _resourcesDisposed; + private int _activeWorkers; + private bool _cleanupDeferredToWorkerExit; + private int _deferredCleanupScheduled; private readonly AutoResetEvent _demuxWake = new(false); private readonly AutoResetEvent _presentWake = new(false); @@ -676,18 +792,24 @@ private sealed class Session : IDisposable /// private static int InterruptCallback(void* opaque) { - if (opaque == null) + if (opaque == null || + GCHandle.FromIntPtr((IntPtr)opaque).Target is not Session session) { return 0; } - return GCHandle.FromIntPtr((IntPtr)opaque).Target is Session { _closing: true } ? 1 : 0; + return ShouldInterruptOpen( + session._closing, + session._owner._disposed, + session._ownerLoadGeneration, + Volatile.Read(ref session._owner._loadGeneration)) ? 1 : 0; } - public Session(FfmpegPlayer owner, string fileName) + public Session(FfmpegPlayer owner, string fileName, int ownerLoadGeneration) { _owner = owner; _fileName = fileName; + _ownerLoadGeneration = ownerLoadGeneration; var format = ffmpeg.avformat_alloc_context(); if (format == null) @@ -831,22 +953,102 @@ public double Speed public void Start() { - _demuxThread = new Thread(DemuxLoop) { IsBackground = true, Name = "ffmpeg demux" }; - _presentThread = new Thread(PresentLoop) { IsBackground = true, Name = "ffmpeg present" }; - _demuxThread.Start(); - _presentThread.Start(); + // CloseFile can take the published session before this load worker reaches Start(). + // Serialize startup with teardown so Dispose either sees every worker thread or wins + // first and makes this start fail without touching already-released native resources. + lock (_lifecycleLock) + { + if (_closing) + { + throw new ObjectDisposedException(nameof(Session)); + } - if (_hasVideo) + try + { + StartWorker(ref _demuxThread, DemuxLoop, "ffmpeg demux"); + StartWorker(ref _presentThread, PresentLoop, "ffmpeg present"); + + if (_hasVideo) + { + StartWorker(ref _videoThread, VideoLoop, "ffmpeg video"); + } + + if (_hasAudio) + { + StartWorker(ref _audioThread, AudioLoop, "ffmpeg audio"); + } + } + catch + { + // Worker startup is all-or-nothing. If a later Thread.Start fails after an + // earlier worker is already running, close/reclaim that partial Session before + // propagating the startup failure to the owner. + DisposeLocked(); + throw; + } + } + } + + private void StartWorker(ref Thread? field, ThreadStart loop, string name) + { + var thread = new Thread(() => { - _videoThread = new Thread(VideoLoop) { IsBackground = true, Name = "ffmpeg video" }; - _videoThread.Start(); + try + { + loop(); + } + finally + { + WorkerExited(); + } + }) + { + IsBackground = true, + Name = name, + }; + + field = thread; + Interlocked.Increment(ref _activeWorkers); + try + { + thread.Start(); } + catch + { + field = null; + Interlocked.Decrement(ref _activeWorkers); + throw; + } + } - if (_hasAudio) + private void WorkerExited() + { + Interlocked.Decrement(ref _activeWorkers); + ScheduleDeferredCleanupIfReady(); + } + + private void ScheduleDeferredCleanupIfReady() + { + if (!ShouldScheduleDeferredSessionCleanup( + Volatile.Read(ref _cleanupDeferredToWorkerExit), + Volatile.Read(ref _activeWorkers)) || + Interlocked.Exchange(ref _deferredCleanupScheduled, 1) != 0) { - _audioThread = new Thread(AudioLoop) { IsBackground = true, Name = "ffmpeg audio" }; - _audioThread.Start(); + return; } + + _ = Task.Run(() => + { + lock (_lifecycleLock) + { + if (!_resourcesDisposed && + _closing && + Volatile.Read(ref _activeWorkers) == 0) + { + ReleaseResources(); + } + } + }); } public void Play() @@ -1084,7 +1286,16 @@ private void DemuxLoop() continue; } - // End of file - or a read error, which ffplay treats the same way. + var avioEnded = _format->pb != null && ffmpeg.avio_feof(_format->pb) != 0; + if (!IsDemuxEndOfInput(result, avioEnded)) + { + // ffplay retries demuxer-level errors that are not real end-of-input. + // Some demuxers advance past malformed data before returning an error, + // so the next av_read_frame can recover and continue the file. + _demuxWake.WaitOne(10); + continue; + } + eof = true; _videoPackets.Push(null); _audioPackets.Push(null); @@ -1223,6 +1434,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 { @@ -1230,7 +1442,9 @@ private void VideoLoop() var hardware = HardwareDeviceTypes.Length > 0; codec = OpenDecoder(stream, hardware); hardware = codec->hw_device_ctx != null; - _owner._decoderName = hardware ? HardwareDeviceName(codec) : string.Empty; + _owner.TrySetDecoderNameFromSession( + _ownerLoadGeneration, + hardware ? HardwareDeviceName(codec) : string.Empty); frame = ffmpeg.av_frame_alloc(); transferFrame = ffmpeg.av_frame_alloc(); var timeBase = stream->time_base; @@ -1239,10 +1453,10 @@ private void VideoLoop() : 1.0 / 25.0; var serial = -1; + var minimumReplaySerial = -1; var dropUntil = -1.0; var presentedForSerial = false; var videoEndPosition = double.NaN; - VideoFrame? lastDropped = null; // kept so a target past the last picture still shows something while (!_closing) { @@ -1251,6 +1465,20 @@ private void VideoLoop() continue; } + if (ShouldDropPacketBeforeHardwareReplay(entry.Serial, minimumReplaySerial)) + { + var stalePacket = entry.Packet; + if (stalePacket != null) + { + ffmpeg.av_packet_free(&stalePacket); + } + + continue; + } + + // Equal is the replay seek itself; greater is a user seek that raced ahead. + minimumReplaySerial = -1; + if (entry.Serial != serial) { ffmpeg.avcodec_flush_buffers(codec); @@ -1263,25 +1491,28 @@ private void VideoLoop() } var packet = entry.Packet; + retryVideoPacket: var sendResult = ffmpeg.avcodec_send_packet(codec, packet); // null = drain at end of stream - if (packet != null) + var packetRejected = sendResult == -ffmpeg.EAGAIN; + if (!packetRejected && packet != null) { ffmpeg.av_packet_free(&packet); } - if (sendResult < 0 && sendResult != -ffmpeg.EAGAIN && sendResult != ffmpeg.AVERROR_EOF) + if (sendResult < 0 && !packetRejected && sendResult != ffmpeg.AVERROR_EOF) { - if (hardware) + if (ShouldReplayHardwareSendFailure(sendResult, hardware)) { - // The hardware decoder rejected the stream - retry it in software. FallBackToSoftware(ref codec, stream, sendResult, ref hardware); serial = -1; + minimumReplaySerial = RequestHardwareFallbackReplay(); } continue; } var hardwareFailed = false; + var receivedOutput = false; while (!_closing) { var receiveResult = ffmpeg.avcodec_receive_frame(codec, frame); @@ -1291,6 +1522,7 @@ private void VideoLoop() break; } + receivedOutput = true; var picture = frame; if (frame->hw_frames_ctx != null) { @@ -1390,6 +1622,22 @@ private void VideoLoop() _presentWake.Set(); } + var interrupted = _closing || SeekRequestedSince(serial); + if (!hardwareFailed && ShouldResendPacket(sendResult, receivedOutput, interrupted)) + { + goto retryVideoPacket; + } + + if (packetRejected && !interrupted && !receivedOutput && !hardwareFailed) + { + Se.LogError("ffmpeg player: decoder returned EAGAIN without output; dropping rejected video packet"); + } + + if (packet != null) + { + ffmpeg.av_packet_free(&packet); + } + if (hardwareFailed) { // The hardware decoder could not decode or hand back this picture @@ -1397,7 +1645,7 @@ private void VideoLoop() // the key frame. FallBackToSoftware(ref codec, stream, 0, ref hardware); serial = -1; - Seek(Position); + minimumReplaySerial = RequestHardwareFallbackReplay(); continue; } @@ -1431,6 +1679,8 @@ private void VideoLoop() } finally { + _videoFrames.Return(lastDropped); + if (sws != null) { ffmpeg.sws_freeContext(sws); @@ -1453,6 +1703,20 @@ private void VideoLoop() } } + private int RequestHardwareFallbackReplay() + { + var target = Position; + Seek(target); + + // Seek is asynchronous. Until demux commits it, queued packets may still belong to + // the failed hardware serial. The fresh software decoder must wait for this serial or + // a newer user-seek serial before consuming compressed input. + lock (_seekLock) + { + return _requestedSerial; + } + } + /// /// Replaces the hardware decoder context with a software one. The caller's pointer is /// nulled before the new decoder is opened, so when OpenDecoder throws the caller's @@ -1466,7 +1730,7 @@ private void FallBackToSoftware(ref AVCodecContext* codec, AVStream* stream, int codec = null; ffmpeg.avcodec_free_context(&old); hardware = false; - _owner._decoderName = string.Empty; + _owner.TrySetDecoderNameFromSession(_ownerLoadGeneration, string.Empty); codec = OpenDecoder(stream, hardware: false); } @@ -1668,8 +1932,19 @@ private static bool SupportsHardwareDevice(AVCodec* decoder, AVHWDeviceType devi result = ffmpeg.avcodec_open2(codec, decoder, null); if (result < 0) { + var hardwareAttached = codec->hw_device_ctx != null; + var retryInSoftware = ShouldRetryDecoderOpenInSoftware(result, hardware, hardwareAttached); + var hardwareName = retryInSoftware ? HardwareDeviceName(codec) : string.Empty; + var errorText = FfmpegLibraries.ErrorText(result); ffmpeg.avcodec_free_context(&codec); - throw new InvalidOperationException($"avcodec_open2: {FfmpegLibraries.ErrorText(result)}"); + + if (retryInSoftware) + { + Se.LogError($"ffmpeg player: {hardwareName} decoder open failed for {ffmpeg.avcodec_get_name(stream->codecpar->codec_id)} ({errorText}), falling back to software"); + return OpenDecoder(stream, hardware: false); + } + + throw new InvalidOperationException($"avcodec_open2: {errorText}"); } return codec; @@ -2400,15 +2675,24 @@ private void ReachEnd(double observedPosition) private void ShowFrame(VideoFrame frame) { - var popped = _videoFrames.Pop(); - if (!ReferenceEquals(popped, frame)) - { - _videoFrames.Return(popped); - return; - } - + var published = false; lock (_seekLock) { + // PresentLoop's earlier serial check is advisory only. A successful seek can + // advance _currentSerial before this frame is popped, so claim + publish while + // holding the same lock that commits seek serials. + 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; @@ -2421,38 +2705,67 @@ private void ShowFrame(VideoFrame frame) { _pausedPosition = frame.Pts; } + + // Timestamp and owner publication are ordered before the seek lock is released. + // The owner applies the independent load-generation fence under _currentFrameLock. + Interlocked.Exchange(ref _lastRestartTimestamp, Stopwatch.GetTimestamp()); + published = _owner.TryPresentFrameFromSession( + _ownerLoadGeneration, + frame, + _videoFrames); } - // 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); + // Never invoke UI/user callbacks while either the seek or current-frame lock is held. + if (published) + { + _owner.NotifyFrameReady(); + } } // ---------------------------------------------------------------- teardown public void Dispose() { - _closing = true; - _playing = false; - _videoPackets.Close(); - _audioPackets.Close(); - _videoFrames.Close(); - _demuxWake.Set(); - _presentWake.Set(); + lock (_lifecycleLock) + { + DisposeLocked(); + } + } - // The constructor disposes a half-built session (stream info failed, no usable - // stream) before the sink exists, so the sink is null on those paths. - var audioSink = _audioSink; - if (audioSink != null) + /// Teardown with already held. + private void DisposeLocked() + { + if (_resourcesDisposed) { - try - { - audioSink.Reset(int.MinValue); - } - catch + return; + } + + // A previous teardown attempt may have timed out and retained the worker-visible + // resources. Keep the Session closed; a later Dispose may retry the joins, and the + // last worker to exit also schedules one-shot reclamation. + if (!_closing) + { + _closing = true; + _playing = false; + _videoPackets.Close(); + _audioPackets.Close(); + _videoFrames.Close(); + _demuxWake.Set(); + _presentWake.Set(); + + // Reject every writer before joining workers. A failed native reset deliberately + // leaves the sink fenced; resources still stay alive until every worker is gone. + var sinkToReset = _audioSink; + if (sinkToReset != null) { - // sink may not have been opened + try + { + sinkToReset.Reset(int.MinValue); + } + catch + { + // Sink may not have been opened on a half-built constructor path. + } } } @@ -2461,19 +2774,26 @@ public void Dispose() stopped &= JoinThread(_audioThread); stopped &= JoinThread(_presentThread); - audioSink?.Dispose(); - _demuxWake.Dispose(); - _presentWake.Dispose(); - if (!stopped) { - // A worker is still inside libavformat/libavcodec with this context; closing it - // now would be a use-after-free. Leak it (and the handle its interrupt callback - // dereferences) rather than crash. - Se.LogError($"ffmpeg player: leaking the format context of '{_fileName}' because a thread did not stop"); + // A live worker may still touch format/codec state, the sink or wake events. + // Retain the complete boundary. If the worker later exits, #140's one-shot + // deferred cleanup reclaims it; a permanently stuck worker remains fail-closed. + Volatile.Write(ref _cleanupDeferredToWorkerExit, true); + Se.LogError($"ffmpeg player: retaining session resources for '{_fileName}' because a thread did not stop"); + ScheduleDeferredCleanupIfReady(); return; } + ReleaseResources(); + } + + private void ReleaseResources() + { + _audioSink?.Dispose(); + _demuxWake.Dispose(); + _presentWake.Dispose(); + if (_format != null) { var format = _format; @@ -2485,16 +2805,24 @@ public void Dispose() { _selfHandle.Free(); } + + _resourcesDisposed = true; } /// False when the thread is still running after the timeout. private static bool JoinThread(Thread? thread) { - if (thread == null || thread == Thread.CurrentThread) + if (thread == null) { return true; } + if (thread == Thread.CurrentThread) + { + Se.LogError($"ffmpeg player: teardown requested from worker thread '{thread.Name}'"); + return false; + } + if (thread.Join(TimeSpan.FromSeconds(5))) { return true; diff --git a/src/ui/Logic/VideoPlayers/Ffmpeg/FfmpegSoftwareControl.cs b/src/ui/Logic/VideoPlayers/Ffmpeg/FfmpegSoftwareControl.cs index f4098968e6a..fafa31074d0 100644 --- a/src/ui/Logic/VideoPlayers/Ffmpeg/FfmpegSoftwareControl.cs +++ b/src/ui/Logic/VideoPlayers/Ffmpeg/FfmpegSoftwareControl.cs @@ -37,6 +37,22 @@ public class FfmpegSoftwareControl : Control public FfmpegPlayer? Player => _player; + /// + /// Transfers final-disposal ownership to the containing VideoPlayerControl before that + /// control removes this render host. This prevents OnDetachedFromVisualTree from scheduling + /// a second concurrent Dispose for the same FfmpegPlayer. + /// + internal void RelinquishPlayerForExternalDispose(FfmpegPlayer player) + { + if (!ReferenceEquals(_player, player)) + { + return; + } + + player.FrameReady -= OnFrameReady; + _player = null; + } + public FfmpegSoftwareControl(FfmpegPlayer player) { _player = player; diff --git a/src/ui/Logic/VideoPlayers/Ffmpeg/VideoFrameQueue.cs b/src/ui/Logic/VideoPlayers/Ffmpeg/VideoFrameQueue.cs index f705fe3a7b5..cb21829071e 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/src/ui/Logic/VideoPlayers/IVideoPlayer.cs b/src/ui/Logic/VideoPlayers/IVideoPlayer.cs index b4793f2e0f0..ab7c745a946 100644 --- a/src/ui/Logic/VideoPlayers/IVideoPlayer.cs +++ b/src/ui/Logic/VideoPlayers/IVideoPlayer.cs @@ -22,6 +22,14 @@ public interface IVideoPlayer Task LoadFile(string fileName, double startPositionSeconds = 0); void CloseFile(); + /// + /// True when may block long enough that definitive control teardown + /// should let perform it on the background dispose + /// thread instead of calling it first on the UI thread. The normal reusable Close path still + /// calls directly. + /// + bool DeferCloseFileToDispose => false; + void Play(); void PlayOrPause(); void Pause(); diff --git a/tests/UI/Controls/VideoPlayerControlTeardownTests.cs b/tests/UI/Controls/VideoPlayerControlTeardownTests.cs index 50f6465a06b..ad2b0e94dde 100644 --- a/tests/UI/Controls/VideoPlayerControlTeardownTests.cs +++ b/tests/UI/Controls/VideoPlayerControlTeardownTests.cs @@ -3,6 +3,7 @@ using Nikse.SubtitleEdit.Controls.VideoPlayer; using Nikse.SubtitleEdit.Logic; using Nikse.SubtitleEdit.Logic.VideoPlayers; +using Nikse.SubtitleEdit.Logic.VideoPlayers.Ffmpeg; using Nikse.SubtitleEdit.Logic.VideoPlayers.LibMpvDynamic; using System; using System.Reflection; @@ -23,6 +24,8 @@ private sealed class FakeVideoPlayer : IVideoPlayer, IDisposable { public int DisposeCount; public int CloseFileCount; + public int CloseFileThreadId; + public bool DeferCloseFileToDispose { get; set; } public string Name => "fake"; public string FileName { get; private set; } = string.Empty; @@ -38,6 +41,7 @@ public Task LoadFile(string fileName, double startPositionSeconds = 0) public void CloseFile() { CloseFileCount++; + CloseFileThreadId = Environment.CurrentManagedThreadId; FileName = string.Empty; } @@ -67,7 +71,14 @@ public void Stop() public double Volume { get; set; } = 50; public double Speed { get; set; } = 1.0; - public void Dispose() => DisposeCount++; + public void Dispose() + { + DisposeCount++; + if (DeferCloseFileToDispose) + { + CloseFile(); + } + } } private static UiTickPump? GetPositionTimer(VideoPlayerControl control) => @@ -129,6 +140,55 @@ public async Task CloseAndDisposePlayerDisposesThePlayer() Assert.True(player.CloseFileCount > 0); } + [AvaloniaFact] + public async Task CloseAndDisposePlayerDefersBlockingCloseToDisposeThread() + { + var player = new FakeVideoPlayer { DeferCloseFileToDispose = true }; + var control = await MakeOpenedControlAsync(player); + var callerThreadId = Environment.CurrentManagedThreadId; + + control.CloseAndDisposePlayer(); + + await WaitForAsync(() => player.DisposeCount > 0); + Assert.Equal(1, player.CloseFileCount); + Assert.NotEqual(callerThreadId, player.CloseFileThreadId); + Assert.False(GetPositionTimer(control)?.IsEnabled); + Assert.Null(control.Content); + } + + [AvaloniaFact] + public async Task CloseStillClosesDeferredPlayerSynchronouslyForReuse() + { + var player = new FakeVideoPlayer { DeferCloseFileToDispose = true }; + var control = await MakeOpenedControlAsync(player); + var callerThreadId = Environment.CurrentManagedThreadId; + + control.Close(); + + Assert.Equal(1, player.CloseFileCount); + Assert.Equal(callerThreadId, player.CloseFileThreadId); + Assert.Equal(0, player.DisposeCount); + Assert.NotNull(control.Content); + } + + [AvaloniaFact] + public void CloseAndDisposePlayerTransfersFfmpegRenderHostDisposalOwnership() + { + var player = new FfmpegPlayer(); + var host = new FfmpegSoftwareControl(player); + var control = new VideoPlayerControl(player) + { + PlayerContent = host, + }; + + control.CloseAndDisposePlayer(); + + // The render host must no longer own/subcribe to the player before Content is detached; + // VideoPlayerControl is now the single background Dispose owner. + Assert.Null(host.Player); + Assert.Null(control.Content); + } + [AvaloniaFact] public async Task CloseAndDisposePlayerIsSafeToRepeat() { @@ -142,6 +202,8 @@ public async Task CloseAndDisposePlayerIsSafeToRepeat() // this twice; the player's own Dispose is the guard, this must not throw. control.CloseAndDisposePlayer(); + Assert.Equal(1, player.CloseFileCount); + Assert.Equal(1, player.DisposeCount); Assert.Null(control.Content); Assert.False(GetPositionTimer(control)?.IsEnabled); } diff --git a/tests/UI/Logic/FfmpegPlayerTests.cs b/tests/UI/Logic/FfmpegPlayerTests.cs index 101220c5047..44ab73ef57e 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() { @@ -460,6 +468,102 @@ public void SpeedChange_MustCaptureWallClockPositionBeforeChangingRate() Assert.Equal(20, wrongTargetIfRateChangesFirst); } + [Theory] + [InlineData(false, false, 7, 7, false)] + [InlineData(true, false, 7, 7, true)] + [InlineData(false, true, 7, 7, true)] + [InlineData(false, false, 7, 8, true)] + public void ShouldInterruptOpen_StopsClosingDisposedOrStaleLoads( + bool closing, + bool ownerDisposed, + int loadGeneration, + int currentGeneration, + bool expected) + { + Assert.Equal( + expected, + FfmpegPlayer.ShouldInterruptOpen(closing, ownerDisposed, loadGeneration, currentGeneration)); + } + + [Fact] + public void StaleSessionCannotOverwriteDecoderBadgeAfterClose() + { + using var player = new FfmpegPlayer(); + + Assert.True(player.TrySetDecoderNameFromSession(0, "old-hardware")); + Assert.Contains("old-hardware", player.Name); + + player.CloseFile(); // generation 0 -> 1 and clears owner-visible state + + Assert.False(player.TrySetDecoderNameFromSession(0, "stale-hardware")); + Assert.Equal("ffmpeg", player.Name); + } + + [Fact] + public void StaleCloseCleanupCannotClearNewerGenerationMediaState() + { + using var player = new FfmpegPlayer(); + var queue = new VideoFrameQueue(1); + var serial = 0; + + player.CloseFile(); // generation 0 -> 1 + + Assert.True(player.TrySetDecoderNameFromSession(1, "current-hardware")); + var frame = queue.Rent(4, 4, 0, ref serial)!; + Assert.True(player.TryPresentFrameFromSession(1, frame, queue)); + var currentVersion = player.FrameVersion; + + Assert.False(player.TryClearOwnerMediaStateForGeneration(0)); + Assert.Contains("current-hardware", player.Name); + Assert.Equal((4, 4), player.CurrentFrameSize); + Assert.Equal(currentVersion, player.FrameVersion); + + Assert.True(player.TryClearOwnerMediaStateForGeneration(1)); + Assert.Equal("ffmpeg", player.Name); + Assert.Equal((0, 0), player.CurrentFrameSize); + Assert.Equal(currentVersion + 1, player.FrameVersion); + + queue.Close(); + } + + [Fact] + public void StaleSessionFrameIsReturnedInsteadOfPublishedAfterClose() + { + using var player = new FfmpegPlayer(); + var queue = new VideoFrameQueue(1); + var serial = 0; + var frame = queue.Rent(4, 4, 0, ref serial)!; + + player.CloseFile(); // invalidate generation 0 before the stale worker publishes + var versionAfterClose = player.FrameVersion; + + Assert.False(player.TryPresentFrameFromSession(0, frame, queue)); + Assert.Equal(versionAfterClose, player.FrameVersion); + Assert.Equal((0, 0), player.CurrentFrameSize); + + // Rejection returns ownership to the originating session's pool instead of leaking the + // frame or handing it to the new/current session. + var reused = queue.Rent(4, 4, 0, ref serial); + Assert.Same(frame, reused); + queue.Return(reused); + queue.Close(); + } + + [Theory] + [InlineData(false, 0, false)] + [InlineData(true, 2, false)] + [InlineData(true, 1, false)] + [InlineData(true, 0, true)] + public void ShouldScheduleDeferredSessionCleanup_RequiresDeferredLastWorkerExit( + bool cleanupDeferred, + int activeWorkers, + bool expected) + { + Assert.Equal( + expected, + FfmpegPlayer.ShouldScheduleDeferredSessionCleanup(cleanupDeferred, activeWorkers)); + } + [Theory] [InlineData(12.5, 60.0, 12.5)] [InlineData(75.0, 60.0, 60.0)] // past the end: clamped to the duration @@ -627,6 +731,49 @@ public void WaveOutPositionWrapBase_FormatSwitchRoundingDoesNotInventAFullWrap() wrapBytes: wrap)); } + [Fact] + public void IsDemuxEndOfInput_DistinguishesRecoverableReadErrors() + { + Assert.True(FfmpegPlayer.IsDemuxEndOfInput(ffmpeg.AVERROR_EOF, avioEnded: false)); + Assert.True(FfmpegPlayer.IsDemuxEndOfInput(ffmpeg.AVERROR_INVALIDDATA, avioEnded: true)); + Assert.False(FfmpegPlayer.IsDemuxEndOfInput(ffmpeg.AVERROR_INVALIDDATA, avioEnded: false)); + Assert.False(FfmpegPlayer.IsDemuxEndOfInput(-ffmpeg.EAGAIN, avioEnded: false)); + } + + [Fact] + public void ShouldRetryDecoderOpenInSoftware_OnlyAfterAttachedHardwareFailure() + { + Assert.True(FfmpegPlayer.ShouldRetryDecoderOpenInSoftware(-1234, hardwareRequested: true, hardwareAttached: true)); + Assert.False(FfmpegPlayer.ShouldRetryDecoderOpenInSoftware(-1234, hardwareRequested: false, hardwareAttached: true)); + Assert.False(FfmpegPlayer.ShouldRetryDecoderOpenInSoftware(-1234, hardwareRequested: true, hardwareAttached: false)); + Assert.False(FfmpegPlayer.ShouldRetryDecoderOpenInSoftware(0, hardwareRequested: true, hardwareAttached: true)); + } + + [Fact] + public void ShouldReplayHardwareSendFailure_OnlyForFatalHardwareErrors() + { + Assert.True(FfmpegPlayer.ShouldReplayHardwareSendFailure(-1234, hardware: true)); + Assert.False(FfmpegPlayer.ShouldReplayHardwareSendFailure(-1234, hardware: false)); + Assert.False(FfmpegPlayer.ShouldReplayHardwareSendFailure(-ffmpeg.EAGAIN, hardware: true)); + Assert.False(FfmpegPlayer.ShouldReplayHardwareSendFailure(ffmpeg.AVERROR_EOF, hardware: true)); + Assert.False(FfmpegPlayer.ShouldReplayHardwareSendFailure(0, hardware: true)); + } + + [Theory] + [InlineData(6, -1, false)] + [InlineData(6, 7, true)] + [InlineData(7, 7, false)] + [InlineData(8, 7, false)] + public void ShouldDropPacketBeforeHardwareReplay_RequiresOlderSerial( + int packetSerial, + int minimumReplaySerial, + bool expected) + { + Assert.Equal( + expected, + FfmpegPlayer.ShouldDropPacketBeforeHardwareReplay(packetSerial, minimumReplaySerial)); + } + [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 00000000000..da35877162a --- /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(); + } +} \ No newline at end of file