[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