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

Paul B Mahol onemda at gmail.com
Fri Oct 5 08:42:42 CEST 2012


On 9/30/12, Michael Niedermayer <michaelni at gmx.at> wrote:
> 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

Not needed, because sample_rate takes 18 bits.
>
>
>
> [...]
>> +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

Fixed.
>
>
> [...]
>> +
>> +static int get_scale(int sample_rate, int shift)
>> +{
>> +    return (((sample_rate + 511 >> 9) + 3) & 0xFFFFFFFC) << shift;
>
> FFALIGN(sample_rate + 511 >> 9, 4) << shift;
Fixed.
>
>
> [...]
>> +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

Done.
>
>
>> +}
>> +
>> +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

Naive try to replace above code with this failed but i will document
function eventually.

> (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()

Fixed.
>
>
> [...]
>> +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

They are unsigned.
>
>
>> +        }
>> +        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 ;

Fixed.
>
>
>> +            *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

Fixed, all sign extensions are not redundant.
>
>
>> +        }
>> +        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()
Fixed.
>
>
> [...]
>> +    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

Fixed.
>
>
> [...]
>> +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

Fixed.

The new patch is in another thread.


More information about the ffmpeg-devel mailing list