[FFmpeg-devel] [PATCH][VAAPI][2/6] Add common data structures and helpers

[Benoit Fouet]

Hi,

On 02/27/2009 10:40 AM, Gwenole Beauchesne wrote:
> On Fri, 27 Feb 2009, Gwenole Beauchesne wrote:
>
>>>> Having lavc allocate *_render_state structs sounds good but in
>>>> practise,
>>>> that's not really the ideal solution for MPlayer. How a ::get_image()
>>>> could retrieve that hwaccel_data member? MPI allocation is well hidden
>>>> mpcodecs_get_image() and it completely doesn't mind about AVFrame.
>>>
>>> you could use data[3] for the hwacell struct that would leave 0-2 for
>>> YV12 pixels
>>
>> Not really in a clean way. :)
>>
>> In the previous model, a VO allocated what we now call the
>> hwaccel_data member, so it was enough for it to pass the resulting
>> struct through data[0] (or data[3]). Thus, data[] (planes[] actually,
>> then copied) was seen as an output arg. Now, we need it to be an
>> in-out arg. i.e. set planes[3] prior to invoking the ::get_image()
>> function and after/inside mpcodecs_get_image() or the
>> vf->get_image(). I don't really see how to achieve it apart extending
>> it with an extra arg. This is not particularly clean as that extra
>> arg is only useful to vd_ffmpeg and not others.
>>
>> FWIW, I have attached the current lavc part. New AVHWAccel hooks may
>> be useful (get_buffer(), release_buffer()?).
>> + after the avctx->get_buffer() call an
>> if(!(avctx->hwaccel->capabilities&FF_HWACCEL_CAP_PIXELS_READBACK))
>> pic->data[0] = pic->hwaccel_data;
>
> Here is the version without memory leaks. ;-)
>
> I have also attached the MPlayer part, through a new control to bind
> an hwaccel_data to an mpi. WDYT? I am not very satisfied yet but the
> new control has the benefit to not change the original ::get_image()
> semantics, thus making it possible to really allocate YV12 pixels (or
> NV12 pixels, generally).
>
> I have checked that H.264 decoding still works, even for bitstreams
> that used to be displayed with unordered frames (that one was fixed
> without hacks by Reimar's numbered mpi for both VDPAU and VAAPI at
> once ;-)).

> diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
> index 3f3a3a8..b8d3572 100644
> --- a/libavcodec/mpegvideo.c
> +++ b/libavcodec/mpegvideo.c
> @@ -166,6 +167,74 @@ void ff_copy_picture(Picture *dst, Picture *src){
>  }
>  
>  /**
> + * Release a frame buffer
> + */
> +static void release_buffer(MpegEncContext *s, Picture *pic)
> +{
> +    if (pic->hwaccel_data) {
> +        assert(s->avctx->hwaccel);
> +        switch (s->avctx->hwaccel->pix_fmt) {
> +        case PIX_FMT_VAAPI_MOCO:
> +        case PIX_FMT_VAAPI_IDCT:
> +        case PIX_FMT_VAAPI_VLD:
> +            ff_free_vaapi_render_state(pic->hwaccel_data);
> +            break;
> +        default:
> +            break;
> +        }
> +        pic->hwaccel_data = NULL;
> +    }
> +
> +    s->avctx->release_buffer(s->avctx, (AVFrame*)pic);
> +}
> +
> +/**
> + * Gets a frame buffer
> + */
> +static int get_buffer(MpegEncContext *s, Picture *pic)
> +{
> +    int r;
> +
> +    if (s->avctx->hwaccel) {
> +        assert(!pic->hwaccel_data);
> +        switch (s->avctx->hwaccel->pix_fmt) {
> +        case PIX_FMT_VAAPI_MOCO:
> +        case PIX_FMT_VAAPI_IDCT:
> +        case PIX_FMT_VAAPI_VLD:
> +            pic->hwaccel_data = ff_alloc_vaapi_render_state();
> +            break;
> +        default:
> +            break;
> +        }
> +        if (!pic->hwaccel_data) {
> +            av_log(s->avctx, AV_LOG_ERROR, "get_buffer() failed
> (hwaccel 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]);
>   

isn't this leaking memory (at least the hwaccel_data) ?

Ben