[FFmpeg-devel] [PATCH] E-AC-3 spectral extension (bump)
Michael Niedermayer
michaelni
Wed Mar 24 03:41:35 CET 2010
On Sat, Mar 20, 2010 at 09:13:36AM -0400, Justin Ruggles wrote:
> Michael Niedermayer wrote:
>
> > On Fri, Mar 19, 2010 at 01:03:16PM +0100, Christophe Gisquet wrote:
> >> Hi,
> >>
> >> in the sake of restarting the review process, I'm restarting a thread left here:
> >> http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2009-October/077490.html
> >>
> >> The one complain remaining on the patch at that time was the following
> >> (sorry for the shoddy editing):
> >>
> >>>> + copy_sizes[num_copy_sections++] = bin - s->spx_copy_start_freq;
> >>>> +
> >>>> + for (ch = 1; ch <= s->fbw_channels; ch++) {
> >>>> + if (!s->channel_uses_spx[ch])
> >>>> + continue;
> >>>> +
> >>>> + /* Copy coeffs from normal bands to extension bands */
> >>>> + bin = s->spx_start_freq;
> >>>> + for (i = 0; i < num_copy_sections; i++) {
> >>>> + memcpy(&s->transform_coeffs[ch][bin],
> >>>> + &s->transform_coeffs[ch][s->spx_copy_start_freq],
> >>>> + copy_sizes[i]*sizeof(float));
> >>>> + bin += copy_sizes[i];
> >>>> + }
> >>>> +
> >>>> + /* Calculate RMS energy for each SPX band. */
> >>>> + bin = s->spx_start_freq;
> >>>> + for (bnd = 0; bnd < s->num_spx_bands; bnd++) {
> >>>> + int bandsize = s->spx_band_sizes[bnd];
> >>>> + float accum = 0.0f;
> >>>> + for (i = 0; i < bandsize; i++) {
> >>>> + float coeff = s->transform_coeffs[ch][bin++];
> >>>> + accum += coeff * coeff;
> >>>> + }
> >>>> + rms_energy[bnd] = sqrtf(accum / bandsize);
> >>>> + }
> >>> if i understand the code correctly, the same source coefficients can be
> >>> copied to several destinations, thus it would be more efficient to not
> >>> calculate this for all destination but rather for the source and copy
> >>> as needed to destination bands
> >> The RMS computations occur on strictly non-overlapping samples from
> >> band to band, and always on the SPX bands, never on the original
> >> source:
> >> s->spx_copy_start_freq+sum(copy_sizes[i]) <= s->spx_start_freq
> >>
> >> If one would output some info over the 2 samples available at:
> >> http://samples.mplayerhq.hu/A-codecs/AC3/eac3/
> >> one would see for the 5.1 sample:
> >> Channel 1: Copying 72 elements to bin 109 from bin 25
> >> RMS for band 0 on 109-121 elements
> >> RMS for band 1 on 121-133 elements
> >> RMS for band 2 on 133-145 elements
> >> RMS for band 3 on 145-157 elements
> >> RMS for band 4 on 157-181 elements
> >> (and so on for each channel and call to that function)
> >> For the stereo sample:
> >> Channel 1: Copying 24 elements to bin 133 from bin 25
> >> RMS for band 0 on 133-145 elements
> >> RMS for band 1 on 145-157 elements
> >> (repeat for each channel and call)
> >>
> >> So:
> >> - no computation is remade
> >
> > this is not neccessarily so. Actually ill bet its very very hard
> > to proof the lack of redundant computations even for much simpler
> > algorihms
> >
> >
> >> - the number of samples for each iteration is for the available
> >> samples 12 and 24
> >
> >> - each band may start on an unaligned position (for SSE code at
> >> least), so scalarproduct & co functions can't be reused directly
> >
> > you bring up an interresting point, do these sometimes or always start
> > at an unaligned address?, if its always x*12*sizeof(float)+1 we can
> > probably do something about it
>
> Yes, it will always be that. The spec says "Transform coefficients #25
> through #228 are grouped into 17 sub-bands of 12 coefficients each."
would we overall have fewer unaligned accesses if we would place the array
so that not element 0 but element 1 is aligned?
>
> >
> >> - ff_eac3_apply_spectral_extension is 0.5% of execution time at best
> >> (from a oprofile log)
> >>
> >> Attached is the patch rediff'ed against current SVN. I declare myself
> >> incompetent for more theoretical discussions, though.
> >
> > iam incompetent as well, but you seem interrested. Maybe we can together
> > resolve this, because one or both of us is not understanding the code
> > completely and its quite complicated code though luckily much cleaner than
> > our aac code.
> >
> > for example:
> >> + /* Copy coeffs from normal bands to extension bands */
> >> + bin = s->spx_start_freq;
> >> + for (i = 0; i < num_copy_sections; i++) {
> >> + memcpy(&s->transform_coeffs[ch][bin],
> >> + &s->transform_coeffs[ch][s->spx_copy_start_freq],
> >> + copy_sizes[i]*sizeof(float));
> >> + bin += copy_sizes[i];
> >> + }
> >
> > this clearly copies the same area several times if num_copy_sections is
> > greater than 1.
> > Can it be greater than 1? or to ask differently does this happen for the
> > samples we have?
> > if its guranteed to be ==1 the code can be simplified alot.
> > if not, how representative are our samples? are these random recordings or
> > from some reference/conformance suite?
> >
> > now if its copied 2 or more times i think the rms calculation is done
> > several times over the same data
> > it maybe just 0.5% for the samples we have but first it feels wrong if
> > we do calculations redundantly and second it maybe more than 0.5% for
> > other samples
>
> num_copy_sections can definitely be >1. That is the whole point of it.
if num_copy_sections is only rarely >1 than this optimization is maybe
not worth it
[...]
> > [...]
> >> @@ -43,6 +66,7 @@
> >> #define AC3_MAX_COEFS 256
> >> #define AC3_BLOCK_SIZE 256
> >> #define MAX_BLOCKS 6
> >> +#define SPX_MAX_BANDS 17
> >>
> >> typedef struct {
> >> AVCodecContext *avctx; ///< parent context
> >> @@ -89,6 +113,22 @@
> >> int cpl_coords[AC3_MAX_CHANNELS][18]; ///< coupling coordinates (cplco)
> >> ///@}
> >>
> >> +///@defgroup spx spectral extension
> >> +///@{
> >> + int spx_in_use; ///< spectral extension in use (spxinu)
> >> + uint8_t channel_uses_spx[AC3_MAX_CHANNELS]; ///< channel uses spectral extension (chinspx)
> >> + int8_t spx_atten_code[AC3_MAX_CHANNELS]; ///< spx attenuation code (spxattencod)
> >
> >> + int spx_start_freq; ///< spx start frequency bin
> >> + int spx_end_freq; ///< spx end frequency bin
> >> + int spx_copy_start_freq; ///< spx starting frequency bin for copying (copystartmant)
> >> + ///< the copy region ends at the start of the spx region.
> >
> > start/end/copy_start are not ideal names, they should contain "src"/"dst"
> > so its clear from where to where things are copied
>
> spx_src_start_freq, spx_dst_start_freq, spx_dst_end_freq ?
sound good
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
There will always be a question for which you do not know the correct awnser.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100324/8210a6dd/attachment.pgp>
More information about the ffmpeg-devel
mailing list