Hi, I have some remarks on this patch.
1. This patch is an alternative fix for the data races in the access to frame progress values in ff_thread_report_progress and ff_thread_await_progress. Between the two patches I submitted, I prefer the first patch (http://ffmpeg.org/pipermail/ffmpeg-devel/2016-February/190100.html). Although C11/C++ made significant progress in standardizing the C/C++ memory model, many good programmers still have trouble understanding it. Code that always accesses progress[field] under the protection of progress_mutex is very easy to understand. Given how expensive it is to decode video frame, I cannot help but suspect the double-checked locking style code in ff_thread_report_progress and ff_thread_await_progress is premature optimization. If we want to preserve it, this patch makes it correct. 2. While working on this patch, I found the order of the load/store operation and the memory barrier in some avpriv_atomic_int_get and avpriv_atomic_int_set implementations seems wrong. For example, https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html has examples of how atomic load and store operations are mapped to assembly code. Some examples for Load Seq Cst and Store Seq Cst use no memory barrier or two memory barriers. But in the examples that use one memory barrier, the order of the load/store operation and the memory barrier is opposite to the order in avpriv_atomic_int_get and avpriv_atomic_int_set. Should I split this change into a separate patch? 3. The Solaris Studio compiler intrinsics for memory barriers (which are used in atomic_suncc.h) are documented at: https://docs.oracle.com/cd/E24457_01/html/E21991/gjzmf.html On Mon, Feb 29, 2016 at 7:05 PM, Ronald S. Bultje <rsbul...@gmail.com> wrote: > Hi, > > On Mon, Feb 29, 2016 at 9:35 PM, Wan-Teh Chang <wtc-at-google....@ffmpeg.org >> wrote: > >> Correct the order of the load/store operation and the memory barrier in >> avpriv_atomic_int_get and avpriv_atomic_int_set. > > This sounds useful. Is there some documentation on which one should be used > under what conditions? I.e. when do I need full, release or acquire? http://en.cppreference.com/w/c/atomic/memory_order is pretty good. I am surprised that I can't find a good Wikipedia article on this subject. The best I can find is https://en.wikipedia.org/wiki/Memory_ordering. I'd like to reiterate that this is difficult for many good programmers. I need the relaxed and acquire/release flavors of avpriv_atomic_int_get and avpriv_atomic_int_set to make the ff_thread_report_progress and ff_thread_await_progress code portable and to ensure that the new code is identical to the original code for x86/x86_64, according to the mappings in https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html. Wan-Teh Chang _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
