[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