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

[Rich Felker]

On Sun, Jun 18, 2006 at 02:33:13AM +0400, Evgeniy Stepanov wrote:
> > > @@ -1925,15 +1955,25 @@
> >
> > [...]
> >
> > > -        subdata = set_of_subtitles[set_of_sub_pos];
> > > -        vo_osd_changed(OSDTYPE_SUBTITLE);
> > > +#ifdef USE_ASS
> > > +        if (ass_enabled && set_of_ass_tracks[set_of_sub_pos])
> > > +            ass_track = set_of_ass_tracks[set_of_sub_pos];
> > > +        else
> > >  #endif
> > > +	{
> > > +            subdata = set_of_subtitles[set_of_sub_pos];
> > > +            vo_osd_changed(OSDTYPE_SUBTITLE);
> >
> > For improved patch-readability, I'd say don't change then indentation of
> > these two lines.
> 
> Even though they have been moved under if() {} ? That's a tradeoff between 
> patch readability and code readability :) I prefer the latter.

You can do the cosmetics in a separate subsequent patch. Mixing them
makes it hard to review and hard for people going back later to audit.

> > > +/**
> > > + * \brief Process embedded matroska font. Saves it to ~/.mplayer/fonts.
> >
> > I do not like this at all.
> > This feature might be misused in many ways, one I can think of is leaving
> > a track record of which films somebody has been watching.
> > The fact that it is disabled by default might make it acceptable though.
> > I prefer if there was a way to use embedded fonts with at most using
> > temporary files created via tmpfile instead (hopefully this function is
> > portable enough).
> 
> Freetype can read fonts directly from memory, but fontconfig can't :(

Is there a reason we need fontconfig? I hate it...

> 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 ?

Yes absolutely. Creating files is a very very bad thing. It will not
work on read-only filesystems (think live cd with MPlayer and movie
files) and it leads to bugs and security issues more often than not.

Why not just bypass fontconfig? I don't even have fontconfig and I'm
quite happy with the way MPlayer works.

> > > +#define MAX_FACE_CACHE_SIZE 100
> > > +
> > > +static face_cache_item_t face_cache[MAX_FACE_CACHE_SIZE];
> >
> > How big is this? Might be better to allocate at runtime because
> > if I understand things correctly this will increase startup time
> > and memory consumption even if ass is not used at all...
> 
> As far as I know, this will only consume some virtual address space. Even so, 
> dynamic allocation is a good idea.

Unless it's absolutely huge, this will translate to physical space,
since it will put more empty space within whatever page it ends up in,
and this page will still be dirty (written) due to other variables in
the same page.

Yes individually these things are small but MPlayer is a huge app and
they will surely add up once there are lots of similarly bad designs.

> > > Index: mencoder.c
> > > ===================================================================
> > > --- mencoder.c	(.../branches/upstream)	(revision 503)
> > > +++ mencoder.c	(.../trunk)	(revision 503)
> > > @@ -54,6 +54,7 @@
> > >  #include "libmpdemux/stheader.h"
> > >  #include "libmpdemux/mp3_hdr.h"
> > >  #include "libmpdemux/muxer.h"
> > > +#include "libmpdemux/matroska.h"
> >
> > Why is this needed? Can it be avoided? (if this is in mplayer.c as well,
> > same question there *g*)
> 
> For mkv_sh_sub_t definition.

Should be some way to fix this to be demuxer-independent.

Rich