[FFmpeg-devel] Patch for folding unaligned access routines into one file
Roman Shaposhnick
rvs
Sat Jul 28 00:11:04 CEST 2007
On Fri, 2007-07-27 at 22:49 +0100, M?ns Rullg?rd wrote:
> > p= FFMIN(p, end)-4;
> > - *state= be2me_32(unaligned32(p));
> > + *state= unaligned32_be(p);
>
> AV_RB32(p)
Good point.
> > - uint64_t a = unaligned64(&top[1]);
> > + uint64_t a = LD64(&top[1]);
>
> Looks OK, although LD/STxx really should be given proper AV_ (or FF_)
> prefixes.
The problem is they are currently being used without prefixes in
quite a few places. I'm not against changing that practice, but that
should be a separate patch IMO.
> > #ifndef ALT_BITSTREAM_WRITER
> > static inline void put_bits(PutBitContext *s, int n, unsigned int value)
> > {
> > @@ -412,7 +381,7 @@
> > const uint8_t *p=v;
> > return (((p[0]<<8) | p[1])<<16) | (p[2]<<8) | (p[3]);
> > #else
> > - return be2me_32( unaligned32(v)); //original
> > + return be2me_32( LD32(v)); //original
>
> AV_RB32(v)
There's even a bigger question at stake here: under what conditions
would CONFIG_ALIGN be defined? Why do we have this #ifdef to begin
with?
> > Index: ffmpeg/libavutil/intreadwrite.h
> > ===================================================================
> > --- ffmpeg/libavutil/intreadwrite.h (revision 9813)
> > +++ ffmpeg/libavutil/intreadwrite.h (working copy)
> > @@ -24,30 +24,43 @@
> >
> > #ifdef __GNUC__
> >
> > -struct unaligned_64 { uint64_t l; } __attribute__((packed));
> > -struct unaligned_32 { uint32_t l; } __attribute__((packed));
> > -struct unaligned_16 { uint16_t l; } __attribute__((packed));
> > +#define ACCESS(x) \
>
> Bad name.
Suggestions? (but then again, it gets undefined right after this block
so we shouldn't be spending too many brain cycles inventing a name for
it ;-)).
> > -#define LD16(a) (((const struct unaligned_16 *) (a))->l)
> > -#define LD32(a) (((const struct unaligned_32 *) (a))->l)
> > -#define LD64(a) (((const struct unaligned_64 *) (a))->l)
> > -
> > -#define ST16(a, b) (((struct unaligned_16 *) (a))->l) = (b)
> > -#define ST32(a, b) (((struct unaligned_32 *) (a))->l) = (b)
> > -#define ST64(a, b) (((struct unaligned_64 *) (a))->l) = (b)
>
> Why the rewrite?
The copy from the bitstream.h looks better to me:
* has an option for __DECC
* doesn't pollute namespace with struct unaligned_XX
* based on inlines
> > #else /* __GNUC__ */
> >
> > -#define LD16(a) (*((uint16_t*)(a)))
> > -#define LD32(a) (*((uint32_t*)(a)))
> > -#define LD64(a) (*((uint64_t*)(a)))
> > +#ifdef __DECC
> > +#define av_unaligned_type_specifier __unaligned
> > +#else
> > +#define av_unaligned_type_specifier
> > +#endif
>
> What happens if !__GNUC__ && !__DECC? You of all should know...
I do ;-) Unless I goofed up the result should be exactly the same
as the original one:
................................
return *(const uint##x##_t *) v;
................................
Which, of course, might cause SIGBUS, but that's a separate problem.
Or do you have something else in mind?
Thanks,
Roman.
More information about the ffmpeg-devel
mailing list