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

[Reimar Döffinger]

On Fri, Oct 16, 2009 at 01:52:16PM -0300, Ramiro Polla wrote:
> 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.

That's how I suspect your check would fail, for this check I think it is
more likely that the initialization was run twice, which causes really
serious issues with the vlc code.

> >> ? ? ?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.

As I think that the problem is initializing twice that's not a useful
approach.
It would be possible to use locking to make sure we really initialize
only ones and possibly sleep to wait for the initialization to finish,
or most reliable but slow:
malloc a table of the same size as the final one, build the vlc table in
it, and at the very end copy over to the static table and then set
vlc->table_size.