[FFmpeg-devel] [PATCH] hook up the atrac1 decoder

[Benjamin Larsson]

Vitor Sessak wrote:
> Benjamin Larsson wrote:
>> Well there is no patch I lied. But is the decoder in good enough shape
>> to be activated ?
> 
> Some comments...
> 
>>     int                 idwls[AT1_MAX_BFU];                 ///< the
>> word length indexes for each BFU
>>     int                 idsfs[AT1_MAX_BFU];                 ///< the
>> scalefactor indexes for each BFU
>>  
> 
> Used only in at1_unpack_dequant(), can be just allocated in the stack.
> Also fit in uint8_t for better cache usage.

Ok.

> 
>> static void at1_imdct(AT1Ctx *q, float *spec, float *out, int nbits,
>>                       int rev_spec)
>> {
>>     FFTContext* mdct_context;
>>     int transf_size = 1 << nbits;
>>
>>     mdct_context = &q->mdct_ctx[nbits - 5 - (nbits > 6)];
> 
> nit: merge init and declaration

Ok.

> 
>>         if (num_blocks == 1) {
>>             /* long blocks */
>>             at1_imdct(q, &q->spec[pos], &su->spectrum[0][ref_pos],
>> nbits, band_num);
>>             pos += block_size; // move to the next mdct block in the
>> spectrum
>>
>>             /* overlap and window long blocks */
>>             q->dsp.vector_fmul_window(q->bands[band_num],
>> &su->spectrum[1][ref_pos + band_samples - 16],
>>                                       &su->spectrum[0][ref_pos],
>> short_window, 0, 16);
>>             memcpy(q->bands[band_num] + 32, &su->spectrum[0][ref_pos +
>> 16], 240 * sizeof(float));
>>         } else {
>>             /* short blocks */
>>             float *prev_buf;
>>             start_pos = 0;
>>             prev_buf = &su->spectrum[1][ref_pos + band_samples - 16];
>>             for (; num_blocks != 0; num_blocks--) {
>>                 at1_imdct(q, &q->spec[pos], &su->spectrum[0][ref_pos +
>> start_pos], 5, band_num);
>>
>>                 /* overlap and window between short blocks */
>>                
>> q->dsp.vector_fmul_window(&q->bands[band_num][start_pos], prev_buf,
>>                                           &su->spectrum[0][ref_pos +
>> start_pos], short_window, 0, 16);
>>
>>                 prev_buf = &su->spectrum[0][ref_pos+start_pos + 16];
>>                 start_pos += 32; // use hardcoded block_size
>>                 pos += 32;
>>             }
>>         }
> 
> I think a good deal of code of both branches of the if() can be
> factorized out.

What code? IMO not much of the code in the 2 cases can be factored out
and still be logical and clear.

> 
>>      /* read in the spectral data and reconstruct MDCT spectrum of
>> this channel */
>>     for (band_num = 0; band_num < AT1_QMF_BANDS; band_num++) {
>>         for (bfu_num = bfu_bands_t[band_num]; bfu_num <
>> bfu_bands_t[band_num+1]; bfu_num++) {
>>             int pos;
>>
>>             int num_specs = specs_per_bfu[bfu_num];
>>             int word_len  = !!su->idwls[bfu_num] + su->idwls[bfu_num];
>>             float scale_factor = sf_table[su->idsfs[bfu_num]];
>>             bits_used    += word_len * num_specs; /* add number of
>> bits consumed by current BFU */
>                         ^^^^
> Nit: weird formatting

Ok.

> 
>>     for (ch = 0; ch < q->channels; ch++) {
>>         AT1SUCtx* su = &q->SUs[ch];
>>
>>         init_get_bits(&gb, &buf[212 * ch], 212 * 8);
>>
>>         /* parse block_size_mode, 1st byte */
>>         ret = at1_parse_bsm(&gb, su->log2_block_count);
>>         if (ret < 0)
>>             return ret;
>>
>>         ret = at1_unpack_dequant(&gb, su, q->spec);
>>         if (ret < 0)
>>             return ret;
>>
>>         ret = at1_imdct_block(su, q);
>>         if (ret < 0)
>>             return ret;
>>         at1_subband_synthesis(q, su, q->out_samples[ch]);
>>     }
> 
> I have a slightly preference for calling init_get_bits() just one per
> frame and skipping a few bits if needed.
> 
>> AVCodec atrac1_decoder = {
>>     .name = "atrac1",
>>     .type = CODEC_TYPE_AUDIO,
>>     .id = CODEC_ID_ATRAC1,
>>     .priv_data_size = sizeof(AT1Ctx),
>>     .init = atrac1_decode_init,
>>     .close = NULL,
> 
> I think a atrac1_decode_close() that calls ff_mdct_end() is missing.

True, I missed that. Will fix.

> 
> Somewhat unrelated to this thread, atrac_iqmf() is pretty time consuming
> and would gain a nice speed boost from SIMD (at least the inner loop
> evaluating s1 and s2)...

Patch welcome ;)

> 
> -Vitor

Thanks for the review. I'll activate the code after I've fixed these issues.

MvH
Benjamin Larsson