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

[Marcelo Galvão Póvoa]

On 12 September 2010 06:28, Vitor Sessak <vitor1001 at gmail.com> wrote:
> 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...
>

Interesting, but how about instead of moving all the code to
amrwb_decode_frame(), we keep a function to decode the header since
there are more than one type of it. I wrote the simpler (and most
common I guess) "MIME/storage" header format but in case someone wants
to implement the others this way would be easier. See the patch I
attached.

-- 
Marcelo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: amrwb.patch
Type: application/octet-stream
Size: 152253 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100912/8af06cd7/attachment.obj>