Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test file cache changes revert #25021

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

phunkyfish
Copy link
Contributor

@phunkyfish phunkyfish commented Apr 19, 2024

Description

Testing reverting the following PRs to fix root cause for kodi-pvr/pvr.mediaportal.tvserver#200. We need a win64 test build for users to try:

@thexai and @fuzzard FYI

Motivation and context

How has this been tested?

What is the effect on users?

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

@phunkyfish phunkyfish added this to the "P" 22.0 Alpha 1 milestone Apr 19, 2024
@phunkyfish phunkyfish added Type: Fix non-breaking change which fixes an issue v22 "P" labels Apr 19, 2024
@thexai
Copy link
Member

thexai commented Apr 19, 2024

Bug reporter says has tried all cache settings, even cache disabled for all sources:

I also tried Release 21.0 with all Video Cache Settings from 0-4 and none of those values made any difference, no go.

So why revert CFileCache changes if with Cache disabled CFileCache code is not used/load at all?

We can be repeat this test with only revert CFileStreamBuffer changes?

#24733
#24504

Even if this works why the solution is revert and not investigate this add-on? (all others add-ons works)

Example:
If all skins are working on Omega but not Amber skin, the solution is fix this skin or revert all core changes until amber works again....

P.S. Is not necessary create a PR for test random reversion code changes.... you can build in Jenkins any cloned kodi repo/branch from other repo that contains any modification.

@thexai thexai removed the v22 "P" label Apr 19, 2024
@thexai thexai removed this from the "P" 22.0 Alpha 1 milestone Apr 19, 2024
@phunkyfish
Copy link
Contributor Author

I’m not proposing any revert. Just simply trying to whittle down the number of commits that might be causing the problem. The PR doesn’t build anyway, due to some ffmpeg build issue which is unrelated. I tried to build the branch from my repo on Jenkins first but did not work, so I tried this.

IMHO, we should only be fixing this forward. If we are to fix the addon we would need to figure out what that fix should be. I don’t know where to start on that, so will need some of your help.

@thexai
Copy link
Member

thexai commented Apr 19, 2024

Can you test this diff in the add-on instead?

diff --git a/src/lib/tsreader/FileReader.cpp b/src/lib/tsreader/FileReader.cpp
index 358b05f..0d87bea 100644
--- a/src/lib/tsreader/FileReader.cpp
+++ b/src/lib/tsreader/FileReader.cpp
@@ -125,7 +125,7 @@ namespace MPTV
         do
         {
             kodi::Log(ADDON_LOG_INFO, "FileReader::OpenFile() %s.", m_fileName.c_str());
-            if (m_hFile.OpenFile(m_fileName, READ_CHUNKED))
+            if (m_hFile.OpenFile(m_fileName, READ_NO_CACHE))
             {
                 break;
             }

@thexai
Copy link
Member

thexai commented Apr 19, 2024

If this works, I think the final version should contain more flags:

READ_NO_CACHE | READ_TRUNCATED | READ_BITRATE

@phunkyfish
Copy link
Contributor Author

If this works, I think the final version should contain more flags:

READ_NO_CACHE | READ_TRUNCATED | READ_BITRATE

Thanks so much!

I've build the original suggestion here: kodi-pvr/pvr.mediaportal.tvserver#206. Will ask users to test, and if it works will update it to this.

@phunkyfish
Copy link
Contributor Author

The first one didn’t work. But we’ll try the second one anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Fix non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants