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

[Michael Niedermayer]

On Fri, Oct 16, 2009 at 06:57:37PM +0200, Reimar D?ffinger wrote:
> 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.

something like that will probably be needed :/
i guess we could simply put the partial table that build_table() works on
on the stack (should be small if nb_bits is small which i think it is ...)
but if that doesnt work then malloc as you suggest is fine too

instead of the vlc struct we can easily use a copy in the whole function
until the end

or the static vlc table code could be largely droped, the dynamic always
used and then just at the end of init_vlc() would we check if the static
table is large enough and copy or fail ...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20091016/3ed04c66/attachment.pgp>