Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags()

2016-12-12 Thread Michael Niedermayer
On Tue, Dec 06, 2016 at 06:16:13PM -0800, Wan-Teh Chang wrote: > Make the one-time initialization in av_get_cpu_flags() thread-safe. The > static variable |cpu_flags| in libavutil/cpu.c is read and written using > normal load and store operations. These are considered as data races. > The fix is to

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags()

2016-12-06 Thread Wan-Teh Chang
On Tue, Dec 6, 2016 at 10:41 AM, Wan-Teh Chang wrote: > Make the one-time initialization in av_get_cpu_flags() thread-safe. The > static variable |cpu_flags| in libavutil/cpu.c is read and written using > normal load and store operations. These are considered as data races. > The fix is to use ato

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-12-06 Thread Wan-Teh Chang
On Thu, Nov 24, 2016 at 1:56 AM, wm4 wrote: > On Wed, 23 Nov 2016 11:40:25 -0800 > Wan-Teh Chang wrote: > >> On Tue, Nov 22, 2016 at 3:30 PM, wm4 wrote: >> > On Tue, 22 Nov 2016 23:57:15 +0100 >> > Michael Niedermayer wrote: >> > >> >> For example the progress code in the frame threading. >> >

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-11-24 Thread Jean-Yves Avenard
Hi. Mozilla also fixed some data races in pthread_frames.c which you may want to look at. It's very similar to problem you mentioned We made the code conditional on being in an TSan build. https://dxr.mozilla.org/mozilla-central/source/media/ffvpx/libavcodec/pthread_frame.c#46 Sorry, using the n

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-11-24 Thread wm4
On Wed, 23 Nov 2016 11:44:04 -0800 Wan-Teh Chang wrote: > On Tue, Nov 22, 2016 at 3:32 PM, wm4 wrote: > > > > [libav/compat/atomics/] is emulation code for compilers which don't provide > > C11 atomics. > > All relevant compilers provide them natively (and presumably implement > > the weaker se

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-11-24 Thread wm4
On Wed, 23 Nov 2016 11:40:25 -0800 Wan-Teh Chang wrote: > On Tue, Nov 22, 2016 at 3:30 PM, wm4 wrote: > > On Tue, 22 Nov 2016 23:57:15 +0100 > > Michael Niedermayer wrote: > > > >> For example the progress code in the frame threading. > > > > Which was recently fixed in Libav AFAIR... >

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-11-23 Thread Michael Niedermayer
On Wed, Nov 23, 2016 at 11:44:04AM -0800, Wan-Teh Chang wrote: > On Tue, Nov 22, 2016 at 3:32 PM, wm4 wrote: > > > > [libav/compat/atomics/] is emulation code for compilers which don't provide > > C11 atomics. > > All relevant compilers provide them natively (and presumably implement > > the weak

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-11-23 Thread Wan-Teh Chang
On Tue, Nov 22, 2016 at 3:30 PM, wm4 wrote: > On Tue, 22 Nov 2016 23:57:15 +0100 > Michael Niedermayer wrote: > >> For example the progress code in the frame threading. > > Which was recently fixed in Libav AFAIR... You're right. libav/libavcodec/pthread_frame.c has code similar to my ffmpeg pat

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-11-23 Thread Wan-Teh Chang
On Tue, Nov 22, 2016 at 3:32 PM, wm4 wrote: > > [libav/compat/atomics/] is emulation code for compilers which don't provide > C11 atomics. > All relevant compilers provide them natively (and presumably implement > the weaker semantics). Thank you for pointing this out. Do you know when the atom

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-11-22 Thread wm4
On Tue, 22 Nov 2016 15:05:36 -0800 Wan-Teh Chang wrote: > Hi wm4, > > On Tue, Nov 22, 2016 at 1:41 PM, wm4 wrote: > > > > Again, once the atomics changes in Libav are merged, the avpriv_atomic_ > > additions will have to be deleted, and code using it rewritten. > > Thanks for the heads-up. I

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-11-22 Thread wm4
On Tue, 22 Nov 2016 23:57:15 +0100 Michael Niedermayer wrote: > On Tue, Nov 22, 2016 at 02:00:12PM -0800, Wan-Teh Chang wrote: > > Hi Michael, > > > > On Tue, Nov 22, 2016 at 1:22 PM, Michael Niedermayer > > wrote: > > > > > > ok, i see th race but do you really need the atomic operations ? >

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-11-22 Thread Wan-Teh Chang
Hi Michael, On Tue, Nov 22, 2016 at 2:57 PM, Michael Niedermayer wrote: > > we have plenty of code that accesses fields without the extra layer > of weak atomics. > For example the progress code in the frame threading. The progress code in the frame threading also has data races as defined by th

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-11-22 Thread Wan-Teh Chang
Hi wm4, On Tue, Nov 22, 2016 at 1:41 PM, wm4 wrote: > > Again, once the atomics changes in Libav are merged, the avpriv_atomic_ > additions will have to be deleted, and code using it rewritten. Thanks for the heads-up. Is there an atomics patch for libav being reviewed right now? I inspected th

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-11-22 Thread Michael Niedermayer
On Tue, Nov 22, 2016 at 02:00:12PM -0800, Wan-Teh Chang wrote: > Hi Michael, > > On Tue, Nov 22, 2016 at 1:22 PM, Michael Niedermayer > wrote: > > > > ok, i see th race but do you really need the atomic operations ? > > isnt merging the 2 variables into 1 as you do enough ? > > (i mean the tools

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-11-22 Thread Wan-Teh Chang
Hi Michael, On Tue, Nov 22, 2016 at 1:22 PM, Michael Niedermayer wrote: > > ok, i see th race but do you really need the atomic operations ? > isnt merging the 2 variables into 1 as you do enough ? > (i mean the tools might still complain but would there be an actual > race remaining?) The atom

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-11-22 Thread wm4
On Tue, 22 Nov 2016 13:36:11 -0800 Wan-Teh Chang wrote: > Make the one-time initialization in av_get_cpu_flags() thread-safe. The > static variables |flags| and |checked| in libavutil/cpu.c are read and > written using normal load and store operations. These are considered as > data races. The fi

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-11-22 Thread Michael Niedermayer
On Tue, Nov 22, 2016 at 11:28:48AM -0800, Wan-Teh Chang wrote: > Hi Michael, > > On Mon, Nov 21, 2016 at 4:35 PM, Michael Niedermayer > wrote: > > On Mon, Nov 21, 2016 at 03:37:49PM -0800, Wan-Teh Chang wrote: > >> Make the one-time initialization in av_get_cpu_flags() thread-safe. > >> The fix a

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-11-22 Thread Wan-Teh Chang
Hi Michael, On Mon, Nov 21, 2016 at 4:35 PM, Michael Niedermayer wrote: > On Mon, Nov 21, 2016 at 03:37:49PM -0800, Wan-Teh Chang wrote: >> Make the one-time initialization in av_get_cpu_flags() thread-safe. >> The fix assumes -1 is an invalid value for cpu flags. > > please explain the bug / rac

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-11-22 Thread wm4
On Mon, 21 Nov 2016 15:37:48 -0800 Wan-Teh Chang wrote: > Hi, > > This patch makes the one-time initialization in av_get_cpu_flags() > thread-safe. The data race was reported by ThreadSanitizer. > > Wan-Teh Chang (1): > avutil: fix data race in av_get_cpu_flags(). > > libavutil/atomic.c

Re: [FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

2016-11-21 Thread Michael Niedermayer
On Mon, Nov 21, 2016 at 03:37:49PM -0800, Wan-Teh Chang wrote: > Make the one-time initialization in av_get_cpu_flags() thread-safe. > The fix assumes -1 is an invalid value for cpu flags. please explain the bug / race that occurs and how it is fixed > > The fix requires new atomic functions to