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

[Diego Biurrun]

On Sat, Jun 19, 2010 at 12:12:04PM +0200, ubitux wrote:
> On Sat, Jun 19, 2010 at 12:07:38PM +0200, ubitux wrote:
> > 
> > The patch is about to be done; the only thing that can be missing is an
> > option to enable/disable the style. At the moment, the -ass switch makes
> > it available or not (old parsing, stripping tags).
> > 
> > A lot of the changes are not mine since it's a port of the tmp_sub branch
> > from Uoti's mplayer-git.
> > 
> > The code has been tested a few times, most of SubRip & MicroDVD tags are
> > supported, stylising is also available if muxed subtitle (it tries to parse
> > SubRip), and it does not require modifications to the libass.
> 
> Oups I forgot bstr.{c,h} in the patch, fixed in this one.
> 
> --- bstr.c	(revision 0)
> +++ bstr.c	(revision 0)
> @@ -0,0 +1,51 @@
> +
> +#include <string.h>
> +#include <libavutil/avutil.h>
> +#include "bstr.h"

nit: I prefer blank lines between system and local #includes.
It's not important...

> --- bstr.h	(revision 0)
> +++ bstr.h	(revision 0)
> @@ -0,0 +1,39 @@
> +
> +#ifndef MPLAYER_BSTR_H
> +#define MPLAYER_BSTR_H
> +
> +#include <stdint.h>
> +#include <stddef.h>
> +#include <string.h>

This header does not appear to require stddef.h, but it seems to require
sys/types.h.

> --- subassconvert.c	(revision 0)
> +++ subassconvert.c	(revision 0)
> @@ -0,0 +1,496 @@
> +
> +#include <string.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <stdarg.h>
> +#include <stdbool.h>
> +
> +#include "subassconvert.h"
> +#include "bstr.h"
> +
> +void subassconvert_subrip(const char *orig, char *dest, int dest_buffer_size)
> +{
> +    /* line is not const to avoid warnings with strtol, etc.
> +     * orig content won't be changed */
> +    char *line = (char *)orig;

Does this assignment generate a warning without the cast?

> +    struct line new_line = {
> +        .buf = dest,
> +        .bufsize = dest_buffer_size,

The = could be aligned.

> +                struct font_tag *tag = &font_stack[sp];
> +                struct font_tag *last_tag = &tag[-1];

ditto

> +                    tag->has_size = true;
> +                    has_valid_attr = true;

ditto

> +                        tag->color = ((tag->color & 0xff) << 16)
> +                            | (tag->color & 0xff00)
> +                            | ((tag->color & 0xff0000) >> 16);

This could be aligned as well

> +                    tag->face.start = line;
> +                    tag->face.len = len;

> +                    tag->has_face = true;
> +                    has_valid_attr = true;

as could these 

> +            tag.data_string.start = s;
> +            tag.data_string.len = len;

and this

> +            tag.data_string.start = s;
> +            tag.data_string.len = len;


> +        .buf = dest,
> +        .bufsize = dest_buffer_size,

..

If this works I think it should be applied soonish.  Reimar?

Also, your patch makes me think that we should move the subtitle
infrastructure into its own subdirectory.

Diego