[FFmpeg-devel] Broken compile with latest libavutil/common.h

[Måns Rullgård]

Aurelien Jacobs <aurel at gnuage.org> writes:

> M?ns Rullg?rd wrote:
>
>> "Chris Silva" <2manybills at gmail.com> writes:
>> 
>> > /usr/local/include/libavutil/common.h:326:14: error: operator '||' has no left operand
>> >
>> > ---- line 326
>> > +#if ARCH_X86 || ARCH_PPC || ARCH_BFIN
>> >
>> > Is this a xine-lib problem or a ffmpeg problem?
>> 
>> Looks like it's our fault.  That certainly won't work in an installed
>> header.  Thanks for telling us.
>
> Ouch ! Indeed !
> Now with this change, we realize that we had part of installed files
> which were always silently disabled, and thus not really part of
> public API despite being in installed header.
> As the corresponding code wasn't really part of public API, I think
> the best solution is to move it out of public header.
> Attached patch is a rough attempt a moving what seems to be non-public.
>
> Aurel
>
> Index: libavutil/internal.h
> ===================================================================
> --- libavutil/internal.h	(revision 16611)
> +++ libavutil/internal.h	(working copy)
> @@ -301,4 +301,140 @@
>  }
>  #endif /* HAVE_TRUNCF */
>  
> +/* median of 3 */
> +static inline av_const int mid_pred(int a, int b, int c)
> +{
> +#if HAVE_CMOV
> +    int i=b;
> +    __asm__ volatile(
> +        "cmp    %2, %1 \n\t"
> +        "cmovg  %1, %0 \n\t"
> +        "cmovg  %2, %1 \n\t"
> +        "cmp    %3, %1 \n\t"
> +        "cmovl  %3, %1 \n\t"
> +        "cmp    %1, %0 \n\t"
> +        "cmovg  %1, %0 \n\t"
> +        :"+&r"(i), "+&r"(a)
> +        :"r"(b), "r"(c)
> +    );
> +    return i;
> +#elif 0
> +    int t= (a-b)&((a-b)>>31);
> +    a-=t;
> +    b+=t;
> +    b-= (b-c)&((b-c)>>31);
> +    b+= (a-b)&((a-b)>>31);
> +
> +    return b;
> +#else
> +    if(a>b){
> +        if(c>b){
> +            if(c>a) b=a;
> +            else    b=c;
> +        }
> +    }else{
> +        if(b>c){
> +            if(c>a) b=c;
> +            else    b=a;
> +        }
> +    }
> +    return b;
> +#endif
> +}

This would be better placed nearer the only place that uses it, which
is somewhere in lavc.

> +/* math */
> +int64_t av_const ff_gcd(int64_t a, int64_t b);

The function is in mathematics.c.  Maybe mathematics.h would be a good
place for this prototype.  Maybe we should even make that function
public.

> +/**
> + * converts fourcc string to int
> + */
> +static inline av_pure int ff_get_fourcc(const char *s){
> +    assert( strlen(s)==4 );
> +    return (s[0]) + (s[1]<<8) + (s[2]<<16) + (s[3]<<24);
> +}

This looks a lot like AV_RL32().  BTW, assert(strlen(s) ... ) is a
stupid idea.  If there's bad data, what's to say there will be a null
terminator anywhere close?

> +#if ARCH_X86 || ARCH_PPC || ARCH_BFIN
> +#define AV_READ_TIME read_time
> +#if ARCH_X86
> +static inline uint64_t read_time(void)
> +{
> +    uint32_t a, d;
> +    __asm__ volatile("rdtsc\n\t"
> +                 : "=a" (a), "=d" (d));
> +    return ((uint64_t)d << 32) + a;
> +}
> +#elif ARCH_BFIN
> +static inline uint64_t read_time(void)
> +{
> +    union {
> +        struct {
> +            unsigned lo;
> +            unsigned hi;
> +        } p;
> +        unsigned long long c;
> +    } t;
> +    __asm__ volatile ("%0=cycles; %1=cycles2;" : "=d" (t.p.lo), "=d" (t.p.hi));
> +    return t.c;
> +}
> +#else //FIXME check ppc64
> +static inline uint64_t read_time(void)
> +{
> +    uint32_t tbu, tbl, temp;
> +
> +     /* from section 2.2.1 of the 32-bit PowerPC PEM */
> +     __asm__ volatile(
> +         "1:\n"
> +         "mftbu  %2\n"
> +         "mftb   %0\n"
> +         "mftbu  %1\n"
> +         "cmpw   %2,%1\n"
> +         "bne    1b\n"
> +     : "=r"(tbl), "=r"(tbu), "=r"(temp)
> +     :
> +     : "cc");
> +
> +     return (((uint64_t)tbu)<<32) | (uint64_t)tbl;
> +}
> +#endif
> +#elif HAVE_GETHRTIME
> +#define AV_READ_TIME gethrtime
> +#endif
> +
> +#ifdef AV_READ_TIME
> +#define START_TIMER \
> +uint64_t tend;\
> +uint64_t tstart= AV_READ_TIME();\
> +
> +#define STOP_TIMER(id) \
> +tend= AV_READ_TIME();\
> +{\
> +    static uint64_t tsum=0;\
> +    static int tcount=0;\
> +    static int tskip_count=0;\
> +    if(tcount<2 || tend - tstart < FFMAX(8*tsum/tcount, 2000)){\
> +        tsum+= tend - tstart;\
> +        tcount++;\
> +    }else\
> +        tskip_count++;\
> +    if(((tcount+tskip_count)&(tcount+tskip_count-1))==0){\
> +        av_log(NULL, AV_LOG_ERROR, "%"PRIu64" dezicycles in %s, %d runs, %d skips\n",\
> +               tsum*10/tcount, id, tcount, tskip_count);\
> +    }\
> +}
> +#else
> +#define START_TIMER
> +#define STOP_TIMER(id) {}
> +#endif

How about a new header, timer.h or so?

> +/**
> + * Returns NULL if CONFIG_SMALL is true otherwise the argument
> + * without modifications, used to disable the definition of strings
> + * (for example AVCodec long_names).
> + */
> +#if CONFIG_SMALL
> +#   define NULL_IF_CONFIG_SMALL(x) NULL
> +#else
> +#   define NULL_IF_CONFIG_SMALL(x) x
> +#endif

This looks like just the place for that.

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