[FFmpeg-devel] Request for review

[Michael Niedermayer]

On Tue, Oct 28, 2008 at 08:26:14AM -0700, Roman V. Shaposhnik wrote:
> 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* ?

no unless it is needed for the frame multithreading stuff ...


> 
> > 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];
> 
> ?

i think that should work


> 
> > > @@ -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. ;-)

ok

> 
> 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];
>     }

i think we should attempt to make sizeof(struct) a multiple of 4 or 8
as the compiler will round it up anyway when theres a int in there.
short of that iam of course in favor of this ...


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

yes, in a seperate patch :)

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- 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-devel/attachments/20081028/03f5e5a8/attachment.pgp>