[FFmpeg-devel] [RFC] Add AVFrame::hwaccel_data{, _private} (Was: [PATCH][VAAPI][2/6] Add common data structures and helpers)

[Michael Niedermayer]

On Fri, Feb 27, 2009 at 06:11:52PM +0100, Gwenole Beauchesne wrote:
> On Fri, 27 Feb 2009, Michael Niedermayer wrote:
>
>>> 1. We want *_render_state structs be stored in a specific AVFrame member,
>>> not hijack some data[i] (mandatory)
>>
>> this is a "if its cleaner" thing, that is if its cleaner otherwise there
>> is no point
>
> Here is the "alternate approach" implemented. I left the vaapi*.c parts 
> since those were mechanical changes only. Is this cleaner for you? Changes 
> to MPlayer are now zero effort.
>
> As a reminder, I think this "alternate approach" is Reimar's if I got it 
> right. That is:
> - lavc: allocate private HW accel data
> - user: ::get_buffer() that does mpi->planes[3] => AVFrame::hwaccel_data
>
> Benefits:
> - Fewer user app changes
> - Keep AVFrame::data[0-2] for the original YV12 pixels, if needed
>
> Drawbacks:
> - More "expressive" in the HWAccel codec implementation part.
>
> Though ,this point is solved by partial public vaapi_render_state struct 
> members copies, once at ::start_frame(), no more expensive than doing 
> ((struct vaapi_render_state *)AVFrame->hwaccel_data)->whatever_needed_there
>
> WDYT?
[...]
> @@ -166,6 +167,100 @@ void ff_copy_picture(Picture *dst, Picture *src){
>  }
>  
>  /**
> + * Releases HW acceleration private data
> + */
> +static void free_hwaccel_data_private(AVCodecContext *avctx, void *pdata)
> +{
> +    if (!pdata)
> +        return;
> +
> +    assert(avctx->hwaccel);
> +    switch (avctx->hwaccel->pix_fmt) {
> +    case PIX_FMT_VAAPI_MOCO:
> +    case PIX_FMT_VAAPI_IDCT:
> +    case PIX_FMT_VAAPI_VLD:
> +        ff_free_vaapi_render_state_private(pdata);
> +        break;
> +    default:
> +        break;
> +    }
> +}
> +
> +/**
> + * Allocates HW acceleration private data
> + */
> +static void *alloc_hwaccel_data_private(AVCodecContext *avctx)
> +{
> +    void *pdata = NULL;
> +    assert(avctx->hwaccel);
> +    switch (avctx->hwaccel->pix_fmt) {
> +    case PIX_FMT_VAAPI_MOCO:
> +    case PIX_FMT_VAAPI_IDCT:
> +    case PIX_FMT_VAAPI_VLD:
> +        pdata = ff_alloc_vaapi_render_state_private();
> +        break;
> +    default:
> +        break;
> +    }
> +    return pdata;
> +}

these probably should be done differently, but i first need to take a
look at ff_free_vaapi_render_state_private()
if its just a wraper around a avmalloc() then a priv_data_size field
in AVHWAccel should do otherwise i guess a open/close in AVHWAccel
would be better


> +
> +/**
> + * Releases a frame buffer
> + */
> +static inline void release_buffer(MpegEncContext *s, Picture *pic)
> +{
> +    s->avctx->release_buffer(s->avctx, (AVFrame*)pic);
> +
> +    if (pic->hwaccel_data_private) {
> +        free_hwaccel_data_private(s->avctx, pic->hwaccel_data_private);
> +        pic->hwaccel_data_private = NULL;
> +    }
> +}
> +
> +/**
> + * Gets a frame buffer
> + */
> +static int get_buffer(MpegEncContext *s, Picture *pic)
> +{
> +    int r;
> +
> +    if (s->avctx->hwaccel) {
> +        assert(!pic->hwaccel_data_private);
> +        pic->hwaccel_data_private = alloc_hwaccel_data_private(s->avctx);
> +        if (!pic->hwaccel_data_private) {
> +            av_log(s->avctx, AV_LOG_ERROR, "get_buffer() failed (hwaccel private data allocation)\n");
> +            return -1;
> +        }
> +    }
> +
> +    r = s->avctx->get_buffer(s->avctx, (AVFrame*)pic);
> +
> +    if (r<0 || !pic->age || !pic->type || !pic->data[0]) {
> +        av_log(s->avctx, AV_LOG_ERROR, "get_buffer() failed (%d %d %d %p)\n", r, pic->age, pic->type, pic->data[0]);
> +        if (pic->hwaccel_data_private) {
> +            free_hwaccel_data_private(s->avctx, pic->hwaccel_data_private);
> +            pic->hwaccel_data_private = NULL;
> +        }
> +        return -1;
> +    }

spliting code out belongs in a seperate patch that does only split code
out, also
i dont like the name get_buffer() it makes grepping hard as theres 
AVCodecContext.get_buffer() 
besides this, spliting that code out is not a bad idea,
cleanup to mpegvideo*.c is indeed quite welcome ...

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

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- 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/20090227/74e98b76/attachment.pgp>