[FFmpeg-devel] [PATCH] QCELP decoder

[Kenan Gillet]

On Nov 21, 2008, at 3:33 AM, Michael Niedermayer wrote:

> On Thu, Nov 20, 2008 at 08:53:37PM -0800, Kenan Gillet wrote:
>> On Thu, Nov 20, 2008 at 5:08 PM, Michael Niedermayer <michaelni at gmx.at 
>> > wrote:
>>> On Thu, Nov 20, 2008 at 10:46:49AM -0800, Kenan Gillet wrote:
>>>> On Sat, Nov 15, 2008 at 3:10 PM, Michael Niedermayer <michaelni at gmx.at 
>>>> > wrote:
>>>>> On Fri, Nov 14, 2008 at 03:32:51PM -0800, Kenan Gillet wrote:
>>>>>> Hi,
>>>>>> On Fri, Nov 14, 2008 at 2:27 PM, Michael Niedermayer <michaelni at gmx.at 
>>>>>> > wrote:
>>>>>>> On Fri, Nov 14, 2008 at 12:17:50PM -0800, Kenan Gillet wrote:
>>>>>>>>
>>>>>>>> On Nov 14, 2008, at 2:14 AM, Michael Niedermayer wrote:
>>
>> [...]
>>>>>> +
>>>>>> +/// @defgroup qcelp_unpacked_data_frame QCELP unpacked data  
>>>>>> frame
>>>>>> +/// @{
>>>>>> +    uint8_t           cbsign[16];
>>>>>
>>>>>> +    uint8_t           cbgain[16];
>>>>>> +    uint8_t           cindex[16];
>>>>>> +    uint8_t           plag[4];
>>>>>> +    uint8_t           pfrac[4];
>>>>>> +    uint8_t           pgain[4];
>>>>>> +    uint8_t           lspv[10];               /*!< LSP for  
>>>>>> RATE_OCTAVE, LSPV for other rates */
>>>>>> +    uint8_t           reserved;               /*!< on all but  
>>>>>> rate 1/2 packets */
>>>>>> +/// @}
>>>>>> +
>>>>>> +    uint8_t           erasure_count;
>>>>>> +    uint8_t           octave_count;           /*!< count the  
>>>>>> consecutive RATE_OCTAVE frames */
>>>>>> +    float             prev_lspf[10];
>>>>>> +    float             predictor_lspf[10];     /*!< LSP  
>>>>>> predictor,
>>>>>> +                                                  only use for  
>>>>>> RATE_OCTAVE and I_F_Q */
>>>>>> +    float             pitch_synthesis_filter_mem[303];
>>>>>> +    float             pitch_pre_filter_mem[303];
>>>>>> +    float             rnd_fir_filter_mem[180];
>>>>>> +    float             formant_mem[170];
>>>>>> +    float             last_codebook_gain;
>>>>>> +    int               prev_g1[2];
>>>>>> +    int               prev_framerate;
>>>>>> +    float             prev_pitch_gain[4];
>>>>>> +    uint8_t           prev_pitch_lag[4];
>>>>>> +    uint16_t          first16bits;
>>>>>> +} QCELPContext;
>>>>>
>>>>> i somehow think this struct does not belong in qcelpdata.h
>>>>> but rather qcelpdec.c
>>>>>
>>>>
>>>> I agree, but it is needed by the unpacking table.
>>>> should I just put the struct in qcelpdec.c and include  
>>>> qcelpdata.h after ?
>>>
>>> ok (maybe diego will want it to be renamed to .c though ...)
>>
>> ok to keep it the way it is?
>>
>> or to move the QCELPContext in qcelpdec.c ?
>
> ok to move QCELPContext to qcelpdec.c
>
> also if someone wants to rename qcelpdata.h to qcelpdata.c iam ok  
> with that
>
> (and no, iam not truely happy about the result if someone has a  
> better idea,
> but keeping it .h will likely break check headers)
> spliting the structs though may be an alternative ...

I thought and still think  splitting of the struct would be cleaner.
Would QCELPFrame ok for the name of struct ?


>
>>
>>>
>>>
>>> [...]
>>>>>
>>>>> [...]
>>>>>
>>>>>
>>>>>> static void warn_insufficient_frame_quality(AVCodecContext  
>>>>>> *avctx,
>>>>>>                                             const char  
>>>>>> *message) {
>>>>>>     av_log(avctx, AV_LOG_WARNING, "Frame #%d, IFQ: %s\n", avctx- 
>>>>>> >frame_number, message);
>>>>>> }
>>>>>>
>>>>>> +static int qcelp_decode_frame(AVCodecContext *avctx,
>>>>>> +                              void *data,
>>>>>> +                              int *data_size,
>>>>>> +                              uint8_t *buf,
>>>>>> +                              const int buf_size) {
>>>>>> +    QCELPContext      *q = avctx->priv_data;
>>>>>> +    float             *outbuffer = data;
>>>>>> +    int               i;
>>>>>> +    float             quantized_lspf[10], lpc[10];
>>>>>> +    float             gain[16];
>>>>>> +    float             *formant_mem;
>>>>>> +
>>>>>> +    if ((q->framerate = determine_framerate(avctx, buf_size,  
>>>>>> &buf)) == I_F_Q) {
>>>>>> +        warn_insufficient_frame_quality(avctx, "Framerate  
>>>>>> cannot be determined.");
>>>>>> +        goto erasure;
>>>>>> +    }
>>>>>> +
>>>>>> +    if (q->framerate == RATE_OCTAVE &&
>>>>>> +       (q->first16bits = AV_RB16(buf)) == 0xFFFF) {
>>>>>> +        warn_insufficient_frame_quality(avctx, "Framerate is  
>>>>>> 1/8 and first 16 bits are on.");
>>>>>> +        goto erasure;
>>>>>> +    }
>>>>>> +
>>>>>> +    if (q->framerate > SILENCE) {
>>>>>> +        const QCELPBitmap *bitmaps     =  
>>>>>> qcelp_unpacking_bitmaps_per_rate[q->framerate];
>>>>>> +        const QCELPBitmap *bitmaps_end =  
>>>>>> qcelp_unpacking_bitmaps_per_rate[q->framerate]
>>>>>> +                                       + qcelp_bits_per_rate[q- 
>>>>>> >framerate];
>>>>>> +        uint8_t           *unpacked_data = (uint8_t *)q;
>>>>>> +
>>>>>
>>>>>> +        init_get_bits(&q->gb, buf, qcelp_bits_per_rate[q- 
>>>>>> >framerate]);
>>>>>
>>>>> qcelp_bits_per_rate does not seem correct here nor does its name  
>>>>> seem
>>>>> to match what it contains
>>>>
>>>>
>>>> yes changed back to buf_size.
>>>>
>>>> what about changing qcelp_bits_per_rate  to  
>>>> qcelp_unpacking_bitmaps_per_rate_len
>>>> because it really is the len of the unpacking bitmaps, or do you  
>>>> have
>>>> a better suggestion ?
>>>
>>> the suggested name is too long IMHO
>>
>> qcelp_unpacking_bitmaps_lengths ?
>
> ok
>

done