[FFmpeg-devel] [PATCH] E-AC-3 spectral extension
Michael Niedermayer
michaelni
Sun Nov 16 17:53:44 CET 2008
On Sat, Nov 15, 2008 at 07:10:53PM -0500, Justin Ruggles wrote:
> Hi,
>
> Here is an updated version of E-AC-3 spectral extension support. I have
> addressed the issues mentioned in Michael's review in ffmpeg-cvslog.
>
> - should not be expoitable now
> - uses smaller int types for context arrays
> - got rid of unused variables
> - the code to determine copy_sizes and wrapflag is 70% faster
> - merged the 2 loops for notch filters
> - moved multiplies outside the loop for signal & noise blending
>
> Thanks,
> Justin
> Index: libavcodec/ac3dec.c
> ===================================================================
> --- libavcodec/ac3dec.c (revision 15818)
> +++ libavcodec/ac3dec.c (working copy)
> @@ -304,6 +304,7 @@
> s->channel_in_cpl[s->lfe_ch] = 0;
> }
>
> + s->spx_in_use = 0;
> if (hdr.bitstream_id <= 10) {
> s->eac3 = 0;
> s->snr_offset_strategy = 2;
ok
> @@ -728,9 +729,10 @@
> int ecpl, int start_subband, int end_subband,
> const uint8_t *default_band_struct,
> uint8_t *band_struct, int *num_subbands,
> - int *num_bands, int *band_sizes)
> + int *num_bands, uint8_t *band_sizes)
> {
> - int subbnd, bnd, n_subbands, n_bands, bnd_sz[22];
> + int subbnd, bnd, n_subbands, n_bands=0;
> + uint8_t bnd_sz[22];
>
> n_subbands = end_subband - start_subband;
>
> @@ -769,7 +771,7 @@
> if (num_bands)
> *num_bands = n_bands;
> if (band_sizes)
> - memcpy(band_sizes, bnd_sz, sizeof(int)*n_bands);
> + memcpy(band_sizes, bnd_sz, n_bands);
> }
>
> /**
ok, though maybe this should be a seperate commit with related changes
if any
> @@ -818,15 +820,92 @@
>
> /* spectral extension strategy */
> if (s->eac3 && (!blk || get_bits1(gbc))) {
> - if (get_bits1(gbc)) {
> - av_log_missing_feature(s->avctx, "Spectral extension", 1);
> - return -1;
> + s->spx_in_use = get_bits1(gbc);
> + if (s->spx_in_use) {
> + int begf, endf;
> + int spx_end_subband;
> +
> + /* determine which channels use spx */
> + if (s->channel_mode == AC3_CHMODE_MONO) {
> + s->channel_in_spx[1] = 1;
> + } else {
> + for (ch = 1; ch <= fbw_channels; ch++)
> + s->channel_in_spx[ch] = get_bits1(gbc);
> + }
> +
> + s->spx_copy_start_freq = get_bits(gbc, 2) * 12 + 25;
> + begf = get_bits(gbc, 3);
> + endf = get_bits(gbc, 3);
> + s->spx_start_subband = begf < 6 ? begf+2 : 2*begf-3;
> + spx_end_subband = endf < 4 ? endf+5 : 2*endf+3;
> + if (s->spx_start_subband >= spx_end_subband) {
> + av_log(s->avctx, AV_LOG_ERROR, "invalid spectral extension range (%d >= %d)\n",
> + s->spx_start_subband, spx_end_subband);
> + return -1;
> + }
with code like this i always ask myself if its enough or not
I mean this code is conditinal on a get_bits1(gbc) apparently otherwise
reusing the last values.
and the return -1 does leave some changed vars in the context, so it
pretty likely can leave invalid vars in the context.
I dont know if they are used or not after a return -1
but the code is not very solid like that either way as it depends on some
variables in the context not being used ...
so IMHO there either should be no invalid values left or it should be
documented in comments why its safe currently and appropriate notes
should be placed in the code to prevent future mistaken use of the
variables, but i guess its easier just not to store invalid values
in the context to begin with
> + s->spx_start_freq = s->spx_start_subband * 12 + 25;
> + s->spx_end_freq = spx_end_subband * 12 + 25;
> + if (s->spx_copy_start_freq >= s->spx_start_freq) {
> + av_log(s->avctx, AV_LOG_ERROR, "invalid spectral extension copy start bin (%d >= %d)\n",
> + s->spx_copy_start_freq, s->spx_start_freq);
> + return -1;
> + }
same
[...]
> - /* TODO: spectral extension coordinates */
> + /* spectral extension coordinates */
> + if (s->spx_in_use) {
> + for (ch = 1; ch <= fbw_channels; ch++) {
> + if (s->channel_in_spx[ch]) {
> + if (s->first_spx_coords[ch] || get_bits1(gbc)) {
> + int bin, spx_blend;
> + int master_spx_coord;
> + s->first_spx_coords[ch] = 0;
if i understand correctly, first_spx_coords could be replaced by checking
if some of the fields below has been initialized already
> + spx_blend = get_bits(gbc, 5) << 18;
> + master_spx_coord = get_bits(gbc, 2) * 3;
> + bin = s->spx_start_freq;
> + for (bnd = 0; bnd < s->num_spx_bands; bnd++) {
> + int spx_coord_exp, spx_coord_mant;
>
> + /* calculate blending factors */
> + int bandsize = s->spx_band_sizes[bnd];
> + int nratio = (((bin + (bandsize >> 1)) << 23) / s->spx_end_freq) - spx_blend;
> + nratio = av_clip(nratio, 0, INT24_MAX);
> + s->spx_noise_blend [ch][bnd] = ff_sqrt(( nratio) << 8) * M_SQRT_POW2_15;
> + s->spx_signal_blend[ch][bnd] = ff_sqrt((INT24_MAX - nratio) << 8) * M_SQRT_POW2_15;
> + s->spx_noise_blend[ch][bnd] = (s->spx_noise_blend[ch][bnd] * 14529495) >> 23;
vertical align
[...]
> @@ -939,8 +1021,14 @@
> if (channel_mode == AC3_CHMODE_STEREO) {
> if ((s->eac3 && !blk) || get_bits1(gbc)) {
> s->num_rematrixing_bands = 4;
> - if(cpl_in_use && s->start_freq[CPL_CH] <= 61)
> + if (cpl_in_use) {
> + if (s->start_freq[CPL_CH] <= 61)
> s->num_rematrixing_bands -= 1 + (s->start_freq[CPL_CH] == 37);
> + } else if (s->spx_in_use) {
> + if (s->spx_start_freq <= 61)
> + s->num_rematrixing_bands -= 1 + (s->spx_start_freq <= 37) +
> + (s->spx_start_freq <= 25);
> + }
> for(bnd=0; bnd<s->num_rematrixing_bands; bnd++)
> s->rematrixing_flags[bnd] = get_bits1(gbc);
> } else if (!blk) {
ok, i think
> @@ -965,6 +1053,8 @@
> int prev = s->end_freq[ch];
> if (s->channel_in_cpl[ch])
> s->end_freq[ch] = s->start_freq[CPL_CH];
> + else if (s->channel_in_spx[ch])
> + s->end_freq[ch] = s->spx_start_freq;
> else {
> int bandwidth_code = get_bits(gbc, 6);
> if (bandwidth_code > 60) {
> @@ -1155,12 +1245,12 @@
>
> /* TODO: generate enhanced coupling coordinates and uncouple */
>
> - /* TODO: apply spectral extension */
> -
> /* recover coefficients if rematrixing is in use */
> if(s->channel_mode == AC3_CHMODE_STEREO)
> do_rematrixing(s);
ok
>
> + ff_eac3_apply_spectral_extension(s);
> +
shouldnt this be under if(spx_in_use) ?
maybe its irrelevant speedwise though, i dunno ...
> /* apply scaling to coefficients (headroom, dynrng) */
> for(ch=1; ch<=s->channels; ch++) {
> float gain = s->mul_bias / 4194304.0f;
> Index: libavcodec/ac3dec.h
> ===================================================================
> --- libavcodec/ac3dec.h (revision 15818)
> +++ libavcodec/ac3dec.h (working copy)
> @@ -42,7 +42,12 @@
> #define AC3_MAX_COEFS 256
> #define AC3_BLOCK_SIZE 256
> #define MAX_BLOCKS 6
> +#define SPX_MAX_BANDS 17
ok
>
> +#define INT24_MIN -8388608
> +#define INT24_MAX 8388607
fine unless this conflicts with something from a standard header
> +#define M_SQRT_POW2_15 181
what does the 15 mean?
> +
> typedef struct {
> AVCodecContext *avctx; ///< parent context
> GetBitContext gbc; ///< bitstream reader
> @@ -88,6 +93,25 @@
> 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_in_spx[AC3_MAX_CHANNELS]; ///< channel in spectral extension (chinspx)
> + int8_t spx_atten_code[AC3_MAX_CHANNELS]; ///< spx attenuation code (spxattencod)
ok
[...]
> @@ -179,4 +203,6 @@
> */
> void ff_eac3_decode_transform_coeffs_aht_ch(AC3DecodeContext *s, int ch);
>
> +void ff_eac3_apply_spectral_extension(AC3DecodeContext *s);
> +
> #endif /* AVCODEC_AC3DEC_H */
ok
[...]
> @@ -459,13 +550,11 @@
> }
>
> /* spectral extension attenuation data */
> - if (parse_spx_atten_data) {
> - av_log_missing_feature(s->avctx, "Spectral extension attenuation", 1);
> - for (ch = 1; ch <= s->fbw_channels; ch++) {
> - if (get_bits1(gbc)) { // channel has spx attenuation
> - skip_bits(gbc, 5); // skip spx attenuation code
> - }
> - }
> + for (ch = 1; ch <= s->fbw_channels; ch++) {
> + if (parse_spx_atten_data && get_bits1(gbc))
> + s->spx_atten_code[ch] = get_bits(gbc, 5);
> + else
> + s->spx_atten_code[ch] = -1;
> }
>
> /* block start information */
ok
[...]
> +/**
> + * Table E.25: Spectral Extension Attenuation Table
> + * 24-bit fixed-point version of the floating-point table in the specification.
> + * ff_eac3_spx_atten_tab[code][bin]=lrint(pow(1<<(bin+1),(code+1)/-15.0)*(1<<23));
> + */
does the table in the spec only specify 24bit or is this rounded somehow?
[...]
PS: please add -p to the diff flags next time, that simplifies review
sometimes
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- 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/20081116/42955696/attachment.pgp>
More information about the ffmpeg-devel
mailing list