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

[Reimar Döffinger]

On Wed, Jun 23, 2010 at 07:55:09AM +0200, ubitux wrote:


A few (preferably doxygen-compatible) comments/function documentation
would be a really good idea.

> +int bstrcmp(struct bstr str1, struct bstr str2)
> +{
> +    int ret = memcmp(str1.start, str2.start, FFMIN(str1.len, str2.len));
> +
> +    if (!ret) {
> +        if (str1.len == str2.len)
> +            return 0;
> +        else if (str1.len > str2.len)
> +            return 1;
> +        else
> +            return -1;

return str1.len - str2.len;
maybe?

> +int bstrcasecmp(struct bstr str1, struct bstr str2)
> +{
> +    int ret = strncasecmp(str1.start, str2.start, FFMIN(str1.len, str2.len));
> +
> +    if (!ret) {
> +        if (str1.len == str2.len)
> +            return 0;
> +        else if (str1.len > str2.len)
> +            return 1;
> +        else
> +            return -1;

same.

> +// Create bstr compound literal from null-terminated string
> +#define BSTR(s) (struct bstr){(s), (s) ? strlen(s) : 0}
> +// create a pair (not single value!) for "%.*s" printf syntax
> +#define BSTR_P(bstr) (int)((bstr).len), (bstr).start

I'm not convinced these are really a good thing for readability.

> +#include <stdbool.h>

Please avoid bool, not all compilers are 100% compliant there, and for
those that are bool often has a performance impact.
Plus, it is not used anywhere else in MPlayer IIRC.

> +#define NB_ELEMS(s)         (sizeof(s) / sizeof((s)[0]))

FF_ARRAY_ELEMS from libavutil/common.h

> +    ret = vsnprintf(dst->buf + dst->len, dst->bufsize - dst->len, fmt, va);
> +    if (ret < 0)
> +        return;
> +    dst->len += ret;
> +    if (dst->len > dst->bufsize)
> +        dst->len = dst->bufsize;
> +    va_end(va);

There's a return before the va_end, sure that is guaranteed to work?
I'd say slower but more reliable would be
dst->len += strlen(dst->buf + dst->len);

> +static int indexof(const char *s, int c)
> +{
> +    char *f = strchr(s, c);
> +    return f ? (f - s) : -1;
> +}

Wouldn't just using strcspn result in more obvious code?

> +struct font_tag {
> +    int size;
> +    uint32_t color;
> +    struct bstr face;
> +    bool has_size : 1;
> +    bool has_color : 1;
> +    bool has_face : 1;

I still shudder from the last time I ended up debugging gcc miscompiling
bitfields, please for my sake don't use them unless it's really important.
You can probably save more memory (on 64 bit systems) by putting bstr first.

> +static const struct tag_conv {
> +    char *from;
> +    char *to;

const char *

> +} subrip_basic_tags[] = {
> +    {"<i>", "{\\i1}"}, {"</i>", "{\\i0}"},
> +    {"<b>", "{\\b1}"}, {"</b>", "{\\b0}"},
> +    {"<u>", "{\\u1}"}, {"</u>", "{\\u0}"},
> +    {"<s>", "{\\s1}"}, {"</s>", "{\\s0}"},
> +    {"{", "\\{"}, {"}", "\\}"},
> +    {"\n", "\\N"}

Also considering these e.g.
char from[8];
char to[8];
would be nicer in some ways.

> +static const struct {
> +    char *s;

basically same comment
Also 0-terminating the arrays might actually result in nicer-looking loops
than using the NB_ELEMS or whatever.

> +// Color, Font, Size, cHarset, stYle, Position, cOordinate
> +#define MICRODVD_TAGS               "cfshyYpo"

Maybe use an enum?

> +    for (i = sizeof(MICRODVD_TAGS) - 2; i; i--) {

This kind of thing isn't really great for understanding the code.

> +    int a1, a2, a3, a4, b1, b2, b3, b4, j = 0;

A bit better names please :-)

> +            int blank = 1, len = 0;
> +            char *p;
> +
> +            if (!stream_read_line(st, line, LINE_LEN, utf16))
> +                break;
> +
> +            for (p = line; *p != '\n' && *p != '\r' && *p; p++, len++)
> +                if (*p != ' ' && *p != '\t')
> +                    blank = 0;

len = p - line;
possibly even
len = strcspn(line, "\n\r");
p[len] = 0;
if (strncspn(line, " \t") == len) // whitespace-only
    break;


> +            if (!(j + 1 + len < sizeof(full_line) - 1))
> +                break;

Why not just >= instead of ! <
Also on 64 bit systems I think this could overflow, it should instead be
len >= sizeof(full_line) - j - 2

> +            char converted_line[LINE_LEN + 1];

Maybe it makes sense to malloc a few of these? I think there's a lot
of these on the stack in the end, and even one is not exactly small...

> @@ -2310,42 +2375,48 @@
>    buf = malloc(MAX_SUBLINE + 1);
>    sub->text[sub->lines] = buf;
>    sub->endpts[sub->lines] = endpts;
> -  for (i = 0; i < len && pos < MAX_SUBLINE; i++) {
> -    char c = txt[i];
> -    if (c == '<') comment |= 1;
> -    if (c == '{') comment |= 2;
> -    if (comment) {
> -      if (c == '}') comment &= ~2;
> -      if (c == '>') comment &= ~1;
> -      continue;
> -    }
> -    if (pos == MAX_SUBLINE - 1) {
> -      i--;
> -      c = 0;
> -    }
> -    if (c == '\\' && i + 1 < len) {
> -      c = txt[++i];
> -      if (c == 'n' || c == 'N') c = 0;
> -    }
> -    if (c == '\n' || c == '\r') c = 0;
> -    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");
> -        break;
> +
> +  if (!strip_markup) {
> +    subassconvert_subrip(txt, buf, MAX_SUBLINE + 1);
> +    sub->text[sub->lines] = buf;
> +  } else {
> +    for (i = 0; i < len && pos < MAX_SUBLINE; i++) {
> +      char c = txt[i];
> +      if (c == '<') comment |= 1;
> +      if (c == '{') comment |= 2;
> +      if (comment) {
> +        if (c == '}') comment &= ~2;
> +        if (c == '>') comment &= ~1;
> +        continue;
>        }
> -      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;
> +      if (pos == MAX_SUBLINE - 1) {
> +        i--;
> +        c = 0;
> +      }
> +      if (c == '\\' && i + 1 < len) {
> +        c = txt[++i];
> +        if (c == 'n' || c == 'N') c = 0;
> +      }
> +      if (c == '\n' || c == '\r') c = 0;
> +      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");
> +          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;
>    }
> -  buf[pos] = 0;

Please do not reindent existing code, that makes it nearly impossible
to see what you changed and thus fixing regressions needlessly grows
from a few-minutes job to a one-hour job.