From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 075774382A for ; Wed, 31 Aug 2022 21:20:23 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 373EE68B99B; Thu, 1 Sep 2022 00:20:22 +0300 (EEST) Received: from mail-yb1-f179.google.com (mail-yb1-f179.google.com [209.85.219.179]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id B847768B874 for ; Thu, 1 Sep 2022 00:20:16 +0300 (EEST) Received: by mail-yb1-f179.google.com with SMTP id j204so6001692ybj.2 for ; Wed, 31 Aug 2022 14:20:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :from:to:cc:subject:date; bh=dHoXMcOxkiN9UEPatWugyrNbZUgr0Pe47Cs7vBn8Lgo=; b=Wt567zumEZzQ6yBeK6/YGMdseJsT2jY+8pDApaX6P9mvi1YjIwL4cIiW4ncjUNj6nD NjkXxv+kZ44Plu19Atwn8geplyrxsuMWyPLf+X84spdelLnDZ5TGEpbeU5RJ8jKtxWps PSPEnj20i5dzl4kVcpx0THXNhToxfJDokljW/NXlDnG7s+36cBJFH7F9+cBjMG2qIbeU jfzEBnvKQ5hBQk2Ilgl2oKWtRqyfgP8kmU0UkzZ1Dx2HTbpc/u+8gXhgg3a9UIAMjh7z W1rtvC50uCbkwZA7x7W7FmEasz1XdglW5u43rX47oBGG7Fer1nZ4fdMAiI8oVRPEwefq ggNw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :x-gm-message-state:from:to:cc:subject:date; bh=dHoXMcOxkiN9UEPatWugyrNbZUgr0Pe47Cs7vBn8Lgo=; b=dKaBIj7eUKbvKwzPuy+GDWRDvNXva58RgS6xjMS5S1MUT8trUShnYVAWLSky7SzZYn PJuiiMZ2uTVr8CYqh1gSoPrPU+v/qOHAHUfiJnpyCMPZWCK8Q96RrRO0hDs7wbGYn3op woS44G9zlEJSuCcpLn+DbIKc934OaiXeWLWYM3ZP5koXIX+aacd2HI0b47JVwOM/oECy JKBUPrd1JrEKYafSaKKH2WoRaspwlPiesMzVegUWRg+XFO6OncI+vwcbob9e6u31sxqt 2XWXGN2GLY8rCUqJWm70bVI+7amUkgKA3c7a7vLNBGZ14AjvQnNzaq/OgZB1EiHERaNj EGwg== X-Gm-Message-State: ACgBeo3QWpLh94/8yrNLBZlQK5wucENg8jJuEqRec73hLEQlGg130kKh 7CxSHHawqnyY9s8fl6QPqfxYl3TUJjMyIVzrTz0QJtL4 X-Google-Smtp-Source: AA6agR5z214xxC3diupKtz9zmgMfFBM7k3GvdK7jVjRfXcVGC9BwNgNvQWPhlXj6EA5OeNVcGuytLealqgGBQvjUN64= X-Received: by 2002:a25:5142:0:b0:691:e8f5:e582 with SMTP id f63-20020a255142000000b00691e8f5e582mr15532829ybb.586.1661980814812; Wed, 31 Aug 2022 14:20:14 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Paul B Mahol Date: Wed, 31 Aug 2022 23:23:26 +0200 Message-ID: To: FFmpeg development discussions and patches X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [FFmpeg-devel] [PATCH] FTR decoder X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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". > _______________________________________________ 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".