[MPlayer-dev-eng] [PATCH] Add DVB teletext support

[Reimar Döffinger]

On Sun, Nov 08, 2009 at 08:23:58PM +0100, Francesco Lavra wrote:
> On Sat, 2009-11-07 at 17:07 +0100, Reimar Döffinger wrote:
> > The C99 type is called uint8_t, not u_int8_t.
> > Also just just ff_reverse from libavcodec or of that's too hackish
> > ask FFmpeg to move it to libavutil and export it.
> 
> Waiting for the relevant FFmpeg patch to reviewed/applied...
> In the meantime, hope you don't mind if I post a "temporary" patch which
> doesn't use ff_reverse, so you can play with it.

Why not just add a
extern const uint8_t ff_reverse[256];
and use it?
It means it won't be forgotten to change when it becomes public in
FFmpeg, and it also means we know for sure it works.

> +static void vbi_decode_dvb(priv_vbi_t *priv, const unsigned char *buf){
> +    unsigned char data[42];

Since it's no character data I'm in favour of using uint8_t as type btw.

> +    /* Reverse bit order, skipping the first two bytes (field parity, line
> +       offset and framing code). */
> +    for (i = 0; i < sizeof(data); i++)
> +        data[i] = byterev8[buf[2 + i]];
> +
> +    vbi_decode_line(priv, data);
> +    if (priv->cache_reset){
> +        pthread_mutex_lock(&priv->buffer_mutex);
> +        priv->cache_reset--;
> +        pthread_mutex_unlock(&priv->buffer_mutex);
> +    }

This only needs to be called at the very end after all lines are decoded
it seems.
Maybe it would make sense to do the sublen parsing in this function,
too, so this can be moved outside that loop?
Not that I think it really matters.
But this reminds me of an issue... since teletext is no longer optional,
I fear MPlayer no longer compiles if pthread isn't available?
This is even worse since that pthread stuff is only needed when actually
reading the teletext from VBI (and even then I am not 100% sure how
necessary it is).

> +        if (type == 'd' && !d_dvdsub->demuxer->teletext) {
> +            tt_stream_props tsp;
> +            void *ptr;
> +
> +            memset(&tsp, 0, sizeof(tsp));
> +            ptr = &tsp;

You can initialize ptr already at its declaration.

> +            if (teletext_control(NULL, TV_VBI_CONTROL_START, &ptr) ==
> +                    VBI_CONTROL_TRUE)
> +                d_dvdsub->demuxer->teletext = ptr;
> +            else
> +                d_dvdsub->demuxer->teletext = NULL;

The else part is completely useless, the if above guarantees it is
already NULL, it won't get any NULLer by assigning NULL once more :-P

> +            if (type == 'd') {
> +                if (d_dvdsub->demuxer->teletext) {
> +                    uint8_t *p = packet;

could be "const uint8_t *"