[FFmpeg-devel] Request for review

[Roman V. Shaposhnik]

On Tue, 2008-10-21 at 19:40 +0200, Michael Niedermayer wrote:
> > @@ -384,10 +401,20 @@ static inline void dv_decode_video_segment(DVVideoContext *s,
> >      assert((((int)mb_bit_buffer)&7)==0);
> >      assert((((int)vs_bit_buffer)&7)==0);
> >  
> > +    j    = buf_offset/150;
> > +    chan = j/s->sys->difseg_size;
> > +    seg  = j%s->sys->difseg_size;
> > +    slot = ((buf_offset%150)*15 - 90)/(5<<4);
> 
> 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?

> > +    /* in 1080i50 and 720p50 some seq are unused */
> > +    if ((DV_PROFILE_IS_1080i50(s->sys) && chan != 0 && seg == 11) ||
> > +        (DV_PROFILE_IS_720p50(s->sys) && seg > 9))
> > +        return;
> 
> Can these not just be ommited from dv_anchor ?

See above.

> > @@ -982,48 +1006,14 @@ static inline void dv_encode_video_segment(DVVideoContext *s,
> >  
> >  static int dv_decode_mt(AVCodecContext *avctx, void* sl)
> >  {
> > -    DVVideoContext *s = avctx->priv_data;
> > -    int slice = (size_t)sl;
> > -
> > -    /* which DIF channel is this? */
> > -    int chan = slice / (s->sys->difseg_size * 27);
> > -
> > -    /* slice within the DIF channel */
> > -    int chan_slice = slice % (s->sys->difseg_size * 27);
> > -
> > -    /* byte offset of this channel's data */
> > -    int chan_offset = chan * s->sys->difseg_size * 150 * 80;
> > -
> > -    /* DIF sequence */
> > -    int seq = chan_slice / 27;
> > -
> > -    /* in 1080i50 and 720p50 some seq are unused */
> > -    if ((DV_PROFILE_IS_1080i50(s->sys) && chan != 0 && seq == 11) ||
> > -        (DV_PROFILE_IS_720p50(s->sys) && seq > 9))
> > -        return 0;
> > -
> > -    dv_decode_video_segment(s, &s->buf[(seq*6+(chan_slice/3)+chan_slice*5+7)*80 + chan_offset],
> > -                            &s->sys->video_place[slice*5]);
> > +    dv_decode_video_segment((DVVideoContext *)avctx->priv_data, (size_t)sl);
> >      return 0;
> >  }
> 
> after this change dv_decode_mt() could maybe be ommited and
> dv_decode_video_segment() be used directly

Absolutely! But I thought you would be happier with that change
in a separate patch. Please let me know if you want me to
roll it into this one.

Thanks,
Roman.