[FFmpeg-soc] [PATCH] AMR-WB Decoder

[Vitor Sessak]

On 09/11/2010 06:08 PM, Marcelo Galvão Póvoa wrote:
> 2010/9/11 Marcelo Galvão Póvoa<marspeoplester at gmail.com>:
>> On 11 September 2010 12:37, Vitor Sessak<vitor1001 at gmail.com>  wrote:
>>> On 09/11/2010 04:47 PM, Marcelo Galvão Póvoa wrote:
>>>>
>>>> On 9 September 2010 16:11, Vitor Sessak<vitor1001 at gmail.com>    wrote:
>>>>>
>>>>> On 09/07/2010 02:05 AM, Marcelo Galvão Póvoa wrote:
>>>>>>
>>>>>> On 6 September 2010 11:31, Vitor Sessak<vitor1001 at gmail.com>      wrote:
>>>>>>>
>>>>>>> On 09/06/2010 02:46 AM, Marcelo Galvăo Póvoa wrote:
>>>>>>>>
>>>
>>> [...]
>>>
>>>>>>> Can celp_filters.c:ff_celp_circ_addf() simplify this?
>>>>>>>
>>>>>>
>>>>>> I already gave some thought to that but I couldn't figure out how to
>>>>>> use ff_celp_circ_addf() there. I wrote the algorithm the simplest way
>>>>>> I could think of.
>>>>>
>>>>> Try
>>>>>
>>>>>         for (i = 0; i<    AMRWB_SFR_SIZE; i++)
>>>>>             if (fixed_vector[i])
>>>>>                 ff_celp_circ_addf(buf, buf, coef, i, fixed_vector[i],
>>>>>                                   AMRWB_SFR_SIZE);
>>>>>
>>>>>
>>>>
>>>> That's right indeed, thanks!
>>>
>>>> /*
>>>>   * AMR wideband decoder
>>>>   * Copyright (c) 2010 Marcelo Galvao Povoa
>>>>   *
>>>>   * This file is part of FFmpeg.
>>>>   *
>>>>   * FFmpeg is free software; you can redistribute it and/or
>>>>   * modify it under the terms of the GNU Lesser General Public
>>>>   * License as published by the Free Software Foundation; either
>>>>   * version 2.1 of the License, or (at your option) any later version.
>>>>   *
>>>>   * FFmpeg is distributed in the hope that it will be useful,
>>>>   * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>   * MERCHANTABILITY or FITNESS FOR A particular PURPOSE.  See the GNU
>>>>   * Lesser General Public License for more details.
>>>>   *
>>>>   * You should have received a copy of the GNU Lesser General Public
>>>>   * License along with FFmpeg; if not, write to the Free Software
>>>>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>>>> 02110-1301 USA
>>>>   */
>>>>
>>>> #include "libavutil/lfg.h"
>>>>
>>>> #include "avcodec.h"
>>>> #include "get_bits.h"
>>>> #include "lsp.h"
>>>> #include "celp_math.h"
>>>> #include "celp_filters.h"
>>>> #include "acelp_filters.h"
>>>> #include "acelp_vectors.h"
>>>> #include "acelp_pitch_delay.h"
>>>>
>>>> #include "amrwbdata.h"
>>>
>>> No need to include amr.h?
>>>
>>
>> Oops, fixed
>>
>
> Sorry, I forgot to define the table type before the include. Fixed

Just one last think I just saw:


>     ctx->fr_cur_mode = unpack_bitstream(ctx, buf, buf_size);
>     expected_fr_size = ((cf_sizes_wb[ctx->fr_cur_mode] + 7) >> 3) + 1;
>
>     if (buf_size < expected_fr_size) {
>         av_log(avctx, AV_LOG_ERROR,
>             "Frame too small (%d bytes). Truncated file?\n", buf_size);
>         *data_size = 0;
>         return buf_size;
>     }

This check is not useful like that. Imagine that buf_size == 1. Then, 
unpack_bitstream will try to read buf[2] and will segfault. This check 
should be done before the call of ff_amr_bit_reorder().

-Vitor