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

[Clément Bœsch]

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.