Re: [FFmpeg-devel] [PATCH] FTR decoder

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Paul B Mahol:
> diff --git a/libavcodec/ftr.c b/libavcodec/ftr.c
> new file mode 100644
> index 0000000000..03d490a0c9
> --- /dev/null
> +++ b/libavcodec/ftr.c
> @@ -0,0 +1,217 @@
> +/*
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "adts_header.h"
> +#include "avcodec.h"
> +#include "codec_internal.h"
> +#include "get_bits.h"
> +#include "internal.h"

You seem to not have rebased your patch upon master: ff_get_buffer() is
now in decode.h and this won't compile; including internal.h seems
superfluous now.

> +
> +typedef struct FTRContext {
> +    AVCodecContext *aac_avctx[64];   // wrapper context for AAC
> +    int nb_context;
> +    AVPacket *packet;
> +} FTRContext;
> +
> +static av_cold int ftr_init(AVCodecContext *avctx)
> +{
> +    FTRContext *s = avctx->priv_data;
> +    const AVCodec *codec;
> +    int ret;
> +
> +    if (avctx->ch_layout.nb_channels > 64 ||
> +        avctx->ch_layout.nb_channels <= 0)
> +        return AVERROR_BUG;

I don't see what is supposed to limit nb_channels to 64. If it isn't
checked somewhere else, you need to return something else then
AVERROR_BUG. EINVAL, ENOSYS or ENOTSUP.

> +
> +    s->packet = av_packet_alloc();
> +    if (!s->packet)
> +        return AVERROR(ENOMEM);
> +
> +    s->nb_context = avctx->ch_layout.nb_channels;
> +
> +    codec = avcodec_find_decoder(AV_CODEC_ID_AAC);

This may return the libfdk-aac decoder if the native ones are disabled.
It uses AV_SAMPLE_FMT_S16, whereas the native ones use a planar format,
namely AV_SAMPLE_FMT_FLTP or . The way you are forwarding the data only
works with planar formats.
IMO you should just add a configure dependency on the native decoder and
force it by using ff_aac_decoder instead of avcodec_find_decoder(). Or
maybe use ff_aac_fixed_decoder to make this codec easily testable?

> +    if (!codec)
> +        return AVERROR_BUG;
> +
> +    for (int i = 0; i < s->nb_context; i++) {
> +        s->aac_avctx[i] = avcodec_alloc_context3(codec);
> +        if (!s->aac_avctx[i])
> +            return AVERROR(ENOMEM);
> +        ret = avcodec_open2(s->aac_avctx[i], codec, NULL);
> +        if (ret < 0)
> +            return ret;
> +    }
> +
> +    avctx->sample_fmt = s->aac_avctx[0]->sample_fmt;
> +
> +    return 0;
> +}
> +
> +static int ftr_decode_frame(AVCodecContext *avctx, AVFrame *frame,
> +                            int *got_frame, AVPacket *avpkt)
> +{
> +    FTRContext *s = avctx->priv_data;
> +    GetBitContext gb;
> +    int ret, ch_offset = 0;
> +
> +    ret = init_get_bits8(&gb, avpkt->data, avpkt->size);
> +    if (ret < 0)
> +        return ret;
> +
> +    frame->nb_samples = 0;
> +
> +    for (int i = 0; i < s->nb_context; i++) {
> +        AVCodecContext *codec_avctx = s->aac_avctx[i];
> +        GetBitContext gb2 = gb;
> +        AACADTSHeaderInfo hdr_info;
> +        AVFrame *iframe = NULL;
> +        int size;
> +
> +        if (get_bits_left(&gb) < 64)
> +            return AVERROR_INVALIDDATA;
> +
> +        memset(&hdr_info, 0, sizeof(hdr_info));
> +
> +        size = ff_adts_header_parse(&gb2, &hdr_info);
> +        if (size <= 0 || size * 8 > get_bits_left(&gb))
> +            return AVERROR_INVALIDDATA;
> +
> +        if (size > s->packet->size) {
> +            if (s->packet->size == 0) {
> +                ret = av_new_packet(s->packet, size);
> +            } else {
> +                ret = av_grow_packet(s->packet, size - s->packet->size);
> +            }

This branch seems superfluous: av_grow_packet() can handle blank packets
just fine.

> +            if (ret < 0)
> +                return ret;
> +        }
> +
> +        ret = av_packet_make_writable(s->packet);
> +        if (ret < 0)
> +            return ret;
> +
> +        memcpy(s->packet->data, avpkt->data + (get_bits_count(&gb) >> 3), size);
> +        s->packet->size = size;
> +
> +        if (size > 12) {
> +            uint8_t *buf = s->packet->data;
> +
> +            if (buf[3] & 0x20) {

Does this happen often? If not, then you can just reuse the given data
(you just need to set pkt->data and size).

> +                int tmp = buf[8];
> +                buf[ 9] = ~buf[9];
> +                buf[11] = ~buf[11];
> +                buf[12] = ~buf[12];
> +                buf[ 8] = ~buf[10];
> +                buf[10] = ~tmp;
> +            }
> +        }
> +
> +        ret = avcodec_send_packet(codec_avctx, s->packet);
> +        if (ret < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Error submitting a packet for decoding\n");
> +            return ret;
> +        }
> +
> +        iframe = av_frame_alloc();

There is no reason to allocate this temp frame in a loop; it can be
allocated during init just like the temp packet.

> +        if (!iframe)
> +            return AVERROR(ENOMEM);
> +
> +        ret = avcodec_receive_frame(codec_avctx, iframe);
> +        if (ret < 0) {
> +            av_frame_free(&iframe);
> +            return ret;
> +        }
> +
> +        if (!avctx->sample_rate) {
> +            avctx->sample_rate = codec_avctx->sample_rate;
> +        } else {
> +            if (avctx->sample_rate != codec_avctx->sample_rate) {
> +                av_frame_free(&iframe);
> +                return AVERROR_INVALIDDATA;
> +            }
> +        }
> +
> +        if (!frame->nb_samples) {
> +            frame->nb_samples = iframe->nb_samples;
> +            if ((ret = ff_get_buffer(avctx, frame, 0)) < 0) {
> +                av_frame_free(&iframe);
> +                return ret;
> +            }
> +        } else {
> +            if (frame->nb_samples != iframe->nb_samples) {
> +                av_frame_free(&iframe);
> +                return AVERROR_INVALIDDATA;
> +            }
> +        }
> +
> +        skip_bits_long(&gb, size * 8);
> +
> +        if (ch_offset + iframe->ch_layout.nb_channels > avctx->ch_layout.nb_channels) {
> +            av_frame_free(&iframe);
> +            return AVERROR_INVALIDDATA;
> +        }
> +
> +        for (int ch = 0; ch < iframe->ch_layout.nb_channels; ch++) {
> +            memcpy(frame->extended_data[ch_offset + ch], iframe->extended_data[ch], sizeof(float) * iframe->nb_samples);

One could ref the corresponding buffers; but this would cause problems
with the DR1 flag. I wonder whether we can simply forward get_buffer2 to
the child contexts and keep DR1. (This presumes that the used AAC
decoder has the DR1 flag set, which is true for the native one.)

> +        }
> +
> +        ch_offset += iframe->ch_layout.nb_channels;
> +
> +        av_frame_free(&iframe);
> +
> +        if (ch_offset >= avctx->ch_layout.nb_channels)
> +            break;
> +    }
> +
> +    *got_frame = 1;
> +
> +    return get_bits_count(&gb) >> 3;
> +}
> +
> +static void ftr_flush(AVCodecContext *avctx)
> +{
> +    FTRContext *s = avctx->priv_data;
> +
> +    for (int i = 0; i < s->nb_context; i++)
> +        avcodec_flush_buffers(s->aac_avctx[i]);
> +}
> +
> +static av_cold int ftr_close(AVCodecContext *avctx)
> +{
> +    FTRContext *s = avctx->priv_data;
> +
> +    for (int i = 0; i < s->nb_context; i++)
> +        avcodec_free_context(&s->aac_avctx[i]);
> +    av_packet_free(&s->packet);
> +
> +    return 0;
> +}
> +
> +const FFCodec ff_ftr_decoder = {
> +    .p.name         = "ftr",
> +    .p.long_name    = NULL_IF_CONFIG_SMALL("FTR Voice"),
> +    .p.type         = AVMEDIA_TYPE_AUDIO,
> +    .p.id           = AV_CODEC_ID_FTR,
> +    .init           = ftr_init,
> +    FF_CODEC_DECODE_CB(ftr_decode_frame),
> +    .close          = ftr_close,
> +    .flush          = ftr_flush,
> +    .priv_data_size = sizeof(FTRContext),
> +    .p.capabilities = AV_CODEC_CAP_SUBFRAMES | AV_CODEC_CAP_DR1,
> +    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
> +};

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".