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

[Gwenole Beauchesne]

Le 18 mars 09 ? 19:37, Michael Niedermayer a ?crit :

> [...]
>> +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

Mmm, semantically, if there is only one check to keep, I'd like it to  
be the one in render_picture(), since it's its role to check for it,  
IMO. However, the check in the callee (ff_vaapi_common_end_frame()) is  
also useful to call ff_draw_horiz_band() or not. i.e. only if there is  
a frame and it was successfully rendered.

I can drop the check in render_picture(), it will just add a headache  
to me. ;-)

> [...]
>> +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?

If slice_count==0, commit_slices() can't be called because pp- 
 >slice_data==buffer && pp->slice_data_size==0.
But yes, if slice_count==0, commit_slices() now returns 0 immediately,  
which wasn't the case before.
So, the first if (!pp->slice_data) check could probably be dropped, is  
this what you meant? (unless buffer can be allocated from 0, or size  
be very large, which is very unlikely).

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

s/hwaccel_data_private/hwaccel_picture_private/ ?

>> +    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?

ff_vaapi_common_end_frame() is always called, but yes, I had an  
alternative with a temporary result var but it did not look  
particularly beautiful to me:

int r;
if ((r = commit_slices(avctx, s->current_picture_ptr)) >= 0) {
     if (pp->n_slice_buf_ids > 0 && ((r = render_picture(avctx, s- 
 >current_picture_ptr)) >= 0)
         ff_draw_horiz_band(s, 0, s->avctx->height);
}
[...]
return r;

would be OK with you?

> [...]
>> +/** This structure holds all temporary data for VA API decoding */
>
> this should mention where this struct is stored and by whom

This is internal API, nobody should care unless someone wants to add a  
new codec. I mean, some bits are filled in by vaapi_common.c to  
factorize things, and others by vaapi_codec.c, nothing else.

>> +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

How/where would you allocate it? The fields are progressively filled  
in by either start_frame(), or decode_slice() or end_frame(). So,  
storage somehow needs to be persistent between those calls. What  
mechanism can guarantee this without memory allocation? If memory  
allocation is a performance problem, we could very well write a pool  
based memory allocator with bins of specific sizes (ranges).

The minimum lifetime of that struct (and its fields, individually) is  
until vaRenderPicture().

However, for some codecs, this needs to be longer because I need to  
peek into another Picture's pic_param bits. e.g. for MPEG-4,  
VAPictureParameterBufferMPEG4.backward_reference_vop_coding_type, that  
is the "vop_coding_type" of the backward reference picture:

/** Reconstruct bitstream vop_coding_tye */
static inline int mpeg4_get_vop_coding_type(MpegEncContext *s)
{
     return s->pict_type - FF_I_TYPE;
}

/** Compute backward_reference_vop_coding_type (7.6.7) */
static inline int  
mpeg4_get_backward_reference_vop_coding_type(MpegEncContext *s)
{
     struct vaapi_picture_private * const pp = s- 
 >next_picture.hwaccel_data_private;
     if (s->pict_type != FF_B_TYPE)
         return 0;
     return pp->pic_param.mpeg4.vop_fields.bits.vop_coding_type;
}