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

[Michael Niedermayer]

On Tue, Mar 10, 2009 at 11:56:04PM +0100, Gwenole Beauchesne wrote:
> Le 10 mars 09 ? 22:20, Michael Niedermayer a ?crit :
>
>> [...]
>>> +/** Initialize VA API render state */
>>> +static int vaapi_render_state_init(struct vaapi_render_state_private 
>>> *rds, Picture *pic)
>>> +{
>>> +    struct vaapi_render_state * const r = 
>>> ff_get_vaapi_render_state(pic);
>>> +
>>> +    assert(r && r->va_context);
>>> +    rds->display                = r->va_context->display;
>>> +    rds->context_id             = r->va_context->context_id;
>>> +    rds->surface                = r->surface;
>>
>>> +    rds->pic_param_buf_id       = 0;
>>> +    rds->iq_matrix_buf_id       = 0;
>>> +    rds->bitplane_buf_id        = 0;
>>> +    rds->slice_buf_ids          = NULL;
>>> +    rds->n_slice_buf_ids        = 0;
>>> +    rds->n_slice_buf_ids_alloc  = 0;
>>> +    rds->slice_params           = NULL;
>>> +    rds->n_slice_params_alloc   = 0;
>>> +    rds->slice_count            = 0;
>>> +    rds->slice_data             = NULL;
>>> +    rds->slice_data_size        = 0;
>>
>> memset(0) seems simpler if these are not guranteed to be allocated by
>> av_mallocz() or another zeroing alloc, but guranteeing this seems like
>> the best choice ...
>
> Assuming you really meant av_mallocz() hwaccel_data_private (see other 
> patch), here is a new patch.
>
>>> +    if (rds->slice_buf_ids) {
>>> +        for (i = 0; i < rds->n_slice_buf_ids; i++) {
>>> +            vaDestroyBuffer(rds->display, rds->slice_buf_ids[i]);
>>> +            rds->slice_buf_ids[i] = 0;
>>> +        }
>>> +        av_freep(&rds->slice_buf_ids);
>>> +    }
>>> +    av_freep(&rds->slice_params);
>>> +    return 0;
>>> +}
>>
>> static destroy_buffer(VADisplay display, VABufferID *id){
>>    if(*id){
>>        vaDestroyBuffer(display, *id);
>>        *id=0;
>>    }
>> }
>
> A destroy_buffers() was preferred.
>

> --- /dev/null
> +++ b/libavcodec/vaapi.c
[...]

> +/** Destroy VA API render state buffers */
> +static void vaapi_render_state_fini(struct vaapi_render_state_private *rds)

the vaapi_ prefixes are still useless


[...]

> +/** Render the picture */
> +static int render_picture(struct vaapi_render_state_private *rds)
> +{
> +    VABufferID va_buffers[3];
> +    unsigned int n_va_buffers = 0;
> +
> +    if (rds->pic_param_buf_id == 0 || rds->n_slice_buf_ids == 0)
> +        return -1;
> +
> +    if (vaBeginPicture(rds->display, rds->context_id,
> +                       rds->surface) != VA_STATUS_SUCCESS)
> +        return -1;
> +
> +    va_buffers[n_va_buffers++]          = rds->pic_param_buf_id;
> +    if (rds->iq_matrix_buf_id)
> +        va_buffers[n_va_buffers++]      = rds->iq_matrix_buf_id;
> +    if (rds->bitplane_buf_id)
> +        va_buffers[n_va_buffers++]      = rds->bitplane_buf_id;

do the functions not skip 0 ? it would make the if unneeded and the index
var as well ...


[...]
> +/** Commit pending slices to HW */
> +static int commit_slices(struct vaapi_render_state_private *rds)
> +{
> +    VABufferID *slice_buf_ids = rds->slice_buf_ids;
> +    VABufferID slice_param_buf_id, slice_data_buf_id;
> +
> +    if (rds->n_slice_buf_ids + 2 > rds->n_slice_buf_ids_alloc) {
> +        rds->n_slice_buf_ids_alloc += 16;
> +        slice_buf_ids = av_realloc(rds->slice_buf_ids,
> +                                   rds->n_slice_buf_ids_alloc * sizeof(slice_buf_ids[0]));
> +        if (!slice_buf_ids) {
> +            av_free(rds->slice_buf_ids);

why is the free needed in addition to the return?
this seems buggy if one considers that theres a return -1 later that does not
free it ...


> +            return -1;
> +        }
> +        rds->slice_buf_ids = slice_buf_ids;
> +    }
> +

[...]

> +void *ff_vaapi_alloc_slice(struct vaapi_render_state_private *rds,
> +                           const uint8_t *buffer, uint32_t size)
> +{
> +    uint8_t *slice_params;
> +    VASliceParameterBufferBase *slice_param;
> +
> +    if (!rds->slice_data)
> +        rds->slice_data = (uint8_t *)buffer;
> +    if (rds->slice_data + rds->slice_data_size != buffer) {
> +        if (commit_slices(rds) < 0)
> +            return NULL;
> +        rds->slice_data = (uint8_t *)buffer;
> +    }

why are the casts needed ? That is why is rds->slice_data not const?


[...]
> +int ff_vaapi_common_start_frame(AVCodecContext *avctx,
> +                                av_unused const uint8_t *buffer, av_unused uint32_t size)
> +{
> +    MpegEncContext * const s = avctx->priv_data;
> +    struct vaapi_render_state_private *rds;
> +
> +    dprintf(avctx, "ff_vaapi_common_start_frame()\n");
> +
> +    rds = ff_get_vaapi_render_state_private(s->current_picture_ptr);
> +    vaapi_render_state_init(rds, s->current_picture_ptr);
> +    return 0;
> +}

thats the only place from where vaapi_render_state_init is called and it
contains just 3 lines of code ...

[...]

> +/**
> + * This structure is used as a callback between the FFmpeg

callback?


> + * library and the client video application.
> + * This is defined by the client application prior to calling the

defined? i think "filled" maybe is a better word


> + * FFmpeg decode functions. The members are considered read-only from
> + * an FFmpeg point of view.
> + */
> +struct vaapi_context {
> +    void       *display;                ///< Window system dependent data
> +    uint32_t    config_id;              ///< Configuration ID
> +    uint32_t    context_id;             ///< Context ID (video decode pipeline)
> +};
> +

> +/**
> + * This structure is used as a callback between the FFmpeg
> + * library and the client video application.
> + * This is created by the client application prior to calling the
> + * FFmpeg decode functions. You must fill in both the surface and
> + * va_context fields from your AVCodecContext::get_buffer() function
> + * override.

"you" is poor terminology, who is "you" there the user app, there is lavc


> + */
> +struct vaapi_render_state {

> +    struct vaapi_context *va_context;   ///< Pointer to display-dependent data

why this pointer? why are the fields not added to this struct?


[...]
> +/** This structure holds all temporary data for VA API decoding */
> +struct vaapi_render_state_private {

> +    VADisplay       display;                ///< Window system dependent data (copied from vaapi_render_state->va_context)
> +    VAContextID     context_id;             ///< Context ID (video decode pipeline, copied from vaapi_render_state->va_context)
> +    VASurfaceID     surface;                ///< Rendering surface (copied from vaapi_render_state->va_context)

copying data is generally not a good idea, its a receipe for ending up
with different data and bugs


[...]
> +/** Extract the vaapi_render_state struct from a Picture */
> +static inline struct vaapi_render_state *ff_get_vaapi_render_state(Picture *pic)
> +{
> +    struct vaapi_render_state *rds = (struct vaapi_render_state *)pic->data[3];
> +    assert(rds);
> +    return rds;
> +}
> +
> +/** Extract the vaapi_render_state_private struct from a Picture */
> +static inline struct vaapi_render_state_private *ff_get_vaapi_render_state_private(Picture *pic)
> +{
> +    struct vaapi_render_state_private *rds = pic->hwaccel_data_private;
> +    assert(rds);
> +    return rds;
> +}

useless wraper functions


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

If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali
-------------- 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/20090311/804ee31b/attachment.pgp>