[PATCH] Subtitles directories
Hi, Not a lot grudge seemed to come up when I proposed the first draft of the subtitles directories, so here is a full working one, with documentation. A few notes about this patch: - It needs the basename patch to work (that should be commited soon, just waiting for a OK.) - I tried not to trash the existing code but I needed some refactorization, so the diff is not quiet clear on the sub_filenames modification (renamed in get_sub_list and set static). - A reindent of get_sub_list will be commited after this one is accepted. - mp_dirname is also part of the patch for a refactorization issue. - It was suggested that -subpaths (or similar) could be more appropriated. What do you think? I hope the man page is clear enough about the feature. If not enough for you developers, get_full_sub_list is quiet explanatory by itself about the new subtitles tracking behaviour. Please review :) -- Clément B. Not sent from a jesusPhone.
On Sat, Nov 13, 2010 at 07:51:33PM +0100, Clément Bœsch wrote:
Hi,
Not a lot grudge seemed to come up when I proposed the first draft of the subtitles directories, so here is a full working one, with documentation.
A few notes about this patch:
- It needs the basename patch to work (that should be commited soon, just waiting for a OK.)
- I tried not to trash the existing code but I needed some refactorization, so the diff is not quiet clear on the sub_filenames modification (renamed in get_sub_list and set static).
- A reindent of get_sub_list will be commited after this one is accepted.
- mp_dirname is also part of the patch for a refactorization issue.
- It was suggested that -subpaths (or similar) could be more appropriated. What do you think?
I hope the man page is clear enough about the feature. If not enough for you developers, get_full_sub_list is quiet explanatory by itself about the new subtitles tracking behaviour.
Please review :)
Ping, it's been a week without any comment. mp_basename is now upstream so the patch can be tested easily. I just fixed the priority behaviour but the rest of the patch stays unchanged. I'll give one more week for a review. If no one notice any problem (I doubt it), I'll commit this. -- Clément B. Not sent from a jesusPhone.
On Sat, Nov 20, 2010 at 09:33:22AM +0100, Clément Bœsch wrote:
+/** + * \brief Allocates a new buffer containing the directory name + * \param path Original path. Must be a valid string. + * + * The path returned always contains a trailing slash '/'. + * On systems supporting DOS paths, '\' is also considered as a directory + * separator in addition to the '/'. + */ +char *mp_dirname(const char *path) +{ + char *tmp, *s; + size_t len; + + tmp = strrchr(path, '/'); + +#if HAVE_DOS_PATHS + if (!tmp) + tmp = strrchr(path, '\\'); +#endif
Please test this with the samples for mp_basename. I think this is not working correctly at least with mixed DOS paths like c:/test\b\c.avi Actually I think you should try reusing mp_basename here, everything before what mp_basename returns should be the path, you should then only need to have a special-case for when there is nothing before it.
@@ -1889,17 +1897,35 @@ static int compare_sub_priority(const void *a, const void *b) } }
-char** sub_filenames(const char* path, char *fname) +static void append_sub(struct sub_list *dst, struct subfn *src) +{ + if (dst->sid >= (int)dst->size - 1) { + dst->size += 32; + dst->subs = realloc(dst->subs, sizeof(*dst->subs) * dst->size); + } + memcpy(&dst->subs[dst->sid], src, sizeof(*src)); + dst->sid++; +} + +static void merge_subs(struct sub_list *dst, struct sub_list *src) +{ + if (src->sid == 0) + return; + dst->size = dst->sid + src->sid; + dst->subs = realloc(dst->subs, sizeof(*dst->subs) * dst->size); + memcpy(&dst->subs[dst->sid], src->subs, src->sid * sizeof(*src->subs)); + dst->sid += src->sid; +}
I think this is too much in one patch, I have a hard time understanding which part solves which issue. I'd strongly prefer it in more parts, e.g. 1) use mp_basename/mp_dirname 2) look at additional paths 3) support loading multiple subtitles (or does it just change it? I don't really know, but I know your changes are at least 4-times as large as for what I consider reasonable just to support loading from one more path). Particularly 3) I consider likely to have subtle issues and I don't like having it mixed up with other changes at all.
@@ -1954,9 +1958,9 @@ char** sub_filenames(const char* path, char *fname) // 1 = any subtitle file // 2 = any sub file containing movie name // 3 = sub file containing movie name and the lang extension - for (j = 0; j <= 1; j++) { - d = opendir(j == 0 ? f_dir : path); + d = opendir(path); if (d) { + mp_msg(MSGT_SUBREADER, MSGL_INFO, "Load subtitles in %s\n", path); while ((de = readdir(d))) {
Indentation is off.
On Sat, Nov 20, 2010 at 11:19:47AM +0100, Reimar Döffinger wrote:
On Sat, Nov 20, 2010 at 09:33:22AM +0100, Clément Bœsch wrote:
+/** + * \brief Allocates a new buffer containing the directory name + * \param path Original path. Must be a valid string. + * + * The path returned always contains a trailing slash '/'. + * On systems supporting DOS paths, '\' is also considered as a directory + * separator in addition to the '/'. + */ +char *mp_dirname(const char *path) +{ + char *tmp, *s; + size_t len; + + tmp = strrchr(path, '/'); + +#if HAVE_DOS_PATHS + if (!tmp) + tmp = strrchr(path, '\\'); +#endif
Please test this with the samples for mp_basename. I think this is not working correctly at least with mixed DOS paths like c:/test\b\c.avi Actually I think you should try reusing mp_basename here, everything before what mp_basename returns should be the path, you should then only need to have a special-case for when there is nothing before it.
Fixed, changed with mp_basename and exported in a standalone patch.
@@ -1889,17 +1897,35 @@ static int compare_sub_priority(const void *a, const void *b) } }
-char** sub_filenames(const char* path, char *fname) +static void append_sub(struct sub_list *dst, struct subfn *src) +{ + if (dst->sid >= (int)dst->size - 1) { + dst->size += 32; + dst->subs = realloc(dst->subs, sizeof(*dst->subs) * dst->size); + } + memcpy(&dst->subs[dst->sid], src, sizeof(*src)); + dst->sid++; +} + +static void merge_subs(struct sub_list *dst, struct sub_list *src) +{ + if (src->sid == 0) + return; + dst->size = dst->sid + src->sid; + dst->subs = realloc(dst->subs, sizeof(*dst->subs) * dst->size); + memcpy(&dst->subs[dst->sid], src->subs, src->sid * sizeof(*src->subs)); + dst->sid += src->sid; +}
I think this is too much in one patch, I have a hard time understanding which part solves which issue. I'd strongly prefer it in more parts, e.g. 1) use mp_basename/mp_dirname 2) look at additional paths 3) support loading multiple subtitles (or does it just change it? I don't really know, but I know your changes are at least 4-times as large as for what I consider reasonable just to support loading from one more path). Particularly 3) I consider likely to have subtle issues and I don't like having it mixed up with other changes at all.
Done. 3 patches attached.
@@ -1954,9 +1958,9 @@ char** sub_filenames(const char* path, char *fname) // 1 = any subtitle file // 2 = any sub file containing movie name // 3 = sub file containing movie name and the lang extension - for (j = 0; j <= 1; j++) { - d = opendir(j == 0 ? f_dir : path); + d = opendir(path); if (d) { + mp_msg(MSGT_SUBREADER, MSGL_INFO, "Load subtitles in %s\n", path); while ((de = readdir(d))) {
Indentation is off.
I was just using spaces instead of mixed tabs and spaces. It's now consistent, but note I'll reindent at least this function after the patches get applied. -- Clément B. Not sent from a jesusPhone.
On Sat, Nov 20, 2010 at 09:21:34PM +0100, Clément Bœsch wrote:
+ * \brief Allocates a new buffer containing the directory name + * \param path Original path. Must be a valid string. + * + * The path returned always contains a trailing slash '/'. + * On systems supporting DOS paths, '\' is also considered as a directory + * separator in addition to the '/'. + */ +char *mp_dirname(const char *path) +{ + const char *base = mp_basename(path); + size_t len = base - path; + char *dirname; + + if (len == 0) + return strdup("./"); + dirname = malloc(len + 1); + if (!dirname) + return NULL; + strncpy(dirname, path, len); + dirname[len] = '\0';
Not sure if it is an improvement, but you could do both in one by using av_strlcpy(dirname, path, len + 1); Either way, that part is ok. Except for one thing I noticed while looking through the next patch: For DOS paths you must also consider ':' a path separator in mp_basename, so that c:test.avi is treated correctly.
From 3ebc481e5eae0e7ffe328d6e23d45cbd250b9994 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <ubitux@gmail.com> Date: Sat, 20 Nov 2010 21:07:43 +0100 Subject: [PATCH 2/3] Make load of n subtitles directories possible
Could you please explain what you are changing and why? Adding doxygen descriptions to all new functions might help as well.
On Sat, Nov 20, 2010 at 09:49:44PM +0100, Reimar Döffinger wrote:
On Sat, Nov 20, 2010 at 09:21:34PM +0100, Clément Bœsch wrote:
+ * \brief Allocates a new buffer containing the directory name + * \param path Original path. Must be a valid string. + * + * The path returned always contains a trailing slash '/'. + * On systems supporting DOS paths, '\' is also considered as a directory + * separator in addition to the '/'. + */ +char *mp_dirname(const char *path) +{ + const char *base = mp_basename(path); + size_t len = base - path; + char *dirname; + + if (len == 0) + return strdup("./"); + dirname = malloc(len + 1); + if (!dirname) + return NULL; + strncpy(dirname, path, len); + dirname[len] = '\0';
Not sure if it is an improvement, but you could do both in one by using av_strlcpy(dirname, path, len + 1);
After looking at the av_strlcpy implementation, it seems a trivial copy is made, so I found the strncpy more appropriated since it can benefits libc optimizations.
Either way, that part is ok. Except for one thing I noticed while looking through the next patch: For DOS paths you must also consider ':' a path separator in mp_basename, so that c:test.avi is treated correctly.
OK, then I'll commit this, and make a second patch to fix that.
From 3ebc481e5eae0e7ffe328d6e23d45cbd250b9994 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <ubitux@gmail.com> Date: Sat, 20 Nov 2010 21:07:43 +0100 Subject: [PATCH 2/3] Make load of n subtitles directories possible
Could you please explain what you are changing and why?
Sure. At the moment, sub_filenames works like a hack: video and ~/.mplayer/sub directories are tracked in the same time. If we had to change this function to add new path to track, it would be a great pain. So sub_filenames is transformed into a function which track only a single given directory (and renamed into get_sub_list). By doing this, a subtitles list system must be done, so here is the reason for functions like append_sub and merge_subs. Since sub_filenames is now able to track only one directory, there must be a wrapper to call it with various paths: get_full_sub_list comes into place. Then, all those subtitles need to be sorted according to their priorities, and added, this is why load_subtitles exists. Making this function also allows the small factorization code you can observe in mplayer.c and mencoder.c The patch subject was just temporary, I'll make a bigger one when committing.
Adding doxygen descriptions to all new functions might help as well.
Done.
_______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
-- Clément B. Not sent from a jesusPhone.
On Sun, Nov 21, 2010 at 12:05:58PM +0100, Clément Bœsch wrote:
+static void append_sub(struct sub_list *dst, struct subfn *src)
I suspect you are lucky here and a check outside that function saves you. However
+ dst->size = dst->sid + src->sid;
Possibly integer overflow.
+ dst->subs = realloc(dst->subs, sizeof(*dst->subs) * dst->size);
Possible integer overflow. Also realloc failure is not handled.
+ struct sub_list tmp_list = get_sub_list(mp_subdir, fname); + merge_subs(&fslist, &tmp_list);
You always call first get_sub_list and then merge_subs, why not just append them to the proper list in the first place in get_sub_list? Also I'd be strongly in favour of limiting the number of overall subs, not just per directory, then you can just avoid all that realloc and overflow checking mess and allocate a array with max size from the start.
On Sun, Nov 21, 2010 at 12:24:32PM +0100, Reimar Döffinger wrote:
On Sun, Nov 21, 2010 at 12:05:58PM +0100, Clément Bœsch wrote:
+static void append_sub(struct sub_list *dst, struct subfn *src)
I suspect you are lucky here and a check outside that function saves you. However
+ dst->size = dst->sid + src->sid;
Possibly integer overflow.
+ dst->subs = realloc(dst->subs, sizeof(*dst->subs) * dst->size);
Possible integer overflow. Also realloc failure is not handled.
+ struct sub_list tmp_list = get_sub_list(mp_subdir, fname); + merge_subs(&fslist, &tmp_list);
You always call first get_sub_list and then merge_subs, why not just append them to the proper list in the first place in get_sub_list? Also I'd be strongly in favour of limiting the number of overall subs, not just per directory, then you can just avoid all that realloc and overflow checking mess and allocate a array with max size from the start.
OK, I removed all the realloc stuff, so it simplifies things a bit. Is it easier to review? -- Clément B. Not sent from a jesusPhone.
On Sun, Nov 21, 2010 at 05:46:16PM +0100, Clément Bœsch wrote:
+/** + * \brief Append a subtitle to a list + * \param dst Destination subtitles list + * \param src Source subtitle + * \warning Source data are copied and not reallocated + */ +static int append_sub(struct sub_list *dst, struct subfn *src) { + if (dst->sid >= MAX_SUBTITLE_FILES) + return -1; + memcpy(&dst->subs[dst->sid], src, sizeof(*src)); + dst->sid++; + return 0;
Either getting rid of this function or at least making it take the name etc. as arguments and having it do the strdup will make things even simpler.
@@ -2012,8 +2013,7 @@ char** sub_filenames(const char* path, char *fname) } if (!prio) { // doesn't contain the movie name - // don't try in the mplayer subtitle directory - if ((j == 0) && (sub_match_fuzziness >= 2)) { + if (sub_match_fuzziness >= 2) {
Huh? Where did handling of this end up? Loading any subtitle file anywhere in the path doesn't sound to me like it would ever be desireable.
+ path = realloc(path, plen + strlen(dir) + 1); + if (!path) + return -1;
memleak in case realloc fails.
+ strcpy(path + plen, dir);
av_strlcpy might make it more obvious.
+ // Load subtitles specified by sub option with highest priority + if (sub_name) { + int i; + for (i = 0; sub_name[i]; i++) { + struct subfn sub = { + .fname = strdup(sub_name[i]), + .priority = INT_MAX - i, + .noerr = 0 + }; + append_sub(&slist, &sub); + }
Seems a bit overkill to push them through qsort. Though it might be the simplest way.
+.br +/tmp/\:subs/ +.br +~/.mplayer/\:sub/ +.RE +.PD 1 +. +.TP .B \-subdelay <sec> Delays subtitles by <sec> seconds. Can be negative. @@ -11714,14 +11735,6 @@ font directory (There must be a font.desc file and files with .RAW extension.) .TP ~/.mplayer/\:DVDkeys/ cached CSS keys -. -.TP -Assuming that /path/\:to/\:movie.avi is played, MPlayer searches for sub files -in this order: -.RS -/path/\:to/\:movie.sub -.br -~/.mplayer/\:sub/\:movie.sub .RE .PD 1 . diff --git a/cfg-common.h b/cfg-common.h index c6dfef0..d50ebe2 100644 --- a/cfg-common.h +++ b/cfg-common.h @@ -562,6 +562,7 @@ const m_option_t common_opts[] = { // ------------------------- subtitles options --------------------
{"sub", &sub_name, CONF_TYPE_STRING_LIST, 0, 0, 0, NULL}, + {"subdirs", &sub_dirs, CONF_TYPE_STRING_LIST, 0, 0, 0, NULL}, #ifdef CONFIG_FRIBIDI {"fribidi-charset", &fribidi_charset, CONF_TYPE_STRING, 0, 0, 0, NULL}, {"flip-hebrew", &flip_hebrew, CONF_TYPE_FLAG, 0, 0, 1, NULL}, diff --git a/mencoder.c b/mencoder.c index 6efe907..403fbac 100644 --- a/mencoder.c +++ b/mencoder.c @@ -179,6 +179,7 @@ char *font_name=NULL; char *sub_font_name=NULL; float font_factor=0.75; char **sub_name=NULL; +char **sub_dirs = NULL; float sub_delay=0; float sub_fps=0; int sub_auto = 0; diff --git a/mpcommon.h b/mpcommon.h index 19110d6..4a2147e 100644 --- a/mpcommon.h +++ b/mpcommon.h @@ -35,6 +35,7 @@ extern int sub_auto; extern float sub_delay; extern float sub_fps; extern char **sub_name; +extern char **sub_dirs; extern char *font_name; extern char *sub_font_name; extern char *audio_lang; diff --git a/mplayer.c b/mplayer.c index 3682b29..7ebc3cd 100644 --- a/mplayer.c +++ b/mplayer.c @@ -292,6 +292,7 @@ char *font_name=NULL; char *sub_font_name=NULL; float font_factor=0.75; char **sub_name=NULL; +char **sub_dirs = NULL; float sub_delay=0; float sub_fps=0; int sub_auto = 1; diff --git a/sub/subreader.c b/sub/subreader.c index 94d0877..fba9c82 100644 --- a/sub/subreader.c +++ b/sub/subreader.c @@ -2117,6 +2117,13 @@ static struct sub_list get_full_sub_list(char *fname) // Load subtitles from current media directory track_sub_directory("", fname, &slist);
+ // Load subtitles in dirs specified by subdirs option + if (sub_dirs) { + int i; + for (i = 0; sub_dirs[i]; i++) + track_sub_directory(sub_dirs[i], fname, &slist); + } + // Load subtitles in ~/.mplayer/sub mp_subdir = get_path("sub/"); if (mp_subdir) -- 1.7.3.2
_______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
On Sun, Nov 21, 2010 at 09:39:03PM +0100, Reimar Döffinger wrote:
On Sun, Nov 21, 2010 at 05:46:16PM +0100, Clément Bœsch wrote:
+/** + * \brief Append a subtitle to a list + * \param dst Destination subtitles list + * \param src Source subtitle + * \warning Source data are copied and not reallocated + */ +static int append_sub(struct sub_list *dst, struct subfn *src) { + if (dst->sid >= MAX_SUBTITLE_FILES) + return -1; + memcpy(&dst->subs[dst->sid], src, sizeof(*src)); + dst->sid++; + return 0;
Either getting rid of this function or at least making it take the name etc. as arguments and having it do the strdup will make things even simpler.
Changed with arguments method.
@@ -2012,8 +2013,7 @@ char** sub_filenames(const char* path, char *fname) } if (!prio) { // doesn't contain the movie name - // don't try in the mplayer subtitle directory - if ((j == 0) && (sub_match_fuzziness >= 2)) { + if (sub_match_fuzziness >= 2) {
Huh? Where did handling of this end up? Loading any subtitle file anywhere in the path doesn't sound to me like it would ever be desireable.
I added a limit_fuzziness flag to keep ignoring sub_fuzziness=2 on ~/.mplayer/sub directory. subdirs patch updates a little the documentation about this point too.
+ path = realloc(path, plen + strlen(dir) + 1); + if (!path) + return -1;
memleak in case realloc fails.
Fixed.
+ strcpy(path + plen, dir);
av_strlcpy might make it more obvious.
Why? It would either add a second strlen call or a new variable. Maybe you meant strlcat; but even so, the problem is the same. Or maybe I missed something.
+ // Load subtitles specified by sub option with highest priority + if (sub_name) { + int i; + for (i = 0; sub_name[i]; i++) { + struct subfn sub = { + .fname = strdup(sub_name[i]), + .priority = INT_MAX - i, + .noerr = 0 + }; + append_sub(&slist, &sub); + }
Seems a bit overkill to push them through qsort. Though it might be the simplest way.
Yes but qsort was already in use, I didn't want to change that. Patches re-attached. -- Clément B. Not sent from a jesusPhone.
On Sun, Nov 21, 2010 at 11:12:32PM +0100, Clément Bœsch wrote:
On Sun, Nov 21, 2010 at 09:39:03PM +0100, Reimar Döffinger wrote:
On Sun, Nov 21, 2010 at 05:46:16PM +0100, Clément Bœsch wrote:
+/** + * \brief Append a subtitle to a list + * \param dst Destination subtitles list + * \param src Source subtitle + * \warning Source data are copied and not reallocated + */ +static int append_sub(struct sub_list *dst, struct subfn *src) { + if (dst->sid >= MAX_SUBTITLE_FILES) + return -1; + memcpy(&dst->subs[dst->sid], src, sizeof(*src)); + dst->sid++; + return 0;
Either getting rid of this function or at least making it take the name etc. as arguments and having it do the strdup will make things even simpler.
Changed with arguments method.
@@ -2012,8 +2013,7 @@ char** sub_filenames(const char* path, char *fname) } if (!prio) { // doesn't contain the movie name - // don't try in the mplayer subtitle directory - if ((j == 0) && (sub_match_fuzziness >= 2)) { + if (sub_match_fuzziness >= 2) {
Huh? Where did handling of this end up? Loading any subtitle file anywhere in the path doesn't sound to me like it would ever be desireable.
I added a limit_fuzziness flag to keep ignoring sub_fuzziness=2 on ~/.mplayer/sub directory. subdirs patch updates a little the documentation about this point too.
+ path = realloc(path, plen + strlen(dir) + 1); + if (!path) + return -1;
memleak in case realloc fails.
Fixed.
+ strcpy(path + plen, dir);
av_strlcpy might make it more obvious.
Why? It would either add a second strlen call or a new variable. Maybe you meant strlcat; but even so, the problem is the same. Or maybe I missed something.
+ // Load subtitles specified by sub option with highest priority + if (sub_name) { + int i; + for (i = 0; sub_name[i]; i++) { + struct subfn sub = { + .fname = strdup(sub_name[i]), + .priority = INT_MAX - i, + .noerr = 0 + }; + append_sub(&slist, &sub); + }
Seems a bit overkill to push them through qsort. Though it might be the simplest way.
Yes but qsort was already in use, I didn't want to change that.
Patches re-attached.
Oups wrong patches, sorry. Patches reattached. -- Clément B. Not sent from a jesusPhone.
On Sun, Nov 21, 2010 at 11:12:32PM +0100, Clément Bœsch wrote:
@@ -2012,8 +2013,7 @@ char** sub_filenames(const char* path, char *fname) } if (!prio) { // doesn't contain the movie name - // don't try in the mplayer subtitle directory - if ((j == 0) && (sub_match_fuzziness >= 2)) { + if (sub_match_fuzziness >= 2) {
Huh? Where did handling of this end up? Loading any subtitle file anywhere in the path doesn't sound to me like it would ever be desireable.
I added a limit_fuzziness flag to keep ignoring sub_fuzziness=2 on ~/.mplayer/sub directory. subdirs patch updates a little the documentation about this point too.
Sure it doesn't make more sense to only apply it to the movie directory and nothing else at all? Doesn't make sense to have a path where there's only the subtitle(s) for one single movie - and in all other cases it will end up with wrong subtitles.
+ strcpy(path + plen, dir);
av_strlcpy might make it more obvious.
Why? It would either add a second strlen call or a new variable. Maybe you meant strlcat; but even so, the problem is the same. Or maybe I missed something.
Yes I meant strlcat. Doesn't matter much, it's just that this is kind of a reimplementation of strcat, which is not so great from a code-readability standpoint.
+ // Load subtitles specified by sub option with highest priority + if (sub_name) { + int i; + for (i = 0; sub_name[i]; i++) { + struct subfn sub = { + .fname = strdup(sub_name[i]), + .priority = INT_MAX - i, + .noerr = 0 + }; + append_sub(&slist, &sub); + }
Seems a bit overkill to push them through qsort. Though it might be the simplest way.
Yes but qsort was already in use, I didn't want to change that.
qsort wasn't used on the subtitles specified with -sub. And actually, -sub did disable auto-loading before. I was assuming you just added more search paths, but you changed the behaviour all over the place, that makes this review really a major pain.
On Mon, Nov 22, 2010 at 12:05:08AM +0100, Reimar Döffinger wrote:
On Sun, Nov 21, 2010 at 11:12:32PM +0100, Clément Bœsch wrote:
@@ -2012,8 +2013,7 @@ char** sub_filenames(const char* path, char *fname) } if (!prio) { // doesn't contain the movie name - // don't try in the mplayer subtitle directory - if ((j == 0) && (sub_match_fuzziness >= 2)) { + if (sub_match_fuzziness >= 2) {
Huh? Where did handling of this end up? Loading any subtitle file anywhere in the path doesn't sound to me like it would ever be desireable.
I added a limit_fuzziness flag to keep ignoring sub_fuzziness=2 on ~/.mplayer/sub directory. subdirs patch updates a little the documentation about this point too.
Sure it doesn't make more sense to only apply it to the movie directory and nothing else at all? Doesn't make sense to have a path where there's only the subtitle(s) for one single movie - and in all other cases it will end up with wrong subtitles.
After the first patch, it will still be the same: only two directories are tracked: ~/.mplayer/sub and the movie one. About the fuzziness behaviour with the subdirs patch, I think it should also honor the value of 2 since subdirs patch is mainly for relative paths. When you have a sub-fuzziness set to 2, you just want to load all the subtitles in the movie directory not matching the movie name, so you have a tree like that for example: ./movie1/ ./movie1/movie1.avi ./movie1/subtitle-en.srt ./movie1/subtitle-fr.srt ./movie1/subtitle-de.srt ./movie2/ ./movie2/movie2.avi ./movie2/subtitle-en.srt ./movie2/subtitle-fr.srt ./movie2/subtitle-de.srt ... ...with a common subtitle format name for example, total random, a hash or whatever. The subdirs option just extend this behaviour with this kind of tree: ./movie1/ ./movie1/movie1.avi ./movie1/sub/subtitle-en.srt ./movie1/sub/subtitle-fr.srt ./movie1/sub/subtitle-de.srt ./movie2/ ./movie2/movie2.avi ./movie2/sub/subtitle-en.srt ./movie2/sub/subtitle-fr.srt ./movie2/sub/subtitle-de.srt ... All the subtitles are always in a sub/ (or anything else) directory where you don't have to care about the filenames format. ~/.mplayer/sub is an exception since it was made to centralize *all* the subtitles all over the place, so it does not make any sense to load them all.
+ strcpy(path + plen, dir);
av_strlcpy might make it more obvious.
Why? It would either add a second strlen call or a new variable. Maybe you meant strlcat; but even so, the problem is the same. Or maybe I missed something.
Yes I meant strlcat. Doesn't matter much, it's just that this is kind of a reimplementation of strcat, which is not so great from a code-readability standpoint.
I see. Changed with a strcat.
+ // Load subtitles specified by sub option with highest priority + if (sub_name) { + int i; + for (i = 0; sub_name[i]; i++) { + struct subfn sub = { + .fname = strdup(sub_name[i]), + .priority = INT_MAX - i, + .noerr = 0 + }; + append_sub(&slist, &sub); + }
Seems a bit overkill to push them through qsort. Though it might be the simplest way.
Yes but qsort was already in use, I didn't want to change that.
qsort wasn't used on the subtitles specified with -sub.
Yes, I changed the patch to stay consistent with the old code: -sub subtitles are directly loaded now.
And actually, -sub did disable auto-loading before.
Huh? I don't think so; the old code do that: if (sub_name) { ... } if (sub_auto) { ... } And I kept this.
I was assuming you just added more search paths, but you changed the behaviour all over the place, that makes this review really a major pain.
OK, sorry about that. I simplified the patch all over the place so it should make more sense. So let me resume this again with the new code: Everything starts in load_subtitles which load all the possible subtitles. This function first add subtitles specified by -sub. After that, if automatic detection is disabled, it stops there. Then a range of subtitles is allocated (this was previously done in sub_filenames). The movie directory is then tracked using the helper track_sub_directory to build the correct path. This helper will be used a second time with the subdirs patch. Next, it tries to load subtitles in ~/.mplayer/sub directory. The list is now complete, so subtitles are sorted by priority and then added. End of the function. sub_filenames now only loads a single path and append the subtitles to a list, so its prototype changed and it was renamed to append_dir_subtitles. Is this better? Note: I also added a workaround for absolute paths in the subdirs patch. -- Clément B. Not sent from a jesusPhone.
On Mon, Nov 22, 2010 at 09:10:40PM +0100, Clément Bœsch wrote:
On Mon, Nov 22, 2010 at 12:05:08AM +0100, Reimar Döffinger wrote:
On Sun, Nov 21, 2010 at 11:12:32PM +0100, Clément Bœsch wrote:
@@ -2012,8 +2013,7 @@ char** sub_filenames(const char* path, char *fname) } if (!prio) { // doesn't contain the movie name - // don't try in the mplayer subtitle directory - if ((j == 0) && (sub_match_fuzziness >= 2)) { + if (sub_match_fuzziness >= 2) {
Huh? Where did handling of this end up? Loading any subtitle file anywhere in the path doesn't sound to me like it would ever be desireable.
I added a limit_fuzziness flag to keep ignoring sub_fuzziness=2 on ~/.mplayer/sub directory. subdirs patch updates a little the documentation about this point too.
Sure it doesn't make more sense to only apply it to the movie directory and nothing else at all? Doesn't make sense to have a path where there's only the subtitle(s) for one single movie - and in all other cases it will end up with wrong subtitles.
After the first patch, it will still be the same: only two directories are tracked: ~/.mplayer/sub and the movie one.
About the fuzziness behaviour with the subdirs patch, I think it should also honor the value of 2 since subdirs patch is mainly for relative paths.
Ok, your examples convinced me. Well, at least enough that I think either is ok. Of course it would be possible to make it depend on relative vs. absolute path, but let's forget about that for now.
And actually, -sub did disable auto-loading before.
Huh? I don't think so; the old code do that:
if (sub_name) { ... }
if (sub_auto) { ... }
And I kept this.
You obviously missed the else. Oh, what a mess. The else is only there in mencoder...
On Mon, Nov 22, 2010 at 09:28:19PM +0100, Reimar Döffinger wrote:
On Mon, Nov 22, 2010 at 09:10:40PM +0100, Clément Bœsch wrote:
On Mon, Nov 22, 2010 at 12:05:08AM +0100, Reimar Döffinger wrote:
On Sun, Nov 21, 2010 at 11:12:32PM +0100, Clément Bœsch wrote:
@@ -2012,8 +2013,7 @@ char** sub_filenames(const char* path, char *fname) } if (!prio) { // doesn't contain the movie name - // don't try in the mplayer subtitle directory - if ((j == 0) && (sub_match_fuzziness >= 2)) { + if (sub_match_fuzziness >= 2) {
Huh? Where did handling of this end up? Loading any subtitle file anywhere in the path doesn't sound to me like it would ever be desireable.
I added a limit_fuzziness flag to keep ignoring sub_fuzziness=2 on ~/.mplayer/sub directory. subdirs patch updates a little the documentation about this point too.
Sure it doesn't make more sense to only apply it to the movie directory and nothing else at all? Doesn't make sense to have a path where there's only the subtitle(s) for one single movie - and in all other cases it will end up with wrong subtitles.
After the first patch, it will still be the same: only two directories are tracked: ~/.mplayer/sub and the movie one.
About the fuzziness behaviour with the subdirs patch, I think it should also honor the value of 2 since subdirs patch is mainly for relative paths.
Ok, your examples convinced me. Well, at least enough that I think either is ok. Of course it would be possible to make it depend on relative vs. absolute path, but let's forget about that for now.
And actually, -sub did disable auto-loading before.
Huh? I don't think so; the old code do that:
if (sub_name) { ... }
if (sub_auto) { ... }
And I kept this.
You obviously missed the else. Oh, what a mess. The else is only there in mencoder...
Oh. This is bad. How do you want me to handle that? Of course I personally consider the factorization also being a fix for the MEncoder code, but something tell me you won't agree with that :-( By the way, is the rest of this first patch clear enough now? -- Clément B. Not sent from a jesusPhone.
On Tue, Nov 23, 2010 at 12:50:45AM +0100, Clément Bœsch wrote:
Oh. This is bad. How do you want me to handle that? Of course I personally consider the factorization also being a fix for the MEncoder code, but something tell me you won't agree with that :-(
I am ok with considering a fix, I am just not convinced it is fixing it the right way round. But it probably is more flexible this way.
By the way, is the rest of this first patch clear enough now?
I didn't look that closely yet. I still think it is too much code, but that is just a gut feeling and may be wrong of course.
On Tue, Nov 23, 2010 at 08:07:25AM +0100, Reimar Döffinger wrote:
On Tue, Nov 23, 2010 at 12:50:45AM +0100, Clément Bœsch wrote:
Oh. This is bad. How do you want me to handle that? Of course I personally consider the factorization also being a fix for the MEncoder code, but something tell me you won't agree with that :-(
I am ok with considering a fix, I am just not convinced it is fixing it the right way round. But it probably is more flexible this way.
By the way, is the rest of this first patch clear enough now?
I didn't look that closely yet. I still think it is too much code, but that is just a gut feeling and may be wrong of course.
Sorry I couldn't simplify it more. Still not good to commit? Also, I noticed the VOB-Sub loading is totally different from the default one. I'd like to work on it to make it benefit from other subtitles options (like sub-fuzziness, sub-directories too, etc); I'd like to get rid of the hackish loading in mplayer.c. I really can't integrate the subdirs option for vobsub in the current state without huge changes (and I don't think you want that, neither I do since it will delay the patch again). -- Clément B. Not sent from a jesusPhone.
On Mon, Nov 29, 2010 at 11:08:55PM +0100, Clément Bœsch wrote:
On Tue, Nov 23, 2010 at 08:07:25AM +0100, Reimar Döffinger wrote:
On Tue, Nov 23, 2010 at 12:50:45AM +0100, Clément Bœsch wrote:
Oh. This is bad. How do you want me to handle that? Of course I personally consider the factorization also being a fix for the MEncoder code, but something tell me you won't agree with that :-(
I am ok with considering a fix, I am just not convinced it is fixing it the right way round. But it probably is more flexible this way.
By the way, is the rest of this first patch clear enough now?
I didn't look that closely yet. I still think it is too much code, but that is just a gut feeling and may be wrong of course.
Sorry I couldn't simplify it more. Still not good to commit?
Also, I noticed the VOB-Sub loading is totally different from the default one. I'd like to work on it to make it benefit from other subtitles options (like sub-fuzziness, sub-directories too, etc); I'd like to get rid of the hackish loading in mplayer.c. I really can't integrate the subdirs option for vobsub in the current state without huge changes (and I don't think you want that, neither I do since it will delay the patch again).
Well, since I still don't have any feedback, I tried to split it more. So There is now 3 patches. I wonder how I can split it more. Please review this so I can move on :) -- Clément B. Not sent from a jesusPhone.
On Thu, Dec 16, 2010 at 09:58:39PM +0100, Clément Bœsch wrote:
On Mon, Nov 29, 2010 at 11:08:55PM +0100, Clément Bœsch wrote:
On Tue, Nov 23, 2010 at 08:07:25AM +0100, Reimar Döffinger wrote:
On Tue, Nov 23, 2010 at 12:50:45AM +0100, Clément Bœsch wrote:
Oh. This is bad. How do you want me to handle that? Of course I personally consider the factorization also being a fix for the MEncoder code, but something tell me you won't agree with that :-(
I am ok with considering a fix, I am just not convinced it is fixing it the right way round. But it probably is more flexible this way.
By the way, is the rest of this first patch clear enough now?
I didn't look that closely yet. I still think it is too much code, but that is just a gut feeling and may be wrong of course.
Sorry I couldn't simplify it more. Still not good to commit?
Also, I noticed the VOB-Sub loading is totally different from the default one. I'd like to work on it to make it benefit from other subtitles options (like sub-fuzziness, sub-directories too, etc); I'd like to get rid of the hackish loading in mplayer.c. I really can't integrate the subdirs option for vobsub in the current state without huge changes (and I don't think you want that, neither I do since it will delay the patch again).
Well, since I still don't have any feedback, I tried to split it more. So There is now 3 patches. I wonder how I can split it more. Please review this so I can move on :)
[...]
Subject: [PATCH 1/3] Factorize subtitles loading between mplayer and mencoder.
--- mencoder.c | 18 +----------------- mplayer.c | 16 +--------------- sub/subreader.c | 27 ++++++++++++++++++++++++++- sub/subreader.h | 2 +- 4 files changed, 29 insertions(+), 34 deletions(-)
I will commit this first patch in the next three days so we can move on. I don't want to let this rot, so if you have anything to say on this or the following patches, please do (even if it's an approval...). -- Clément B. Not sent from a jesusPhone.
On 21 dec 2010, at 11:00, Clément Bœsch <ubitux@gmail.com> wrote:
On Thu, Dec 16, 2010 at 09:58:39PM +0100, Clément Bœsch wrote:
On Mon, Nov 29, 2010 at 11:08:55PM +0100, Clément Bœsch wrote:
On Tue, Nov 23, 2010 at 08:07:25AM +0100, Reimar Döffinger wrote:
On Tue, Nov 23, 2010 at 12:50:45AM +0100, Clément Bœsch wrote:
Oh. This is bad. How do you want me to handle that? Of course I personally consider the factorization also being a fix for the MEncoder code, but something tell me you won't agree with that :-(
I am ok with considering a fix, I am just not convinced it is fixing it the right way round. But it probably is more flexible this way.
By the way, is the rest of this first patch clear enough now?
I didn't look that closely yet. I still think it is too much code, but that is just a gut feeling and may be wrong of course.
Sorry I couldn't simplify it more. Still not good to commit?
Also, I noticed the VOB-Sub loading is totally different from the default one. I'd like to work on it to make it benefit from other subtitles options (like sub-fuzziness, sub-directories too, etc); I'd like to get rid of the hackish loading in mplayer.c. I really can't integrate the subdirs option for vobsub in the current state without huge changes (and I don't think you want that, neither I do since it will delay the patch again).
Well, since I still don't have any feedback, I tried to split it more. So There is now 3 patches. I wonder how I can split it more. Please review this so I can move on :)
[...]
Subject: [PATCH 1/3] Factorize subtitles loading between mplayer and mencoder.
--- mencoder.c | 18 +----------------- mplayer.c | 16 +--------------- sub/subreader.c | 27 ++++++++++++++++++++++++++- sub/subreader.h | 2 +- 4 files changed, 29 insertions(+), 34 deletions(-)
I will commit this first patch in the next three days so we can move on.
You removed the filename == NULL check that was in the mencoder code, either keep that or double and triple check the code. Otherwise it seems ok to me, did not yet properly check the other patches.
On Wed, Dec 22, 2010 at 05:53:24PM +0100, Reimar Döffinger wrote:
On 21 dec 2010, at 11:00, Clément Bœsch <ubitux@gmail.com> wrote:
On Thu, Dec 16, 2010 at 09:58:39PM +0100, Clément Bœsch wrote:
On Mon, Nov 29, 2010 at 11:08:55PM +0100, Clément Bœsch wrote:
On Tue, Nov 23, 2010 at 08:07:25AM +0100, Reimar Döffinger wrote:
On Tue, Nov 23, 2010 at 12:50:45AM +0100, Clément Bœsch wrote:
Oh. This is bad. How do you want me to handle that? Of course I personally consider the factorization also being a fix for the MEncoder code, but something tell me you won't agree with that :-(
I am ok with considering a fix, I am just not convinced it is fixing it the right way round. But it probably is more flexible this way.
By the way, is the rest of this first patch clear enough now?
I didn't look that closely yet. I still think it is too much code, but that is just a gut feeling and may be wrong of course.
Sorry I couldn't simplify it more. Still not good to commit?
Also, I noticed the VOB-Sub loading is totally different from the default one. I'd like to work on it to make it benefit from other subtitles options (like sub-fuzziness, sub-directories too, etc); I'd like to get rid of the hackish loading in mplayer.c. I really can't integrate the subdirs option for vobsub in the current state without huge changes (and I don't think you want that, neither I do since it will delay the patch again).
Well, since I still don't have any feedback, I tried to split it more. So There is now 3 patches. I wonder how I can split it more. Please review this so I can move on :)
[...]
Subject: [PATCH 1/3] Factorize subtitles loading between mplayer and mencoder.
--- mencoder.c | 18 +----------------- mplayer.c | 16 +--------------- sub/subreader.c | 27 ++++++++++++++++++++++++++- sub/subreader.h | 2 +- 4 files changed, 29 insertions(+), 34 deletions(-)
I will commit this first patch in the next three days so we can move on.
You removed the filename == NULL check that was in the mencoder code, either keep that or double and triple check the code. Otherwise it seems ok to me, did not yet properly check the other patches.
There should not be any problem with the mencoder since there is an exit upper in case of no filename. But I don't get how it works with mplayer code... I fixed this in the patch (reattached). Btw, the check was put back in the second patch. For the other patches, they're attached to the previous mail I just sent. -- Clément B. Not sent from a jesusPhone.
On Thu, Dec 23, 2010 at 04:33:19PM +0100, Clément Bœsch wrote:
On Wed, Dec 22, 2010 at 05:53:24PM +0100, Reimar Döffinger wrote:
On 21 dec 2010, at 11:00, Clément Bœsch <ubitux@gmail.com> wrote:
On Thu, Dec 16, 2010 at 09:58:39PM +0100, Clément Bœsch wrote:
On Mon, Nov 29, 2010 at 11:08:55PM +0100, Clément Bœsch wrote:
On Tue, Nov 23, 2010 at 08:07:25AM +0100, Reimar Döffinger wrote:
On Tue, Nov 23, 2010 at 12:50:45AM +0100, Clément Bœsch wrote: > Oh. This is bad. How do you want me to handle that? Of course I personally > consider the factorization also being a fix for the MEncoder code, but > something tell me you won't agree with that :-(
I am ok with considering a fix, I am just not convinced it is fixing it the right way round. But it probably is more flexible this way.
> By the way, is the rest of this first patch clear enough now?
I didn't look that closely yet. I still think it is too much code, but that is just a gut feeling and may be wrong of course.
Sorry I couldn't simplify it more. Still not good to commit?
Also, I noticed the VOB-Sub loading is totally different from the default one. I'd like to work on it to make it benefit from other subtitles options (like sub-fuzziness, sub-directories too, etc); I'd like to get rid of the hackish loading in mplayer.c. I really can't integrate the subdirs option for vobsub in the current state without huge changes (and I don't think you want that, neither I do since it will delay the patch again).
Well, since I still don't have any feedback, I tried to split it more. So There is now 3 patches. I wonder how I can split it more. Please review this so I can move on :)
[...]
Subject: [PATCH 1/3] Factorize subtitles loading between mplayer and mencoder.
--- mencoder.c | 18 +----------------- mplayer.c | 16 +--------------- sub/subreader.c | 27 ++++++++++++++++++++++++++- sub/subreader.h | 2 +- 4 files changed, 29 insertions(+), 34 deletions(-)
I will commit this first patch in the next three days so we can move on.
You removed the filename == NULL check that was in the mencoder code, either keep that or double and triple check the code. Otherwise it seems ok to me, did not yet properly check the other patches.
There should not be any problem with the mencoder since there is an exit upper in case of no filename. But I don't get how it works with mplayer code...
I fixed this in the patch (reattached).
Btw, the check was put back in the second patch. For the other patches, they're attached to the previous mail I just sent.
-- Clément B. Not sent from a jesusPhone.
From d50c977cf7c082e7ed39ab7a7355d5157c26d52e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <ubitux@gmail.com> Date: Thu, 16 Dec 2010 21:24:48 +0100 Subject: [PATCH 1/3] Factorize subtitles loading between mplayer and mencoder.
--- mencoder.c | 18 +----------------- mplayer.c | 16 +--------------- sub/subreader.c | 28 +++++++++++++++++++++++++++- sub/subreader.h | 2 +- 4 files changed, 30 insertions(+), 34 deletions(-)
Applied. -- Clément B. Not sent from a jesusPhone.
On Thu, Dec 16, 2010 at 09:58:39PM +0100, Clément Bœsch wrote:
Well, since I still don't have any feedback, I tried to split it more. So There is now 3 patches. I wonder how I can split it more. Please review this so I can move on :)
--- a/sub/subreader.c +++ b/sub/subreader.c @@ -1889,7 +1890,7 @@ static int compare_sub_priority(const void *a, const void *b)
-char** sub_filenames(const char* path, char *fname) +static char** sub_filenames(const char* path, const char *fname)
Please fix the * placement to K&R while you're changing this line anyway.
@@ -2070,6 +2071,30 @@ char** sub_filenames(const char* path, char *fname)
+/** + * \brief Load all subtitles matching the subtitle filename + * \param fname Path to subtitle filename + * \param fps FPS parameter for the add subtitle function + * \param add_f Add subtitle function to call for each sub
Please use @ syntax with Doxygen. I'm afraid we have a mix of both in MPlayer, but @ is more common and easier to grep for :)
--- a/DOCS/man/en/mplayer.1 +++ b/DOCS/man/en/mplayer.1 @@ -2625,6 +2625,27 @@ Guess the encoding for Polish, fall back on cp1250. . .TP +.B \-subdirs <dirname1,dirname2,...> +Specify extra subtitles directories to track in the media directory. +.sp 1 +.I EXAMPLE: +Assuming that /path/\:to/\:movie/\:movie.avi is played and \-subdirs +sub,subtitles,/tmp/subs is specified, MPlayer searches for subtitles files in these
subtitle files And please break this line earlier.
--- a/sub/subreader.c +++ b/sub/subreader.c @@ -2123,6 +2123,18 @@ void load_subtitles(const char *fname, int fps, void add_f(char *, float, int))
+ // Load subtitles in dirs specified by subdirs option + if (sub_dirs) + for (i = 0; sub_dirs[i]; i++) +#if HAVE_DOS_PATHS + if (sub_dirs[i][1] == ':') +#else + if (sub_dirs[i][0] == '/') +#endif
Maybe this can be factorized into a global PATH_SEP #define or similar? I suspect we have something similar in other places... Diego
On Wed, Dec 22, 2010 at 01:06:11PM +0100, Diego Biurrun wrote:
On Thu, Dec 16, 2010 at 09:58:39PM +0100, Clément Bœsch wrote:
Well, since I still don't have any feedback, I tried to split it more. So There is now 3 patches. I wonder how I can split it more. Please review this so I can move on :)
--- a/sub/subreader.c +++ b/sub/subreader.c @@ -1889,7 +1890,7 @@ static int compare_sub_priority(const void *a, const void *b)
-char** sub_filenames(const char* path, char *fname) +static char** sub_filenames(const char* path, const char *fname)
Please fix the * placement to K&R while you're changing this line anyway.
Fixed. Btw, I didn't feel the need to since the prototype is changed in a later commit.
@@ -2070,6 +2071,30 @@ char** sub_filenames(const char* path, char *fname)
+/** + * \brief Load all subtitles matching the subtitle filename + * \param fname Path to subtitle filename + * \param fps FPS parameter for the add subtitle function + * \param add_f Add subtitle function to call for each sub
Please use @ syntax with Doxygen. I'm afraid we have a mix of both in MPlayer, but @ is more common and easier to grep for :)
Changed. I also prefer the '@' syntax, but it was just to be consistent with the rest of the file. New syntax is now in use in the 3 commits.
--- a/DOCS/man/en/mplayer.1 +++ b/DOCS/man/en/mplayer.1 @@ -2625,6 +2625,27 @@ Guess the encoding for Polish, fall back on cp1250. . .TP +.B \-subdirs <dirname1,dirname2,...> +Specify extra subtitles directories to track in the media directory. +.sp 1 +.I EXAMPLE: +Assuming that /path/\:to/\:movie/\:movie.avi is played and \-subdirs +sub,subtitles,/tmp/subs is specified, MPlayer searches for subtitles files in these
subtitle files
Fixed.
And please break this line earlier.
Done.
--- a/sub/subreader.c +++ b/sub/subreader.c @@ -2123,6 +2123,18 @@ void load_subtitles(const char *fname, int fps, void add_f(char *, float, int))
+ // Load subtitles in dirs specified by subdirs option + if (sub_dirs) + for (i = 0; sub_dirs[i]; i++) +#if HAVE_DOS_PATHS + if (sub_dirs[i][1] == ':') +#else + if (sub_dirs[i][0] == '/') +#endif
Maybe this can be factorized into a global PATH_SEP #define or similar? I suspect we have something similar in other places...
The condition does not check the same character, but I since this condition may not be explicit about what it does, I could make an helper in path.c, something like: int mp_path_is_absolute(const char *path) { #if HAVE_DOS_PATHS return path[0] && path[1] == ':'; #else return path[0] == '/'; #endif } Would you prefer this? I can't find any other place where it can be used atm so... -- Clément B. Not sent from a jesusPhone.
On Thu, Dec 23, 2010 at 04:29:45PM +0100, Clément Bœsch wrote:
On Wed, Dec 22, 2010 at 01:06:11PM +0100, Diego Biurrun wrote:
On Thu, Dec 16, 2010 at 09:58:39PM +0100, Clément Bœsch wrote:
@@ -2070,6 +2071,30 @@ char** sub_filenames(const char* path, char *fname)
+/** + * \brief Load all subtitles matching the subtitle filename + * \param fname Path to subtitle filename + * \param fps FPS parameter for the add subtitle function + * \param add_f Add subtitle function to call for each sub
Please use @ syntax with Doxygen. I'm afraid we have a mix of both in MPlayer, but @ is more common and easier to grep for :)
Changed. I also prefer the '@' syntax, but it was just to be consistent with the rest of the file. New syntax is now in use in the 3 commits.
Feel free to change the rest of the file or any \ Doxygen syntax you encounter. I will at some point otherwise.
--- a/sub/subreader.c +++ b/sub/subreader.c @@ -2123,6 +2123,18 @@ void load_subtitles(const char *fname, int fps, void add_f(char *, float, int))
+ // Load subtitles in dirs specified by subdirs option + if (sub_dirs) + for (i = 0; sub_dirs[i]; i++) +#if HAVE_DOS_PATHS + if (sub_dirs[i][1] == ':') +#else + if (sub_dirs[i][0] == '/') +#endif
Maybe this can be factorized into a global PATH_SEP #define or similar? I suspect we have something similar in other places...
The condition does not check the same character, but I since this condition may not be explicit about what it does, I could make an helper in path.c, something like:
int mp_path_is_absolute(const char *path) { #if HAVE_DOS_PATHS return path[0] && path[1] == ':'; #else return path[0] == '/'; #endif }
Would you prefer this?
I can't find any other place where it can be used atm so...
Whatever you prefer. I just had a hunch we might have similar code in other places, I did not check.
--- a/DOCS/man/en/mplayer.1 +++ b/DOCS/man/en/mplayer.1 @@ -2625,6 +2625,27 @@ Guess the encoding for Polish, fall back on cp1250. . .TP +.B \-subdirs <dirname1,dirname2,...> +Specify extra subtitles directories to track in the media directory.
subtitle directories BTW, no need to send a new patch for such nits, just change locally. Diego
On Fri, Dec 24, 2010 at 04:57:16PM +0100, Diego Biurrun wrote:
On Thu, Dec 23, 2010 at 04:29:45PM +0100, Clément Bœsch wrote:
On Wed, Dec 22, 2010 at 01:06:11PM +0100, Diego Biurrun wrote:
On Thu, Dec 16, 2010 at 09:58:39PM +0100, Clément Bœsch wrote:
@@ -2070,6 +2071,30 @@ char** sub_filenames(const char* path, char *fname)
+/** + * \brief Load all subtitles matching the subtitle filename + * \param fname Path to subtitle filename + * \param fps FPS parameter for the add subtitle function + * \param add_f Add subtitle function to call for each sub
Please use @ syntax with Doxygen. I'm afraid we have a mix of both in MPlayer, but @ is more common and easier to grep for :)
Changed. I also prefer the '@' syntax, but it was just to be consistent with the rest of the file. New syntax is now in use in the 3 commits.
Feel free to change the rest of the file or any \ Doxygen syntax you encounter. I will at some point otherwise.
OK. Changed in sub/subreader.c.
--- a/sub/subreader.c +++ b/sub/subreader.c @@ -2123,6 +2123,18 @@ void load_subtitles(const char *fname, int fps, void add_f(char *, float, int))
+ // Load subtitles in dirs specified by subdirs option + if (sub_dirs) + for (i = 0; sub_dirs[i]; i++) +#if HAVE_DOS_PATHS + if (sub_dirs[i][1] == ':') +#else + if (sub_dirs[i][0] == '/') +#endif
Maybe this can be factorized into a global PATH_SEP #define or similar? I suspect we have something similar in other places...
The condition does not check the same character, but I since this condition may not be explicit about what it does, I could make an helper in path.c, something like:
int mp_path_is_absolute(const char *path) { #if HAVE_DOS_PATHS return path[0] && path[1] == ':'; #else return path[0] == '/'; #endif }
Would you prefer this?
I can't find any other place where it can be used atm so...
Whatever you prefer. I just had a hunch we might have similar code in other places, I did not check.
Function added.
--- a/DOCS/man/en/mplayer.1 +++ b/DOCS/man/en/mplayer.1 @@ -2625,6 +2625,27 @@ Guess the encoding for Polish, fall back on cp1250. . .TP +.B \-subdirs <dirname1,dirname2,...> +Specify extra subtitles directories to track in the media directory.
subtitle directories
BTW, no need to send a new patch for such nits, just change locally.
OK, changed locally then :) -- Clément B. Not sent from a jesusPhone.
On Sun, Nov 21, 2010 at 12:05:58PM +0100, Clément Bœsch wrote:
On Sat, Nov 20, 2010 at 09:49:44PM +0100, Reimar Döffinger wrote:
On Sat, Nov 20, 2010 at 09:21:34PM +0100, Clément Bœsch wrote:
+ * \brief Allocates a new buffer containing the directory name + * \param path Original path. Must be a valid string. + * + * The path returned always contains a trailing slash '/'. + * On systems supporting DOS paths, '\' is also considered as a directory + * separator in addition to the '/'. + */ +char *mp_dirname(const char *path) +{ + const char *base = mp_basename(path); + size_t len = base - path; + char *dirname; + + if (len == 0) + return strdup("./"); + dirname = malloc(len + 1); + if (!dirname) + return NULL; + strncpy(dirname, path, len); + dirname[len] = '\0';
Not sure if it is an improvement, but you could do both in one by using av_strlcpy(dirname, path, len + 1);
After looking at the av_strlcpy implementation, it seems a trivial copy is made, so I found the strncpy more appropriated since it can benefits libc optimizations.
Either way, that part is ok. Except for one thing I noticed while looking through the next patch: For DOS paths you must also consider ':' a path separator in mp_basename, so that c:test.avi is treated correctly.
OK, then I'll commit this, and make a second patch to fix that.
Applied. What about this patch for the ':' separator? According to these tests, it works: printf("%s\n", mp_basename("bar")); printf("%s\n", mp_basename("c:\\foo\\bar")); printf("%s\n", mp_basename("c:/foo/bar")); printf("%s\n", mp_basename("c:\\foo/bar")); printf("%s\n", mp_basename("c:/foo\\bar")); printf("%s\n", mp_basename("c:foo\\bar")); printf("%s\n", mp_basename("c:foo/bar")); printf("%s\n", mp_basename("c:foo/bar")); printf("%s\n", mp_basename("c:foo\\bar")); But since c:test.avi must be supported, I wonder if c:foo:test.avi has to. The current patch consider ':' as a normal path separator, just like subreader.c does, but maybe a smarter thing must be done? -- Clément B. Not sent from a jesusPhone.
OK, new try with a better base. I have simplified the patches (especially the "Make load of n subtitles directories possible." one) so review is easier again. So basically, the first patch transforms sub_filenames in a new function (append_dir_subtitles) only able to load one directory while the old one was doing all the nasty stuff loading subtitles in the video directory and in ~/.mplayer/sub. load_subtitles is then updated to load each directory one at a time. This first patch makes the second one much simpler with just a loop to load subtitles in the specified directories. 'Hope this will finally be mplayer-compliant to be committed :) Regards, -- Clément B. Not sent from a jesusPhone.
On Mon, Jan 03, 2011 at 09:02:57PM +0100, Clément Bœsch wrote:
'Hope this will finally be mplayer-compliant to be committed :)
:)
--- a/DOCS/man/en/mplayer.1 +++ b/DOCS/man/en/mplayer.1 @@ -2625,6 +2625,27 @@ Guess the encoding for Polish, fall back on cp1250. .PD 1 . .TP +.B \-subdirs <dirname1,dirname2,...> +Specify extra subtitle directories to track in the media directory.
"subdir" is a subdirectory, not a subtitle-directory. I think you should come up with another name like "subsdir" or "subtitledir" or similar. Diego
On Mon, Jan 03, 2011 at 09:19:22PM +0100, Diego Biurrun wrote:
On Mon, Jan 03, 2011 at 09:02:57PM +0100, Clément Bœsch wrote:
'Hope this will finally be mplayer-compliant to be committed :)
:)
Well, at least some review then :)
--- a/DOCS/man/en/mplayer.1 +++ b/DOCS/man/en/mplayer.1 @@ -2625,6 +2625,27 @@ Guess the encoding for Polish, fall back on cp1250. .PD 1 . .TP +.B \-subdirs <dirname1,dirname2,...> +Specify extra subtitle directories to track in the media directory.
"subdir" is a subdirectory, not a subtitle-directory. I think you should come up with another name like "subsdir" or "subtitledir" or similar.
I took the "sub" prefix because of the rest of the subtitles options. I should have called it -sub-dirs btw, but it does not solve the issue here. I could call it -subtitles-dir, but any option starting with "-subtitle" will likely break the consistency of the subtitles options. Then, maybe -sub-paths just like Uoti proposed a while ago would be fine? I'm not a native english speaker, but if I'm not wrong, "sub-path" does not exist so it should be fine. Can you confirm? -- Clément B. Not sent from a jesusPhone.
On Mon, Jan 03, 2011 at 09:46:50PM +0100, Clément Bœsch wrote:
On Mon, Jan 03, 2011 at 09:19:22PM +0100, Diego Biurrun wrote:
On Mon, Jan 03, 2011 at 09:02:57PM +0100, Clément Bœsch wrote:
--- a/DOCS/man/en/mplayer.1 +++ b/DOCS/man/en/mplayer.1 @@ -2625,6 +2625,27 @@ Guess the encoding for Polish, fall back on cp1250. .PD 1 . .TP +.B \-subdirs <dirname1,dirname2,...> +Specify extra subtitle directories to track in the media directory.
"subdir" is a subdirectory, not a subtitle-directory. I think you should come up with another name like "subsdir" or "subtitledir" or similar.
I took the "sub" prefix because of the rest of the subtitles options. I should have called it -sub-dirs btw, but it does not solve the issue here.
I could call it -subtitles-dir, but any option starting with "-subtitle" will likely break the consistency of the subtitles options.
Then, maybe -sub-paths just like Uoti proposed a while ago would be fine? I'm not a native english speaker, but if I'm not wrong, "sub-path" does not exist so it should be fine. Can you confirm?
Yes, that's better. Diego
On Mon, Jan 03, 2011 at 09:02:57PM +0100, Clément Bœsch wrote:
OK, new try with a better base. I have simplified the patches (especially the "Make load of n subtitles directories possible." one) so review is easier again.
So basically, the first patch transforms sub_filenames in a new function (append_dir_subtitles) only able to load one directory while the old one was doing all the nasty stuff loading subtitles in the video directory and in ~/.mplayer/sub. load_subtitles is then updated to load each directory one at a time.
This first patch makes the second one much simpler with just a loop to load subtitles in the specified directories.
'Hope this will finally be mplayer-compliant to be committed :)
I think it's ok, but I do not feel like reviewing it extensively. I'll assume you tested it quite well.
On Wed, Jan 05, 2011 at 04:26:55PM +0100, Reimar Döffinger wrote:
On Mon, Jan 03, 2011 at 09:02:57PM +0100, Clément Bœsch wrote:
OK, new try with a better base. I have simplified the patches (especially the "Make load of n subtitles directories possible." one) so review is easier again.
So basically, the first patch transforms sub_filenames in a new function (append_dir_subtitles) only able to load one directory while the old one was doing all the nasty stuff loading subtitles in the video directory and in ~/.mplayer/sub. load_subtitles is then updated to load each directory one at a time.
This first patch makes the second one much simpler with just a loop to load subtitles in the specified directories.
'Hope this will finally be mplayer-compliant to be committed :)
I think it's ok, but I do not feel like reviewing it extensively. I'll assume you tested it quite well.
First one applied. I'll commit the second one (re-attached) in the next days. -- Clément B.
participants (3)
-
Clément Bœsch -
Diego Biurrun -
Reimar Döffinger