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

[FileSystem] Fix playback stop when read external SRT subtitles files #25128

Merged
merged 1 commit into from May 5, 2024

Conversation

thexai
Copy link
Member

@thexai thexai commented May 5, 2024

Description

Fix playback stop when read external SRT subtitles files. Read automatically when are named same as MKV file in the same folder.

Motivation and context

Fixes an issue highly reported on forums:

https://forum.kodi.tv/showthread.php?tid=376989&pid=3192688#pid3192688
https://forum.kodi.tv/showthread.php?tid=376989&pid=3194378#pid3194378
https://forum.kodi.tv/showthread.php?tid=377229&pid=3194012#pid3194012
https://forum.kodi.tv/showthread.php?tid=377321&pid=3194890#pid3194890
https://forum.kodi.tv/showthread.php?tid=377304&pid=3194735#pid3194735
...

At first users found that changing the SMB chunk size to 64K fixes the problem and because SMB v1 is limited to 64K it seemed to be related, but can't be this because it only happens with external SRT subtitles: if the user uses SMB v1, then 128K wouldn't go well at all.

Seems chunk size 64K "fixes" this by change. The root cause is a regression from #24504 because (one more time) READ_CHUNKED flag has unexpected values. First is opened .mkv file and is flagged correctly but when opening external SRT part of flags are reused here:

unsigned int flags = m_flags;
// If this file is audio and/or video (= not a subtitle) flag to caller
if (!VIDEO::IsSubtitle(m_item))
flags |= READ_AUDIO_VIDEO;

Then is signaled with no READ_AUDIO_VIDEO to avoid use FileCache for subtitles, this is good. The most immediate solution is use this same flag to avoid use also StreamBuffer for subtitles but this not possible because not all video sources sets this READ_AUDIO_VIDEO flag…. would break Blu-Ray and ISOs which was precisely the main use case of StreamBuffer.

The most solid solution thinking in a fix and backport is create a new flag only for this and since FileCache already has a flag to signal not be used (READ_NO_CACHE) the same is logical with StreamBuffer: new flag READ_NO_BUFFER.

In this way no risk of cause regressions because this flag currently only is used in this specific case. I also think it is good to have this flag in case other situations arise in the future where StreamBuffer should not be used in any way.

I have to say that the READ_CHUNKED flag has always seemed confusing to me and its use is not entirely clear. On the other hand, the flag READ_AUDIO_VIDEO is much clearer since in fact StreamBuffer should only be used for audio/video (same as FileCache).

So, after merging this and the backport I would like to do a rework to start use more READ_AUDIO_VIDEO and try to eliminate/deprecate use of READ_CHUNKED flag if possible.

How has this been tested?

I cannot reproduce this even tested on Sony Android TV (arm-v7) with Play Store version with default settings nor on Shield (arm64/arm-v8). So it is clear that it does not always fails, it depends on more things or the server SMB implementation.

But reproduced locally that SteamBuffer is used to read external SRT subtitles when it shouldn't.

Affected users has confirmed that fix works:
https://forum.kodi.tv/showthread.php?tid=376989&pid=3195845#pid3195845

and even done a double check in current Omega nightly with all others fixes but without this specific fix still failing.

What is the effect on users?

Fix playback stop when read external SRT subtitles files in some setups using SMB and chunk size > 64K.

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

@thexai thexai added Type: Fix non-breaking change which fixes an issue Backport: Needed Component: FileSystem Filesystem v22 "P" labels May 5, 2024
@thexai thexai added this to the "P" 22.0 Alpha 1 milestone May 5, 2024
Copy link
Member

@fritsch fritsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks also for the detailed explanation.

@jenkins4kodi

This comment was marked as off-topic.

@thexai
Copy link
Member Author

thexai commented May 5, 2024

Ignored jenkins diff for now as whole file has bad formatting.

@thexai thexai merged commit 222259a into xbmc:master May 5, 2024
2 checks passed
@thexai thexai deleted the fix-stream-buffer branch May 5, 2024 09:28
@neo1973 neo1973 linked an issue May 8, 2024 that may be closed by this pull request
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport: Done Component: FileSystem Filesystem Type: Fix non-breaking change which fixes an issue v22 "P"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Selecting Arabic subtitles causes the player to stop functioning
3 participants