[FFmpeg-devel] [PATCH] TAK demuxer and decoder

Michael Niedermayer michaelni at gmx.at
Sun Sep 30 03:59:02 CEST 2012


On Sat, Sep 29, 2012 at 04:27:20PM +0000, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol <onemda at gmail.com>
[...]
> +static int tak_get_nb_samples(AVCodecContext *avctx, int sample_rate,
> +                              enum TAKFrameSizeType type)
> +{
> +    int nb_samples, max_nb_samples;
> +
> +    if (type <= TAK_FST_250ms) {
> +        nb_samples     = sample_rate * frame_duration_type_quants[type] >>
> +                                       TAK_FRAME_DURATION_QUANT_SHIFT;
> +        max_nb_samples = 16384;
> +    } else if (type < FF_ARRAY_ELEMS(frame_duration_type_quants)) {
> +        nb_samples     = frame_duration_type_quants[type];
> +        max_nb_samples = sample_rate * frame_duration_type_quants[TAK_FST_250ms] >>
> +                                       TAK_FRAME_DURATION_QUANT_SHIFT;

the multiplies should use int64_t to ensure they dont overflow



[...]
> +int avpriv_tak_parse_streaminfo(void *log, GetBitContext *gb, TAKStreamInfo *s)
> +{
> +    uint64_t channel_mask = 0;
> +    int frame_type, i;
> +
> +    s->codec = get_bits(gb, TAK_ENCODER_CODEC_BITS);
> +    if (s->codec != 2 && s->codec != 4) {
> +        av_log(log, AV_LOG_ERROR, "unsupported codec: %d\n", s->codec);
> +        return AVERROR_PATCHWELCOME;
> +    }
> +
> +    skip_bits(gb, TAK_ENCODER_PROFILE_BITS);
> +
> +    frame_type = get_bits(gb, TAK_SIZE_FRAME_DURATION_BITS);

> +    s->samples = get_bits_long(gb, TAK_SIZE_SAMPLES_NUM_BITS);

get_bits_longlong() (see my patch) or something similar is needed


[...]
> +
> +static int get_scale(int sample_rate, int shift)
> +{
> +    return (((sample_rate + 511 >> 9) + 3) & 0xFFFFFFFC) << shift;

FFALIGN(sample_rate + 511 >> 9, 4) << shift;


[...]
> +static int get_code(GetBitContext *gb, int nbits)
> +{
> +    int ret;
> +
> +    if (nbits == 1) {
> +        skip_bits1(gb);
> +        ret = 0;
> +    } else {
> +        ret = get_sbits(gb, nbits);
> +    }
> +
> +    return ret;

you could drop the ret variable and use 2 returns directly


> +}
> +
> +static void decode_lpc(int32_t *ptr, int lpc, int length)
> +{
> +    int i, a1, a2, a3, a4, a5;
> +
> +    if (length < 2)
> +        return;
> +
> +    if (lpc == 1) {
> +        a1 = *ptr++;
> +        for (i = 0; i < (length - 1 >> 1); i++) {
> +            *ptr   += a1;
> +            ptr[1] += *ptr;
> +            a1      = ptr[1];
> +            ptr    += 2;
> +        }
> +        if ((length - 1) & 1)
> +            *ptr += a1;
> +    } else if (lpc == 2) {
> +        a1     = ptr[1];
> +        a2     = a1 + *ptr;
> +        ptr[1] = a2;
> +        if (length > 2) {
> +            ptr += 2;
> +            for (i = 0; i < (length - 2 >> 1); i++) {
> +                a3     = *ptr + a1;
> +                a4     = a3 + a2;
> +                *ptr   = a4;
> +                a1     = ptr[1] + a3;
> +                a2     = a1 + a4;
> +                ptr[1] = a2;
> +                ptr   += 2;
> +            }
> +            if (length & 1)
> +                *ptr  += a1 + a2;
> +        }
> +    } else if (lpc == 3) {
> +        a1     = ptr[1];
> +        a2     = a1 + *ptr;
> +        ptr[1] = a2;
> +        if (length > 2) {
> +            a3   = ptr[2];
> +            a4   = a3 + a1;
> +            a5   = a4 + a2;
> +            ptr += 3;
> +            for (i = 0; i < length - 3; i++) {
> +                a3  += *ptr;
> +                a4  += a3;
> +                a5  += a4;
> +                *ptr = a5;
> +                ptr++;
> +            }
> +        }
> +    }
> +}

this looks like code that applies p[i] = (v+=p[i]) 1-3 times
if thats true, it should be documented
(the code could also be simplified but would then be slower)


[...]
> +static int decode_subframe(TAKDecContext *s, int32_t *ptr, int subframe_size, int prev_subframe_size)
> +{
> +    GetBitContext  *gb = &s->gb;
> +    int tmp, x, y, i, j, ret = 0;
> +
> +    if (get_bits1(gb)) {
> +        s->nb_predictors = predictor_sizes[get_bits(gb, 4)];
> +
> +        if (prev_subframe_size > 0 && get_bits1(gb)) {
> +            if (s->nb_predictors > prev_subframe_size)
> +                return AVERROR_INVALIDDATA;
> +
> +            ptr           -= s->nb_predictors;
> +            subframe_size += s->nb_predictors;
> +
> +            if (s->nb_predictors > subframe_size)
> +                return AVERROR_INVALIDDATA;
> +        } else {
> +            int lpc;
> +
> +            if (s->nb_predictors > subframe_size)
> +                return AVERROR_INVALIDDATA;
> +
> +            lpc = get_bits(gb, 2);
> +            if (lpc > 2)
> +                return AVERROR_INVALIDDATA;
> +
> +            if ((ret = decode_m(s, ptr, s->nb_predictors)) < 0)
> +                return ret;
> +
> +            decode_lpc(ptr, lpc, s->nb_predictors);
> +        }
> +
> +        s->xred = get_b(gb);
> +        s->size = get_bits1(gb) + 5;
> +
> +        if (get_bits1(gb)) {
> +            s->ared = get_bits(gb, 3) + 1;
> +            if (s->ared > 7)
> +                return AVERROR_INVALIDDATA;
> +        } else {
> +            s->ared = 0;
> +        }
> +        s->predictors[0] = get_code(gb, 10);
> +        s->predictors[1] = get_code(gb, 10);
> +        s->predictors[2] = get_code(gb, s->size + 1) << (9 - s->size);
> +        s->predictors[3] = get_code(gb, s->size + 1) << (9 - s->size);
> +        if (s->nb_predictors > 4) {
> +            tmp = s->size + 1 - get_bits1(gb);
> +
> +            for (i = 4; i < s->nb_predictors; i++) {
> +                if (!(i & 3))
> +                    x = tmp - get_bits(gb, 2);
> +                s->predictors[i] = get_code(gb, x) << (9 - s->size);
> +            }
> +        }
> +
> +        s->ncoeff[0] = s->predictors[0] << 6;
> +        for (i = 1; i < s->nb_predictors; i++) {
> +            int32_t *p1 = s->ncoeff;
> +            int32_t *p2 = &s->ncoeff[i - 1];
> +
> +            for (j = 0; j < (i + 1) / 2; j++) {
> +                x     = *p1 + (s->predictors[i] * *p2 + 256 >> 9);
> +                *p2  += s->predictors[i] * *p1 + 256 >> 9;
> +                *p1++ = x;
> +                p2--;
> +            }
> +
> +            s->ncoeff[i] = s->predictors[i] << 6;
> +        }
> +
> +        x = -1 << (32 - (s->ared + 5));
> +        y =  1 << ((s->ared + 5) - 1);
> +        for (i = 0, j = s->nb_predictors - 1; i < s->nb_predictors / 2; i++, j--) {
> +            tmp = y + s->ncoeff[j];
> +            s->ncoeff[j] = -(x & -(y + s->ncoeff[i] >> 31) |
> +                            (y + s->ncoeff[i]) >> (s->ared + 5));
> +            s->ncoeff[i] = -(x & -(tmp >> 31) | (tmp >> s->ared + 5));
> +        }
> +
> +        if ((ret = decode_m(s, &ptr[s->nb_predictors], subframe_size - s->nb_predictors)) < 0)
> +            return ret;
> +
> +        for (i = 0; i < s->nb_predictors; i++)
> +            s->f[i] = *ptr++ >> s->xred;
> +
> +        y    = FF_ARRAY_ELEMS(s->f) - s->nb_predictors;
> +        x    = subframe_size - s->nb_predictors;
> +        while (x > 0) {
    
> +            if (y >= x)
> +                tmp = x;
> +            else
> +                tmp = y;

FFMIN()


[...]
> +static int decorrelate(TAKDecContext *s, int c1, int c2, int length)
> +{
> +    GetBitContext  *gb = &s->gb;
> +    int32_t *p1 = &s->decode_buffer[c1 * s->nb_samples + 1];
> +    int32_t *p2 = &s->decode_buffer[c2 * s->nb_samples + 1];
> +    int a, b, i, j, x, tmp;
> +
> +    if (s->dmode > 3) {
> +        s->dshift = get_b(gb);
> +        if (s->dmode > 5) {
> +            if (get_bits1(gb))
> +                s->nb_dcoeffs = 16;
> +            else
> +                s->nb_dcoeffs = 8;
> +
> +            s->dval1 = get_bits1(gb);
> +            s->dval2 = get_bits1(gb);
> +
> +            for (i = 0; i < s->nb_dcoeffs; i++) {
> +                if (!(i & 3))
> +                    x = 14 - get_bits(gb, 3);
> +                s->dcoeffs[i] = get_code(gb, x);
> +            }
> +        } else {
> +            s->dfactor = get_code(gb, 10);
> +        }
> +    }
> +
> +    switch (s->dmode) {
> +    case 1:
> +        for (i = 0; i < length; i++, p1++, p2++)
> +            *p2 += *p1;
> +        break;
> +    case 2:
> +        for (i = 0; i < length; i++, p1++, p2++)
> +            *p1 = *p2 - *p1;
> +        break;
> +    case 3:

> +        for (i = 0; i < length; i++, p1++, p2++) {
> +            x   = (*p2 & 1) + 2 * *p1;
> +            *p1 = (  x - *p2) & 0x80000000 | (  x - *p2 >> 1);
> +            *p2 = (*p2 + x  ) & 0x80000000 | (*p2 + x   >> 1);

the & 0x80000000 | seems redundant if these are signed 32bit variables


> +        }
> +        break;
> +    case 4:
> +        FFSWAP(int32_t *, p1, p2);
> +    case 5:
> +        if (s->dshift)
> +            tmp = -1 << (32 - s->dshift);
> +        else
> +            tmp = 0;
> +
> +        for (i = 0; i < length; i++, p1++, p2++) {
> +            x   = *p2 >> s->dshift;;

double ;


> +            *p1 = ((-((s->dfactor * (tmp & -(*p2 >> 31) | x) + 128) >> 31) &
> +                    0xFF000000 |
> +                  ((   s->dfactor * (tmp & -(*p2 >> 31) | x) + 128) >> 8)) <<
> +                   s->dshift) - *p1;

theres a common subexpression that can be factored out
and the sign extension stuff looks redundant
and theres more redundant sign extension stuff in the patch


> +        }
> +        break;
> +    case 6:
> +        FFSWAP(int32_t *, p1, p2);
> +    case 7:
> +        if (length < 256)
> +            return AVERROR_INVALIDDATA;
> +
> +        a = s->nb_dcoeffs / 2;
> +        b = length - (s->nb_dcoeffs - 1);
> +
> +        if (s->dval1) {
> +            for (i = 0; i < a; i++)
> +                p1[i] += p2[i];
> +        }
> +
> +        if (s->dval2) {
> +            x = a + b;
> +            for (i = 0; i < length - x; i++)
> +                p1[x + i] += p2[x + i];
> +        }
> +
> +        for (i = 0; i < s->nb_dcoeffs; i++)
> +            s->f[i] = *p2++ >> s->dshift;
> +
> +        p1 += a;
> +        x = FF_ARRAY_ELEMS(s->f) - s->nb_dcoeffs;
> +        for (; b > 0; b -= tmp) {
    
> +            if (b <= x)
> +                tmp = b;
> +            else
> +                tmp = x;

FFMIN()


[...]
> +    if (avctx->bits_per_coded_sample <= 16) {
> +        av_fast_malloc(&s->decode_buffer, &s->decode_buffer_size,
> +                       sizeof(*s->decode_buffer) * s->nb_samples * avctx->channels +
> +                       FF_INPUT_BUFFER_PADDING_SIZE);
> +        if (!s->decode_buffer)
> +            return AVERROR(ENOMEM);
> +    } else {
> +        s->decode_buffer = (int32_t *)s->frame.data[0];
> +    }

I suggest not to assign something "random" to a pointer that normally
has been allocated and has to be freed.
Its easy to mix it up and end up freeing the wrong pointer


[...]
> +static int tak_read_header(AVFormatContext *s)
> +{
> +    AVIOContext *pb = s->pb;
> +    GetBitContext gb;
> +    AVStream *st;
> +    uint8_t *buffer = NULL;
> +    int ret;
> +
> +    st = avformat_new_stream(s, 0);
> +    if (!st)
> +        return AVERROR(ENOMEM);
> +    st->codec->codec_type = AVMEDIA_TYPE_AUDIO;
> +    st->codec->codec_id   = AV_CODEC_ID_TAK;
> +    st->need_parsing      = AVSTREAM_PARSE_FULL_RAW;
> +
> +    if (avio_rl32(pb) != MKTAG('t', 'B', 'a', 'K')) {
> +        avio_seek(pb, -4, SEEK_CUR);
> +        return 0;
> +    }
> +

> +    while (!url_feof(pb)) {
> +        int metadata_type, metadata_size;

metadata_type should have enum type


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

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120930/a3135388/attachment.asc>


More information about the ffmpeg-devel mailing list