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

[Uoti Urpala]

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

+static struct arib_conv_state state;

+// rendering
+static int current_eid_start = -1;
+static ass_track_t *dtrack = NULL;

Eventually the subtitle decoders should be changed to work more like
audio and video ones already do, and use a context pointer in the sh_sub
struct for all internal data. So adding and using such static variables
should be avoided. For now I think it'd be OK to have a single static
context struct in the file, which would only be directly used in
functions called from outside (and its address passed as a parameter
from those to static functions). Later the public functions could then
be changed to take the address from sh_sub->context instead of the
static variable.


+void arib_clear_current_captions ()

Is there a reason for these to not be static? Missing "void" (now it's
an old-style unchecked declaration) and inconsistent space before '('
here too.


+#define arib_stub(...) mp_msg(MSGT_ARIB, MSGL_WARN, __VA_ARGS__)

+               arib_stub("cc: APB\n");

Messages like these will confuse most users...


+#ifdef CONFIG_ASS
+                                               sub->ass_track = ass_default_track(ass_library);
+#endif

This doesn't belong in an individual demuxer, especially when using
libass at all is in implementation detail of the decoder. Current
MPlayer code is somewhat ugly in this area too, but allocating the track
in demuxer.c where the other ass_track variables are allocated would at
least be better than this.


The embedded libass copy in the svn repo is obsolete, and at least some
type names have changed in current libass.
git://repo.or.cz/mplayer-build.git has a version with current libass.
Changing those later should not be a significant problem though as it
should only require some simple renamings.


K&R indentation style with 4-space indents would be preferred.