[FFmpeg-devel] [PATCH 1/4] libavcodec: Implementation of AC3 fixed point decoder
Nedeljko Babic
Nedeljko.Babic at imgtec.com
Wed Oct 16 14:49:05 CEST 2013
Hello Michael and thanks for the review,
>> index 20b4b61..6427840 100644
>> --- a/libavcodec/ac3dec.c
>> +++ b/libavcodec/ac3dec.c
>> @@ -64,7 +64,7 @@ static const uint8_t quantization_tab[16] = {
>> static float dynamic_range_tab[256];
>>
>> /** Adjustments in dB gain */
>> -static const float gain_levels[9] = {
>> +static const INTFLOAT AC3_RENAME(gain_levels)[9] = {
>> LEVEL_PLUS_3DB,
>> LEVEL_PLUS_1POINT5DB,
>> LEVEL_ONE,
>> @@ -168,14 +168,22 @@ static av_cold int ac3_decode_init(AVCodecContext *avctx)
>> ac3_tables_init();
>> ff_mdct_init(&s->imdct_256, 8, 1, 1.0);
>> ff_mdct_init(&s->imdct_512, 9, 1, 1.0);
>> - ff_kbd_window_init(s->window, 5.0, 256);
>> + AC3_RENAME(ff_kbd_window_init)(s->window, 5.0, 256);
>> ff_dsputil_init(&s->dsp, avctx);
>> +#if CONFIG_AC3_FIXED
>> + avpriv_fixed_dsp_init(&s->fdsp, avctx->flags & CODEC_FLAG_BITEXACT);
>> +#else
>> avpriv_float_dsp_init(&s->fdsp, avctx->flags & CODEC_FLAG_BITEXACT);
>> +#endif
>> ff_ac3dsp_init(&s->ac3dsp, avctx->flags & CODEC_FLAG_BITEXACT);
>> ff_fmt_convert_init(&s->fmt_conv, avctx);
>> av_lfg_init(&s->dith_state, 0);
>>
>
>> +#if CONFIG_AC3_FIXED
>> + avctx->sample_fmt = AV_SAMPLE_FMT_S16P;
>> +#else
>> avctx->sample_fmt = AV_SAMPLE_FMT_FLTP;
>> +#endif
>
>if (CONFIG_AC3_FIXED)
>can be used here, no need for ifdefs
>
The preprocessors if - else directives were used instead of regular commands
because in this way only parts that are necessary for particular (fixed or
floating point) decoder are compiled (similar as it is done in other parts of ffmpeg).
>
>[...]
>> @@ -322,22 +330,23 @@ static void set_downmix_coeffs(AC3DecodeContext *s)
>> }
>>
>> /* renormalize */
>> - norm0 = norm1 = 0.0;
>> + norm0 = norm1 = FIXR(0.0);
>> for (i = 0; i < s->fbw_channels; i++) {
>> norm0 += s->downmix_coeffs[i][0];
>> norm1 += s->downmix_coeffs[i][1];
>> }
>> - norm0 = 1.0f / norm0;
>> - norm1 = 1.0f / norm1;
>> +
>> + norm0 = AC3_NORM(norm0);
>> + norm1 = AC3_NORM(norm1);
>> +
>> for (i = 0; i < s->fbw_channels; i++) {
>> - s->downmix_coeffs[i][0] *= norm0;
>> - s->downmix_coeffs[i][1] *= norm1;
>> + s->downmix_coeffs[i][0] = (SHORTFLOAT)AC3_MUL(s->downmix_coeffs[i][0],norm0);
>> + s->downmix_coeffs[i][1] = (SHORTFLOAT)AC3_MUL(s->downmix_coeffs[i][1],norm1);
>> }
>>
>> if (s->output_mode == AC3_CHMODE_MONO) {
>> for (i = 0; i < s->fbw_channels; i++)
>> - s->downmix_coeffs[i][0] = (s->downmix_coeffs[i][0] +
>> - s->downmix_coeffs[i][1]) * LEVEL_MINUS_3DB;
>> + s->downmix_coeffs[i][0] = AC3_LEVEL(s->downmix_coeffs[i][0] + s->downmix_coeffs[i][1]);
>> }
>> }
>
>i think this function has no speed relevance
>and a simple
>for all
> downmix_coeffs_fixed[x][y] = FIXR(downmix_coeffs[x][y]);
>
>should work as well
>
The problem in using simple FIXR here is in that for fixed point code value has
to be rounded before it is multiplied by LEVEL_MINUS_3DB. That is the reason why
AC3_LEVEL macro is used here instead of FIXR15.
>
>>
>> @@ -601,25 +610,66 @@ static inline void do_imdct(AC3DecodeContext *s, int channels)
>> for (ch = 1; ch <= channels; ch++) {
>> if (s->block_switch[ch]) {
>> int i;
>> - float *x = s->tmp_output + 128;
>> + FFTSample *x = s->tmp_output + 128;
>> for (i = 0; i < 128; i++)
>> x[i] = s->transform_coeffs[ch][2 * i];
>> s->imdct_256.imdct_half(&s->imdct_256, s->tmp_output, x);
>> +#if CONFIG_AC3_FIXED
>> + s->fdsp.vector_fmul_window_fixed_scaled(s->outptr[ch - 1], s->delay[ch - 1],
>> + s->tmp_output, s->window, 128, 8);
>> +#else
>> s->fdsp.vector_fmul_window(s->outptr[ch - 1], s->delay[ch - 1],
>> s->tmp_output, s->window, 128);
>> +#endif
>> for (i = 0; i < 128; i++)
>> x[i] = s->transform_coeffs[ch][2 * i + 1];
>> s->imdct_256.imdct_half(&s->imdct_256, s->delay[ch - 1], x);
>> } else {
>> s->imdct_512.imdct_half(&s->imdct_512, s->tmp_output, s->transform_coeffs[ch]);
>> +#if CONFIG_AC3_FIXED
>> + s->fdsp.vector_fmul_window_fixed_scaled(s->outptr[ch - 1], s->delay[ch - 1],
>> + s->tmp_output, s->window, 128, 8);
>> +#else
>> s->fdsp.vector_fmul_window(s->outptr[ch - 1], s->delay[ch - 1],
>> s->tmp_output, s->window, 128);
>> - memcpy(s->delay[ch - 1], s->tmp_output + 128, 128 * sizeof(float));
>> +#endif
>> + memcpy(s->delay[ch - 1], s->tmp_output + 128, 128 * sizeof(FFTSample));
>> }
>> }
>> }
>
>why do these use #ifdefs and dont just use fixed point implementations
>of the existing vector_fmul_window API ?
>
>
As consequence of review from April, version of vector_fmul_windov was created
that accepts additional scaling parameter and it is used here. Since this
version has 6 arguments, existing vector_fmul_window API cannot be used here.
And even if regular (5 argument) version was used, it is defined as static in
libavutil/fixed_dsp.c file (emulating the way it was done for floating point
version) and it has to use vector_fmul_window_fixed API, or it has to be
moved to float_dsp.c file in which case file should be renamed.
>
>>
>> /**
>> + * Downmix samples from original signal to stereo or mono (this is for 16-bit samples
>> + * and fixed point decoder - original (for 32-bit samples) is in ac3dsp.c).
>> + */
>> +#if CONFIG_AC3_FIXED
>> +static void ac3_downmix_c_fixed16(int16_t **samples, int16_t (*matrix)[2],
>> + int out_ch, int in_ch, int len)
>> +{
>> + int i, j;
>> + int v0, v1;
>> + if (out_ch == 2) {
>> + for (i = 0; i < len; i++) {
>> + v0 = v1 = 0;
>> + for (j = 0; j < in_ch; j++) {
>> + v0 += samples[j][i] * matrix[j][0];
>> + v1 += samples[j][i] * matrix[j][1];
>> + }
>> + samples[0][i] = (v0+2048)>>12;
>> + samples[1][i] = (v1+2048)>>12;
>> + }
>> + } else if (out_ch == 1) {
>> + for (i = 0; i < len; i++) {
>> + v0 = 0;
>> + for (j = 0; j < in_ch; j++)
>> + v0 += samples[j][i] * matrix[j][0];
>> + samples[0][i] = (v0+2048)>>12;
>> + }
>> + }
>> +}
>> +#endif
>
>the #if doesnt seem needed
>
The directive is used here because this function is used only in ac3 fixed point
code and there is no need for it to be compiled for floating point decoder.
>
>> +
>> +/**
>> * Upmix delay samples from stereo to original channel layout.
>> */
>> static void ac3_upmix_delay(AC3DecodeContext *s)
>> @@ -747,10 +797,9 @@ static int decode_audio_block(AC3DecodeContext *s, int blk)
>> i = !s->channel_mode;
>> do {
>> if (get_bits1(gbc)) {
>> - s->dynamic_range[i] = ((dynamic_range_tab[get_bits(gbc, 8)] - 1.0) *
>> - s->drc_scale) + 1.0;
>> + s->dynamic_range[i] = AC3_DYNAMIC_RANGE(get_bits(gbc, 8));
>> } else if (blk == 0) {
>> - s->dynamic_range[i] = 1.0f;
>> + s->dynamic_range[i] = AC3_DYNAMIC_RANGE1;
>> }
>> } while (i--);
>>
>
>> @@ -776,6 +825,10 @@ static int decode_audio_block(AC3DecodeContext *s, int blk)
>> if (start_subband > 7)
>> start_subband += start_subband - 7;
>> end_subband = get_bits(gbc, 3) + 5;
>> +#if CONFIG_AC3_FIXED
>> + s->spx_dst_end_freq = end_freq_inv_tab[end_subband];
>> + end_subband += 5;
>> +#endif
>> if (end_subband > 7)
>> end_subband += end_subband - 7;
>> dst_start_freq = dst_start_freq * 12 + 25;
>> @@ -796,7 +849,9 @@ static int decode_audio_block(AC3DecodeContext *s, int blk)
>>
>> s->spx_dst_start_freq = dst_start_freq;
>> s->spx_src_start_freq = src_start_freq;
>> +#if !CONFIG_AC3_FIXED
>> s->spx_dst_end_freq = dst_end_freq;
>> +#endif
>>
>> decode_band_structure(gbc, blk, s->eac3, 0,
>> start_subband, end_subband,
>
>why is a variable with one and the same name used to represet
>different things between fixed and float versions ?
>
If you are referring to s->spx_dst_end_freq, it represents the same thing in
both versions of the code. The only difference is that in fixed point code
value is taken from the table and in floating point version it is calculated.
>
>
>[...]
>> @@ -1203,6 +1288,17 @@ static int decode_audio_block(AC3DecodeContext *s, int blk)
>> if (s->channel_mode == AC3_CHMODE_STEREO)
>> do_rematrixing(s);
>>
>> +#if CONFIG_AC3_FIXED
>> + for (ch = 1; ch <= s->channels; ch++) {
>> + int gain;
>> + if(s->channel_mode == AC3_CHMODE_DUALMONO) {
>> + gain = s->dynamic_range[2-ch];
>> + } else {
>> + gain = s->dynamic_range[0];
>> + }
>> + scale_coefs(s->transform_coeffs[ch], s->fixed_coeffs[ch], gain, 256);
>> + }
>> +#else
>> /* apply scaling to coefficients (headroom, dynrng) */
>> for (ch = 1; ch <= s->channels; ch++) {
>> float gain = 1.0 / 4194304.0f;
>> @@ -1214,6 +1310,7 @@ static int decode_audio_block(AC3DecodeContext *s, int blk)
>> s->fmt_conv.int32_to_float_fmul_scalar(s->transform_coeffs[ch],
>> s->fixed_coeffs[ch], gain, 256);
>> }
>> +#endif
>
>the if() can be factored out
>gain can be changed to INTFLOAT
>
Ok. This will be done in the next patch.
>
>>
>> /* apply spectral extension to high frequency bins */
>> if (s->spx_in_use && CONFIG_EAC3_DECODER) {
>
> > @@ -1235,23 +1332,26 @@ static int decode_audio_block(AC3DecodeContext *s, int blk)
>> }
>>
>> do_imdct(s, s->channels);
>> -
>> if (downmix_output) {
>
>unrelated
>
You are correct and this will be removed form the patch.
>
>> +#if CONFIG_AC3_FIXED
>> + ac3_downmix_c_fixed16(s->outptr, s->downmix_coeffs,
>> + s->out_channels, s->fbw_channels, 256);
>> +#else
>> s->ac3dsp.downmix(s->outptr, s->downmix_coeffs,
>> s->out_channels, s->fbw_channels, 256);
>> +#endif
>
>why is s->ac3dsp.downmix not set to ac3_downmix_c_fixed16 ?
>
Function ac3_downmix_c_fixed16 is used only here. On the other hand, in file
ac3dsp.c function ac3_downmix_c_fixed is defined that is used elsewhere and
this function uses downmix_fixed API. Later in code AC3_RENAME macro is used
to switch between downmix and downmix_fixed, so setting s->ac3dsp.downmix
would create additional complication.
>
>[...]
>> +#include "ac3dec.c"
>> +
>> +static const AVOption options[] = {
>> + { "drc_scale", "percentage of dynamic range compression to apply", OFFSET(drc_scale), AV_OPT_TYPE_INT, {.i64 = 1}, 0, 1, PAR },
>> +
>> +{"dmix_mode", "Preferred Stereo Downmix Mode", OFFSET(preferred_stereo_downmix), AV_OPT_TYPE_INT, {.i64 = -1 }, -1, 2, 0, "dmix_mode"},
>> +{"ltrt_cmixlev", "Lt/Rt Center Mix Level", OFFSET(ltrt_center_mix_level), AV_OPT_TYPE_INT, {.i64 = -1 }, -1, 2, 0},
>> +{"ltrt_surmixlev", "Lt/Rt Surround Mix Level", OFFSET(ltrt_surround_mix_level), AV_OPT_TYPE_INT, {.i64 = -1 }, -1, 2, 0},
>> +{"loro_cmixlev", "Lo/Ro Center Mix Level", OFFSET(loro_center_mix_level), AV_OPT_TYPE_INT, {.i64 = -1 }, -1, 2, 0},
>> +{"loro_surmixlev", "Lo/Ro Surround Mix Level", OFFSET(loro_surround_mix_level), AV_OPT_TYPE_INT, {.i64 = -1 }, -1, 2, 0},
>
>in the float decoder these have a range from -1.0 to 2.0 or 0.0 to
>1.0 and here they are integers with range from 0 to 1 and -1 to 2
>
>that dramatic loss of precission looks odd
>
This is a mistake and it will be corrected in the next patch.
>
>[...]
>> diff --git a/libavcodec/kbdwin.c b/libavcodec/kbdwin.c
>> index 5a62e9d..c6cdabc 100644
>> --- a/libavcodec/kbdwin.c
>> +++ b/libavcodec/kbdwin.c
>> @@ -45,3 +45,26 @@ av_cold void ff_kbd_window_init(float *window, float alpha, int n)
>> for (i = 0; i < n; i++)
>> window[i] = sqrt(local_window[i] / sum);
>> }
>> +
>> +av_cold void ff_kbd_window_init_fixed(int32_t *window, float alpha, int n)
>> +{
>> + int i, j;
>> + double sum = 0.0, bessel, tmp;
>> + double local_window[FF_KBD_WINDOW_MAX];
>> + double alpha2 = (alpha * M_PI / n) * (alpha * M_PI / n);
>> +
>> + av_assert0(n <= FF_KBD_WINDOW_MAX);
>> +
>> + for (i = 0; i < n; i++) {
>> + tmp = i * (n - i) * alpha2;
>> + bessel = 1.0;
>> + for (j = BESSEL_I0_ITER; j > 0; j--)
>> + bessel = bessel * tmp / (j * j) + 1;
>> + sum += bessel;
>> + local_window[i] = sum;
>> + }
>> +
>> + sum++;
>> + for (i = 0; i < n; i++)
>> + window[i] = (int)floor(2147483648.0 * sqrt(local_window[i] / sum) + 0.5);
>> +}
>
>this can be implemneted by caling the existing function and converting
>its output to int instead of duplicating the code
>
This function was created so additional loop for converting results to int is
avoided. However, since this function is called only during initialization, I
guess that it doesn't make much difference, so it will be removed in the new
patch.
>
>[...]
>> +static av_always_inline int fixed_sqrt(int x, int bits)
>
>why isnt ff_sqrt() used ?
Function fixed_sqrt was created because of the fixed point format that is
needed.
- Nedeljko
More information about the ffmpeg-devel
mailing list