[FFmpeg-devel] [PATCH][VAAPI][2/6] Add common data structures and helpers (take 12)
Gwenole Beauchesne
gbeauchesne
Wed Mar 18 23:43:14 CET 2009
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;
}
More information about the ffmpeg-devel
mailing list