[FFmpeg-devel] FreeBSD libamr.c block_size error

[Benoit Fouet]

Hi,

John Fitzgerald wrote:
> On Thu, Feb 28, 2008 at 8:16 AM, Reimar D?ffinger
> <Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:
>   
>> On Thu, Feb 28, 2008 at 01:33:43PM +0100, Michael Niedermayer wrote:
>>     
>>> On Wed, Feb 27, 2008 at 10:23:27AM -0500, John J Fitzgerald wrote:
>>>       
>>>> Ah, right, thanks.
>>>>
>>>> The diff -urN patch is named freebsd_libamr.patch and can be downloaded
>>>> here:
>>>>
>>>> http://www.mediafire.com/?dnl0z5xrrae
>>>>
>>>> Contents of freebsd_libamr.patch:
>>>>
>>>> --- libavcodec/libamr.c.orig    Wed Feb 27 11:21:44 2008
>>>> +++ libavcodec/libamr.c Wed Feb 27 11:21:58 2008
>>>> @@ -663,6 +663,7 @@
>>>>      }
>>>>
>>>>      mode = (amrData[0] >> 3) & 0x000F;
>>>> +    static short block_size[16]={18, 23, 33, 37, 41, 47, 51, 59, 61, 6, 6,
>>>> 0, 0, 0, 1, 1};
>>>>         
>>> The patch is mangled, attach it properly!
>>> Also const is missing and it should be unit8_t.
>>>       
>> uint8_t actually. And the variable declaration should not be in the
>> middle of the code either.
>>
>>     
>
> How's this diff -urN patch look? I hardly ever code in C or create
> patches, so please look it over. I prepended the block_size variables
> with nb_ and wb_ respectively because I think (?) that static
> variables are program-wide, so if that's the case, I wanted to avoid
> any conflict. But I don't know if I even declared the variables
> correctly.
>
> Cheers,
>
> JJ
>   
> ------------------------------------------------------------------------
>
> --- libavcodec/libamr.c.orig	Wed Feb 27 11:21:44 2008
> +++ libavcodec/libamr.c	Thu Feb 28 10:34:14 2008
> @@ -444,14 +444,14 @@
>  {
>      AMRContext *s = avctx->priv_data;
>      uint8_t*amrData=buf;
> -    static short block_size[16]={ 12, 13, 15, 17, 19, 20, 26, 31, 5, 0, 0, 0, 0, 0, 0, 0 };
> +    static const uint8_t nb_block_size[16]={ 12, 13, 15, 17, 19, 20, 26, 31, 5, 0, 0, 0, 0, 0, 0, 0 };
>   

this should be in a separate patch (and you don't need to rename the
variable)

>      enum Mode dec_mode;
>      int packet_size;
>  
>      /* av_log(NULL,AV_LOG_DEBUG,"amr_decode_frame buf=%p buf_size=%d frameCount=%d!!\n",buf,buf_size,s->frameCount); */
>  
>      dec_mode = (buf[0] >> 3) & 0x000F;
> -    packet_size = block_size[dec_mode]+1;
> +    packet_size = nb_block_size[dec_mode]+1;
>  
>      if(packet_size > buf_size) {
>          av_log(avctx, AV_LOG_ERROR, "amr frame too short (%u, should be %u)\n", buf_size, packet_size);
> @@ -652,6 +652,7 @@
>              void *data, int *data_size,
>              uint8_t * buf, int buf_size)
>  {
> +    static const uint8_t wb_block_size[16]= {18, 23, 33, 37, 41, 47, 51, 59, 61, 6, 6, 0, 0, 0, 1, 1};
>   

you don't need to rename the variable neither

>      AMRWBContext *s = avctx->priv_data;
>      uint8_t*amrData=buf;
>      int mode;
> @@ -663,7 +664,7 @@
>      }
>  
>      mode = (amrData[0] >> 3) & 0x000F;
> -    packet_size = block_size[mode];
> +    packet_size = wb_block_size[mode];
>  
>      if(packet_size > buf_size) {
>          av_log(avctx, AV_LOG_ERROR, "amr frame too short (%u, should be %u)\n", buf_size, packet_size+1);
>   
>   

Otherwise, I guess the patch is ok

-- 
Benoit Fouet
Purple Labs S.A.
www.purplelabs.com