[MPlayer-dev-eng] [PATCH] Subtitles directories

[Clément Bœsch]

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 at 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 at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng

-- 
Clément B.
Not sent from a jesusPhone.