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

In this case the frame always start there but it's not sure when the
parsing of the old frame ends. So then I would need to add some
calculation to skip to the correct position which I think is not nice as
I already know where to start.


> 
>> 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.
> 
> 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)...
> 
> -Vitor


MvH
Benjamin Larsson