[FFmpeg-devel] [PATCH 1/4] libavcodec: Implementation of AC3 fixed point decoder
Nedeljko Babic
Nedeljko.Babic at imgtec.com
Mon Oct 28 14:57:10 CET 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).
>
>the compiler will remove if(0) code
>and thats used quite extensively in ffmpeg
>git grep 'if *(CONFIG' | wc
> 172 903 13446
>
#if CONFIG is also commonly used in ffmpeg but ok, if (CONFIG_AC3_FIXED) will
be used here.
>
>also iam not sure about using the CONFIG_* namesapce for things not
>set by configure but thats used elsewhere so its rather independant
>of this patch
>
>
>>
>> >
>> >[...]
>> >> @@ -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.
>
>I meant that the calculation of the downmix coefficients does not need
>to be changed, only the final rounding. That is you shouldnt need to
>change the float code, just once the float coefficients are calculated
>scale&round to integers. FIXR was just meant as a placeholder for
>whatever scaling ad rounding is needed
>
>
This will change values a little, but ok, it will be implemented in the next
patch.
>
>>
>> >
>> >>
>> >> /**
>> >> + * 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.
>
>why is the function then not in libavcodec/ac3dec_fixed.c ?
>
You have a point and this will be moved.
>
>>
>> >
>> >> +
>> >> +/**
>> >> * 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.
>
>the value stored is an inverse in one case but not the other i
>suspect. The varible name says "freq" which stands for frequency.
>A inverse of a frequency is not a frequency
>
The value is indeed inverse of a frequency, but it is used for multiplication
instead for division when nratio is being calculated so in terms of usage it
comes to the same.
Besides, it is used only here. Should we create new structure field just for
this?
>also doing the +5 twice is wrong, the code will read over the array
>end
>
You are correct and this will be corrected.
>
>
>>
>> >
>> >[...]
>> >> +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.
>
>can ff_sqrt() used instead or why is it not used ?
>are there speed or precission advanatges ?
Values obtained with ff_sqrt() can not be used because of the fixed point format
that we use in the code.
- Nedeljko
More information about the ffmpeg-devel
mailing list