[FFmpeg-devel] [PATCH 1/4] libavcodec: Implementation of AC3 fixed point decoder
Michael Niedermayer
michaelni at gmx.at
Fri Oct 18 15:08:52 CEST 2013
On Wed, Oct 16, 2013 at 12:49:05PM +0000, Nedeljko Babic wrote:
> 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
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
[...]
>
> >
> >>
> >> /**
> >> + * 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 ?
>
> >
> >> +
> >> +/**
> >> * 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
also doing the +5 twice is wrong, the code will read over the array
end
>
> >
> >[...]
> >> +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 ?
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
You can kill me, but you cannot change the truth.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131018/a7e3b8f8/attachment.asc>
More information about the ffmpeg-devel
mailing list