Re: [FFmpeg-devel] [PATCH 3/3] avcodec/aac/aacdec: Fix linking errors with only one decoder enabled

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Lynne:
> May 6, 2024, 11:31 by andreas.rheinhardt@outlook.com:
> 
>> The approach used here has the advantage not to rely
>> on any DCE.
>> Also improve certain the checks from
>> 3390693bfb907765f833766f370e0ba8c7894f44 a bit.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>  libavcodec/aac/aacdec.c | 62 ++++++++++++++++++++---------------------
>>  1 file changed, 31 insertions(+), 31 deletions(-)
>>
>> diff --git a/libavcodec/aac/aacdec.c b/libavcodec/aac/aacdec.c
>> index c6b93e33a2..6a74b05168 100644
>> --- a/libavcodec/aac/aacdec.c
>> +++ b/libavcodec/aac/aacdec.c
>> @@ -63,6 +63,20 @@
>>  #include "libavutil/version.h"
>>  #include "libavutil/thread.h"
>>  
>> +#if CONFIG_AAC_DECODER && CONFIG_AAC_FIXED_DECODER
>> +#define IS_FIXED(is_fixed) (is_fixed)
>> +#define FIXED_OR_FLOAT(is_fixed, func_or_obj, func_args) \
>> +    ((is_fixed) ? RENAME_FIXED(func_or_obj) func_args : (func_or_obj) func_args)
>> +#elif CONFIG_AAC_DECODER
>> +#define IS_FIXED(is_fixed) 0
>> +#define FIXED_OR_FLOAT(is_fixed, func_or_obj, func_args) \
>> +    ((func_or_obj) func_args)
>> +#else
>> +#define IS_FIXED(is_fixed) 1
>> +#define FIXED_OR_FLOAT(is_fixed, func_or_obj, func_args) \
>> +    (RENAME_FIXED(func_or_obj) func_args)
>> +#endif
>> +
>>  /*
>>  * supported tools
>>  *
>> @@ -150,11 +164,8 @@ static av_cold int che_configure(AACDecContext *ac,
>>  return AVERROR_INVALIDDATA;
>>  if (che_pos) {
>>  if (!ac->che[type][id]) {
>> -            int ret;
>> -            if (ac->is_fixed)
>> -                ret = ff_aac_sbr_ctx_alloc_init_fixed(ac, &ac->che[type][id], type);
>> -            else
>> -                ret = ff_aac_sbr_ctx_alloc_init(ac, &ac->che[type][id], type);
>> +            int ret = FIXED_OR_FLOAT(ac->is_fixed, ff_aac_sbr_ctx_alloc_init,
>> +                                     (ac, &ac->che[type][id], type));
>>  if (ret < 0)
>>  return ret;
>>  }
>> @@ -171,10 +182,7 @@ static av_cold int che_configure(AACDecContext *ac,
>>  }
>>  } else {
>>  if (ac->che[type][id]) {
>> -            if (ac->is_fixed)
>> -                ff_aac_sbr_ctx_close_fixed(ac->che[type][id]);
>> -            else
>> -                ff_aac_sbr_ctx_close(ac->che[type][id]);
>> +            FIXED_OR_FLOAT(ac->is_fixed, ff_aac_sbr_ctx_close, (ac->che[type][id]));
>>  }
>>  av_freep(&ac->che[type][id]);
>>  }
>> @@ -1122,8 +1130,7 @@ static av_cold int decode_close(AVCodecContext *avctx)
>>  {
>>  AACDecContext *ac = avctx->priv_data;
>>  int is_fixed = ac->is_fixed;
>> -    void (*sbr_close)(ChannelElement *che) = is_fixed ? ff_aac_sbr_ctx_close_fixed :
>> -                                                        ff_aac_sbr_ctx_close;
>> +    void (*sbr_close)(ChannelElement *che) = FIXED_OR_FLOAT(is_fixed, ff_aac_sbr_ctx_close, );
>>  
>>  for (int type = 0; type < FF_ARRAY_ELEMS(ac->che); type++) {
>>  for (int i = 0; i < MAX_ELEM_ID; i++) {
>> @@ -1154,7 +1161,7 @@ static av_cold int decode_close(AVCodecContext *avctx)
>>  static av_cold int init_dsp(AVCodecContext *avctx)
>>  {
>>  AACDecContext *ac = avctx->priv_data;
>> -    int is_fixed = ac->is_fixed, ret;
>> +    int is_fixed = IS_FIXED(ac->is_fixed), ret;
>>  float scale_fixed, scale_float;
>>  const float *const scalep = is_fixed ? &scale_fixed : &scale_float;
>>  enum AVTXType tx_type = is_fixed ? AV_TX_INT32_MDCT : AV_TX_FLOAT_MDCT;
>> @@ -1188,8 +1195,8 @@ static av_cold int init_dsp(AVCodecContext *avctx)
>>  if (ret < 0)
>>  return ret;
>>  
>> -    ac->dsp = is_fixed ? aac_dsp_fixed : aac_dsp;
>> -    ac->proc = is_fixed ? aac_proc_fixed : aac_proc;
>> +    ac->dsp  = FIXED_OR_FLOAT(is_fixed, aac_dsp, );
>> +    ac->proc = FIXED_OR_FLOAT(is_fixed, aac_proc, );
>>  
>>  return ac->dsp.init(ac);
>>  }
>> @@ -1315,9 +1322,9 @@ static void decode_ltp(AACDecContext *ac, LongTermPrediction *ltp,
>>  int sfb;
>>  
>>  ltp->lag  = get_bits(gb, 11);
>> -    if (CONFIG_AAC_FIXED_DECODER && ac->is_fixed)
>> +    if (IS_FIXED(ac->is_fixed))
>>  ltp->coef_fixed = Q30(ff_ltp_coef[get_bits(gb, 3)]);
>> -    else if (CONFIG_AAC_DECODER)
>> +    else
>>  ltp->coef = ff_ltp_coef[get_bits(gb, 3)];
>>  
>>  for (sfb = 0; sfb < FFMIN(max_sfb, MAX_LTP_LONG_SFB); sfb++)
>> @@ -1626,9 +1633,9 @@ static int decode_tns(AACDecContext *ac, TemporalNoiseShaping *tns,
>>  tmp2_idx = 2 * coef_compress + coef_res;
>>  
>>  for (i = 0; i < tns->order[w][filt]; i++) {
>> -                        if (CONFIG_AAC_FIXED_DECODER && ac->is_fixed)
>> +                        if (IS_FIXED(ac->is_fixed))
>>  tns->coef_fixed[w][filt][i] = Q31(ff_tns_tmp2_map[tmp2_idx][get_bits(gb, coef_len)]);
>> -                        else if (CONFIG_AAC_DECODER)
>> +                        else
>>  tns->coef[w][filt][i] = ff_tns_tmp2_map[tmp2_idx][get_bits(gb, coef_len)];
>>  }
>>  }
>> @@ -1977,11 +1984,8 @@ static int decode_extension_payload(AACDecContext *ac, GetBitContext *gb, int cn
>>  ac->avctx->profile = AV_PROFILE_AAC_HE;
>>  }
>>  
>> -        if (CONFIG_AAC_FIXED_DECODER && ac->is_fixed)
>> -            res = ff_aac_sbr_decode_extension_fixed(ac, che, gb, crc_flag, cnt, elem_type);
>> -        else if (CONFIG_AAC_DECODER)
>> -            res = ff_aac_sbr_decode_extension(ac, che, gb, crc_flag, cnt, elem_type);
>> -
>> +        res = FIXED_OR_FLOAT(ac->is_fixed, ff_aac_sbr_decode_extension,
>> +                             (ac, che, gb, crc_flag, cnt, elem_type));
>>  
>>  if (ac->oc[1].m4ac.ps == 1 && !ac->warned_he_aac_mono) {
>>  av_log(ac->avctx, AV_LOG_VERBOSE, "Treating HE-AAC mono as stereo.\n");
>> @@ -2090,14 +2094,10 @@ static void spectral_to_sample(AACDecContext *ac, int samples)
>>  ac->dsp.update_ltp(ac, &che->ch[1]);
>>  }
>>  if (ac->oc[1].m4ac.sbr > 0) {
>> -                        if (CONFIG_AAC_FIXED_DECODER && ac->is_fixed)
>> -                            ff_aac_sbr_apply_fixed(ac, che, type,
>> -                                                   (void *)che->ch[0].output,
>> -                                                   (void *)che->ch[1].output);
>> -                        else if (CONFIG_AAC_DECODER)
>> -                            ff_aac_sbr_apply(ac, che, type,
>> -                                             (void *)che->ch[0].output,
>> -                                             (void *)che->ch[1].output);
>> +                        FIXED_OR_FLOAT(ac->is_fixed,ff_aac_sbr_apply,
>> +                                       (ac, che, type,
>> +                                        (void *)che->ch[0].output,
>> +                                        (void *)che->ch[1].output));
>>
> 
> I'm not particularly a fan of FIXED_OR_FLOAT, with the
> way that function arguments are given to the macro.
> We already rely on DCE globally, and the code only
> has a few different paths for fixed vs float.
> Would it be possible to fix the linking errors without
> adding more abstractions?

If you don't like the way function arguments are given to the macro,
then you can take a look at v2. It avoids this (except for the one
special case where the current code uses casts...).
And we should actually not rely on DCE. It is not mandated by ISO C.

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