[FFmpeg-devel] Headers moving?

[Aurelien Jacobs]

On Fri, 4 Jan 2008 12:03:15 +0100
Michael Niedermayer <michaelni at gmx.at> wrote:

> 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

Sure.

> > +    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

OK.

> [...]
> > +#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]

I guess you meant:
av_crc_table[crc_id][256] and av_crc_table[crc_id][1023]
:-)

> will work as well and need an instruction less

Right. Done.

Applied with suggested changes.

Now I guess I can commit original patch (installing crc.h) ?

Aurel