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

[Vitor Sessak]

Benjamin Larsson wrote:
> 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.

The calls to at1_imdct() and vector_fmul_window() are very similar. They 
are exactly the same if block_size == 32 and nbits == 5 when num_blocks 
== 1. Note also that the for() loop of the else{} branch will be run 
only once when num_blocks == 1, as it should. Of course, there will 
always be a if() for the memcpy().

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

Ok, I thought that the number of remaining bits was just some fixed 
constant.

-Vitor