diff --git a/src/ui/Controls/VideoPlayer/VideoPlayerControl.cs b/src/ui/Controls/VideoPlayer/VideoPlayerControl.cs
index 5077f4d34a..d6dbbeb843 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 8da2fdb139..f3f2c23272 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 f4098968e6..fafa31074d 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 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/src/ui/Logic/VideoPlayers/IVideoPlayer.cs b/src/ui/Logic/VideoPlayers/IVideoPlayer.cs
index b4793f2e0f..ab7c745a94 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 50f6465a06..ad2b0e94dd 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 101220c504..44ab73ef57 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 0000000000..da35877162
--- /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