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

The same amount of lines as ifdefs? V1 adds as many lines as it removes,
v2 adds one line. Fixing this via ifdefs (or even ordinary CONFIG checks
with dce) will certainly amount to more.

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