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

[Reynaldo H. Verdejo Pinochet]

Hi

On Fri, Jun 25, 2010 at 08:36:19PM +0200, ubitux wrote:
> [...]
> +    if (ret < 0)
> +        goto end;
> +    dst->len += ret;
> +    if (dst->len > dst->bufsize)
> +        dst->len = dst->bufsize;
> +end:
> +    va_end(va);
> +}

Use if(ret > 0) and get rid of the goto.

> +
> +static int indexof(const char *s, int c)
> +{
> +    char *f = strchr(s, c);
> +    return f ? (f - s) : -1;
> +}
> +
> +
> +
> +/*
> + *      SubRip
> + *
> + *      Support basic tags (italic, bold, underline, strike-through)
> + *      and font tag with size, color and face attributes.
> + *
> + */
> +
> +struct font_tag {
> +    struct bstr face;
> +    int size;
> +    uint32_t color;
> +    int has_size : 1;

has_size seems redundant, has_color too if you make color a pointer. Guess
the same can be said about has_face. These changes alone would trim quite
some lines out of this changeset.

> [..]
> +    {"{", "\\{"}, {"}", "\\}"},
> +    {"\n", "\\N"}
> +};
> +
> +static const struct {
> +    const char *s;
> +    uint32_t v;
> +} subrip_web_colors[] = {
> +    /* 16 named html colors in BGR format */

HTML

> +    {"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}
> +};
> +
> +#define SUBRIP_MAX_STACKED_FONT_TAGS    16

The only time I see it used you are adding one to it. Why not making it 17
to begin with?

> +
> +/**
> + * \brief Convert SubRip lines into ASS markup
> + * \param orig original SubRip lines. The content will remain untouched.
> + * \param dest ASS markup destination buffer.
> + * \param dest_buffer_size maximum size for the destination buffer.
> + */
> +void subassconvert_subrip(const char *orig, char *dest, int dest_buffer_size)
> +{

dest_buffer_size might be better of as a size_t

> [..]
> +                        if (i == FF_ARRAY_ELEMS(subrip_web_colors)) {
> +                            /* We didn't find any matching color */
> +                            line = strchr(line, '"'); // can't be NULL, see above
> +                            mp_msg(MSGT_SUBREADER, MSGL_WARN,
> +                                   "[SubRip] Unknown font color in subtitle: %s\n", orig);

Should be translatable.

> [..]
> +                break;
> +            s++;
> +            tag.data2 = strtol(s, &s, 10);
> +            if (*s != '}')
> +                break;
> +            tag.key = 'o';
> +            break;
> +
> +        default:    /* Unknown tag, we considere it's text */

consider?

> [..]
> +}
> +
> +static void microdvd_close_no_persistent_tags(struct line *new_line,
> +                                              struct microdvd_tag *tags)
> +{
> +    int i;
> +
> +    for (i = sizeof(MICRODVD_TAGS) - 2; i; i--) {
> +        if (tags[i].persistent != MICRODVD_PERSISTENT_OFF)

Guess the negative subscript chance on tags is intended?


> [..]
> +      if (c) {
> +        double_newline = 0;
> +        buf[pos++] = c;
> +      } else if (!double_newline) {
> +        if (sub->lines >= SUB_MAX_TEXT - 1) {
> +          mp_msg(MSGT_VO, MSGL_WARN, "Too many subtitle lines\n");

Should be translatable.

> +          break;
> +        }
> +        double_newline = 1;
> +        buf[pos] = 0;
> +        sub->lines++;
> +        pos = 0;
> +        buf = malloc(MAX_SUBLINE + 1);
> +        sub->text[sub->lines] = buf;
> +        sub->endpts[sub->lines] = endpts;
> +      }
>      }
> +    buf[pos] = 0;

What if malloc fails?


Best regards

--
Reynaldo