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

[Michael Niedermayer]

On Wed, Mar 18, 2009 at 03:33:59PM +0100, Gwenole Beauchesne wrote:
> On Thu, 12 Mar 2009, Michael Niedermayer wrote:
>
>> [...]
>>> +    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
>
> Resend. Did all changes even this one though I prefer grouping code by 
> semantics rather than by cosmetics... slice_params kept as is though. 
> Reminder: pointer arith on void * is not possible and 
> sizeof(VASliceParameterBufferBase) != sizeof(VASliceParameterBufferCODEC).
>
> BTW, also fixed commit_slices() to commit slices only if there are actually 
> ones pending.

[...]
> +static int render_picture(AVCodecContext *avctx, Picture *pic)
> +{
> +    const struct vaapi_context * const va_context = avctx->hwaccel_context;
> +    struct vaapi_picture_private * const pp = pic->hwaccel_data_private;
> +    VABufferID va_buffers[3];
> +    unsigned int n_va_buffers = 0;
> +
> +    if (pp->n_slice_buf_ids == 0)
> +        return -1;

the only point from where this is called checks n_slice_buf_ids > 0 before


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

can commit_slices() here be called with slice_count=0 ?
if so does that work?


> +
> +    slice_params =
> +        av_fast_realloc(pp->slice_params,
> +                        &pp->slice_params_alloc,
> +                        (pp->slice_count + 1) * pp->slice_param_size);
> +    if (!slice_params)
> +        return NULL;
> +    pp->slice_params = slice_params;
> +
> +    slice_param = (VASliceParameterBufferBase *)(slice_params + pp->slice_count * pp->slice_param_size);
> +    slice_param->slice_data_size   = size;
> +    slice_param->slice_data_offset = pp->slice_data_size;
> +    slice_param->slice_data_flag   = VA_SLICE_DATA_FLAG_ALL;
> +
> +    pp->slice_count++;
> +    pp->slice_data_size += size;
> +    return slice_param;
> +}
> +
> +int ff_vaapi_common_end_frame(AVCodecContext *avctx)
> +{
> +    MpegEncContext * const s = avctx->priv_data;
> +    const struct vaapi_context * const va_context = avctx->hwaccel_context;

> +    struct vaapi_picture_private * const pp = s->current_picture_ptr->hwaccel_data_private;
                    ^^^^^^^                                                      ^^^^
inconsistent names


> +
> +    dprintf(avctx, "ff_vaapi_common_end_frame()\n");
> +
> +    if (commit_slices(avctx, s->current_picture_ptr) < 0)
> +        return -1;
> +    if (pp->n_slice_buf_ids > 0) {
> +        if (render_picture(avctx, s->current_picture_ptr) < 0)
> +            return -1;
> +        ff_draw_horiz_band(s, 0, s->avctx->height);
> +    }
> +
> +    destroy_buffers(va_context->display, &pp->pic_param_buf_id, 1);
> +    destroy_buffers(va_context->display, &pp->iq_matrix_buf_id, 1);
> +    destroy_buffers(va_context->display, &pp->bitplane_buf_id, 1);
> +    destroy_buffers(va_context->display, pp->slice_buf_ids, pp->n_slice_buf_ids);
> +    av_freep(&pp->bitplane_buffer);
> +    av_freep(&pp->slice_buf_ids);
> +    av_freep(&pp->slice_params);

do the return -1 above lead to memleaks?

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

this should mention where this struct is stored and by whom


> +struct vaapi_picture_private {
> +    VABufferID      pic_param_buf_id;       ///< VAPictureParameterBuffer ID
> +    VABufferID      iq_matrix_buf_id;       ///< VAIQMatrixBuffer ID
> +    VABufferID      bitplane_buf_id;        ///< VABitPlaneBuffer ID (for VC-1 decoding)
> +    VABufferID     *slice_buf_ids;          ///< Slice parameter/data buffer IDs
> +    unsigned int    n_slice_buf_ids;        ///< Number of effective slice buffer IDs to send to the HW
> +    unsigned int    slice_buf_ids_alloc;    ///< Size of pre-allocated slice_buf_ids
> +    union {
> +        VAPictureParameterBufferMPEG2 mpeg2;
> +        VAPictureParameterBufferMPEG4 mpeg4;
> +        VAPictureParameterBufferH264  h264;
> +        VAPictureParameterBufferVC1   vc1;
> +    } pic_param;                            ///< Picture parameter buffer
> +    unsigned int    pic_param_size;         ///< Size of a VAPictureParameterBuffer element
> +    union {
> +        VAIQMatrixBufferMPEG2         mpeg2;
> +        VAIQMatrixBufferMPEG4         mpeg4;
> +        VAIQMatrixBufferH264          h264;
> +    } iq_matrix;                            ///< Inverse quantization matrix buffer
> +    unsigned int    iq_matrix_size;         ///< Size of a VAIQMatrixBuffer element
> +    uint8_t         iq_matrix_present;      ///< Flag: is quantization matrix present?

> +    uint8_t        *bitplane_buffer;        ///< VC-1 bitplane buffer
> +    unsigned int    bitplane_buffer_size;   ///< Size of VC-1 bitplane buffer

the way i understand it, is that this stuff is filled in and then
decoded by the hardware before filling the next in, and it isnt used
otherwise, if so it is like a local context and does not really need to
be allocated and stored per picture
but maybe there are some concurency/ race issues why that is needed?

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

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- 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/20090318/ce32788c/attachment.pgp>