[FFmpeg-devel] armeb fix for mpegaudio

[Måns Rullgård]

Siarhei Siamashka wrote:
> On Friday 01 August 2008, M?ns Rullg?rd wrote:
>> matthieu castet <castet.matthieu at free.fr> writes:
>> > Hi,
>> >
>> > this patch fix mp2 regression test on arm big endian.
>> >
>> > Matthieu
>> >
>> > PS : as Mans said dct is broken as you can see with the video
>> > regression failure.
>> >
>> > PS2 : test were done with qemu-armeb
>> > Index: libavcodec/armv4l/mathops.h
>> > ===================================================================
>> > --- libavcodec/armv4l/mathops.h	(r?vision 14493)
>> > +++ libavcodec/armv4l/mathops.h	(copie de travail)
>> > @@ -22,6 +22,14 @@
>> >  #ifndef FFMPEG_ARMV4L_MATHOPS_H
>> >  #define FFMPEG_ARMV4L_MATHOPS_H
>> >
>> > +#ifdef WORDS_BIGENDIAN
>> > +#define LOW_WORD 1
>> > +#define HIGH_WORD 0
>> > +#else
>> > +#define LOW_WORD 0
>> > +#define HIGH_WORD 1
>> > +#endif
>> > +
>>
>> I can imagine those being useful in other places.  Perhaps defining
>> them in a more central location would be worthwhile.
>>
>> >  #ifdef FRAC_BITS
>> >  #   define MULL(a, b) \
>> >          ({  int lo, hi;\
>> > @@ -52,7 +60,7 @@
>> >  {
>> >      union { uint64_t x; unsigned hl[2]; } x;
>> >      asm ("smull %0, %1, %2, %3"
>> > -         : "=r"(x.hl[0]), "=r"(x.hl[1]) : "r"(a), "r"(b));
>> > +         : "=r"(x.hl[LOW_WORD]), "=r"(x.hl[HIGH_WORD]) : "r"(a),
>> > "r"(b)); return x.x;
>> >  }
>> >  #define MUL64 MUL64
>> > @@ -61,7 +69,7 @@
>> >  {
>> >      union { uint64_t x; unsigned hl[2]; } x = { d };
>> >      asm ("smlal %0, %1, %2, %3"
>> > -         : "+r"(x.hl[0]), "+r"(x.hl[1]) : "r"(a), "r"(b));
>> > +         : "+r"(x.hl[LOW_WORD]), "+r"(x.hl[HIGH_WORD]) : "r"(a),
>> > "r"(b)); return x.x;
>> >  }
>> >  #define MAC64(d, a, b) ((d) = MAC64(d, a, b))
>>
>> This looks OK.
>
> 'armv4l/mathops.h' gets included only when ARCH_ARMV4L is defined. ARCH_ARMV4L
> implies little endian byte order. The problem is that apparently it also gets
> defined for big endian ARM systems and it looks like a bug in configure. If

IMHO, the bug is in the naming.  I'm planning to rename this and the armv4l
directory at some point.  Most of the code works fine on big endian ARM,
and very little of it works on ARMv4, so the name is quite inaccurate.

> there is interest to also support big endian ARM systems and share code with
> current little endian codebase, renaming ARCH_ARMV4L to something less
> misleading would be very nice. As far as I remember, you were also somewhat
> affected by DECLARE_ALIGNED_8 related issue some time ago?

Only for the NEON optimisations that are not yet in svn.  I have a fix
for that in my FFmpeg tree.

> I still would like to know if there is any practical use for big endian ARM
> support or it is just academic interest? Hardware sometimes supports features
> that are not much used nowadays (segmented model on x86, 16-bit real mode,
> etc.). Supporting something only because it exists potentially has a negative
> side of just introducing bloat. But again, I don't know anything about big
> endian ARM systems and may be wrong. No harm intended, just curious.

I share this curiosity.

> BTW, does regression test in qemu-armeb succeed without using any assembly
> optimized code for FFmpeg (just generic big endian C)?

A lot of floating-point code gives different results on ARM (or indeed any
architecture) than on x86.

-- 
M?ns Rullg?rd
mans at mansr.com