[FFmpeg-devel] Request for review

[Roman V. Shaposhnik]

On Tue, 2008-10-28 at 13:53 +0100, Michael Niedermayer wrote:
> [...]
> > @@ -89,6 +80,37 @@ static struct dv_vlc_pair {
> >     uint8_t  size;
> >  } dv_vlc_map[DV_VLC_MAP_RUN_SIZE][DV_VLC_MAP_LEV_SIZE];
> >  
> > +static int dv_init_dynamic_tables(const DVprofile *d)
> > +{
> > +     int j,i,c,s,p;
> > +
> 
> > +     if (!d->dv_tbls->work_chunks) {
> > +         d->dv_tbls->work_chunks = av_malloc(d->n_difchan*d->difseg_size*27*sizeof(void*));
> 
> This has a small chance for a race condition that could leave one of 2 codecs
> without a initalized or half initialized table.

Yes indeed. Speaking of which -- do we plan to offer any kind of locking
mechanism in libav* ?

> Still, after that there is a second race condition left that can cause a
> memleak. Avoiding that can be done by using normal uninitialized arrays
> instead of malloc(). Normal OSs would use copy on write pages for these
> thus not requireing any physical memory until the arrays are written to.
> This of course still needs the check, checking the last written element.

Wow! That's really a cool idea. I haven't thought of that! 
Just to be clear: by normal uninitialized array do you mean something
like:
    typedef struct DVtables {
        void *work_chunk[4*12*27];
    };

    static DVtables dv_tables[9];

?

> > @@ -384,10 +418,13 @@ static inline void dv_decode_video_segment(DVVideoContext *s,
> >      assert((((int)mb_bit_buffer)&7)==0);
> >      assert((((int)vs_bit_buffer)&7)==0);
> >  
> > +    if (work_chunk == -1)
> > +        return;
> > +
> 
> I think the -1 cases should be removed from the array, like
> instead of        ...0x1234,0x2346,-1,0x111,0x222...
> it should IMHO be ...0x1234,0x2346,0x111,0x222...

Actually, I'm holding this possibility off till the next patch. 
If you don't mind, of course. ;-)

As a matter of fact, I would really like to go as far as combining
the video_place array with the work_chunk, so that each work_chunk
is a structure of something like this:
    struct DVwork_chunk {
        int buf_offset;
        uint16_t mb_coordinates[5];
    }

Now, in order for the above to be efficient I would really like
our multithreading ->execute() routine to be more like qsort(3) and
accept the size of each data element as an argument. Otherwise
I'm stuck with creating extra layer of pointers to these structures
or doing the semi-ugly tricks of manipulating bits of structure into
being a (void*).

Would it be possible to modify ->execute like that?

Thanks,
Roman.