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 C89F240A39 for ; Mon, 31 Oct 2022 23:18:18 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 31E6E68BDF4; Tue, 1 Nov 2022 01:18:15 +0200 (EET) Received: from mail-oo1-f48.google.com (mail-oo1-f48.google.com [209.85.161.48]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 7216A68BD2B for ; Tue, 1 Nov 2022 01:18:08 +0200 (EET) Received: by mail-oo1-f48.google.com with SMTP id g15-20020a4a894f000000b0047f8e899623so1846119ooi.5 for ; Mon, 31 Oct 2022 16:18:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=a4CCyyUZRDYZOXhiY1lNHlmkDwJUtwDz2gz/6wcaxN0=; b=gHrFBc50fN8g8WfGEVllSLnMP4cHfoBFhlkeK3layQAEDc/tyD9I5D+FjX5wVho/rb n636y0iK+eHXN0lKewQDiRTMReitqKMB3BlHx6k1h5VjMZSWBLYz+Xg8MEGo5y9ionU9 s4pgTcctCyWKeokduf8GLHReEIKmhV9/J0ic50dxhZBv5doALleDhlO+Ajm7iGt548lo ZuBWKYu16+Z9NbZ84QyHCbk9k43HvaGMZushKnrWD/FsdgouVLZIcfU0FCcW+Z1g1pIY Qn2TKnTdgJ17WeU3ZNea/hHs/DO5YCZcOHazghAKDgOPM3Ag6YLcaVxJZ+yrJKfA+qBb qXRQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=a4CCyyUZRDYZOXhiY1lNHlmkDwJUtwDz2gz/6wcaxN0=; b=OUgwD02YC6cd1cIKHLkb3S0D2MO5CaF0jOTlX/NgDfDwJAEpkcXVUcD7PGcWZpIK7x KvuWPrV8O/SeLzFCUbS9mYmWADivLqIM4IF1GEsNFhKgrbTBaNSM3AMRMH5GjjiPAxY3 q1azqE1z3Jcy5AJwC0DKyE2iUai86lQVZ8WoXS4nIbgPB73YNhGvwuqCbTPMY/20gpI1 KEP8nYOyUjnpqAL/5aJNjowl+0ACedcewd+cjBUW3clMzbOfsdKmrW+nHygy/ym0G4Uf Kou8iqNKXCFnlJXZyA7Zx/DJ6FaWVfJiXzymx2GVfGBA3iBJy+MtaUqnbPtV2AAURcCd 8AhQ== X-Gm-Message-State: ACrzQf0zUo6v2HPrif+wx7GWxyAMY9t6RK3HYAXj2nL6rsLqj3RStx7r 0R9mKx1p4IF4TWqLcwhWIA2/eBTc4xU= X-Google-Smtp-Source: AMsMyM7GWXvB4Q/ojDtVDiDUhhDJVZhmdY7QbvVSwdyit0AQDSOv5fm2Pb36u+iemGnode9pe7qaGQ== X-Received: by 2002:a4a:a50c:0:b0:49b:1912:b535 with SMTP id v12-20020a4aa50c000000b0049b1912b535mr3559878ook.45.1667258286411; Mon, 31 Oct 2022 16:18:06 -0700 (PDT) Received: from [192.168.0.13] ([191.97.187.183]) by smtp.gmail.com with ESMTPSA id d8-20020a4ab208000000b0044b47bb023fsm2836967ooo.37.2022.10.31.16.18.05 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 31 Oct 2022 16:18:06 -0700 (PDT) Message-ID: <022ee282-5eda-8e08-d21b-70f72a72a3c8@gmail.com> Date: Mon, 31 Oct 2022 20:18:06 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.4.1 Content-Language: en-US To: ffmpeg-devel@ffmpeg.org References: <20221031202845.1860-1-jamrial@gmail.com> From: James Almer In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH v2] avcodec/atrac3plus: reorder channels to match the output layout 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On 10/31/2022 7:13 PM, Andreas Rheinhardt wrote: > James Almer: >> The order in which the channels are coded in the bitstream do not always follow >> the native, bitmask-based order of channels both signaled by the WAV container >> and forced by this same decoder. This is the case with layouts containing an >> LFE channel, as it's always coded last. >> >> Fixes ticket #9964. >> >> Signed-off-by: James Almer >> --- >> libavcodec/atrac3plusdec.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/libavcodec/atrac3plusdec.c b/libavcodec/atrac3plusdec.c >> index ee71645a3c..9e12f21930 100644 >> --- a/libavcodec/atrac3plusdec.c >> +++ b/libavcodec/atrac3plusdec.c >> @@ -48,6 +48,17 @@ >> #include "atrac.h" >> #include "atrac3plus.h" >> >> +static uint8_t channel_map[8][8] = { >> + { 0, }, >> + { 0, 1, }, >> + { 0, 1, 2, }, >> + { 0, 1, 2, 3, }, >> + { 0, }, >> + { 0, 1, 2, 4, 5, 3, }, >> + { 0, 1, 2, 4, 5, 8, 3, }, >> + { 0, 1, 2, 4, 5, 9, 10, 3, }, >> +}; >> + >> typedef struct ATRAC3PContext { >> GetBitContext gb; >> AVFloatDSPContext *fdsp; >> @@ -378,7 +389,7 @@ static int atrac3p_decode_frame(AVCodecContext *avctx, AVFrame *frame, >> channels_to_process, avctx); >> >> for (i = 0; i < channels_to_process; i++) >> - memcpy(samples_p[out_ch_index + i], ctx->outp_buf[i], >> + memcpy(samples_p[channel_map[frame->ch_layout.nb_channels - 1][out_ch_index + i]], ctx->outp_buf[i], >> ATRAC3P_FRAME_SAMPLES * sizeof(**samples_p)); >> >> ch_block++; > > Looking at the last two entries, it seems to me that you simply used the > numerical values of the AV_CHAN_* constants, i.e. as if you wanted to > write { AV_CHAN_FRONT_LEFT, AV_CHAN_FRONT_RIGHT, AV_CHAN_FRONT_CENTER, > AV_CHAN_BACK_LEFT, AV_CHAN_BACK_RIGHT, AV_CHAN_SIDE_LEFT, > AV_CHAN_SIDE_RIGHT, AV_CHAN_LOW_FREQUENCY } for the last entry. This is > wrong, as it conflates the enum AVChannel domain with the index domain; > it will segfault for 6.1 and 7.1, because there are no data planes with > indices 8, 9 and 10 in the output frame. > > The array for 6.1 is { 0, 1, 2, 4, 5, 6, 3 }, for 7.1 it is { 0, 1, 2, > 4, 5, 6, 7, 3, } (presuming that your first patch was right). The > derivation for 6.1 is as follows: The first channel in atrac is FL, > which is also the first channel in AV_CHANNEL_LAYOUT_6POINT1_BACK. So > the first entry is 0; the next channel is FR which is the second channel > in AV_CHANNEL_LAYOUT_6POINT1_BACK, so the second entry is 1. The next > atrac entry is FC, which is also the next entry in > AV_CHANNEL_LAYOUT_6POINT1_BACK, so the next entry is 2. The next atrac > entry is BL, which is not the next channel in > AV_CHANNEL_LAYOUT_6POINT1_BACK (which is lfe), but the one after that. > So the next entry is four. Similarly, the next entry is five. The atrac > entry after that is BC, which is the next (and last) entry of > AV_CHANNEL_LAYOUT_6POINT1_BACK and has index six. It doesn't matter for > this that there are several channels in enum AVChannel between BR and > BC, as these channels are not present in AV_CHANNEL_LAYOUT_6POINT1_BACK > (it would also not matter if there were any gaps in the values of the > AV_CHAN_* constants in between). The next (and last) atrac entry is LFE, > which is the fourth channel in AV_CHANNEL_LAYOUT_6POINT1_BACK and > therefore has index 3. > > Is it really absolutely guaranteed that atrac only has one channel > layout per channel count? It seems to me that adding a const uint8_t > *channel_map to the context that gets set like ctx->channel_map = (const > uint8_t[]){ 0, 1, 2, 4, 5, 6, 3 } (for 6.1) would be simpler. Unless i'm missing something, this would be local to set_channel_params(). > > - Andreas > > _______________________________________________ > 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".