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

[Michael Wu]

--- On Mon, 10/26/09, Uoti Urpala <uoti.urpala at pp1.inet.fi> wrote:
> 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.
> 
Agreed.

> 
> +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.
> 
Just an oversight. Will fix.

> 
> +#define arib_stub(...) mp_msg(MSGT_ARIB, MSGL_WARN,
> __VA_ARGS__)
> 
> +           
>    arib_stub("cc: APB\n");
> 
> Messages like these will confuse most users...
> 
> 
I'll just convert everything to dbg then. The spec is quite big and I'm not sure which parts are actually used in real streams, so these messages are set up in the hopes that users will report them if they find streams that uses those features. However, I think I've implemented enough of the spec that most users won't end up noticing or caring about these messages. I should be playing enough arib ts's to find the remaining issues anyway..

> +#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.
> 
I couldn't use the current ass_track allocation code in demuxer.c because it assumes detection at open, whereas demux_ts.c is much more likely to detect a subtitle stream at a later point. I don't feel too strongly about this since having any specific subtitle decoder handling in libmpdemux is pretty bad, but I'll try to move the libass stuff to demuxer.c.

> 
> 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.
> 
Will take a look.

Thanks for the review!

-Michael Wu