[Ffmpeg-devel] [PATCH] ATRAC3 decoder
Michael Niedermayer
michaelni
Sat Apr 14 21:19:08 CEST 2007
Hi
On Thu, Apr 12, 2007 at 07:16:19PM +0200, Benjamin Larsson wrote:
> Benjamin Larsson wrote:
> > Michael Niedermayer wrote:
> >
> >> Hi
> >>
> >>
> >> is it your code or sonys? if its yours you should be able to explain
> >> it if not, you should NOT submit it here ...
> >>
> >> [...]
> >>
> >>
> >
> > Ok, I think you are right those tables aren't needed. I'll sort it out
> > and resubmit the patch.
> >
> >
> > MvH
> > Benjamin Larsson
> >
>
> Patch updated.
[...]
> typedef struct {
> GetBitContext gb;
> /* stream data */
> int channels;
> int codingMode;
> int bit_rate;
> int sample_rate;
> int samples_per_channel;
> int samples_per_frame;
>
> int bits_per_frame;
> int bytes_per_frame;
> int pBs;
> channel_unit* pUnits;
>
> /* joint-stereo related variables */
> int matrix_coeff_index_prev[4];
> int matrix_coeff_index_now[4];
> int matrix_coeff_index_next[4];
> int weighting_delay[6];
>
> /* data buffers */
> float outSamples[2048];
> uint8_t* decoded_bytes_buffer;
> float tempBuf[1070];
> DECLARE_ALIGNED_16(float,mdct_tmp[512]);
>
> /* extradata */
> int atrac3version;
> int delay;
> int scrambled_stream;
> int frame_factor;
all the comments should be doxygen compatible i think the syntax for
blocks was ///@{ and @} or something like that
[...]
> /* Regarding multiple instances. */
maybe add a "FIXME check if this is ok" here
> static MDCTContext mdct_ctx;
> static DSPContext dsp;
>
>
> /* quadrature mirror synthesis filter */
> static void iqmf (float *inlo, float *inhi, unsigned int nIn, float *pOut, float *delayBuf, float *temp, float *pWindow)
not doxygen compatible
[...]
> /**
> * Regular 512 points IMDCT without overlapping, with the exception of the swapping of odd bands
> * caused by the reverse spectra of the QMF.
> *
> * @param pInput float input
> * @param pOutput float output
> * @param odd_band 1 if the band is an odd band
> * @param mdct_tmp aligned temporary buffer for the mdct
> */
>
> void IMLT(float *pInput, float *pOutput, int odd_band, float* mdct_tmp)
this function should be static
[...]
> /**
> * Atrac 3 indata descrambling, only used for data coming from the rm container
> *
> * @param in pointer to 8 bit array of indata
> * @param bits amount of bits
> * @param out pointer to 8 bit array of outdata
> */
>
> static inline int decode_bytes(uint8_t* inbuffer, uint8_t* out, int bytes){
as this is only called once per frame it doesnt make much sense to
inline it. also every sane compiler should inline once used static functions
anyway
[...]
>
> /* Inverse quantize the coefficients. */
> for (pIn=mantissas ; first<last; first++, pIn++)
> pOut[first] = (float)*pIn * SF;
the cast seems useless
> } else {
> /* This subband was not coded, so zero the entire subband. */
> memset(&(pOut[first]), 0, subbWidth * 4);
s/4/sizeof(float)/
and &(pOut[first]) maybe by pOut + first
[...]
> static int decodeTonalComponents (GetBitContext *gb, int *numComponents, tonal_component *pComponent, int numBands)
> {
> int i,j,k,cnt;
> int component_count, components, coding_mode_selector, coding_mode, coded_values_per_component;
> int sfIndx, coded_values, max_coded_values, quant_step_index, coded_components;
> int band_flags[4], mantissa[8];
> float *pCoef;
> float scalefactor;
>
> component_count = 0;
> *numComponents = 0;
>
> components = get_bits(gb,5);
>
> /* no tonal components */
> if (components == 0)
> return 0;
>
> coding_mode_selector = get_bits(gb,2);
> if (coding_mode_selector == 2)
> return -1;
>
> coding_mode = coding_mode_selector & 1;
>
> for (i = 0; i < components; i++) {
> for (cnt = 0; cnt <= numBands; cnt++)
> band_flags[cnt] = get_bits1(gb);
>
> coded_values_per_component = get_bits(gb,3);
>
> quant_step_index = get_bits(gb,3);
> if (quant_step_index <= 1)
> return -1;
>
> if (coding_mode_selector == 3)
> coding_mode = get_bits1(gb);
>
> for (j = 0; j < (numBands + 1) * 4; j++) {
> if (band_flags[j >> 2] == 0)
> continue;
>
> coded_components = get_bits(gb,3);
>
> for (k=0; k<coded_components; k++) {
> sfIndx = get_bits(gb,6);
> pComponent[component_count].pos = j * 64 + (get_bits(gb,6));
> max_coded_values = 1024 - pComponent[component_count].pos;
> coded_values = coded_values_per_component + 1;
> coded_values = FFMIN(max_coded_values,coded_values);
>
> scalefactor = SFTable[sfIndx] * iMaxQuant[quant_step_index];
>
> readQuantSpectralCoeffs(gb, quant_step_index, coding_mode, mantissa, coded_values);
>
> pComponent[component_count].numCoefs = coded_values;
>
> /* inverse quant */
> pCoef = pComponent[k].coef;
> for (cnt = 0; cnt < coded_values; cnt++)
> pCoef[cnt] = (float)mantissa[cnt] * scalefactor;
senseless cast?
>
> component_count++;
> }
> }
> }
>
> *numComponents = component_count;
>
> return 0;
> }
hmm why isnt numComponents returned per return numComponents?
[...]
> static void addTonalComponents (float *pSpectrum, int numComponents, tonal_component *pComponent)
> {
> int cnt, i;
> float *pIn, *pOut;
>
> for (cnt = 0; cnt < numComponents; cnt++){
> pIn = &(pComponent[cnt].coef[0]);
:/
please grep for '&(' and clean them up
> pOut = &(pSpectrum[pComponent[cnt].pos]);
>
> for (i = 0; i < (pComponent[cnt].numCoefs); i++)
superfluous ()
[...]
> static void reverseMatrixing(float *su1, float *su2, int *pPrevCode, int *pCurrCode)
> {
> int band, nsample, s1, s2;
> float c1, c2;
> float mc1_l, mc1_r, mc2_l, mc2_r;
>
> for (band = 0; band < 4; band++) {
> s1 = pPrevCode[band];
> s2 = pCurrCode[band];
> nsample = 0;
>
> if (s1 != s2) {
> /* Selector value changed, interpolation needed. */
> mc1_l = matrixCoeffs[s1*2];
> mc1_r = matrixCoeffs[s1*2+1];
> mc2_l = matrixCoeffs[s2*2];
> mc2_r = matrixCoeffs[s2*2+1];
>
> /* Interpolation is done over the first eight samples. */
> for(; nsample < 8; nsample++) {
> c1 = su1[band*256+nsample];
> c2 = su2[band*256+nsample];
> c2 = c1 * INTERPOLATE(mc1_l,mc2_l,nsample) + c2 * INTERPOLATE(mc1_r,mc2_r,nsample);
> su1[band*256+nsample] = c2;
> su2[band*256+nsample] = c1 * 2.0 - c2;
> }
> }
>
> /* Apply the matrix without interpolation. */
> switch (s2) {
> case 0: /* M/S decoding */
> for (; nsample < 256; nsample++) {
> c1 = su1[band*256+nsample];
> c2 = su2[band*256+nsample];
> su1[band*256+nsample] = c2 * 2.0;
> su2[band*256+nsample] = (c1 - c2) * 2.0;
> }
> break;
>
> case 1:
> for (; nsample < 256; nsample++) {
> c1 = su1[band*256+nsample];
> c2 = su2[band*256+nsample];
> su1[band*256+nsample] = (c1 + c2) * 2.0;
> su2[band*256+nsample] = c2 * -2.0;
> }
> break;
> case 2:
> case 3:
> for (; nsample < 256; nsample++) {
> c1 = su1[band*256+nsample];
> c2 = su2[band*256+nsample];
> su1[band*256+nsample] = c1 + c2;
> su2[band*256+nsample] = c1 - c2;
> }
> break;
> default:
> assert(0);
> }
> }
as all accesses top su1/su2 in this function are +band*256 i suggest that
you simply add 256 to both in the loop which would simplify the code
[...]
> /* Convert number of subbands into number of MLT/QMF bands */
> numBands = ((subbandTab[numSubbands] + 255) >> 8) - 1;
i think thats the same as:
(subbandTab[numSubbands] - 1) >> 8
[...]
> int16_t* samples = (int16_t*)data;
unneeded cast
[...]
> if (q->channels == 1) {
> /* mono */
> for (i = 0; i<1024; i++)
> samples[i] = av_clip(lrintf(q->outSamples[i]), -32768, 32767);
> *data_size = 1024 * sizeof(int16_t);
> } else {
> /* stereo */
> for (i = 0; i < 1024; i++) {
> samples[i*2] = av_clip(round(q->outSamples[i]), -32768, 32767);
> samples[i*2+1] = av_clip(round(q->outSamples[1024+i]), -32768, 32767);
> }
lrintf() vs. round() inconsistency
[...]
> /* Take care of the codec-specific extradata. */
> if (avctx->extradata_size == 14) {
> /* Parse the extradata, WAV format */
> av_log(avctx,AV_LOG_DEBUG,"[0-1] %d\n",bytestream_get_le16(&edata_ptr)); //Unknown value always 1
> q->samples_per_channel = bytestream_get_le32(&edata_ptr);
> q->codingMode = bytestream_get_le16(&edata_ptr);
> av_log(avctx,AV_LOG_DEBUG,"[8-9] %d\n",bytestream_get_le16(&edata_ptr)); //Dupe of coding mode
> q->frame_factor = bytestream_get_le16(&edata_ptr); //Unknown always 1
> av_log(avctx,AV_LOG_DEBUG,"[12-13] %d\n",bytestream_get_le16(&edata_ptr)); //Unknown always 0
i think the code would be more readable if the 3 reads would be together and
the 3 av_log together after them
[...]
> if ((q->samples_per_frame != 1024) && (q->samples_per_frame != 2048)) {
superfluous ()
[...]
> for (i=-15 ; i<16 ; i++)
> gain_tab2[i+15] = powf (2.0, (float)i * -0.125);
senseless cast
[...]
iam fine with the patch except these
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070414/1db2ed56/attachment.pgp>
More information about the ffmpeg-devel
mailing list