[FFmpeg-devel] [PATCH] Add the relaxed and acquire/releae flavors of avpriv_atomic_int_get and avpriv_atomic_int_set.

[Wan-Teh Chang]

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 <rsbultje at gmail.com> wrote:
> Hi,
>
> On Mon, Feb 29, 2016 at 9:35 PM, Wan-Teh Chang <wtc-at-google.com at 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