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

[Clément Bœsch]

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.