[FFmpeg-devel] Headers moving?

[Michael Niedermayer]

On Fri, Jan 04, 2008 at 03:21:27AM +0100, Aurelien Jacobs wrote:
> On Thu, 3 Jan 2008 01:16:35 +0100
> Michael Niedermayer <michaelni at gmx.at> wrote:
> 
> > On Thu, Jan 03, 2008 at 12:36:15AM +0100, Aurelien Jacobs wrote:
> > > On Wed, 2 Jan 2008 23:44:31 +0100
> > > Michael Niedermayer <michaelni at gmx.at> wrote:
> > > 
> > > > Hi
> > > > 
> > > > On Wed, Jan 02, 2008 at 08:25:08PM +0100, Diego 'Flameeyes' Petten? wrote:
> > > > > 
> > > > > May I try to poke again about moving headers around so that crc.h can be
> > > > > installed? I'd very much like to start using libavutil for xine-lib-1.2
> > > > > (and replacing sha1, base64 and crc16/32 implementations from xine-lib).
> > > > 
> > > > yes you may, what was the result of the last discussion?
> > > > IIRC we were at
> > > > * crc.h should be installed
> > > > * we need to get rid of the non const static (=API change) before that or
> > > >   there will be problems with at least x86_64 lavu-PIC + app-NOPIC on some
> > > >   systems
> > > > 
> > > > -> we either need to hardcode all CRC tables or use functions to return a
> > > > pointer to them
> > > > ideal would be if both were possible selectable with a ./configure switch
> > > 
> > > Hum... So you mean you want to have one function per CRC table
> > > (similar to the patch I posted some times ago) ?
> > > And also the hardcoded tables, if enabled in ./configure.
> > > The functions returning tables pointer would always be
> > > enabled, since they would be part of the API. And they
> > > would obviously return either a generated or a harcoded
> > > table depending on what was configured.
> > > 
> > > If that's what you want, I guess I will implement it.
> > 
> > no i dont want 1 function per crc, rather:
> > 
> > AVCRC *av_get_crc_table(unsigned int crc){
> >     int tab_id;
> > 
> >     switch(crc){
> >     case 0x1234567: tab_id=0; break;
> >     case 0x2222222: tab_id=1; break;
> >     default: return NULL;
> >     }
> > #ifndef HARDCODED_TABLES
> >     if(!table[tab_id][last_entry])
> >         av_crc_init(table[tab_id], crc);
> > #endif
> >     return table[tab_id];
> > }
> 
> Oh, I see. I like this proposition.
> Attached patch implements it and it looks nice.
> Was that what you meant ? IOW, patch OK ?

[...]
> +static struct {

> +    int inited;
> +    int le;
> +    int bits;

uint8_t will do for these 3

> +    uint32_t poly;
> +} av_crc_table_params[AV_CRC_MAX] = {
> +    [AV_CRC_8_ATM]      = { 0, 0,  8,       0x07 },

> +    [AV_CRC_16]         = { 0, 0, 16,     0x8005 },

that one is from ANSI


[...]
> +#ifndef CONFIG_HARDCODED_TABLES
> +    if (!av_crc_table_params[crc_id].inited) {
> +        if (av_crc_init(av_crc_table[crc_id],
> +                        av_crc_table_params[crc_id].le,
> +                        av_crc_table_params[crc_id].bits,
> +                        av_crc_table_params[crc_id].poly,
> +                        sizeof(av_crc_table[crc_id])) < 0)
> +            return NULL;
> +        av_crc_table_params[crc_id].inited = 1;
> +    }

i agree the inited is cleaner but 
av_crc_table[crc_id][257] and av_crc_table[crc_id][1024]
will work as well and need an instruction less


[...]
>  #endif /* FFMPEG_CRC_H */
> Index: libavcodec/ac3dec.c
> ===================================================================
> --- libavcodec/ac3dec.c	(revision 11389)
> +++ libavcodec/ac3dec.c	(working copy)
> @@ -1110,7 +1110,7 @@
>  
>      /* check for crc mismatch */
>      if(avctx->error_resilience > 0) {
> -        if(av_crc(av_crc8005, 0, &buf[2], s->frame_size-2)) {
> +        if(av_crc(av_crc_get_table(AV_CRC_16), 0, &buf[2], s->frame_size-2)) {
>              av_log(avctx, AV_LOG_ERROR, "frame CRC mismatch\n");
>              return -1;
>          }

my original idea was
static const AVCRC *crc= av_crc_get_table(AV_CRC_16);
but as av_crc_get_table is quite fast i guess thats not really needed

remainder of the patch looks ok

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080104/2699002d/attachment.pgp>