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

[Vitor Sessak]

On 09/12/2010 12:25 AM, Marcelo Galvão Póvoa wrote:
> On 11 September 2010 18:54, Vitor Sessak<vitor1001 at gmail.com>  wrote:
>> 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().
>>
>
> I thought about that, but how I would calculate expected_fr_size if I
> don't know of which mode the frame is? One solution would be doing
> this check inside ff_amr_bit_reorder() just after decoding the header.

Another solution would be to get rid of the unpack_bitstrem() function 
and move its code to amrwb_decode_frame(). It is already almost a 
wrapper around ff_amr_bit_reorder(). It would also allows to avoid 
checking (ctx->fr_cur_mode < MODE_SID) twice...

-Vitor