[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