[VideoPlayer] Fixed: Infinite video halt when cache is full & valid

[honest-mule]

+ added SCacheStatus parameters to GetGeneralInfo()

Description

• The function HandlePlaySpeed() in CVideoPlayer class has been added with a multi-conditional clause to account for resuming playback when cache is at it's appropriate level.
  If the user the has set cache->memorysize in their advancedsettings.xml then it will be used for basic gauging purposes.
• The function GetGeneralInfo() has been modified to keep track of SCacheStatus parameters for debug purposes.

Motivation and context

In the quest to enhance Kodi's performance and reliability, I've identified and addressed a specific bug that affects users with low-speed network connections. This issue is quite intricate and impacts the initial caching process during video playback.

Here's the crux of the problem: When the cache initially reaches its full capacity and Kodi notifies the user that the rate is too slow for continuous playback, the video stutters momentarily but then plays back normally utilizing the cache as it should. Once the cache gets depleted, it unexpectedly results in video playback stalling, even after the cache returns to its 100% capacity, the VideoPlayer fails to resume the video.

The motivation behind this pull request is to highlight Kodi's commitment to providing a seamless streaming experience for all Kodi users, irrespective of their network speed. By resolving this bug, I aim to ensure that video playback remains uninterrupted as long as cache reserve is at its appropriate level, offering a smoother and more enjoyable viewing experience, even under challenging network conditions.

I look forward to your feedback and collaboration.

How has this been tested?

The fix has been tested in emulated & real-time environment.
Low-speed internet connection doesn't lead to an infinite halt of video playback anymore.

What is the effect on users?

Cache-oriented smooth playback as per either user's cache memory settings or Kodi's default settings.

Screenshots (if appropriate):

Types of change

• Bug fix (non-breaking change which fixes an issue)
• Clean up (non-breaking change which removes non-working, unmaintained functionality)
• Improvement (non-breaking change which improves existing functionality)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that will cause existing functionality to change)
• Cosmetic change (non-breaking change that doesn't touch code)
• Student submission (PR was done for educational purposes and will be treated as such)
• None of the above (please explain below)

Checklist:

• My code follows the Code Guidelines of this project
• My change requires a change to the documentation, either Doxygen or wiki
• I have updated the documentation accordingly
• I have read the Contributing document
• I have added tests to cover my change
• All new and existing tests passed

[ksooo]

Cannot say anything about functional correctness of the change. Just some stylistic comments from my end.

[honest-mule]

All suggestions have been duly noted and respective changes made to the code. Thanks! @ksooo

[ksooo]

@FernetMenta do you have any thoughts on this fix?

[thexai]

I'm not saying that the fix is not necessary but I see several things that are wrong:

[FernetMenta]

@FernetMenta do you have any thoughts on this fix?

I never liked the dependency of VP to FileCache. So far this was only for displaying some cache related info for what reason ever. I would not make behaviour of VP dependent on FileCache.

[honest-mule]

@thexai @FernetMenta thanks for your very informational input, guys!

I will be further working on this to improve the relationship between VideoPlayer & Caching modules. Hopefully, good things will come out of this!

Humbly request to whomever is responsible to not close this as such.

[honest-mule]

@thexai @FernetMenta I believe the latest commit should be it.

The actual fault was in FileCache.cpp file where m_bFilling bool flag wasn't being reset after m_writeRateLowSpeed was being reset - and since they go hand-in-hand, it was leading to the infinite video halt.

Fixing the above meant the user being shown rate too low for continuous playback notification every single time the cache gets filled to 100% & the video resumes.

Thus, the line #1873 in VideoPlayer.cpp had to be commented out for now:

CGUIDialogKaiToast::QueueNotification(g_localizeStrings.Get(21454), g_localizeStrings.Get(21455));

The ideal situation for this toast to be appropriate would be when VideoPlayer only shows it the first time to the user when lowrate is encountered, with only exceptions being further manual seek events.
Since people might unknowingly activate the notification again in the future, a todo info comment has also been added just above the line to ensure intent.

[arnova]

I think the actual problem that's triggering your issue is the fact that the level is set to -1 in GetCachingTimes() when low-speed conditions are hit. Perhaps introduce an additional bool in CacheInfo to allow passing proper level, even with low-speed conditions (and use the bool where appropriate)? @FernetMenta probably has a better understanding of the logic low-speed logic and how it should "work"?

[honest-mule]

@arnova As per your suggestions, the fix has been re-introduced by adding relevant bool flags in some of the video/file cache-status structs.

[FernetMenta]

@honest-mule have you tried just to remove the existing logic that depends on FileCache from VP? I don't think the state of FileCache should have any impact on behaviour of VideoPlayer.

[honest-mule]

@FernetMenta Might as well, soon as I get some free time in my hands. Would be a nice & fun experiment, tbh!

[thexai]

Cache level calculation is wrong and in some cases is 0 when cache level is 100%. This is similar to #22881 (the root cause is the same, but I don't full understand how this level is calculated)

As now exist cache.time variable can be used as replacement. 8s is queue time, then >8 means queue is at 100% and cache start filling. This should fix related issue (infinite halt).

@honest-mule test this change:

diff --git a/xbmc/cores/VideoPlayer/VideoPlayer.cpp b/xbmc/cores/VideoPlayer/VideoPlayer.cpp
index 72419823d8..64fea25605 100644
--- a/xbmc/cores/VideoPlayer/VideoPlayer.cpp
+++ b/xbmc/cores/VideoPlayer/VideoPlayer.cpp
@@ -1871,7 +1871,7 @@ void CVideoPlayer::HandlePlaySpeed()
         CGUIDialogKaiToast::QueueNotification(g_localizeStrings.Get(21454), g_localizeStrings.Get(21455));
         SetCaching(CACHESTATE_INIT);
       }
-      if (cache.level >= 1.0)
+      if (cache.time > 8.0)
         SetCaching(CACHESTATE_INIT);
     }
     else

[honest-mule]

@thexai I agree wholeheartedly.

In my opinion the cache level should always be between 0-100%. This way all player-related elements can work intelligently & predictably knowing the obvious min-max levels of cache. For ex: the video buffer gauging meter, which as of now, does not help predict when the video playback will resume, if at all...

Thus, the calculation in line 1850 this is wrong:

1850  info.level = (cached + queued) / (cache_need + queued);

--

The change you suggested, of course, gets rid of video halting issue but introduces another predicament where the cache is never allowed to fill to a 100%. Meaning, the video playback stops every 8 seconds and then waits until the next 8 seconds of playback is available.

[thexai]

but introduces another predicament where the cache is never allowed to fill to a 100%. Meaning, the video playback stops every 8 seconds and then waits until the next 8 seconds of playback is available.

This is not true: I tested proposed change and cache still used same as before:

As commented VideoPlayer queue allows 8 seconds of playback and the rest from 8 to 14s is due FileCache.

In the capture cache has 7.44 MB that is the default without advanced settings, then is used 100% (same as before).

You a confused with "cache level required to start playback after cache dry" That doesn't have to be 100% and in fact it usually isn't. The logical strategy is to fill the cache to 100% only when start playback, but if due to an (unusual) event it is accidentally emptied during playback, you want to resume playback as soon as possible and therefore not wait to fill the cache to 100%. A usual percentage could be 20 or 30%. Since VideoPlayer queue already has 8s of data is fine even start playback with 0% of cache.

It is assumed that the bandwidth of the connection on average is higher than the bit rate of the stream and therefore the cache will recover slowly (and there is also 8 seconds of margin for it to do so).

If the bandwidth of the connection is less than the bit rate of the stream it will never work well, neither with cache nor without cache. The cache only covers very temporary events and high latency issues.

Current trigger condition is cache.level >= 1.0 and the equivalent is cache.time > 8.0

If the condition was cache at 100% then it would be cache.level >= 100.

Conclusion: proposed change fixes the issue without changing the current operating mode.

[thexai]

Also reproduced and confirmed original issue limiting NAS bandwidth to ~80 Mbit/s and playing high bitrate 4K video files

Confirmed two things:

1. Issue reproduced --> Kodi halts auto-playback after cache fills 100% #23752
2. Confirmed that change cache.time > 8.0 fixes the issue.

[honest-mule]

@thexai Your explanation makes it transparent that indeed, VideoPlayer should have to wait only 8 seconds before it starts playing from cache again and I concur my logic was flawed thinking the cache isn't filling up to a 100%.

Most likely this should be the fix. I just have apprehensions if the logic is calculating the seconds of footage in cache correctly. Did you ran any tests for that?

[thexai]

VideoPlayer should have to wait only 8 seconds before it starts playing from cache again

No. You still not understand this :)

VideoPlayer should have to wait queue filled 100% and this allows 8 seconds of play. But is not need necessarily 8 s. to fill queue: this time depends on bandwidth of connection.

But you are run in extreme case in which bandwidth is extremely limited and then read rate is only 1x or even 0.9x stream bit rate then in this special case (only) YES, you need 8 s. to read data of 8 s. of playback.

This is not due code change but insufficient bandwidth to play this video. For same reason cache never is filled.

[honest-mule]

VideoPlayer should have to wait only 8 seconds before it starts playing from cache again

Yes, forgive me for the poor phrasing. What I meant was VideoPlayer should only have to wait for 8 seconds of footage in cache before it starts playing from cache again.

I suppose we can close this and I believe you'll be pushing the fix from your endpoint! Cheers 🍻

[thexai]

I suppose we can close this and I believe you'll be pushing the fix from your endpoint!

You can do it in this PR, simply remove all other changes and left only:

diff --git a/xbmc/cores/VideoPlayer/VideoPlayer.cpp b/xbmc/cores/VideoPlayer/VideoPlayer.cpp
index 72419823d8..64fea25605 100644
--- a/xbmc/cores/VideoPlayer/VideoPlayer.cpp
+++ b/xbmc/cores/VideoPlayer/VideoPlayer.cpp
@@ -1871,7 +1871,7 @@ void CVideoPlayer::HandlePlaySpeed()
         CGUIDialogKaiToast::QueueNotification(g_localizeStrings.Get(21454), g_localizeStrings.Get(21455));
         SetCaching(CACHESTATE_INIT);
       }
-      if (cache.level >= 1.0)
+      if (cache.time > 8.0)
         SetCaching(CACHESTATE_INIT);
     }
     else

[honest-mule]

@thexai Done!

[fritsch]

Please rebase and squash: https://medium.com/@slamflipstrom/a-beginners-guide-to-squashing-commits-with-git-rebase-8185cf6e62ec <- like presented here as a howto.

What I can already tell you is, that there is a behaviour change. For people using very large caches, that want to wait for playback start until this is full, it will now start after VideoPlayer has 8 seconds and first byte landed into the additional cache. Just saying - from my local tests - with wifi, this change works great and is most likely better than the original implementation that - as documented in this thread - no one understands anymore.

[thexai]

For commit name is fine use some like PR name:

[VideoPlayer] Fixed: Infinite video halt when cache is full & valid

and please add a comment in second line like:

The reason is 'cache.level' variable some times is 0 when cache is full due the way is calculated. Replaced with 'cache.time' that not has these problems.

[honest-mule]

@fritsch I've done it, hope it's correct.

Looks like this at my end:

[fritsch]

Sadly not. As long as you see more than one commit in this web view, 66 now for your PR, it did not work.

[honest-mule]

What do you suggest, another rebase attempt or a new PR?

[thexai]

From your source folder:

git reset --hard 1f4ee051090a27f0f37348e8229bdce271ba7036

To start from scratch

[fritsch]

Here is a small howto:
Needs to be done once:
git remote add xbmc-upstream https://github.com/xbmc/xbmc.git
git remote add myself https://github.com/honest-mule/xbmc.git

Doing every time, when rebasing, being on the branch, you want to rebase:
git fetch xbmc-upstream
git rebase -i xbmc-upstream/master

Here now select all the commits, you want to "put" on top. In your case, it's exactly one.

Now - force push to yourself:
git push -f myself The_Name_Of_My_Branch

Good luck with that. But it's important to learn it now, as some changes will follow, e.g. 8.0 being a constant falling from heaving, which we make constexpr later on.

[honest-mule]

@fritsch Is this correct?

[fritsch]

Yes - you did it correctly.

[arnova]

I think it would also still be an improved if we would add an additional (bool) member to CacheInfo to flag low-speed conditions instead of setting level to -1.0. That way CVideoPlayer::GetCachingTimes() can always return proper level AND flag low-speed conditions at the same time. This has always felt a bit akward.

[thexai]

Current issue is when level is not -1 precisely. The level calculation bug is for positive values so changing this would not solve this problem.

Anyway, is supposed that when is produced "low speed condition" cache is not filled because connection bandwidth is less than required stream bitrate, then level is ~0 and is not need calculate (seems fine use -1 to signal this condition).

[honest-mule]

@fritsch made a great point which no one has picked on yet:

What I can already tell you is, that there is a behaviour change. For people using very large caches, that want to wait for playback start until this is full, it will now start after VideoPlayer has 8 seconds and first byte landed into the additional cache.

This can only be resolved by adding new user-flags to advancedsettings.xml which when set should make the player only playback the video when either the cache is 100% full or there's no more file left to read from.

[fritsch]

Maybe or based if user has a cache related advanced setting or not. My point is: the usecase to load 200 or 300 MB into a cache so that consequent Playback of the 600M file would work with an ISDN 56kbit/s Connection is most likely a usecase from the past anyways. If not, it should not break ordinary Joe's playback cause we have forgotten how to properly calculate level internally ;-)

From my pov: cache handling needs huge overhaul in general, the change here makes it at least understandable by using time based judgement. Curious though if it breaks other things that worked by luck. We will find out.

[thexai]

As this fixes a real issue let's merge it and leave others changes/improvements for future

[arnova]

Current issue is when level is not -1 precisely. The level calculation bug is for positive values so changing this would not solve this problem.

Anyway, is supposed that when is produced "low speed condition" cache is not filled because connection bandwidth is less than required stream bitrate, then level is ~0 and is not need calculate (seems fine use -1 to signal this condition).

Ah thanks for explaining that. I finally understand that piece of the puzzle (=why the level gets set to -1 with low-speed conditions).

[pannal]

Edit: this PR is not the issue. The linked one is.

Hey, I'm currently developing PlexMod4Kodi and have added a "Slow connection" feature recently. It pauses the video and waits for Player.ProgressCache to reach a reasonable size compared to Player.Progress. Since the latest master this functionality doesn't work anymore.

I'm currently using an unreasonable cache size of 2000 MB and a readfactor of 20. After this PR, it seems like the buffer doesn't fill anymore (when the video is paused) - which it did before. Right now Kodi only buffers a certain percentage even while playing back, even though the buffer is huge and the readfactor is 20.

This is unexpected, as it was possible before to fill up the buffer as much as possible (even loading the full file into the buffer, then resuming, worked flawlessly).

Edit: Oh wait, this seems to be the issue. Are the advancedsettings ignored for buffer size and readfactor? Yes, they are.

[thexai]

I'm currently using an unreasonable cache size of 2000 MB and a readfactor of 20.

20 readfactor also is unreasonable. Reason:

120 Mbps BluRay file * 20 = 2400 Mbps this means saturate 1000 Mbps (1 Gbps Ethernet) unnecessarily and creating CPU spikes to fill the cache.

Player.ProgressCache and Player.CacheLevel probably reports wrong numbers due the same reason "cache level" was wrong calculated and fixed here:

#22881

Then correct way is fix this internally while use 2000 MB cache size, 20 read factor and hack advancersettings is the wrong way.