[FFmpeg-cvslog] r20254 - trunk/libavcodec/mlpdec.c

[Ramiro Polla]

On Fri, Oct 16, 2009 at 1:20 PM, Reimar D?ffinger
<Reimar.Doeffinger at gmx.de> wrote:
> On Fri, Oct 16, 2009 at 06:10:00PM +0200, ramiro wrote:
>> Author: ramiro
>> Date: Fri Oct 16 18:10:00 2009
>> New Revision: 20254
>>
>> Log:
>> mlp: Only initialize VLC tables once. This caused a crash when multiple
>> instances of the decoder were started at different times.
>
> Why? I don't think it should.
> init_vlc_sparse already does this with the following check:
>> if (vlc->table_size && vlc->table_size == vlc->table_allocated)
>> ? ? return 0;
>
> Unfortunately with this it still seems possible that two threads end up
> in "build_table" at the same time, which that one does not seem to be
> designed for (in order to be thread-safe, it may not write any of the
> static memory locations more than once).

That check only checks that the table has been initialized, it might
have nothing written to it yet. I assume that's how the bug was
triggered.

>> ?static av_cold void init_static(void)
>> ?{
>> + ? ?if (!huff_vlc[0].bits) {
>
> In addition this is just wrong since huff_vlc[0].bits is certainly not
> written last in build_table, thus this might end up with the second
> decoder skipping the initialization despite the initialization not
> having finished.

I just put this back in from Ian's code. I had removed this part
during review after seeing that check you pointed to above.

>> ? ? ?INIT_VLC_STATIC(&huff_vlc[0], VLC_BITS, 18,
>> ? ? ? ? ? ? ? ? ?&ff_mlp_huffman_tables[0][0][1], 2, 1,
>> ? ? ? ? ? ? ? ? ?&ff_mlp_huffman_tables[0][0][0], 2, 1, 512);
>
> Just try adding a sleep(1000); or so here and you'll see it very easily.

Right, so should init_vlc_sparse make sure the table has been entirely
initialized before returning 0 on that check? Maybe return some sort
of EAGAIN if it hasn't been entirely initialized so whoever called it
can just while(); around waiting.