[FFmpeg-devel] [PATCH 001/114] avcodec/bitstream: Add second function to create VLCs

[Andreas Rheinhardt]

Anton Khirnov:
> Quoting Andreas Rheinhardt (2020-11-12 21:51:28)
>> Derek Buitenhuis:
>>> On 10/11/2020 10:46, Andreas Rheinhardt wrote:
>>>>  
>>>> +#define INIT_VLC_STATIC_FROM_LENGTHS(vlc, bits, nb_codes, lens, len_wrap,  \
>>>> +                                     symbols, symbols_wrap, symbols_size,  \
>>>> +                                     offset, flags, static_size)           \
>>>> +    do {                                                                   \
>>>> +        static VLC_TYPE table[static_size][2];                             \
>>>> +        (vlc)->table           = table;                                    \
>>>> +        (vlc)->table_allocated = static_size;                              \
>>>> +        ff_init_vlc_from_lengths(vlc, bits, nb_codes, lens, len_wrap,      \
>>>> +                                 symbols, symbols_wrap, symbols_size,      \
>>>> +                                 offset, flags | INIT_VLC_USE_NEW_STATIC); \
>>>> +    } while (0)
>>>
>>> If I am reading correctly, wherever you add/use this, you are adding non-thread
>>> safe global init code to a decoder. This is a huge step back and not acceptable.
>>>
>>> It should be made to properly use ff_thread_once if possible, or reworked.
>>>
>> The ff_init_vlc_... functions are inherently thread-safe: Everything is
>> modified only once and directly set to its final value; so it's no
>> problem if two threads are initializing the same static VLC at the same
>> time.
> 
> Strictly speaking it's still a race (and therefore UB), even if you
> store the same values. I suspect tools like tsan will not like it
> either. 
> 
I at first thought it was not so, because the definition of data races
in C11 speaks of conflicting actions in different threads; but you are
right: It is conflicting according to the definition: "Two expression
evaluations conflict if one of them modifies a memory location and the
other one reads or modifies the same memory location."
Furthermore, the current code has the problem of not using atomic
operations to modify the VLC table. So I'll use ff_thread_once() for the
cases affected by this patchset; a later patchset will then fix the
other ones and also implement the simplifications that will be possible
once this is done (no volatile!). This will also improve performance in
general.

- Andreas