[Ffmpeg-devel] [PATCH] ATRAC3 decoder

Benjamin Larsson banan
Wed Apr 11 18:34:10 CEST 2007


Michael Niedermayer skrev:
> 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/+/-/
>   

Worked better, thanks for the simplification.

>
> [...]
>   
>> /* 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];
> }
>
>
>   

No, changed.

>   
>>     /* 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
>
>   

Fixed.

> [...]
>   
>>     /* for (i=0; i<512; i++) {
>>          pOutput[i] *= mdct_window[i]; */
>>
>> }
>>     
>
> something is wrong with the {} here
>
>
> [...]
>
>   

Fixed, removed.

>>         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
>
>   

Fixed.

>   
>>         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?
>
>   

No idea, ask Sony.

>
> [...]
>   
>>     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 ()
>
>   

Fixed.

>   
>>         } else {
>>             /* This subband was not coded, so zero the entire subband. */
>>             memset(&(pOut[first]), 0, subbWidth * 4);
>>     
>
> pOut+first
>
>   

Fixed.

> [...]
>   
>>                 coded_values = coded_values_per_component + 1;
>>                 if (coded_values > max_coded_values)
>>                     coded_values = max_coded_values;
>>     
>
> FFMIN()
>
>   

Fixed.

>   
>>                 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()
>
>   

Yes, they are quite similar but I don't see the benefit to factor out 
that code. It's just a few lines and a for loop.

>   
>>             }
>>         }
>>     }
>>
>>     *numComponents = component_count;
>>     
>
> why isnt return not used to return this value?
>
>   

Fixed in one function, the other the return is used.

> [...]
>   
>> 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 ()
>
>
>   

Fixed.

>   
>>     } 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 ()
>
>
>   

Fixed.

>>             /* interpolate */
>>             for (; nsample < startLoc; nsample++)
>>                 pOut[nsample] = ((pIn[nsample] * gain1) + pPrev[nsample]) * gain2;
>>     
>
> superfluous ()
>
>   

Fixed.

> [...]
>   
>>         for (cnt1 = 0; cnt1 < (pComponent[cnt].numCoefs); cnt1++)
>>         {
>>             *pOut += *pIn;
>>             pIn++;
>>             pOut++;
>>         }
>>     
>
> pOut[i] += pIn[i] or *pOut++ += *pIn++; is more readable IMHO
>
>   

Fixed.

> [...]
>   
>>             /* 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 ()
>   

Fixed.

> [...]
>   
>>     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 ()
>
>
>   

Fixed.

>>     decodeSpectrum (gb, pSnd->spectrum, &numSubbands);
>>
>>     /* Merge the decoded spectrum and tonal components. */
>>     addTonalComponents (&(pSnd->spectrum[0]), pSnd->numComponents, &(pSnd->components[0]));
>>     
>
> &(pSnd->spectrum[0]) ?!
>
>
>   

Fixed this and alot of others.

>>     /* 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? 
>
> [...]
>   

Removed it.

MvH
Benjamin Larsson
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: atrac3.c
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070411/74575e22/attachment.asc>



More information about the ffmpeg-devel mailing list