AAC decoder frame->nb_samples & frame->channels is wrong at get_buffer2

[zylthinking]

Summary of the bug:

when set the get_buffer2 to a custom one, trace in the custom get_buffer2

1. if the profile is FF_PROFILE_AAC_HE, then frame->channels is 1 at the first call, then changed to 2 in the following calls, In this case, the result pcm sounds well (but it is just luck I guess)

2. if the profile is FF_PROFILE_AAC_LOW, then the frame->channels is 2 at all time, in this case, the result pcm is full of noise

Both conditions have a frame->nb_samples == 2048
however, it is indeed should be 1024 at both case after some research and the frame->channels should be 2

How to reproduce:

any aac can be used for testing

[Carl Eugen Hoyos]

Sorry, this has no similarity with a valid ticket afaict. If the issue is not reproducible with the ffmpeg command line interface please provide sample code that allows to reproduce it.

[zylthinking]

Replying to zylthinking:

more detail:
when in he-aac case, the frame->nb_samples is 2048, you can't modify it to 1024 whether or not setting the frame->channels to 2, or it will crash
while, in low aac case, the frame->channels is 2 and leave it untouched, you must modify the frame->nb_samples = frame->nb_samples / 2 to produce the correct sound

[Carl Eugen Hoyos]

You have now added some analysis of the issue you experienced but what is needed is both an explanation what goes wrong (such as "libavcodec crashes" or "the output is just noise") and an explanation how I can reproduce the issue: This is can either be a command line like ffmpeg -i input.aac out.wav or the source code that is needed to reproduce the issue.

[zylthinking]

Replying to cehoyos:
Ok, the code:

static void ffmpeg_frame_put(void* opaque, uint8_t* data)
{
    (void) data;
    struct my_buffer* mbuf = (struct my_buffer *) opaque;
    mbuf->mop->free(mbuf);
}

static int frame_buffer_get(struct AVCodecContext* s, AVFrame* frame, int flags)
{
    my_assert(frame->format == AV_SAMPLE_FMT_FLTP);
    audio_format* fmt = (audio_format *) s->opaque;

    mark("frame->channels = %d, frame->nb_samples = %d\n", frame->channels, frame->nb_samples);

'''    // I know I should not set these, just testing...
    // in he-aac, need the line below
    // in lc-aac, must not
    frame->nb_samples /= 2;

    // the frame->channels changes in the first call and the next calls in lc-aac
    // set it with my correct value.
    frame->channels = fmt->pcm->channel;'''

    int channel_bytes = frame->nb_samples * sizeof(float);
    uintptr_t bytes = channel_bytes * frame->channels;
    frame->linesize[0] = (int) channel_bytes;
    // ffmpeg says some code may access extra 16 bytes
    // and cpu alignment should be honored.
    // arm64 require 4, however, some neon instruction like vst1
    // require alignment 32 at max.
    struct my_buffer* mbuf = mbuf_alloc_2((uint32_t) (sizeof(media_buffer) + bytes + 32 + 16));
    if (mbuf == NULL) {
        return -1;
    }

    media_buffer* media = (media_buffer *) mbuf->ptr[0];
    memset(media->vp, 0, sizeof(media->vp));
    media->angle = 0;
    gidx_field(media) = 0;
    group_field(media) = 1;

    mbuf->length = bytes;
    mbuf->ptr[1] += sizeof(media_buffer);
    mbuf->ptr[1] = (char *) roundup(((uintptr_t) mbuf->ptr[1]), 32);

    my_assert(frame->nb_extended_buf == 0);
    frame->extended_data = frame->data;
    frame->data[0] = (uint8_t *) mbuf->ptr[1];
    if (frame->channels) {
        frame->data[1] = (uint8_t *) (mbuf->ptr[1] + channel_bytes);
    }

    // because documents says buf reference chunk address than data entry
    // e.g. ffmpeg don't know which data[x] buf referenced exactly.
    // ok, it is another word of ffmpeg will never try to get data[x] from
    // buf, then it should use buf only to call addref/release things.
    // in this condition, passing mbuf than data[x] to av_buffer_create is safe.
    frame->buf[0] = av_buffer_create((uint8_t *) mbuf->ptr[1], (int) bytes, ffmpeg_frame_put, (uint8_t *) mbuf, 0);
    if (frame->buf[0] == NULL) {
        mbuf->mop->free(mbuf);
        return -1;
    }

    frame->extended_buf = NULL;
    frame->nb_extended_buf = 0;
    return 0;
}

static void* ffmpeg_open(fourcc** in, fourcc** out)
{
    ffmpeg_wrapper_t* wrapper = (ffmpeg_wrapper_t *) my_malloc(sizeof(ffmpeg_wrapper_t));
    if (wrapper == NULL) {
        return NULL;
    }
    wrapper->bytes = 0;
    wrapper->nb_frame = 0;
    wrapper->seq = 0;
    wrapper->in = to_audio_format(in);
    wrapper->out = to_audio_format(out);

    avcodec_register_all();
    AVCodec* codec = avcodec_find_decoder(AV_CODEC_ID_AAC);
    if (codec == NULL) {
        my_free(wrapper);
        return NULL;
    }
    my_assert(codec->capabilities & CODEC_CAP_DR1);

    AVCodecContext* context = avcodec_alloc_context3(codec);
    if (context == NULL) {
        my_free(wrapper);
        return NULL;
    }
    wrapper->context = context;

    AVFrame* frame_buffer = av_frame_alloc();
    if(frame_buffer == NULL){
        avcodec_free_context(&context);
        my_free(wrapper);
        return NULL;
    }
    wrapper->frame = frame_buffer;
    context->channels = wrapper->in->pcm->channel;
    context->sample_rate = wrapper->in->pcm->samrate;
    context->refcounted_frames = 1;
    context->opaque = wrapper->out;
    context->get_buffer2 = frame_buffer_get;

    context->flags |= CODEC_FLAG_LOW_DELAY;
    if (codec->capabilities & CODEC_CAP_TRUNCATED) {
        context->flags |= CODEC_FLAG_TRUNCATED;
        context->flags2 |= CODEC_FLAG2_CHUNKS;
    }

    if (0 != avcodec_open2(context, codec, NULL)) {
        av_frame_free(&frame_buffer);
        avcodec_free_context(&context);
        my_free(wrapper);
        return NULL;
    }
    return wrapper;
}

[zylthinking]

Replying to cehoyos:

I think there are errors in the information passed to get_buffer2, which will be used to allocate buffers and set pointers to the buffer. However, because of the wrong information, the buffers of wrong size or pointers to wrong address is produced by get_buffer2, that is cause of all these issues.

[Hendrik]

Replying to zylthinking:

Replying to cehoyos:

I think there are errors in the information passed to get_buffer2, which will be used to allocate buffers and set pointers to the buffer. However, because of the wrong information, the buffers of wrong size or pointers to wrong address is produced by get_buffer2, that is cause of all these issues.

If that would be the case, then the default get_buffer2 thats used when you don't replace it would also produce broken audio - but it does not, in fact, audio works just perfectly fine.

[zylthinking]

Replying to cehoyos:

it just happens to be correct.
to be precise, I print information in the default audio_get_buffer with the code:
android_log_print(6, "zylthinking", "frame->linesize[0] = %d, planes = %d, frame->nb_extended_buf = %d:%d\n", frame->linesize[0], planes, frame->nb_extended_buf, AV_NUM_DATA_POINTERS);

and I get:
E/zylthinking(13386): frame->linesize[0] = 8192, planes = 2, frame->nb_extended_buf = 0:8
this is the lc-aac, and you see

1. channel is always 2, not (1 for the first call, and 2 others)
2. pool->linesize[0] is 8192, this is the exactly a buffer size large enough to avoid the errors; in fact, only 4096 bytes is needed for 1 channel

the conclusion is that:

1. the default get_buffer known what I don't know, e.g. the internal FramePool, all information is query from it and it even use a large buffer to avoid buffer size issues; though it has enough and correct information(maybe, if it still have some other source to query the correct nb_samples) to get the buffer size needed

while, I have to say, for a user provided get_buffer2 hook, the only information for me to use is the information decoder passed to me. So, the decoder should not tell me wrong things.

see the code in utils.c

static int audio_get_buffer(AVCodecContext *avctx, AVFrame *frame)
{
    FramePool *pool = avctx->internal->pool;
    int planes = pool->planes;
    int i;

    frame->linesize[0] = pool->linesize[0];
    __android_log_print(6, "zylthinking", "frame->linesize[0] = %d, planes = %d, frame->nb_extended_buf = %d:%d\n", frame->linesize[0], planes, frame->nb_extended_buf, AV_NUM_DATA_POINTERS);

    if (planes > AV_NUM_DATA_POINTERS) {
        frame->extended_data = av_mallocz_array(planes, sizeof(*frame->extended_data));
        frame->nb_extended_buf = planes - AV_NUM_DATA_POINTERS;
        frame->extended_buf  = av_mallocz_array(frame->nb_extended_buf,
                                          sizeof(*frame->extended_buf));
        if (!frame->extended_data || !frame->extended_buf) {
            av_freep(&frame->extended_data);
            av_freep(&frame->extended_buf);
            return AVERROR(ENOMEM);
        }
    } else {
        frame->extended_data = frame->data;
        av_assert0(frame->nb_extended_buf == 0);
    }

    for (i = 0; i < FFMIN(planes, AV_NUM_DATA_POINTERS); i++) {
        frame->buf[i] = av_buffer_pool_get(pool->pools[0]);
        if (!frame->buf[i])
            goto fail;
        frame->extended_data[i] = frame->data[i] = frame->buf[i]->data;
    }
    for (i = 0; i < frame->nb_extended_buf; i++) {
        frame->extended_buf[i] = av_buffer_pool_get(pool->pools[0]);
        if (!frame->extended_buf[i])
            goto fail;
        frame->extended_data[i + AV_NUM_DATA_POINTERS] = frame->extended_buf[i]->data;
    }

    if (avctx->debug & FF_DEBUG_BUFFERS)
        av_log(avctx, AV_LOG_DEBUG, "default_get_buffer called on frame %p", frame);

    return 0;
fail:
    av_frame_unref(frame);
    return AVERROR(ENOMEM);
}

[gjdfgh]

Note that get_buffer2 doesn't really have to allocate an AVFrame as it's output later. It's basically just memory allocation. A decoder can change an AVFrame as it likes after allocation, and for example reduce the sample count.

For example, I see this code in aacdec_template.c:

    ac->frame->nb_samples = 2048;
    if ((ret = ff_get_buffer(avctx, ac->frame, 0)) < 0)
        return ret;

and later:

    ac->frame->nb_samples = samples;
    ac->frame->sample_rate = avctx->sample_rate;

So it's completely expected that the returned frame can be slightly different.

I don't know about this channel count business though.

[zylthinking]

Replying to gjdfgh:

Yes, it is my fault then, but another question is how many bytes should I allocate if I don't know how many samples and channels ?

[gjdfgh]

As much as get_buffer2 requests.

[Elon Musk]

So can this be closed?