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

Michael Niedermayer michaelni
Thu Mar 19 00:01:59 CET 2009


On Wed, Mar 18, 2009 at 11:43:14PM +0100, Gwenole Beauchesne wrote:
> 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. ;-)

take your favorite headache (placebo) pill ;)


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

i didnt mean anything beyond what i said, that is that it looks odd, i
did not investigate what exactly would be the best solution


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

yes


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

no but a goto would


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

there are plenty of people who want to add new codecs
and just at the very moment i curse myself for not documenting what code
needed a funny relation between luma and chroma strides ...
so yes please document it, its internal but its usefull to us and in
10 years even you might not immedeatly remember ...


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

i felt it would fit better in AVCodecContext.hwaccel_context


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

my suggestion was not about pic_param, just the parts that are short
lived


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

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- 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/20090319/92d74bc1/attachment.pgp>



More information about the ffmpeg-devel mailing list