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

[Michael Niedermayer]

On Thu, Dec 13, 2007 at 12:09:08AM +0100, Aurelien Jacobs wrote:
> 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)

i would prefer not to increase the complexity nor size of the crc code
iam fine with just droping shared lib support for the crc code if that is
the only alternative
i also would prefer to avoid per CRC polynom functions, especially inlined
ones, this goes completely against the goal of libavutil to be small and
lightweight

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

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- 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/20071213/25ecdd56/attachment.pgp>