[FFmpeg-devel] Request for review

[Roman V. Shaposhnik]

Hi Michael!

Thanks for reviewing it so quickly!

On Tue, 2008-11-18 at 14:32 +0100, Michael Niedermayer wrote:
> > +    DVVideoContext *s = (DVVideoContext *)avctx->priv_data;
> 
> i think the cast is unneeded

Agreed.

> except that first patch ok

Committed.

> > +    const uint8_t off[] = { 2, 6, 8, 0, 4 };
> 
> should be const static to make sure stupid compilers dont generate these
> tables in each function call on the stack

Agreed.

> except that, second patch ok

Committed.

> > -    if (d->work_chunks[dv_work_pool_size(d)-1].buf_offset)
> > -        return 0;
> > -
> > +    if (!d->work_chunks[dv_work_pool_size(d)-1].buf_offset) {
> >      p = i = 0;
> >      for (c=0; c<d->n_difchan; c++) {
> >          for (s=0; s<d->difseg_size; s++) {
> > @@ -227,42 +225,45 @@ static int dv_init_dynamic_tables(const DVprofile *d)
> >              }
> >          }
> >      }
> > -    return 0;
> >  }
> >  
> 
> ok, though this seems unrelated and could be a seperate commit

Do you mean that something like I've attached should be a separate
commit?

> >      DVwork_chunk    *work_chunks;           /* each thread gets its own chunk of frame to work on */
> 
> > +    uint32_t        *idct_factor;           /* FIXME */
> 
> why "FIXME" ?

I thought I had ways of optimizing it somewhat even further, but
I don't think that anymore. Besides, a comment like that has nothing
to do with the published history. 

Thanks for spotting it -- I'll try to be more careful in the future.

Thanks,
Roman.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 1.patch
Type: text/x-patch
Size: 1500 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081118/f3623291/attachment.bin>