[MPlayer-dev-eng] [PATCH] Tags support for SubRip and MicroDVD subtitles

[ubitux]

Just some notes about my opinion about your patch (code only):

On Mon, May 17, 2010 at 10:34:50AM -0300, Kazuo Teramoto wrote:
> +    struct colormapping {
> +        const char *c;
> +        const int val;
> +    };
> +    /* 16 named html colors in BGR format */
> +    const struct colormapping web_colors[] = {
> +        {"red",     0x0000ff}, {"blue",   0xff0000}, {"lime",   0x00ff00},
> +        {"aqua",    0xffff00}, {"purple", 0x800080}, {"yellow", 0x00ffff},
> +        {"fuchsia", 0xff00ff}, {"white",  0xffffff}, {"gray",   0x808080},
> +        {"maroon",  0x000080}, {"olive",  0x008080}, {"black",  0x000000},
> +        {"silver",  0xc0c0c0}, {"teal",   0x808000}, {"green",  0x008000},
> +        {"navy",    0x800000}
> +    };

You can merge both declarations.

> +                                /* Check the standard web color names. */
> +                                for (i = 0; i < sizeof web_colors / sizeof (struct colormapping); i++) {

Then you can use sizeof(web_colors) / sizeof(*web_colors), and not depend
on the web_colors type.

> +                                /* Check if any color is found. */
> +                                if (i == sizeof web_colors / sizeof (struct colormapping))

Same here.

> +                        inc_line:
> +                        if (*line != '>')
> +                            line++;

As I said in the previous mail, in my mind mis-formating must appears to
the user, so this is pointless.

-- 
ubitux