[Ffmpeg-devel] [PATCH] support S302M streams in MPEG TS

[Reimar Döffinger]

Hello,

Sorry, don't have time to properly try to understand the code, so I'll
mostly ask questions.

On Fri, Dec 01, 2006 at 11:58:21AM +0100, Baptiste Coudurier wrote:

> +static void s302m_parse_vucf_bits(S302MContext *s, const uint8_t *buf)
> +{
> +    uint8_t vucf[2];
> +    if (s->avctx->bits_per_sample == 16) {
> +        s->avctx->codec_id = CODEC_ID_PCM_S16BE;
> +        vucf[0] = (buf[2] & 0xf0) | (buf[4] & 0x0f);
> +        vucf[1] = (buf[7] & 0xf0) | (buf[9] & 0x0f);
> +    } else if (s->avctx->bits_per_sample == 20) {
> +        /* store 20 bit samples on 24 bits */
> +        s->avctx->bits_per_sample = 24;

IMO it would be cleaner to separate the real bits per sample and those
exported instead of overwriting it. E.g. we might for some reason want
to call s302m_parse_vucf_bits more than once some time in the future..

> +    if ((vucf[0] >> 6) & 0x01)

You mean (vucf[0] & 0x40) ?

> +    if (((vucf[1] >> 4) & 0x0f) > 6)

unless you can give that 6 a special meaning,
(vucf[1] > 0x60)
would be simpler and just as good.

> +static void s302m_subframe_16bit(S302MContext *s, const uint8_t *buf)
> +{
> +    /* reverse sample to suit big endian */
> +    s->outbuf_ptr[0] = ff_reverse[buf[0]];
> +    s->outbuf_ptr[1] = ff_reverse[buf[1]];
> +    s->outbuf_ptr[3] = ff_reverse[((buf[3] & 0x0f) << 4) | (buf[4] >> 4)];
> +    s->outbuf_ptr[4] = ff_reverse[((buf[2] & 0x0f) << 4) | (buf[3] >> 4)];
> +    s->outbuf_ptr += 4;

typo? [2] is not initialized and [4] gets overwritten later...
And wouldn't it be nicer if these transformations were done in-place,
with only one buffer?

> +    s->frame_size = (buf[0] << 8) | buf[1];
> +    if (s->frame_size > 57360)

could you add a comment where 57360? And writing it as hex 0xe010 at
least wouldn't hurt.

> +    s->avctx->channels = 2 + ((buf[2] >> 6) & 3) * 2;

(buf[2] >> 5) & 6? Makes it easier to see the max. Not sure which one is better.

> +    s->avctx->bits_per_sample = 16 + ((buf[3] >> 4) & 3) * 4;

similarly here (buf[3] >> 2) & 0x0c

> +    s->byte_align = ((s->avctx->bits_per_sample + 4) * 2) >> 3;

maybe it is clearer to just put this into the bits_per_sample ifs?

> +            if (!s->avctx->sample_rate)
> +                s302m_parse_vucf_bits(s, s->inbuf);

Isn't this misusing sample_rate a bit?

> +S302MContext *s302m_init_demuxer(AVStream *st)
> +{
> +    S302MContext *s = av_mallocz(sizeof(*s));
> +    s->avctx = st->codec;
> +    s->avctx->codec_type = CODEC_TYPE_AUDIO;

Might set some other defaults, too, at least I think there is something
s302m_parse_vucf_bits sets that does not depend on data? Maybe I just
misremember.

Greetings,
Reimar D?ffinger