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 C02EA41D13 for ; Sun, 20 Mar 2022 04:37:46 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id BE3A268B09B; Sun, 20 Mar 2022 06:37:43 +0200 (EET) Received: from mx.sdf.org (mx.sdf.org [205.166.94.24]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id ECA9E68A77A for ; Sun, 20 Mar 2022 06:37:36 +0200 (EET) Received: from 06d99489bb7677c9a95e511720568327 ([1.145.201.90]) (authenticated (0 bits)) by mx.sdf.org (8.15.2/8.14.5) with ESMTPSA id 22K4bPSw005752 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256 bits) verified NO) for ; Sun, 20 Mar 2022 04:37:30 GMT Date: Sun, 20 Mar 2022 15:37:21 +1100 From: Peter Ross To: FFmpeg development discussions and patches Message-ID: Mail-Followup-To: FFmpeg development discussions and patches References: <20220318130417.47935-1-onemda@gmail.com> MIME-Version: 1.0 In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH] avcodec/binkaudio: add support for >2 channels dct codec 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: multipart/mixed; boundary="===============7566030436083127384==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============7566030436083127384== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Vbo+g5DZJZtOIcuh" Content-Disposition: inline --Vbo+g5DZJZtOIcuh Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Mar 18, 2022 at 04:21:44PM +0100, Paul B Mahol wrote: > On 3/18/22, Andreas Rheinhardt wrote: > > Paul B Mahol: > >> As presented in .binka files. > >> > >> Signed-off-by: Paul B Mahol > >> --- > >> libavcodec/binkaudio.c | 50 +++++++++++++++++++++++++++--------------- > >> 1 file changed, 32 insertions(+), 18 deletions(-) > >> > >> diff --git a/libavcodec/binkaudio.c b/libavcodec/binkaudio.c > >> index b4ff15beeb..54b7e22854 100644 > >> --- a/libavcodec/binkaudio.c > >> +++ b/libavcodec/binkaudio.c > >> @@ -51,13 +51,14 @@ typedef struct BinkAudioContext { > >> int version_b; ///< Bink version 'b' > >> int first; > >> int channels; > >> + int ch_offset; > >> int frame_len; ///< transform size (samples) > >> int overlap_len; ///< overlap size (samples) > >> int block_size; > >> int num_bands; > >> float root; > >> unsigned int bands[26]; > >> - float previous[MAX_CHANNELS][BINK_BLOCK_MAX_SIZE / 16]; ///< coe= ffs > >> from previous audio block > >> + float previous[6][BINK_BLOCK_MAX_SIZE / 16]; ///< coeffs from > >> previous audio block > >> float quant_table[96]; > >> AVPacket *pkt; > >> union { > >> @@ -74,6 +75,7 @@ static av_cold int decode_init(AVCodecContext *avctx) > >> int sample_rate_half; > >> int i, ret; > >> int frame_len_bits; > >> + int max_channels =3D avctx->codec->id =3D=3D AV_CODEC_ID_BINKAUDI= O_RDFT ? > >> MAX_CHANNELS : 6; > > > > If you allow up to six channels, then MAX_CHANNELS (i.e. two) needs to > > be renamed. > > > >> int channels =3D avctx->ch_layout.nb_channels; > >> > >> /* determine frame length */ > >> @@ -85,7 +87,7 @@ static av_cold int decode_init(AVCodecContext *avctx) > >> frame_len_bits =3D 11; > >> } > >> > >> - if (channels < 1 || channels > MAX_CHANNELS) { > >> + if (channels < 1 || channels > max_channels) { > >> av_log(avctx, AV_LOG_ERROR, "invalid number of channels: %d\n= ", > >> channels); > >> return AVERROR_INVALIDDATA; > >> } > >> @@ -110,7 +112,7 @@ static av_cold int decode_init(AVCodecContext *avc= tx) > >> > >> s->frame_len =3D 1 << frame_len_bits; > >> s->overlap_len =3D s->frame_len / 16; > >> - s->block_size =3D (s->frame_len - s->overlap_len) * s->channel= s; > >> + s->block_size =3D (s->frame_len - s->overlap_len) * > >> FFMIN(MAX_CHANNELS, s->channels); > >> sample_rate_half =3D (sample_rate + 1LL) / 2; > >> if (avctx->codec->id =3D=3D AV_CODEC_ID_BINKAUDIO_RDFT) > >> s->root =3D 2.0 / (sqrt(s->frame_len) * 32768.0); > >> @@ -166,7 +168,8 @@ static const uint8_t rle_length_tab[16] =3D { > >> * @param[out] out Output buffer (must contain s->block_size elements) > >> * @return 0 on success, negative error code on failure > >> */ > >> -static int decode_block(BinkAudioContext *s, float **out, int use_dct) > >> +static int decode_block(BinkAudioContext *s, float **out, int use_dct, > >> + int channels, int ch_offset) > >> { > >> int ch, i, j, k; > >> float q, quant[25]; > >> @@ -176,8 +179,8 @@ static int decode_block(BinkAudioContext *s, float > >> **out, int use_dct) > >> if (use_dct) > >> skip_bits(gb, 2); > >> > >> - for (ch =3D 0; ch < s->channels; ch++) { > >> - FFTSample *coeffs =3D out[ch]; > >> + for (ch =3D 0; ch < channels; ch++) { > >> + FFTSample *coeffs =3D out[ch + ch_offset]; > >> > >> if (s->version_b) { > >> if (get_bits_left(gb) < 64) > >> @@ -252,17 +255,17 @@ static int decode_block(BinkAudioContext *s, flo= at > >> **out, int use_dct) > >> s->trans.rdft.rdft_calc(&s->trans.rdft, coeffs); > >> } > >> > >> - for (ch =3D 0; ch < s->channels; ch++) { > >> + for (ch =3D 0; ch < channels; ch++) { > >> int j; > >> - int count =3D s->overlap_len * s->channels; > >> + int count =3D s->overlap_len * channels; > >> if (!s->first) { > >> j =3D ch; > >> - for (i =3D 0; i < s->overlap_len; i++, j +=3D s->channels) > >> - out[ch][i] =3D (s->previous[ch][i] * (count - j) + > >> - out[ch][i] * j) / coun= t; > >> + for (i =3D 0; i < s->overlap_len; i++, j +=3D channels) > >> + out[ch + ch_offset][i] =3D (s->previous[ch + ch_offse= t][i] > >> * (count - j) + > >> + out[ch + ch_offset][i] * > >> j) / count; ^^^ This line needs to be indented some more, to match the previous line. > >> } > >> - memcpy(s->previous[ch], &out[ch][s->frame_len - s->overlap_le= n], > >> - s->overlap_len * sizeof(*s->previous[ch])); > >> + memcpy(s->previous[ch + ch_offset], &out[ch + > >> ch_offset][s->frame_len - s->overlap_len], > >> + s->overlap_len * sizeof(*s->previous[ch + ch_offset])); > >> } > >> > >> s->first =3D 0; > >> @@ -293,6 +296,7 @@ static int binkaudio_receive_frame(AVCodecContext > >> *avctx, AVFrame *frame) > >> GetBitContext *gb =3D &s->gb; > >> int ret; > >> > >> +again: > >> if (!s->pkt->data) { > >> ret =3D ff_decode_get_packet(avctx, s->pkt); > >> if (ret < 0) > >> @@ -313,22 +317,31 @@ static int binkaudio_receive_frame(AVCodecContext > >> *avctx, AVFrame *frame) > >> } > >> > >> /* get output buffer */ > >> - frame->nb_samples =3D s->frame_len; > >> - if ((ret =3D ff_get_buffer(avctx, frame, 0)) < 0) > >> - return ret; > >> + if (s->ch_offset =3D=3D 0) { > >> + frame->nb_samples =3D s->frame_len; > >> + if ((ret =3D ff_get_buffer(avctx, frame, 0)) < 0) > >> + return ret; > >> + } > >> > >> if (decode_block(s, (float **)frame->extended_data, > >> - avctx->codec->id =3D=3D AV_CODEC_ID_BINKAUDIO_DC= T)) { > >> + avctx->codec->id =3D=3D AV_CODEC_ID_BINKAUDIO_DC= T, > >> + FFMIN(MAX_CHANNELS, s->channels), s->ch_offset))= { > >> av_log(avctx, AV_LOG_ERROR, "Incomplete packet\n"); > >> return AVERROR_INVALIDDATA; > >> } > >> + s->ch_offset +=3D MAX_CHANNELS; > >> get_bits_align32(gb); > >> if (!get_bits_left(gb)) { > >> memset(gb, 0, sizeof(*gb)); > >> av_packet_unref(s->pkt); > >> } > >> + if (s->ch_offset >=3D s->channels) { > >> + s->ch_offset =3D 0; > >> + } else { > >> + goto again; > >> + } > > > > Is it really intended that the data for one multi-channel frame is > > divided into several input packets? >=20 > You are missing big picture here, >2 files have channels in different > packets interleaved. > Something like in XMA. (And nothing signals how are they interleaved. > so its worse than in XMA) So it is working fine. I just need another > look for possible regressions and security implications. Renaming > MAX_CHANNELS is not useful as that is not property of both codecs. MAX_CHANNELS (2) *is* a property of both codecs, and should be left alone. I would prefer the '6' magic number be put into a descriptive macro. LGTM. -- Peter (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B) --Vbo+g5DZJZtOIcuh Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABECAB0WIQSpB+AvpuUM0jTNINJnYHnFrEDdawUCYjavfgAKCRBnYHnFrEDd a1JDAKCL9yj6ARMkoeSuhVtMNjGLS9o1TgCgnQrtjfWJcR+LCornC9A9GigfmEE= =ZUOQ -----END PGP SIGNATURE----- --Vbo+g5DZJZtOIcuh-- --===============7566030436083127384== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ 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". --===============7566030436083127384==--