[FFmpeg-devel] [PATCH] E-AC-3 spectral extension
Michael Niedermayer
michaelni
Thu Dec 4 12:48:44 CET 2008
On Wed, Dec 03, 2008 at 10:41:59PM -0500, Justin Ruggles wrote:
> Michael Niedermayer wrote:
> > 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
[...]
> >
> >> @@ -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
>
> I have simplified things by committing a change that prevents decoding
> subsequent blocks in a frame after an invalid block is found. It was
> pointless to try anyways. This way it doesn't matter if invalid values
> are in the context since the decoder will always re-read them before
> using them in the next frame.
what about:
/* block start information */
if (s->num_blocks > 1 && get_bits1(gbc)) {
/* reference: Section E2.3.2.27
nblkstrtbits = (numblks - 1) * (4 + ceiling(log2(words_per_frame)))
The spec does not say what this data is or what it's used for.
It is likely the offset of each block within the frame. */
int block_start_bits = (s->num_blocks-1) * (4 + av_log2(s->frame_size-2));
skip_bits(gbc, block_start_bits);
}
?
with this it should be very well possible to decode blocks after a damaged
block, or am i missing something?
some bitstream dependancy between blocks that makes that impossible?
or are these block offsets never stored?
also above allows to do error checking, that is to check that the bitstream
pointer matches the stored offset after each block.
our mp3 decoder also decodes all "blocks" that are undamaged and out mpeg2
decoder also decodes all slices that are undamaged not discarding thw whole
frame or GOP after an error
besides this should be skip_bits_long() instead of skip_bits()
[...]
> >
> >> /* 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)
> >>
> >> +#define INT24_MIN -8388608
> >> +#define INT24_MAX 8388607
> >
> > fine unless this conflicts with something from a standard header
>
> I didn't think it did, but after checking C99, I found I was wrong. I
> have changed it to MIN_INT24 and MAX_INT24.
why not use (1<<23) and (1<<23)-1 ? seems clearer than MIN/MAX_INT24
>
>
> >> +#define M_SQRT_POW2_15 181
> >
> > what does the 15 mean?
>
> Aurel is right, sqrt(pow(2,15)). This was just the first name that came
> to mind that relates to how it is used in the code (sqrt of 2 in signed
> 16-bit fixed-point). I'm open to other suggestions.
iam fine with leaving it at M_SQRT_POW2_15
>
> > [...]
> >
> >> +/**
> >> + * 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?
> >
>
> The spec uses floating point values to 9 decimal places.
why do you round to 24 bits instead of 32?
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato
-------------- 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/20081204/e03822d7/attachment.pgp>
More information about the ffmpeg-devel
mailing list