[MPlayer-dev-eng] [PATCH] Add support for ARIB STD-B24 captions v2

[Diego Biurrun]

On Sun, Oct 25, 2009 at 01:28:26PM -0700, Michael Wu wrote:
> 
> These patches add support for ARIB STD-B24 captions.

Please get rid of all tabs and trailing whitespace, use K&R style with 4
space indentation.

> --- libmpdemux/demux_ts.c	(revision 29794)
> +++ libmpdemux/demux_ts.c	(working copy)
> @@ -38,6 +38,11 @@
>  
> +#ifdef CONFIG_ASS
> +#include "libass/ass.h"
> +#include "libass/ass_mp.h"
> +#endif

Is this #ifdef necessar?

> --- Makefile	(revision 29794)
> +++ Makefile	(working copy)
> @@ -28,6 +28,7 @@
>  SRCS_AUDIO_INPUT-$(OSS)              += stream/ai_oss.c
>  SRCS_COMMON-$(AUDIO_INPUT)           += $(SRCS_AUDIO_INPUT-yes)
> +SRCS_COMMON-$(ARIB)                  += sub_arib.c

alphabetical order

> --- configure	(revision 29794)
> +++ configure	(working copy)
> @@ -6150,6 +6154,30 @@
>  
> +echocheck "ARIB Caption support"
> +# depends on libass to render subs
> +if test "$_ass" = no ; then
> +    _arib=no
> +    _res_comment="libass support needed"
> +fi
> +
> +# depends on iconv to convert captions to UTF-8
> +if test "$_iconv" = no ; then
> +    _arib=no
> +    _res_comment="iconv support needed"
> +fi

This can be merged with an elif.

> +if test "$_arib" = auto ; then
> +    _arib=yes
> +fi

This can be simplified with &&.

> +if test "$_arib" = yes ; then
> +    def_arib='#define CONFIG_ARIB'
> +else
> +    def_arib='#undef CONFIG_ARIB'
> +fi

This can be simplified if you first unset CONFIG_ARIB.

Also, set CONFIG_ARIB to 1.

> --- mpcommon.c	(revision 29794)
> +++ mpcommon.c	(working copy)
> @@ -24,6 +24,10 @@
>  
> +#ifdef CONFIG_ARIB
> +#include "sub_arib.h"
> +#endif

Remove this nonsense #ifdef.

> --- sub_arib.c	(revision 0)
> +++ sub_arib.c	(revision 0)
> @@ -0,0 +1,796 @@
> +/* sub_arib.c
> + * Caption decoder for ARIB STD-B24
> + *
> + * Copyright 2009, Michael Wu <altape at ymail.com>
> + *
> + * sub_arib is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.

Use our standard license boilerplate.

> +#define arib_stub(...) mp_msg(MSGT_ARIB, MSGL_WARN, __VA_ARGS__)
> +#define arib_dbg(...) mp_msg(MSGT_ARIB, MSGL_DBG2, __VA_ARGS__)
> +
> +void arib_extend_current_captions ()
> +
> +void arib_clear_current_captions ()
> +
> +void arib_add_caption ()

Here and below:

- Drop the space between the opening parenthesis and the
  function name.
- These functions can likely be static.
- Declare parameterless functions as "void foo(void)".

> +	event->Text = strdup(state.inbuf);
> +	event->MarginL = state.x;
> +	event->MarginV = 540 - state.y;
> +	event->Layer = state.layer++;

The '=' could be aligned.

> +static const unsigned char *arib_default_macros[] = {
> +	"\x1B\x24\x42\x1B\x29\x4A\x1B\x2A\x30\x1B\x2B\x20\x70\x0F\x1B\x7D",
> +	"\x1B\x24\x42\x1B\x29\x31\x1B\x2A\x30\x1B\x2B\x20\x70\x0F\x1B\x7D",
> +	"\x1B\x24\x42\x1B\x29\x20\x41\x1B\x2A\x30\x1B\x2B\x20\x70\x0F\x1B\x7D",
> +	"\x1B\x28\x32\x1B\x29\x34\x1B\x2A\x35\x1B\x2B\x20\x70\x0F\x1B\x7D",
> +	"\x1B\x28\x32\x1B\x29\x33\x1B\x2A\x35\x1B\x2B\x20\x70\x0F\x1B\x7D",
> +	"\x1B\x28\x32\x1B\x29\x20\x41\x1B\x2A\x35\x1B\x2B\x20\x70\x0F\x1B\x7D",
> +	"\x1B\x28\x20\x41\x1B\x29\x20\x42\x1B\x2A\x20\x43\x1B\x2B\x20\x70\x0F\x1B\x7D",
> +	"\x1B\x28\x20\x44\x1B\x29\x20\x45\x1B\x2A\x20\x46\x1B\x2B\x20\x70\x0F\x1B\x7D",
> +	"\x1B\x28\x20\x47\x1B\x29\x20\x48\x1B\x2A\x20\x49\x1B\x2B\x20\x70\x0F\x1B\x7D",
> +	"\x1B\x28\x20\x4A\x1B\x29\x20\x4B\x1B\x2A\x20\x4C\x1B\x2B\x20\x70\x0F\x1B\x7D",
> +	"\x1B\x28\x20\x4D\x1B\x29\x20\x4E\x1B\x2A\x20\x4F\x1B\x2B\x20\x70\x0F\x1B\x7D",
> +	"\x1B\x24\x42\x1B\x29\x20\x42\x1B\x2A\x30\x1B\x2B\x20\x70\x0F\x1B\x7D",
> +	"\x1B\x24\x42\x1B\x29\x20\x43\x1B\x2A\x30\x1B\x2B\x20\x70\x0F\x1B\x7D",
> +	"\x1B\x24\x42\x1B\x29\x20\x44\x1B\x2A\x30\x1B\x2B\x20\x70\x0F\x1B\x7D",
> +	"\x1B\x28\x31\x1B\x29\x30\x1B\x2A\x4A\x1B\x2B\x20\x70\x0F\x1B\x7D",
> +	"\x1B\x28\x4A\x1B\x29\x32\x1B\x2A\x20\x41\x1B\x2B\x20\x70\x0F\x1B\x7D",
> +};

Umm, what?

> +static void arib_process_statement(const unsigned char *data, size_t len);

Is this forward declaration really necessary?

> +void arib_init(ass_track_t *track)
> +{
> +	state.G[0] = GS_KANJI;
> +	state.G[1] = GS_ASCII;
> +	state.G[2] = GS_HIRAGANA;
> +	state.G[3] = GS_MACRO | GS_DRCS;
> +	state.G_WIDTH[0] = 2;
> +	state.G_WIDTH[1] = 1;
> +	state.G_WIDTH[2] = 1;
> +	state.G_WIDTH[3] = 1;
> +	state.GL = 0;
> +	state.GR = 2;
> +	state.fg_color = 7;
> +	state.font_height = 48;

I see some more alignment opportunities.

> +	state.pts = subpts;
> +	state.inptr = NULL;
> +	state.layer = 0;
> +	state.delay = 0;

ditto

> --- mp_msg.h	(revision 29794)
> +++ mp_msg.h	(working copy)
> @@ -104,6 +104,8 @@
>  
>  #define MSGT_STATUSLINE 45 // playback/encoding status line
>  
> +#define MSGT_ARIB 46 // sub_arib.c

Do we really need a new message type?  Is there no subtitle-related one
that you can use?

> --- sub_arib.h	(revision 0)
> +++ sub_arib.h	(revision 0)
> @@ -0,0 +1,8 @@
> +#ifndef MPLAYER_SUB_ARIB_H
> +#define MPLAYER_SUB_ARIB_H

missing license boilerplate

Diego