Jump / seek makes video to freeze for few seconds if playback speed is increased

[antonsoroko]

Bug report

Describe the bug

Here is a clear and concise description of what the problem is:

(Note: this is about tempoup and not about "fast forward".)

Video always freezes for few seconds even if I do "jump" just for 10 seconds ahead.
I tried to increase cache - it did not help. I mean for local video file, not even a network stream. I also tried to disable hardware decoding, switching other video related settings - no effect.
And I have this issue on all my devices / OSes: certified android tv box with Amlogic S905X4 and PC with Linux/Windows with i5-8259U. For Windows it was freshly installed kodi with default settings.
I tested on videos such: 720p h264, 1080p h264, 4k h265.
Just to be sure that this is not hardware issue: there is no freeze on Linux with vlc / mpv and Android TV with mx player / vimu.

Expected Behavior

Here is a clear and concise description of what was expected to happen:

Jump / seek should not make video to freeze if playback speed is increased.

Actual Behavior

Jump / seek makes video to freeze for few seconds if playback speed is increased.

Possible Fix

To Reproduce

Steps to reproduce the behavior:

1. open video file
2. increase speed (1.5 fox example) via tempoup (not via "fast forward")
3. do "jump ahead" for 10 seconds (e.g. press right arrow on keyboard)
4. see how video freezes for few seconds

Debuglog

The debuglog can be found here:
https://paste.kodi.tv/uyiyorisox.kodi

Screenshots

Here are some links or screenshots to help explain the problem:

Additional context or screenshots (if appropriate)

Here is some additional context or explanation that might help:

Your Environment

Used Operating system:

• Android

• iOS

• tvOS

• Linux

• macOS

• Windows

• Windows UWP

• Operating system version/name: Linux 6.5.0-14 (Ubuntu 23.10), Android (Google) TV 11, Windows 11

• Kodi version: 20.2

note: Once the issue is made we require you to update it with new information or Kodi versions should that be required.
Team Kodi will consider your problem report however, we will not make any promises the problem will be solved.

[kambala-decapitator]

also noticed this in Omega on macOS

[enen92]

This happens because of consecutive seeks that happen (after an actual seek) by https://github.com/xbmc/xbmc/blob/master/xbmc/cores/VideoPlayer/VideoPlayer.cpp#L2179-L2193
Removing this correction fixes the issue and actually improves the ff behaviour for high rates (8x 32x) but ofc it's not a proper fix. Maybe @FernetMenta can give some advice.
A bit above we have this:

we'd need to have some way to not resync the clock
after a seek to remember timing

which might explain the issue

[FernetMenta]

This happens because of consecutive seeks that happen (after an actual seek) by https://github.com/xbmc/xbmc/blob/master/xbmc/cores/VideoPlayer/VideoPlayer.cpp#L2179-L2193 Removing this correction fixes the issue and actually improves the ff behaviour for high rates (8x 32x) but ofc it's not a proper fix. Maybe @FernetMenta can give some advice. A bit above we have this:

we'd need to have some way to not resync the clock
after a seek to remember timing

which might explain the issue

The comment you quoted is from stone age (svn times) and misleading. Not sure why I didn't remove it 9 years ago. Also a long time :)
I think it is very likely that the scenario of doing a seek while going ff was just not considered and the logic interprets the position after seek as an error it wants to compensate with an additional seek, which finally leads to inconsistancy in states between VideoPlayer and its stream players for audio and video.
Maybe resetting m_SpeedState in FlushBuffers (gets called after a seek) helps.

@enen92 going 1.5 or >2x is a different story. 1.5 is tempo with working audio (implemented much later than this logic here) and ff/rw. First check it seek also fails with ff.

[enen92]

@FernetMenta resetting m_SpeedState unfortunately does not work, check (https://github.com/xbmc/xbmc/blob/master/xbmc/cores/VideoPlayer/VideoPlayer.cpp#L2148) is still true and it enters the branch - forcing a seek since the error is greater than the threshold.
However, setting the lastseek pts in flushbuffers fixes the issue:

From 616b2413eb884e79aab7971e42a59fa521fb3652 Mon Sep 17 00:00:00 2001
From: enen92 <92enen@gmail.com>
Date: Thu, 8 Feb 2024 19:58:52 +0000
Subject: [PATCH] Set lastseek value on flushbuffers

---
 xbmc/cores/VideoPlayer/VideoPlayer.cpp | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xbmc/cores/VideoPlayer/VideoPlayer.cpp b/xbmc/cores/VideoPlayer/VideoPlayer.cpp
index 9efe8f4bf7..bfb96326f5 100644
--- a/xbmc/cores/VideoPlayer/VideoPlayer.cpp
+++ b/xbmc/cores/VideoPlayer/VideoPlayer.cpp
@@ -3965,6 +3965,9 @@ void CVideoPlayer::FlushBuffers(double pts, bool accurate, bool sync)
     m_CurrentRadioRDS.inited  = false;
   }
 
+  if (pts != DVD_NOPTS_VALUE)
+    m_SpeedState.lastseekpts = pts;
+
   m_CurrentAudio.dts         = DVD_NOPTS_VALUE;
   m_CurrentAudio.startpts    = startpts;
   m_CurrentAudio.packets = 0;
-- 
2.43.0

Would this be a proper fix or do you have any other suggestions?

[FernetMenta]

Would this be a proper fix or do you have any other suggestions?

I think this is correct. Maybe some cosmetics: instead of memset m_SpeedState in some places you could add a reset method (with current pts) to this struct and call it FlushBuffers too.

[CrystalP]

It improved seeking while fast forwarding but not with tempo play unfortunately.

[enen92]

Hum I did not test seeks with tempo up/down, only the actual playback. Will try to take a look

[enen92]

@CrystalP I think I've found the issue. Can you give a go to the following patch?

diff --git a/xbmc/cores/VideoPlayer/VideoPlayer.cpp b/xbmc/cores/VideoPlayer/VideoPlayer.cpp
index ad1741751b..7ac7c576ea 100644
--- a/xbmc/cores/VideoPlayer/VideoPlayer.cpp
+++ b/xbmc/cores/VideoPlayer/VideoPlayer.cpp
@@ -2157,7 +2157,7 @@ void CVideoPlayer::HandlePlaySpeed()
         // the the bigger is the error we allow
         if (m_playSpeed > DVD_PLAYSPEED_NORMAL)
         {
-          int errorwin = m_playSpeed / DVD_PLAYSPEED_NORMAL;
+          double errorwin = static_cast<double>(m_playSpeed) / DVD_PLAYSPEED_NORMAL;
           if (errorwin > 8)
             errorwin = 8;
           error /= errorwin;

I think the issue is that since tempo gives non 1000ms multiplier playspeeds (e.g. 1100, 1500, etc) when correcting the overall error with the error window we are in fact dividing by 1 instead of 1.5/1.3, etc. I can't get into the seek a bit below all the time when seeking with tempo enabled but regardless, the few times I hit it (the source of picture freeze) the error is really small (and we can avoid the thresold if we correctly calculate the error window).

[enen92]

@FernetMenta sorry to bother yet again. Made a few more tests with seeking with tempo enabled and the player in some conditions still freezes for a few seconds. It doesn't happen all the time but seems to depend on whether PLAYER_STARTED messages are received due to synchronisation or something similar. This does not happen when doing seeks with ff/rw.
Played a bit with the code and this patch works:

@@ -3989,20 +3989,21 @@ void CVideoPlayer::FlushBuffers(double pts, bool accurate, bool sync)
   m_VideoPlayerRadioRDS->Flush();
   m_VideoPlayerAudioID3->Flush();
 
+  // make sure players are properly flushed, should put them in stalled state
+  auto msg = std::make_shared<CDVDMsgGeneralSynchronize>(1s, SYNCSOURCE_AUDIO | SYNCSOURCE_VIDEO);
+  m_VideoPlayerAudio->SendMessage(msg, 1);
+  m_VideoPlayerVideo->SendMessage(msg, 1);
+  msg->Wait(m_bStop, 0);
+
+  // purge any pending PLAYER_STARTED messages
+  m_messenger.Flush(CDVDMsg::PLAYER_STARTED);
+
+  // we should now wait for init cache
+  SetCaching(CACHESTATE_FLUSH);
+
   if (m_playSpeed == DVD_PLAYSPEED_NORMAL ||
       m_playSpeed == DVD_PLAYSPEED_PAUSE)
   {
-    // make sure players are properly flushed, should put them in stalled state
-    auto msg = std::make_shared<CDVDMsgGeneralSynchronize>(1s, SYNCSOURCE_AUDIO | SYNCSOURCE_VIDEO);
-    m_VideoPlayerAudio->SendMessage(msg, 1);
-    m_VideoPlayerVideo->SendMessage(msg, 1);
-    msg->Wait(m_bStop, 0);
-
-    // purge any pending PLAYER_STARTED messages
-    m_messenger.Flush(CDVDMsg::PLAYER_STARTED);
-
-    // we should now wait for init cache
-    SetCaching(CACHESTATE_FLUSH);
     if (sync)
     {
       m_CurrentAudio.syncState = IDVDStreamPlayer::SYNC_STARTING;

I.e. the player seem to require the discard of the any PLAYER_STARTED messages. Looking at git blame you seem to have introduced this condition in this place to fix EDL skips (290d279) however it doesn't seem to have any effect on that (they still work as they should and were in fact broken not so long ago). Feedback on the patch is appreciated as it definitely fixes the issue and before that change the condition was close to the sync just like this patch purposes.
Thanks in advance and sorry for bothering you all the time, hope that doesn't happen when I decide to retire :)

[FernetMenta]

I won't remove this condition

   if (m_playSpeed == DVD_PLAYSPEED_NORMAL ||
       m_playSpeed == DVD_PLAYSPEED_PAUSE)

but just add the tempo condition. Something like

   if (m_playSpeed == DVD_PLAYSPEED_NORMAL || <m_playSpeed in range of tempo> ||
       m_playSpeed == DVD_PLAYSPEED_PAUSE)

The logic is very simple here. If both stream players, audio and video, are active, you need to sync audio and video. This is not true for ff/rw where audio is muted.

[enen92]

That was my first attempt actually. The issue is that in the case of tempo (if tempo is smaller than 1 - e.g. 0.8) this block of code below can stall the player completely:

    if (sync)
    {
      m_CurrentAudio.syncState = IDVDStreamPlayer::SYNC_STARTING;
      m_CurrentVideo.syncState = IDVDStreamPlayer::SYNC_STARTING;
    }

We're left with a 100% buffer spinning wheel with no video or audio. It does not happen for tempo > 1 though (and also does not happen if we only allow it for speed normal or paused)

[FernetMenta]

We're left with a 100% buffer spinning wheel with no video or audio

If this is the case, you should investigate why this happens and what conditions keep player in this situation.

[enen92]

We're left with a 100% buffer spinning wheel with no video or audio

If this is the case, you should investigate why this happens and what conditions keep player in this situation.

I should have anticipated that answer :D
So, I digged a bit more on this and I think I found the reason, the audiosink does not account for the clock speed when calculating the timeout. So when playing with tempo values smaller than 1.0 the player gets stuck waiting for a bit more time than it should (with the 100% cache wheel on top) - then playback continues. The reason it did not before was because I commented a bit more than I should :)

All things combined, the patch below seems to fix it for all seek use cases (normal, tempo and ff/rw):

commit 669dad856353ba97a912c106da0892f57a61bedf (HEAD -> tempo_seek)
Author: enen92 <92enen@gmail.com>
Date:   Mon Feb 12 14:42:50 2024 +0000

    VideoPlayer: Fix stalls when seeking with tempo enabled

diff --git a/xbmc/cores/VideoPlayer/AudioSinkAE.cpp b/xbmc/cores/VideoPlayer/AudioSinkAE.cpp
index 0af936fde2..7e9fb0a629 100644
--- a/xbmc/cores/VideoPlayer/AudioSinkAE.cpp
+++ b/xbmc/cores/VideoPlayer/AudioSinkAE.cpp
@@ -112,11 +112,12 @@ unsigned int CAudioSinkAE::AddPackets(const DVDAudioFrame &audioframe)
     m_syncError = 0.0;
   }
 
-  //Calculate a timeout when this definitely should be done
+  // Calculate a timeout when this definitely should be done
   double timeout;
   timeout  = DVD_SEC_TO_TIME(m_pAudioStream->GetDelay()) + audioframe.duration;
   timeout += DVD_SEC_TO_TIME(1.0);
   timeout += m_pClock->GetAbsoluteClock();
+  timeout *= m_pClock->GetClockSpeed();
 
   unsigned int total = audioframe.nb_frames - audioframe.framesOut;
   unsigned int frames = total;
diff --git a/xbmc/cores/VideoPlayer/VideoPlayer.cpp b/xbmc/cores/VideoPlayer/VideoPlayer.cpp
index c93beaa6ac..bf3f606798 100644
--- a/xbmc/cores/VideoPlayer/VideoPlayer.cpp
+++ b/xbmc/cores/VideoPlayer/VideoPlayer.cpp
@@ -3989,8 +3989,9 @@ void CVideoPlayer::FlushBuffers(double pts, bool accurate, bool sync)
   m_VideoPlayerRadioRDS->Flush();
   m_VideoPlayerAudioID3->Flush();
 
-  if (m_playSpeed == DVD_PLAYSPEED_NORMAL ||
-      m_playSpeed == DVD_PLAYSPEED_PAUSE)
+  if (m_playSpeed == DVD_PLAYSPEED_NORMAL || m_playSpeed == DVD_PLAYSPEED_PAUSE ||
+      (m_playSpeed >= DVD_PLAYSPEED_NORMAL * m_processInfo->MinTempoPlatform() &&
+       m_playSpeed <= DVD_PLAYSPEED_NORMAL * m_processInfo->MaxTempoPlatform()))
   {
     // make sure players are properly flushed, should put them in stalled state
     auto msg = std::make_shared<CDVDMsgGeneralSynchronize>(1s, SYNCSOURCE_AUDIO | SYNCSOURCE_VIDEO);

Moved the tempo range condition to the end to save clock cycles when paused (the other conditions should be faster since they don't have calculations involved).

Would you consider this a proper fix for the issue @FernetMenta?
Again, thanks for the help.

[FernetMenta]

Looks good to me. Good job!

[antonsoroko]

@enen92 thank you very much for the fix!
for a some time i have tested new kodi 21 with multiple videos - no more freezes on seek.
high tempoup (>1.5) in general seems to be less smooth than in other players (or it just needs more powerful device than other players), but with this fix "user experience" was significantly improve. thanks again!