[FFmpeg-devel] [PATCH 1/4] libavcodec: Implementation of AC3 fixed point decoder

[Michael Niedermayer]

On Mon, Oct 28, 2013 at 01:57:10PM +0000, Nedeljko Babic wrote:
> >> Hello Michael and thanks for the review,
[...]
> >
> >> 
> >> >
> >> >> +
> >> >> +/**
> >> >>   * 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?

i think it would make the code a tiny bit easier to read/understand
its not really important though but if not then it should be
documented


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

i assume the difference is a matter of a single shift ?
is the speed difference of using ff_sqrt() with a shift compared to
a new function significant ?

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- 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/20131111/437388bf/attachment.asc>