[FFmpeg-devel] [RFC] Installing libavutil's crc.h?

Aurelien Jacobs aurel
Thu Dec 13 00:09:08 CET 2007


Michael Niedermayer wrote:

> On Wed, Dec 12, 2007 at 11:22:26PM +0100, Aurelien Jacobs wrote:
> > Michael Niedermayer wrote:
> > 
> > > On Wed, Dec 12, 2007 at 01:48:55AM +0100, Aurelien Jacobs wrote:
> > > > Diego Biurrun wrote:
> > > > 
> > > > > On Tue, Dec 11, 2007 at 11:23:26PM +0100, Michael Niedermayer
> > > > > wrote:
> > > > > > On Tue, Dec 11, 2007 at 07:35:20PM +0100, Diego Biurrun
> > > > > > wrote:
> > > > > > > On Tue, Dec 11, 2007 at 04:30:25PM +0100, Diego
> > > > > > > 'Flameeyes' Petten? wrote:
> > > > > > > > 
> > > > > > > > So okay for bswap.h there's the configh dependency that
> > > > > > > > disallows from installing it (but I'm still thinking
> > > > > > > > how to get over the dep, maybe I'll provide a patch to
> > > > > > > > use unifdef).
> > > > > > > > 
> > > > > > > > For crc.h is there something else stopping it from being
> > > > > > > > installed?
> > > > > > > 
> > > > > > > This sounds backwards to me.  Headers should not be
> > > > > > > installed without a good reason.
> > > > > > 
> > > > > > is it not enogh that its needed to use the crc code in
> > > > > > libavuitl? its a mere oversight that it isnt installed
> > > > > 
> > > > > It is a good enough reason.
> > > > > 
> > > > > I'm thinking that we could solve the LIBAVUTIL_VERSION_INT
> > > > > issue by switching the condition around, i.e.:
> > > > > 
> > > > > #if LIBAVUTIL_VERSION_INT  < (50<<16)
> > > > > extern AVCRC *av_crcEDB88320;
> > > > > extern AVCRC *av_crc04C11DB7;
> > > > > extern AVCRC *av_crc8005    ;
> > > > > extern AVCRC *av_crc07      ;
> > > > > #else
> > > > > extern AVCRC av_crcEDB88320[];
> > > > > extern AVCRC av_crc04C11DB7[];
> > > > > extern AVCRC av_crc8005    [];
> > > > > extern AVCRC av_crc07      [];
> > > > > #endif
> > > > 
> > > > If crc.h wasn't part of public API up to now, I wonder why do we
> > > > try to keep stable API/ABI ?
> > > > Is it to ensure compatibility between lavu and lavf compiled
> > > > from different svn revision ? (I thought this wasn't
> > > > supported ?)
> > > > 
> > > > So a simple solution to solve the LIBAVUTIL_VERSION_INT issue
> > > > would be to just drop the legacy version of those declaration.
> > > 
> > > hmm, i guess thats ok as well
> > 
> > I now realize that we are talking about making those extern tables
> > part of the public API. Exactly what we are trying to avoid right
> > now... So I think the right thing to do is to make those tables
> > static and provide another API to use them. Right ?
> > See attached patch.
> > 
> > Aurel
> 
> > Index: libavutil/crc.c
> > ===================================================================
> > --- libavutil/crc.c	(revision 11206)
> > +++ libavutil/crc.c	(working copy)
> > @@ -21,17 +21,11 @@
> >  #include "common.h"
> >  #include "crc.h"
> >  
> > -#if LIBAVUTIL_VERSION_INT  < (50<<16)
> > -AVCRC *av_crcEDB88320;
> > -AVCRC *av_crc04C11DB7;
> > -AVCRC *av_crc8005    ;
> > -AVCRC *av_crc07      ;
> > -#else
> > -AVCRC av_crcEDB88320[257];
> > -AVCRC av_crc04C11DB7[257];
> > -AVCRC av_crc8005    [257];
> > -AVCRC av_crc07      [257];
> > -#endif
> > +static AVCRC av_crcEDB88320[257];
> > +static AVCRC av_crc04C11DB7[257];
> > +static AVCRC av_crc1021    [257];
> > +static AVCRC av_crc8005    [257];
> > +static AVCRC av_crc07      [257];
> 
> 
> /**
>  * Inits a crc table.
>  * @param ctx must be an array of sizeof(AVCRC)*257 or sizeof(AVCRC)*1024
>                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Ok, but currently the code already contains av_crcEDB88320[257].
This don't prevent using sizeof(AVCRC)*1024 but not with those tables.

> and besides this i do not like the patch at all, it would
> significantly increase the call overhead of the crc functions

If that's the only thing which bother you, this slightly modified
patch should fix the issue. It may even lead to a very small
performance improvement... (but a slightly bigger object file)

> why dont you just hardcode the tables then broken linkers can copy
> them around as they see fit? that also would allow them to be
> actually shared

That's another option. (which will also slightly enlarge object file)
If you prefer this one, I can give it a try.

Aurel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: crc_func2.diff
Type: text/x-patch
Size: 8707 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071213/31212d8a/attachment.bin>



More information about the ffmpeg-devel mailing list