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

[Michael Wu]

>> --- 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?
>
Yes. All uses of ass.h I've seen so far are inside #ifdef CONFIG_ASS.

The latest patch takes a different approach and doesn't need this include.

>> --- 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
>
Ok.

>> --- 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.
>
Ok

>> +if test "$_arib" = auto ; then
>> +    _arib=yes
>> +fi
>
> This can be simplified with &&.
>
Ok

>> +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.
>
I just used the standard code used in other feature tests.

>> --- mpcommon.c	(revision 29794)
>> +++ mpcommon.c	(working copy)
>> @@ -24,6 +24,10 @@
>>
>> +#ifdef CONFIG_ARIB
>> +#include "sub_arib.h"
>> +#endif
>
> Remove this nonsense #ifdef.
>
Latest patch no longer has this #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.
>
Which is? I just checked three files and none of them had a license.

>> +#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.
All done in the latest patch.

> - Declare parameterless functions as "void foo(void)".
>
Ok.

>> +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?
>
Read the spec (ARB STD-B24, Table 7-18 on page 101 in the english
edition). That's just how it's defined. They're just a set of useful
default macros captions can call to configure the graphic set assignments.

>> +static void arib_process_statement(const unsigned char *data, size_t
>> len);
>
> Is this forward declaration really necessary?
>
Yes.

>> --- 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?
>
Please suggest one if you think there is a subtitle related one that I
should use. I looked and I didn't see any that I thought were appropriate.

Thanks,
-Michael Wu