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

[Reimar Döffinger]

Hello,
On Wed, Jun 14, 2006 at 01:09:42PM +0400, Evgeniy Stepanov wrote:
> Several bugfixes.

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

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.

in mplayer.c:
> @@ -1845,6 +1861,20 @@
>              return 1;
>          }
>  #endif
> +#ifdef USE_ASS
> +        if (ass_track && ass_track->name) {
> +            char *tmp,*tmp2;
> +            tmp = ass_track->name;
> +            if ((tmp2 = strrchr(tmp, '/')))
> +                tmp = tmp2+1;
> +
> +            snprintf(*(char**)arg, 63, "(%d) %s%s",
> +                     set_of_sub_pos + 1,
> +                     strlen(tmp) < 20 ? "" : "...",
> +                     strlen(tmp) < 20 ? tmp : tmp+strlen(tmp)-19);
> +            return 1;
> +        }
> +#endif

Please do not duplicate code. Either move it in a seperate function or just
use the existing code e.g. by changing
"if (subdata)" to "if (subdata || ass_track)"
and
"tmp = subdata->filename;" to "tmp = subdata ? subdata->filename : ass_track->name;"
You can check for tmp == NULL then and set some useful substitute...

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

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

> @@ -5084,9 +5160,15 @@
>      current_module="sub_free";
>      for (i = 0; i < set_of_sub_size; ++i)
>          sub_free( set_of_subtitles[i] );
> -    set_of_sub_size = 0;
>      vo_sub_last = vo_sub=NULL;
>      subdata=NULL;
> +#ifdef USE_ASS
> +    for (i = 0; i < set_of_sub_size; ++i)
> +      if ( set_of_ass_tracks[i] )
> +        ass_free_track( set_of_ass_tracks[i] );
> +    ass_track = NULL;
> +#endif
> +    set_of_sub_size = 0;

Why not save some code and do both frees in the same loop?

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

in ass_render.c:
> +#define PI 3.1415926535897932384626433832795029L

why not just use M_PI? I think we already (e.g. in the audio filters) require
that to be defined properly in math.h

> +#define _r(c)  ((c)>>24)
> +#define _g(c)  (((c)>>16)&0xFF)
> +#define _b(c)  (((c)>>8)&0xFF)
> +#define _a(c)  ((c)&0xFF)
> +#define rgba2y(c)  ( (( 263*_r(c)  + 516*_g(c) + 100*_b(c)) >> 10) + 16  )
> +#define rgba2u(c)  ( ((-152*_r(c) - 298*_g(c) + 450*_b(c)) >> 10) + 128 )
> +#define rgba2v(c)  ( (( 450*_r(c) - 376*_g(c) -  73*_b(c)) >> 10) + 128 )

These seem to be unused??

> +/**
> + * \brief Parse color value from ass script.
> + */
> +static inline unsigned ampcolor(char* p, char** q) {
> +#define badcolor { mp_msg(MSGT_GLOBAL, MSGL_WARN, "bad color at %s:%d\n", __FILE__, __LINE__); *q = p; return 0; }

IMHO this is ugly and also bloats code. Normally I woud suggest using goto,
but here:

> +	unsigned color = 0;
> +	if (*p++ != '&') badcolor
> +	if (*p++ != 'H') badcolor

why not just
if (*p++ != '&' || *p++ != 'H') {
  mp_msg(...);
}

Ok, you can't make a difference between the '&' and 'H' expected cases then...

> +/**
> + * \brief Calculate a weighted average of two colors
> + * calculates c1*(1-a) + c2*a, but separately for each component
> + */
> +static unsigned color_avg(unsigned c1, unsigned c2, double a)
> +{
> +	unsigned char* p1 = (unsigned char*)&c1;
> +	unsigned char* p2 = (unsigned char*)&c2;
> +	unsigned c3;
> +	unsigned char* p3 = (unsigned char*)&c3;
> +	int i;
> +	for (i = 0; i < 4; ++i)
> +		p3[i] = p1[i] * (1 - a) + p2[i] * a;
> +	return c3;

If something absolutely needs to have 4 bytes size, please use uint32_t instead of
just unsigned. Also

> +static void change_alpha(unsigned* var, unsigned new_val, double pwr)
> +{
> +	(*var) = color_avg(*var, ((*var) & 0xFFFFFF00) | new_val, pwr);
> +}
> +
> +static void change_color(unsigned* var, unsigned new_val, double pwr)
> +{
> +	(*var) = color_avg(*var, ((*var) & 0x000000FF) | new_val, pwr);
> +}

Interpolating all four component when only one respectively three values
are different is a bit overkill...
Maybe it would be simpler to make c1, c2 and all internal color stuff
uin8_t c1[4] instead of unsigned and convert via the usual
(c1[0] << 24) | (c1[1] << 16) | (c1[2] << 8) | c1[3]
(see the BE_32 macro in libavcodec), though you might have to be more careful
to make sure it works with any endianness.

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

> +static unsigned string2color(char* p) {
> +#define badcolor { mp_msg(MSGT_GLOBAL, MSGL_WARN, "bad color at %s:%d\n", __FILE__, __LINE__); return 0; }

Huh? What is the difference between this function and the previous one where
I complained about this macro?

> +#define NEXT(str,token) \
> +	token = next_token(&str); \
> +	if (!token) break;
> +
> +#define ANYVAL(name,func) \
> +	} else if (strcasecmp(tname, #name) == 0) { \
> +		target->name = func(token); \
> +		mp_msg(MSGT_GLOBAL, MSGL_DBG2, "%s = %s\n", #name, token);
> +#define STRVAL(name) ANYVAL(name,strdup)
> +#define COLORVAL(name) ANYVAL(name,string2color)
> +#define INTVAL(name) ANYVAL(name,atoi)
> +#define FPVAL(name) ANYVAL(name,atof)
> +#define TIMEVAL(name) ANYVAL(name,string2timecode)
> +#define STYLEVAL(name) \
> +	} else if (strcasecmp(tname, #name) == 0) { \
> +		target->name = lookup_style(track, token); \
> +		mp_msg(MSGT_GLOBAL, MSGL_DBG2, "%s = %s\n", #name, token);
> +
> +#define ALIAS(alias,name) \
> +	if (strcasecmp(tname, #alias) == 0) {tname = #name;}

Hmmm... maybe you can convert some of these into functions, I think it might
help readability a bit (esp. when it comes to checking for possible
vulnerabilities - which I admit I am not doing).

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

> +static char* validate_fname(char* name)

I am not sure this is careful enough - though I think this feature is a
problem in itself.

> +/**
> + * \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).

> Index: libass/ass_utils.c
> ===================================================================
> --- libass/ass_utils.c	(.../branches/upstream)	(revision 0)
> +++ libass/ass_utils.c	(.../trunk)	(revision 503)
> @@ -0,0 +1,39 @@
> +#include "config.h"
> +
> +#include <stdint.h>
> +#include <sys/time.h>
> +#include <time.h>
> +
> +#include "mp_msg.h"
> +#include "ass_utils.h"
> +
> +unsigned utf8_to_ucs4(char** str)

I give whoever commits the wonderful duty to find a place for this
function and the utf8_get_char in libvo/sub.c (they are the same).

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

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

Somebody please make Moritz review the changes to demux_mkv *g*

> 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*)

And now for something completely different. What do you think would be needed
to support this in vo_gl.c? What would be the steps needed to get one or
several bitmaps containing the overlay data for the current frame?

Greetings,
Reimar Döffinger

P.S. I did not review any of the parts under #if 0, might be a good idea to
remove them before committing.