[MPlayer-dev-eng] Re: [PATCH] SSA/ASS subtitles support

[Uoti Urpala]

On Sun, 2006-06-18 at 02:33 +0400, Evgeniy Stepanov wrote:
> On Saturday 17 June 2006 23:27, Reimar Döffinger wrote:
> > > +if test "$_freetype" = no ; then
> > > +    _ass=no
> > > +    _res_comment="FreeType support needed"
> > > +fi
> >
> > IMHO this should go in the "auto" case, --enable should always be
> > respected, regardless of the consequences.
> 
> FreeType and FontConfig checks just above do exactly the same thing.
> If you are right, they should also be fixed to avoid inconsistency.

I think this does not need to go in the "auto" case. This isn't about
checking whether the library is available but about whether it's
internally enabled; the user should --enable freetype too if he wants to
force it.

> Even though they have been moved under if() {} ? That's a tradeoff between 
> patch readability and code readability :) I prefer the latter.

I also prefer having such indentation changes in the same patch in this
case.

> This file is intended for mplayer-specific things. Currently there are only 
> configuration variables, which i otherwise would have to place in both 
> mplayer.c and mencoder.c. Soon I'm going to add subdata -> ass_track_t 
> conversion function here and achieve the dream of colored srt subtitles.

Not directly related to above case, but I think it might be better add
support to mencoder separately later. Also adding the timestamps to the
filter chain is not currently safe for mencoder and does break things
(mencoder is quite broken in several ways and would probably need a
large rewrite).

> > > +static char* validate_fname(char* name)
> >
> > I am not sure this is careful enough - though I think this feature is a
> > problem in itself.
> 
> I'm not sure either. I've found this list of chars somewhere in MSDN. Probably 
> it would be better to only allow some subset of ascii (letters, digits, so 
> on).

The current version seems questionable in a couple of ways. It replaces
"unsafe" characters with '_', but the resulting filename isn't stored
anywhere and it seems that such files are unlikely to ever be
successfully loaded by anything after they've been saved on disk
(fontconfig won't find fonts after the name has been changed etc), so
you could just as well skip saving them. The utf-8 decoding also seems
unnecessary and could allow bytes with high bit set in filenames even
though the code now tries to disallow that (the decoding doesn't check
for nonstandard longer-than-normal representations so you could encode a
small character with a long utf-8 sequence having high bits set).

> Freetype can read fonts directly from memory, but fontconfig can't :(
> I like to have a cache of fonts. What about creating persistent files 
> with -embeddedfonts and temporary ones otherwise ? Do we need a mode when no 
> files will be created at all ?

There does need to be a mode that doesn't require writing on disk.
Having a temporary-file mode might be desirable but IMO it's not a
feature that would be necessary to have before committing the patch to
MPlayer svn.