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

[Vitor Sessak]

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.

> 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