[FFmpeg-devel] FTR decoder

Patch attached.

Paul B Mahol:
> Patch attached.

> +    union {
> +        uint64_t u64;
> +        uint8_t  u8[8 + AV_INPUT_BUFFER_PADDING_SIZE];
> +    } tmp;
> +
> +    *poutbuf_size = 0;
> +    *poutbuf = NULL;
> +
> +    if (s->flags & PARSER_FLAG_COMPLETE_FRAMES) {
> +        next = buf_size;
> +    } else {
> +        for (int i = 0; i < buf_size; i++) {
> +            if (ftr->skip > 0) {
> +                ftr->skip--;
> +                if (ftr->skip == 0 && ftr->split) {
> +                    ftr->split = 0;
> +                    next = i;
> +                    break;
> +                } else if (ftr->skip > 0) {
> +                    continue;
> +                }
> +            }
> +
> +            state = (state << 8) | buf[i];
> +            tmp.u64 = av_be2ne64(state);

It is simpler to just use an uint8_t buf[8 +
AV_INPUT_BUFFER_PADDING_SIZE] that is set via AV_RB64(buf, state).

> +            init_get_bits(&bits, tmp.u8 + 8 - AV_AAC_ADTS_HEADER_SIZE,
> +                          AV_AAC_ADTS_HEADER_SIZE * 8);
> +

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,
> +};

On Wed, 31 Aug 2022, at 18:42, Paul B Mahol wrote:
> Patch attached.

gg

New patch updated from feedback received.

On Wed, Aug 31, 2022 at 9:15 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

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

It happens almost always.


>
> > +                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".
>

On 8/31/22, Paul B Mahol <onemda@gmail.com> wrote:
> New patch updated from feedback received.
>

Will apply soon.

Quoting Paul B Mahol (2022-09-22 11:09:03)
> On 8/31/22, Paul B Mahol <onemda@gmail.com> wrote:
> > New patch updated from feedback received.
> >
> 
> Will apply soon.

This needs tests.