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

[Michael Niedermayer]

On Wed, Mar 11, 2009 at 11:37:31PM +0100, Gwenole Beauchesne wrote:
> Le 11 mars 09 ? 21:29, Michael Niedermayer a ?crit :
>
>> On Wed, Mar 11, 2009 at 06:45:28AM +0100, Gwenole Beauchesne wrote:
>>> Le 11 mars 09 ? 02:10, Michael Niedermayer a ?crit :
>>>
>>>>> +/** Destroy VA API render state buffers */
>>>>> +static void vaapi_render_state_fini(struct
>>>>> vaapi_render_state_private *rds)
>>>>
>>>> the vaapi_ prefixes are still useless
>>>
>>> vaapi_render_state is the name of the struct...
>>
>> yes but why?
>
> VDPAU heritage that you liked...
>
> New patch attached:
> - Mandate AVCodecContext.hwaccel_context to hold a struct vaapi_context
> - Dropped struct vaapi_render_state, Picture.data[3] is the VASurfaceID
> - Renamed vaapi_render_state_private to vaapi_hwaccel_data_private (akin to 
> Picture.hwaccel_data_private)
>
> Compile tested. I have yet to visually check if my MPlayer vo control works 
> to retrieve avctx->hwaccel_context.

[...]
> +/** Render the picture */
> +static int render_picture(AVCodecContext *avctx, Picture *pic)

redundant comment, it just repeats the function name


> +{
> +    const struct vaapi_context *va_context = avctx->hwaccel_context;
> +    struct vaapi_hwaccel_data_private * const p = pic->hwaccel_data_private;

the structs should be named in a way that makes it obvious which is
"global" and which is per frame like
hwaccel_frame/surface/..._private or so


> +    VABufferID va_buffers[3];
> +    unsigned int n_va_buffers = 0;
> +
> +    if (p->n_slice_buf_ids == 0)
> +        return -1;
> +

> +    if (vaCreateBuffer(va_context->display, va_context->context_id,
> +                       VAPictureParameterBufferType,
> +                       p->pic_param_size,
> +                       1, &p->pic_param,
> +                       &p->pic_param_buf_id) != VA_STATUS_SUCCESS)
> +        return -1;
> +    assert(p->pic_param_buf_id != 0);

Does VAAPI gurantee that the value is not 0 ?
Yes -> assert() is useless
No -> your code is broken

I mean this isnt checking your code, it is checking VAAPI behavior
thats like

i= atoi("5")
assert(i==5);

whats the point of that?
its not ffmpegs job to test VAAPI nor libc


[...]
> +void *ff_vaapi_alloc_slice(AVCodecContext *avctx, const uint8_t *buffer, uint32_t size)
> +{
> +    MpegEncContext * const s = avctx->priv_data;
> +    struct vaapi_hwaccel_data_private *p = s->current_picture_ptr->hwaccel_data_private;
> +    uint8_t *slice_params;
> +    VASliceParameterBufferBase *slice_param;
> +
> +    if (!p->slice_data)
> +        p->slice_data = buffer;
> +    if (p->slice_data + p->slice_data_size != buffer) {
> +        if (commit_slices(avctx, s->current_picture_ptr) < 0)
> +            return NULL;
> +        p->slice_data = buffer;
> +    }
> +
> +    slice_params =
> +        av_fast_realloc(p->slice_params,
> +                        &p->slice_params_alloc,
> +                        (p->slice_count + 1) * p->slice_param_size);
> +    if (!slice_params)
> +        return NULL;
> +    p->slice_params = slice_params;
> +

> +    slice_param = (VASliceParameterBufferBase *)(slice_params + p->slice_count * p->slice_param_size);

why the cast? why is slice_params uint8_t and not that type to begin
with?


> +    slice_param->slice_data_size        = size;
> +    slice_param->slice_data_offset      = p->slice_data_size;
> +    slice_param->slice_data_flag        = VA_SLICE_DATA_FLAG_ALL;

extra useless whitespace


[...]
> +    av_freep(&p->bitplane_buffer);
> +    av_freep(&p->slice_buf_ids);
> +    p->n_slice_buf_ids     = 0;
> +    p->slice_buf_ids_alloc = 0;
> +    av_freep(&p->slice_params);
> +    p->slice_count         = 0;
> +    p->slice_params_alloc  = 0;

you can group the av_freep() together


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

Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- 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/20090312/89aaf056/attachment.pgp>