[Ffmpeg-devel] [PATCH] ATRAC3 decoder
Michael Niedermayer
michaelni
Wed Apr 11 15:15:48 CEST 2007
Hi
On Tue, Apr 10, 2007 at 07:55:49PM +0200, Benjamin Larsson wrote:
> Hi, better late then never. Fixed all issues from the first review also.
[...]
> >> /* Generate the scale factors. */
> >> for (i=0 ; i<64 ; i++)
> >> SFTable[i] = powf(2.0, (i + 2) / 3 - 5) * mantissaTab[(i + 2) % 3];
> >
> > pow(2.0, (i + 15) / 3.0);
> >
> > and remove the useless mantissaTab
>
> The scalefactors are not even close to being equal if I do.
s/+/-/
[...]
> /* quadrature mirror synthesis filter */
> static void iqmf (float *inlo, float *inhi, unsigned int nIn, float *pOut, float *delayBuf, float *temp, float *pWindow)
> {
> int cnt, j;
> float *p1, *p3;
> float s1, s2;
>
> memcpy(temp, delayBuf, 46*sizeof(float));
>
> p3 = temp + 46;
>
> /* loop1 */
> for (cnt = nIn / 2; cnt != 0; cnt--) {
> p3[0] = inlo[0] + inhi[0];
> p3[1] = inlo[0] - inhi[0];
> p3[2] = inlo[1] + inhi[1];
> p3[3] = inlo[1] - inhi[1];
>
> inlo += 2;
> inhi += 2;
> p3 += 4;
> }
any reason why its not the obvious:
for(i=0; i<nIn; i++){
p3[2*i+0] = inlo[i] + inhi[i];
p3[2*i+1] = inlo[i] - inhi[i];
}
or
for(i=0; i<nIn; i+=2){
p3[2*i+0] = inlo[i ] + inhi[i ];
p3[2*i+1] = inlo[i ] - inhi[i ];
p3[2*i+2] = inlo[i+1] + inhi[i+1];
p3[2*i+3] = inlo[i+1] - inhi[i+1];
}
>
> /* loop2 */
> p1 = temp;
> for (j = nIn; j != 0; j--) {
> s1 = 0.0;
> s2 = 0.0;
as s1, s2 are not used outside this loop
float s1= 0.0 ...
would be simpler and shorter lifetime of variables likely allows more
optimization to be done by gcc
[...]
> /* for (i=0; i<512; i++) {
> pOutput[i] *= mdct_window[i]; */
>
> }
something is wrong with the {} here
[...]
> pTable = (uint8_t*)decTables[selector];
what is that cast doing here? is pTable or decTables type wrong?
either way a cast is not the correct solution, fix the types
>
> if (selector != 1) {
> for (cnt = 0; cnt < numCodes; cnt++) {
> huffSymb = get_vlc2(gb, spectral_coeff_tab[selector-1].table, spectral_coeff_tab[selector-1].bits, 3);
> huffSymb += 1;
> code = pTable[huffSymb >> 1];
> if (huffSymb & 1)
> code = -code;
> mantissas[cnt] = code;
> }
whats the sense in this decTables[] based remapping? why isnt the
spectral_coeff_tab in the proper order?
[...]
> for (cnt = 0; cnt <= numSubbands; cnt++) {
> first = subbandTab[cnt];
> last = subbandTab[cnt+1];
>
> subbWidth = last - first;
>
> if (subband_vlc_index[cnt] != 0) {
> /* Decode spectral coefficients for this subband. */
> /* TODO: This can be done faster is several blocks share the
> * same VLC selector (subband_vlc_index) */
> readQuantSpectralCoeffs (gb, subband_vlc_index[cnt], codingMode, mantissas, subbWidth);
>
> /* Decode the scale factor for this subband. */
> SF = SFTable[SF_idxs[cnt]] * iMaxQuant[subband_vlc_index[cnt]];
>
> /* Inverse quantize the coefficients. */
> for (pIn=mantissas ; first<last; first++, pIn++)
> pOut[first] = ((float)*pIn * SF);
superflouos ()
> } else {
> /* This subband was not coded, so zero the entire subband. */
> memset(&(pOut[first]), 0, subbWidth * 4);
pOut+first
[...]
> coded_values = coded_values_per_component + 1;
> if (coded_values > max_coded_values)
> coded_values = max_coded_values;
FFMIN()
>
> 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;
>
> component_count++;
this code looks duplicated relative to part of decodeSpectrum()
> }
> }
> }
>
> *numComponents = component_count;
why isnt return not used to return this value?
[...]
> static void gainCompensateAndOverlap (float *pIn, float *pPrev, float *pOut, gain_info *pGain1, gain_info *pGain2)
> {
> /* gain compensation function */
> float gain1, gain2, gain_inc;
> int cnt, numdata, nsample, startLoc, endLoc;
>
>
> if (pGain2->num_gain_data == 0)
> gain1 = 1.0;
> else
> gain1 = gain_tab1[pGain2->levcode[0]];
>
> if (pGain1->num_gain_data == 0) {
> for (cnt = 0; cnt < 256; cnt++)
> pOut[cnt] = (pIn[cnt] * gain1) + pPrev[cnt];
superfluous ()
> } else {
> numdata = pGain1->num_gain_data;
> pGain1->loccode[numdata] = 32;
> pGain1->levcode[numdata] = 4;
>
> nsample = 0; // current sample = 0
>
> for (cnt = 0; cnt < numdata; cnt++) {
> startLoc = pGain1->loccode[cnt] * 8;
> endLoc = startLoc + 8;
>
> gain2 = gain_tab1[pGain1->levcode[cnt]];
> gain_inc = gain_tab2[(pGain1->levcode[cnt+1] - pGain1->levcode[cnt])+15];
superfluous ()
>
> /* interpolate */
> for (; nsample < startLoc; nsample++)
> pOut[nsample] = ((pIn[nsample] * gain1) + pPrev[nsample]) * gain2;
superfluous ()
[...]
> for (cnt1 = 0; cnt1 < (pComponent[cnt].numCoefs); cnt1++)
> {
> *pOut += *pIn;
> pIn++;
> pOut++;
> }
pOut[i] += pIn[i] or *pOut++ += *pIn++; is more readable IMHO
[...]
> /* 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));
superfluous ()
[...]
> result = decodeGainControl (gb, &(pSnd->gainBlock[pSnd->gcBlkSwitch]), pSnd->bandsCoded);
> if (result) return result;
>
> result = decodeTonalComponents (gb, &(pSnd->numComponents), &(pSnd->components[0]), pSnd->bandsCoded);
> if (result) return (result);
supefluous ()
>
> decodeSpectrum (gb, pSnd->spectrum, &numSubbands);
>
> /* Merge the decoded spectrum and tonal components. */
> addTonalComponents (&(pSnd->spectrum[0]), pSnd->numComponents, &(pSnd->components[0]));
&(pSnd->spectrum[0]) ?!
>
>
> /* Convert number of subbands into number of MLT/QMF bands
> * CAUTION: This may not match the bandsCoded parameter! */
> numBands = ((subbandTab[numSubbands] + 255) >> 8) - 1;
>
> /* Check if the first 8 coefficients in the not-coded band are zero,
> * otherwise increment numBands. Not used at the moment */
>
> /* if ((numBands+1) < 4) {
> for (i = 0, pCoef = &pSnd -> spectrum[(numBands+1)*256]; i < 8; i++) {
> if (pCoef[i] != (float)0.0) {
> numBands++;
> break;
> }
> }
> }*/
why is this out commented?
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- 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/20070411/5eb47e70/attachment.pgp>
More information about the ffmpeg-devel
mailing list