[FFmpeg-devel] Request for review

[Michael Niedermayer]

On Mon, Oct 27, 2008 at 01:17:50PM -0700, Roman V. Shaposhnik wrote:
> On Thu, 2008-10-23 at 12:05 +0200, Michael Niedermayer wrote:
> > > > I think this could be put in dv_anchor or referenced by it, 
> > > 
> > > I've thought about exactly the same thing. But! If we do
> > > that we would have to potentially regenerate dv_anchor
> > > every time there's a change of DV profile midstream.
> > > Would that be more acceptable?
> > 
> > If dv_anchor is in the context, allocated by avmalloc() then it should be
> > no problem.
> 
> Sure. Although, it might be better to add it to the DVprofile so that
> if profiles change midstream like this: A, B, A then the anchor from
> A doesn't need to be reinitialized.
> 
> Anyway, attached is the new patch that tries to do exactly that. Please
> keep in mind that DVtables will contain *all* dynamic tables
> (work_chunks, video_place, unweight) in the future. The only bit that
> I don't like about how it works is the awkward initialization sequence
> in dvdata.h. If anybody has better ideas on how to separate dynamic/RW
> portion from the static/const DVprofile[] -- please let me know.

[...]
> @@ -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.
This could be avoided by setting d->dv_tbls->work_chunks only after the table
has been initialized or checking the last written element of the table.

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.


[...]

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


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

I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- 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/1ba8f39d/attachment.pgp>