[FFmpeg-devel] [PATCH] HWAccel infrastructure (take 3)

[Michael Niedermayer]

On Wed, Feb 18, 2009 at 02:38:01PM +0100, Gwenole Beauchesne wrote:
> On Wed, 18 Feb 2009, Michael Niedermayer wrote:
> 
> >> +    /**
> >> +     * Called at the beginning of each slice.
> >> +     *
> >> +     * libavcodec will try its best to provide a single \p buf base at
> >> +     * each call. However, this is not guaranteed for all codecs but
> >> +     * for MPEG-1/2. This function is mandatory only if end_slice()
> >> +     * is implemented.
> >> +     *
> >> +     * @param avctx the codec context
> >> +     * @param buf the slice, or frame, data buffer base
> >> +     * @param buf_offset the offset to the actual slice data
> >> +     * @return zero if successful, a negative value otherwise
> >> +     */
> >> +    int (*start_slice)(AVCodecContext *avctx, const uint8_t *buf, uint32_t buf_offset);
> >
> > hmm this seems poorly designed
> > first, it needs a buf_size or you will segfault
> > second buf / buf_offset can be replaced by buf or why not?
> 
> Actually, the correct sequence order was meant to be start_slice(), 
> decode_slice(), end_slice() so that to follow natural processing of the 
> bitstream in the codec implementation. Otherwise, we can end up 
> duplicating code to advance to the next slice and/or add other variables 
> to record the previous position then compute size. i.e. IMO, uglification 
> in some locations (in h263dec.c in particular).
> 
> If it's OK with you to slightly uglify h263dec but make it possible to 
> merge all 3 into a single decode_slice() call, then that'd be fine with me 
> too.

start&decode as in your patch can be merged without any other changes
i cant speak about if i would accept some uglification without really seeing
it
also considering the special casing in mpeg1/2 it does not seem the split in
3 really avoids uglification


> 
> Concerning buf / buf_offset, this was meant for optimization purposes in 
> the HWAccel implementation. That way we can reduce control blocks and/or 
> (it depends on the HWAccel) copy only one buffer in the end.

if you have buf + buf_size and the buffers overlap you can do that too,
besides if you have a buf/buf_size from start_frame you could check if
the slices buffer is within that.


> 
> >> +} AVHWAccel;
> >
> > please add a "const char name"
> 
> And what/how would this be used for? I mean, in the current approach, this 
> is not needed at all as we can already automatically select the HWAccel 
> and fallback to pure SW mode otherwise.

The user app might wish to print the name of the hw accel in use.


> 
> >> @@ -950,7 +950,8 @@ void MPV_frame_end(MpegEncContext *s)
> >>      //just to make sure that all data is rendered.
> >>      if(CONFIG_MPEG_XVMC_DECODER && s->avctx->xvmc_acceleration){
> >
> >>          ff_xvmc_field_end(s);
> >> -    }else if(!(s->avctx->codec->capabilities&CODEC_CAP_HWACCEL_VDPAU)
> >> +    }else if(!s->avctx->hwaccel
> >> +       && !(s->avctx->codec->capabilities&CODEC_CAP_HWACCEL_VDPAU)
> >>         && s->unrestricted_mv
> >
> >    }else if(!(s->avctx->codec->capabilities&CODEC_CAP_HWACCEL_VDPAU)
> > +      !s->avctx->hwaccel
> >       && s->unrestricted_mv
> 
> Order does not really matter here since the next patch will remove the 
> HWACCEL_VDPAU thing. So, currently it's 1-/2+ then 1-, and with your 
> suggestion that will be 1+ then 2-/1+ -- do you prefer keeping some more 
> work for next time? ;-)

you can do it as 1+ and then 1-/1+

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

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/20090218/4bf5beaa/attachment.pgp>