[Ffmpeg-devel] [PATCH] ATRAC3 decoder

Benjamin Larsson banan
Tue Apr 17 23:02:50 CEST 2007


Michael Niedermayer wrote:
> Hi
>
>   
> [...]
>   
>> 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
>
>
> [...]
>
>   

Fixed.

>> /* Regarding multiple instances. */
>>     
>
> maybe add a "FIXME check if this is ok" here
>   

I checked it, the mdct tables will be inited once per instance. So that
shouldn't cause any problems.

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

Fixed.

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

Fixed.

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

Fixed.

>
> [...]
>   
>>             /* Inverse quantize the coefficients. */
>>             for (pIn=mantissas ; first<last; first++, pIn++)
>>                 pOut[first] = (float)*pIn * SF;
>>     
>
> the cast seems useless
>
>   

Fixed.

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

Fixed.

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

Fixed.

>   
>>                 component_count++;
>>             }
>>         }
>>     }
>>
>>     *numComponents = component_count;
>>
>>     return 0;
>> }
>>     
>
> hmm why isnt numComponents returned per return numComponents?
>   

The return is used by the error state.

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

Fixed.

>   
>>         pOut = &(pSpectrum[pComponent[cnt].pos]);
>>
>>         for (i = 0; i < (pComponent[cnt].numCoefs); i++)
>>     
>
> superfluous ()
>
>   

Fixed.

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

Fixed.

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

Fixed.

> [...]
>   
>>     int16_t* samples = (int16_t*)data;
>>     
>
> unneeded cast
>
>   
Fixed.
> [...]
>   
>>     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
>
>   

Fixed.

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

Fixed.

>
> [...]
>   
>>     if ((q->samples_per_frame != 1024) && (q->samples_per_frame != 2048)) {
>>     
>
> superfluous ()
>   

Fixed.

>
> [...]
>   
>>     for (i=-15 ; i<16 ; i++)
>>         gain_tab2[i+15] = powf (2.0, (float)i * -0.125);
>>     
>
> senseless cast
>   

Fixed.

>
> [...]
>
> iam fine with the patch except these
>
> [...]
>   

Commited.


MvH
Benjamin Larsson




More information about the ffmpeg-devel mailing list