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

[Gwenole Beauchesne]

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.

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.

>> +} 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.

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

I could make it a one-line but someone will surely complain that the line 
is too long.