Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fixes to VobSub subtitles loading #1505

Merged
merged 2 commits into from

6 participants

@anssih
Collaborator

The first commit fixes manual loading of VobSub subtitles, while the second, larger commit moves the matching of .idx/.sub files to happen earlier in the autoloading process to avoid several pitfalls. The commit log entries contain more detailed information.

Comments welcome, especially regarding the second commit and the SubFile and SubFileList there in Util.h (not sure if it is preferred to do that in some other way).

@anssih anssih was assigned
@anssih
Collaborator

@taxigps, you were previously involved with VobSub stuff, mind giving your two cents and/or testing? :)

@MartijnKaijser

ping
Go for merge?

@anssih
Collaborator

Sorry, been too busy for the last week or so. I think the second commit could probably be changed to what taxigps suggested, but I have to test it first to be sure. I'll try to do that this week.

@elupus elupus was assigned
@ghost

ping

@elupus
Collaborator

ping, still valid and useful?

@anssih
Collaborator

Oops, possibly. Will take another look in the next two weeks, honest!

@anssih
Collaborator

From what I can see, taxigps' suggestion is OK, so I can change the second patch to that. I'm sure I had some reason to do it like I did, but I think that must've been theoretical (or simply wrong) since I'm not finding any samples that don't work with the simpler version.

I'll update the first patch to also work with zips, and re-test and re-confirm everything.

@da-anda
Collaborator

is this fix still required?

@mkortstiege
Collaborator

@anssih what's the status here?

anssih added some commits
@anssih anssih Fixed: Manual loading of VobSub subtitles
Currently VobSub (.idx/.sub) subtitles can not be loaded manually.

Allow loading VobSub subtitles manually by specifying either the .idx
or the .sub file. The other file is detected automatically. The
following naming schemes are supported:
- foobar.idx and foobar.sub
- foobar.idx and foobar.(rar|zip) containing foobar.sub

All of the above can be inside an archive (commonly .rar) as well, in
which case the user can select either .idx or .rar inside the archive.
25f9f39
@anssih anssih Fixed: Subtitles autoloading with both VobSub and MicroDVD files
For example, this directory structure:
- movie.mp4
- movie.sub (MicroDVD subtitles)
- Subs/movie.rar (VobSub subtitles)
will prevent any subtitles from being actually loaded. DVDPlayer calls
IsVobSub() for any added .sub files to check if they should be ignored
(since VobSub files are supposed to be added via .idx during
autoloading, we do not want to add them a second time via .sub). In this
case IsVobSub() actually returns true even for the MicroDVD subtitles
since movie.rar contains movie.idx, which matches the name of the
MicroDVD subtitle file. Additionally, the movie.idx inside movie.rar
will actually be paired with the MicroDVD movie.sub instead of the
correct VobSub movie.sub, and will therefore fail to load.

To fix this, make the VobSub .idx/.sub pairing requirement more strict,
allowing a match only if they are in the same directory (or, as
previously allowed, .sub is rarred/zipped in an archive matching the
name of .idx).

Suggested by taxigps.
f05f850
@anssih
Collaborator

Updated, finally. Sorry all for forgetting about this multiple times, and thanks for the pings.

Trivial updates to first patch plus CStdString => std::string, second patch replaced with a simpler version suggested by @taxigps.

I still think this stuff could use some cleanup, including moving to somewhere else from Util.cpp and minimizing code duplicated across players. The previous version of the 2nd patch ( anssih@2176021 ) had some changes to address the latter point, but it would require some refactoring as some things could be done even better so I'll leave that until later to get this PR finally done.

jenkins build this please

@jmarshallnz
Owner

Looks good. Agreed it can be cleaned up further later: If you happen to have a subs rar inside a main movie rar, then things can get really, really slow as it iterates through all the possible combinations...

jenkins build this please

@jmarshallnz
Owner

jenkins build this please

@jmarshallnz
Owner

jenkins seems no-worky (PR too old?) - merging anyway...

@jmarshallnz jmarshallnz merged commit ef3ac86 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 29, 2014
  1. @anssih

    Fixed: Manual loading of VobSub subtitles

    anssih authored
    Currently VobSub (.idx/.sub) subtitles can not be loaded manually.
    
    Allow loading VobSub subtitles manually by specifying either the .idx
    or the .sub file. The other file is detected automatically. The
    following naming schemes are supported:
    - foobar.idx and foobar.sub
    - foobar.idx and foobar.(rar|zip) containing foobar.sub
    
    All of the above can be inside an archive (commonly .rar) as well, in
    which case the user can select either .idx or .rar inside the archive.
  2. @anssih

    Fixed: Subtitles autoloading with both VobSub and MicroDVD files

    anssih authored
    For example, this directory structure:
    - movie.mp4
    - movie.sub (MicroDVD subtitles)
    - Subs/movie.rar (VobSub subtitles)
    will prevent any subtitles from being actually loaded. DVDPlayer calls
    IsVobSub() for any added .sub files to check if they should be ignored
    (since VobSub files are supposed to be added via .idx during
    autoloading, we do not want to add them a second time via .sub). In this
    case IsVobSub() actually returns true even for the MicroDVD subtitles
    since movie.rar contains movie.idx, which matches the name of the
    MicroDVD subtitle file. Additionally, the movie.idx inside movie.rar
    will actually be paired with the MicroDVD movie.sub instead of the
    correct VobSub movie.sub, and will therefore fail to load.
    
    To fix this, make the VobSub .idx/.sub pairing requirement more strict,
    allowing a match only if they are in the same directory (or, as
    previously allowed, .sub is rarred/zipped in an archive matching the
    name of .idx).
    
    Suggested by taxigps.
This page is out of date. Refresh to see the latest.
View
63 xbmc/Util.cpp
@@ -2210,7 +2210,7 @@ bool CUtil::FindVobSubPair( const std::vector<CStdString>& vecSubtitles, const C
if (URIUtils::IsInArchive(vecSubtitles[j]))
strSubDirectory = CURL::Decode(strSubDirectory);
if (URIUtils::HasExtension(strSubFile, ".sub") &&
- (URIUtils::ReplaceExtension(strIdxFile,"").Equals(URIUtils::ReplaceExtension(strSubFile,"")) ||
+ (URIUtils::ReplaceExtension(strIdxPath,"").Equals(URIUtils::ReplaceExtension(vecSubtitles[j],"")) ||
(strSubDirectory.size() >= 11 &&
StringUtils::EqualsNoCase(strSubDirectory.substr(6, strSubDirectory.length()-11), URIUtils::ReplaceExtension(strIdxPath,"")))))
{
@@ -2239,7 +2239,7 @@ bool CUtil::IsVobSub( const std::vector<CStdString>& vecSubtitles, const CStdStr
CStdString strIdxDirectory;
URIUtils::Split(vecSubtitles[j], strIdxDirectory, strIdxFile);
if (URIUtils::HasExtension(strIdxFile, ".idx") &&
- (URIUtils::ReplaceExtension(strIdxFile,"").Equals(URIUtils::ReplaceExtension(strSubFile,"")) ||
+ (URIUtils::ReplaceExtension(vecSubtitles[j],"").Equals(URIUtils::ReplaceExtension(strSubPath,"")) ||
(strSubDirectory.size() >= 11 &&
StringUtils::EqualsNoCase(strSubDirectory.substr(6, strSubDirectory.length()-11), URIUtils::ReplaceExtension(vecSubtitles[j],"")))))
return true;
@@ -2248,6 +2248,65 @@ bool CUtil::IsVobSub( const std::vector<CStdString>& vecSubtitles, const CStdStr
return false;
}
+/*! \brief find a plain or archived vobsub .sub file corresponding to an .idx file
+ */
+std::string CUtil::GetVobSubSubFromIdx(const std::string& vobSubIdx)
+{
+ std::string vobSub = URIUtils::ReplaceExtension(vobSubIdx, ".sub");
+
+ // check if a .sub file exists in the same directory
+ if (CFile::Exists(vobSub))
+ {
+ return vobSub;
+ }
+
+ // look inside a .rar or .zip in the same directory
+ const std::string archTypes[] = { "rar", "zip" };
+ std::string vobSubFilename = URIUtils::GetFileName(vobSub);
+ for (unsigned int i = 0; i < sizeof(archTypes) / sizeof(archTypes[0]); i++)
+ {
+ vobSub = URIUtils::CreateArchivePath(archTypes[i],
+ CURL(URIUtils::ReplaceExtension(vobSubIdx, std::string(".") + archTypes[i])),
+ vobSubFilename).Get();
+ if (CFile::Exists(vobSub))
+ return vobSub;
+ }
+
+ return std::string();
+}
+
+/*! \brief find a .idx file from a path of a plain or archived vobsub .sub file
+ */
+std::string CUtil::GetVobSubIdxFromSub(const std::string& vobSub)
+{
+ std::string vobSubIdx = URIUtils::ReplaceExtension(vobSub, ".idx");
+
+ // check if a .idx file exists in the same directory
+ if (CFile::Exists(vobSubIdx))
+ {
+ return vobSubIdx;
+ }
+
+ // look outside archive (usually .rar) if the .sub is inside one
+ if (URIUtils::IsInArchive(vobSub))
+ {
+
+ std::string archiveFile = URIUtils::GetDirectory(vobSub);
+ std::string vobSubIdxDir = URIUtils::GetParentPath(archiveFile);
+
+ if (!vobSubIdxDir.empty())
+ {
+ std::string vobSubIdxFilename = URIUtils::GetFileName(vobSubIdx);
+ std::string vobSubIdx = URIUtils::AddFileToFolder(vobSubIdxDir, vobSubIdxFilename);
+
+ if (CFile::Exists(vobSubIdx))
+ return vobSubIdx;
+ }
+ }
+
+ return std::string();
+}
+
bool CUtil::CanBindPrivileged()
{
#ifdef TARGET_POSIX
View
4 xbmc/Util.h
@@ -98,7 +98,9 @@ class CUtil
static int ScanArchiveForSubtitles( const CStdString& strArchivePath, const CStdString& strMovieFileNameNoExt, std::vector<CStdString>& vecSubtitles );
static void GetExternalStreamDetailsFromFilename(const CStdString& strMovie, const CStdString& strSubtitles, ExternalStreamInfo& info);
static bool FindVobSubPair( const std::vector<CStdString>& vecSubtitles, const CStdString& strIdxPath, CStdString& strSubPath );
- static bool IsVobSub( const std::vector<CStdString>& vecSubtitles, const CStdString& strSubPath );
+ static bool IsVobSub( const std::vector<CStdString>& vecSubtitles, const CStdString& strSubPath );
+ static std::string GetVobSubSubFromIdx(const std::string& vobSubIdx);
+ static std::string GetVobSubIdxFromSub(const std::string& vobSub);
static int64_t ToInt64(uint32_t high, uint32_t low);
static CStdString GetNextFilename(const CStdString &fn_template, int max);
static CStdString GetNextPathname(const CStdString &path_template, int max);
View
15 xbmc/cores/dvdplayer/DVDPlayer.cpp
@@ -3826,8 +3826,12 @@ int CDVDPlayer::AddSubtitleFile(const std::string& filename, const std::string&
std::string vobsubfile = subfilename;
if(ext == ".idx")
{
- if (vobsubfile.empty())
- vobsubfile = URIUtils::ReplaceExtension(filename, ".sub");
+ if (vobsubfile.empty()) {
+ // find corresponding .sub (e.g. in case of manually selected .idx sub)
+ vobsubfile = CUtil::GetVobSubSubFromIdx(filename);
+ if (vobsubfile.empty())
+ return -1;
+ }
CDVDDemuxVobsub v;
if(!v.Open(filename, vobsubfile))
@@ -3851,9 +3855,10 @@ int CDVDPlayer::AddSubtitleFile(const std::string& filename, const std::string&
}
if(ext == ".sub")
{
- CStdString strReplace(URIUtils::ReplaceExtension(filename,".idx"));
- if (XFILE::CFile::Exists(strReplace))
- return -1;
+ // if this looks like vobsub file (i.e. .idx found), add it as such
+ std::string vobsubidx = CUtil::GetVobSubIdxFromSub(filename);
+ if (!vobsubidx.empty())
+ return AddSubtitleFile(vobsubidx, filename, flags);
}
SelectionStream s;
s.source = m_SelectionStreams.Source(STREAM_SOURCE_TEXT, filename);
View
15 xbmc/cores/omxplayer/OMXPlayer.cpp
@@ -4269,8 +4269,12 @@ int COMXPlayer::AddSubtitleFile(const std::string& filename, const std::string&
std::string vobsubfile = subfilename;
if(ext == ".idx")
{
- if (vobsubfile.empty())
- vobsubfile = URIUtils::ReplaceExtension(filename, ".sub");
+ if (vobsubfile.empty()) {
+ // find corresponding .sub (e.g. in case of manually selected .idx sub)
+ vobsubfile = CUtil::GetVobSubSubFromIdx(filename);
+ if (vobsubfile.empty())
+ return -1;
+ }
CDVDDemuxVobsub v;
if(!v.Open(filename, vobsubfile))
@@ -4294,9 +4298,10 @@ int COMXPlayer::AddSubtitleFile(const std::string& filename, const std::string&
}
if(ext == ".sub")
{
- CStdString strReplace(URIUtils::ReplaceExtension(filename,".idx"));
- if (XFILE::CFile::Exists(strReplace))
- return -1;
+ // if this looks like vobsub file (i.e. .idx found), add it as such
+ std::string vobsubidx = CUtil::GetVobSubIdxFromSub(filename);
+ if (!vobsubidx.empty())
+ return AddSubtitleFile(vobsubidx, filename, flags);
}
OMXSelectionStream s;
s.source = m_SelectionStreams.Source(STREAM_SOURCE_TEXT, filename);
Something went wrong with that request. Please try again.