On Wed, Aug 31, 2022 at 9:15 PM Andreas Rheinhardt <
andreas.rheinha...@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-requ...@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
