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

[Lynne <dev@lynne.ee>]

May 6, 2024, 21:39 by 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...).
>

I'm still not a fan of adding as many lines of macros to deal with it
when that would be basically the same amount of lines as ifdefs.


> And we should actually not rely on DCE. It is not mandated by ISO C.
>

As long as there's no effort done to reduce it, I don't think
it will be a problem to introduce more of it. Its everywhere so
much that adding more doesn't increase the work significantly.
_______________________________________________
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".