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

[Evgeniy Stepanov]

On Saturday 17 June 2006 23:27, Reimar Döffinger wrote:
> Some reviewing and a question.
> Maybe even those developers without the time to review themselves can
> review my review...
>
> > Index: libmpcodecs/vf.h
> > ===================================================================
> > --- libmpcodecs/vf.h	(.../branches/upstream)	(revision 503)
> > +++ libmpcodecs/vf.h	(.../trunk)	(revision 503)
> > @@ -75,6 +75,7 @@
> >  #define VFCTRL_SKIP_NEXT_FRAME 12 /* For encoding - drop the next frame
> > that passes thru */ #define VFCTRL_FLUSH_FRAMES    13 /* For encoding -
> > flush delayed frames */ #define VFCTRL_SCREENSHOT      14 /* Make a
> > screenshot */
> > +#define VFCTRL_EOSD 15 /* Select EOSD renderer */
>
> The other values are aligned so they are in the same column, so it should
> be done here, too

You mean on the same column as the last four of them ? No problem :)

> in configure:
> > @@ -5387,6 +5391,35 @@
> >  fi
> >  echores "$_fontconfig"
> >
> > +echocheck "SSA/ASS support"
> > +# libass depends on freetype
> > +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.

> > @@ -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.

>
> > @@ -3459,11 +3505,37 @@
>
> [...]
>
> > +#ifdef USE_ASS
> > +if (ass_enabled)
> > +  ((vf_instance_t *)sh_video->vfilter)->control(sh_video->vfilter,
> > VFCTRL_EOSD, 0); +#endif
> > +
>
> What is the intended purpose of this?

This is to select who will display subtitles - either filter or vo. We can 
have both vf_ass and eosd-capable vo at the same time, if user explicitly 
asked for it (with "-ass -vf ass"). In this case the vo will not be used.

>
> > Index: libass/ass_mp.c
> > ===================================================================
> > --- libass/ass_mp.c	(.../branches/upstream)	(revision 0)
> > +++ libass/ass_mp.c	(.../trunk)	(revision 503)
> > @@ -0,0 +1,9 @@
> > +#include "ass_mp.h"
> > +
> > +// libass-related command line options
> > +float ass_font_scale = 1.;
> > +float ass_line_spacing = 0.;
> > +int ass_top_margin = 0;
> > +int ass_bottom_margin = 0;
> > +int extract_embedded_fonts = 0;
> > +
>
> A whole file for just nine lines seems a bit like overkill to me, but your
> decision.

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.

> > +static void skip_spaces(char** str) {
> > +	char* p = *str;
> > +	while ((*p==' ') || (*p=='\t'))
> > +		++p;
> > +	*str = p;
> > +}
>
> Is this what the specification defines as valid spaces? I think a comment
> on this function would be nice, since e.g. UTF-8 sure specifies a lot more
> as space...

Specification says nothing about spaces, and this function works with all 
scripts I've seen. I'll fix it someday.

> > +static void* guess_cp(char* data, int size, char *preferred_language,
> > char *fallback)
>
> How does this function relate to the code in subreader.c? Can that one be
> change to use this function, too?

They are almost the same except for stream_t stuff. That's possible, but this 
one should be moved out from #ifdef USE_ASS then.

> > +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).

> > +/**
> > + * \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 :(
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 ?

> > +#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.

> > +static unsigned glyph_hash(glyph_hash_key_t* key) {
> > +	unsigned val = 0;
> > +	unsigned i;
> > +	for (i = 0; i < sizeof(key->face); ++i)
> > +		val += *(unsigned char *)(&(key->face) + i);
> > +	val <<= 21;
>
> This does not look like the keys will be very unique. Can this be a
> problem?

Usually there are few different font faces used, so this should be ok.

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

I agree with everything else you mentioned, and will fix it in a few days.