[FFmpeg-devel] FreeBSD libamr.c block_size error

[John Fitzgerald]

On Thu, Feb 28, 2008 at 9:46 AM, Benoit Fouet
<benoit.fouet at purplelabs.com> wrote:
> 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

I see, thanks. Is it good enough to get into the next nightly snapshot
or should I submit two new patches without renaming of the block_size
variable? And I assume that changing the nb block_size to a uint8_t is
OK.