[Ffmpeg-devel] [RFC] SMPTE 302M AES 3 stream in mpeg ts

[Michael Niedermayer]

Hi

On Sat, Sep 16, 2006 at 02:27:02PM +0200, Baptiste Coudurier wrote:
> Hi
> 
> This patch add support to play S302M streams in mpeg ts.
> Actually daud demuxer was S302M stream demuxer. I implemented bitstream
> parsing using a "shadow" parser, since S302M streams can carry lpcm
> audio, ac3, mp3, dolby e...
> 
> It's quite ugly and of course parser can be simplified, its just a proof
> of concept about which I would like to have opinions.

iam glad iam not mpegts maintainer and no i dont like how this is implemented
at all ...

still some random comments are below


[some stuff about a possible bug with dissapearing first packets]

[...]
> -static int daud_header(AVFormatContext *s, AVFormatParameters *ap) {
> +typedef struct S302MParseContext {
> +    uint8_t inbuf[7]; /* worst case 24 bit */
> +    uint8_t *inbuf_ptr;
> +    int byte_align;
> +    int frame_size;
> +    uint8_t *outbuf;
> +    uint8_t *outbuf_ptr;
> +    int (*parse_subframe)(struct S302MParseContext *s, AVCodecContext *avctx,
> +                          const uint8_t *buf, int buf_size);
> +} S302MParseContext;
> +
> +#define S302M_HEADER_SIZE 4
> +
> +static const int s302m_channels[4] = {
> +    2, 4, 6, 8
> +};

this doesnt need a table, a simple 2+2*a will do


> +
> +static const int s302m_bits_per_sample[3] = {
> +    16, 20, 24
> +};

it seems [3] of this table can be accessed which could lead to a segfault


> +
> +static const int aes3_sample_rates[4] = {
> +    0, 48000, 44100, 32000
> +};

a `grep 44100 libav*/*{c,h}` will show that this table is redundant

[...]

> +        if (!avctx->sample_rate) { /* get vucf bits to find codec params */
> +            *vucf_ptr++ = (buf_ptr[3] & 0xf0) | (buf_ptr[6] & 0x0f);
> +            if (vucf_ptr - vucf == 2)
> +                s302m_parse_vucf_bits(avctx, vucf);
> +        }

this code is duplicated a few times and should be in its own function


[...]

> +int s302m_parser_init(AVStream *st)
> +{
> +    st->parser = av_mallocz(sizeof(AVCodecParserContext));
> +    if (!st->parser)
> +        return -1;
> +    st->parser->parser = &s302m_parser;
> +    st->parser->priv_data = av_mallocz(sizeof(S302MParseContext));
> +    st->parser->fetch_timestamp = 1;
> +    st->need_parsing = 1;
> +    return 0;
> +}

this is a ugly hack parsers should be initalized with av_parser_init()


[...]
> -#ifdef CONFIG_DAUD_DEMUXER
> -    av_register_input_format(&daud_demuxer);
> +#ifdef CONFIG_S302M_DEMUXER
> +    av_register_input_format(&s302m_demuxer);

all the DAUD/S302M renaming should be a seperate change


[...]

> +static inline int get32(const uint8_t **pp, const uint8_t *p_end)
> +{
> +    const uint8_t *p;
> +    int c;
> +
> +    p = *pp;
> +    if ((p + 3) >= p_end)
> +        return -1;
> +    c = (p[0] << 24) | (p[1] << 16) | (p[2] << 8) | p[3];
> +    p += 4;
> +    *pp = p;
> +    return c;
> +}

IMHO this is redundant, either merge the get*() like:
static inline int get32(const uint8_t **pp, const uint8_t *p_end){
    return get_internal(pp,p_end, 32);
}

or use some of the many already redundant reading functions like BE_32()


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is