On Mon, Mar 04, 2013 at 11:35:04AM +0100, Anton Khirnov wrote:
> From: "Ronald S. Bultje" <[email protected]>
> 
> These could be used for reference counting, or for keeping track of
> decoding progress in references in multithreaded decoders.
> 
> Support is provided by gcc/msvc/suncc intrinsics, with a fallback using
> pthread mutexes.

Why not use the fallback in all cases?  Performance?

> --- a/configure
> +++ b/configure
> @@ -3440,6 +3443,10 @@ check_func  strerror_r
>  check_func  strtok_r
>  check_func  sched_getaffinity
> +# cannot use check_func, because those are builtins
> +check_code  ld "" "__sync_synchronize()" && enable sync_synchronize
> +check_code  ld mbarrier.h "__machine_rw_barrier()" && enable 
> machine_rw_barrier
> +check_code  ld windows.h "MemoryBarrier()" && enable MemoryBarrier

What is the problem with check_func?

> --- a/libavutil/Makefile
> +++ b/libavutil/Makefile
> @@ -100,6 +101,7 @@ SKIPHEADERS          = old_pix_fmts.h
>  
>  TESTPROGS = adler32                                                     \
>              aes                                                         \
> +            atomic                                                      \
>              avstring                                                    \
>              base64                                                      \

If you add a test program, add a FATE test as well.

> --- /dev/null
> +++ b/libavutil/atomic.c
> @@ -0,0 +1,120 @@
> +
> +#include "atomic.h"
> +
> +#if !HAVE_MEMORYBARRIER && !HAVE_SYNC_SYNCHRONIZE && !HAVE_MACHINE_RW_BARRIER
> +
> +#if HAVE_PTHREADS
> +
> +#else
> +// assume there is no threading at all, noop implementations of everything

That assumption would seem to be wrong.  We support more than just the
pthreads thread implementation.

> +#endif // HAVE_PTHREADS
> +
> +#endif

Please /* */ comment all the #endifs.

> +#ifdef TEST
> +#include <assert.h>
> +
> +int main(int argc, char *argv[])

The arguments are unused.

> --- /dev/null
> +++ b/libavutil/atomic.h
> @@ -0,0 +1,75 @@
> +
> +/**
> + * Loads the current value stored in an atomic integer.

Load

> + * @note this acts as a memory barrier

Capitalize, end in a period.

> +/**
> + * Stores a new value in an atomic integer.

Store

> + * @param ptr atomic integer
> + * @param val the value to store in the atomic integer
> + * @note this acts as a memory barrier

Capitalize, end in a period.

> +/**
> + * Adds up a value to an atomic integer.

Add

What do you mean by "add up" here - I assume it should be plain "add"?

> + * @param ptr atomic integer
> + * @param inc the value to add up to the atomic integer (may be
> + *            negative)

same

> + * @note this does NOT act as a memory barrier. This is primarily
> + *       intended for reference counting.

Capitalize, end in a period.

> +/**
> + * Atomic pointer compare and swap.
> + *
> + * @param ptr pointer to the pointer to operate on
> + * @param oldval do the swap if the current value of *ptr equals to oldval
> + * @param newval value to replace *ptr with
> + * @return the value of *ptr before comparison
> + */
> +void *avpriv_atomic_ptr_cas(void * volatile *ptr, void *oldval, void 
> *newval);

This is a void function, why the @return?

> +#endif
> +#endif /* AVUTIL_ATOMIC_H */

Please comment the #endif.

Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel

Reply via email to