[MPlayer-dev-eng] [PATCH] Tags support for SubRip and MicroDVD subtitles
ubitux
ubitux at gmail.com
Mon May 17 16:14:50 CEST 2010
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
More information about the MPlayer-dev-eng
mailing list