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

[Michael Niedermayer]

On Thu, Mar 19, 2009 at 12:44:06AM +0100, Gwenole Beauchesne wrote:
> Le 19 mars 09 ? 00:01, Michael Niedermayer a ?crit :
[...]
> >>>> +    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
> 
> int r = -1;
> if (commit_slices(...) < 0)
>      goto done;
> [...]
> r = 0;
> done:
> [... free stuff ...]
> return r;
> 
> ?

yes
and s/r/ret/


> 
> I don't really see how to do that without a temp var. If you know,  
> please tell.
> 
> >>>> +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
> 
> I'd prefer keep hwaccel_context (for VA API case) read-only from an  
> FFmpeg POV, wasn't that what you intended too?

that was just your idea :)


> I mean, having bits of  
> a struct filled in by either the user app or by lavc (as temp data)  
> could be confusing. The goal was also to not expose internal data to  
> the user, even though it's somewhat short-lived, and user normally has  
> zero chance to disturb this process.

the struct could even be split ...


> 
> I don't mind though I finally found the current separation/approach  
> quite elegant.
> 

> Besides, if the temporary bits that are lazily-allocated (e.g.  
> slice_params) are moved to hwaccel_context, we'd need another  
> AVCodecContext member function to clean that up. i.e. no, I don't want  

thats no problem


anyway iam not insisting on this, it just feels strange to  have "local"
variables in AVFrame.*

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

Democracy is the form of government in which you can choose your dictator
-------------- 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/51c5aa27/attachment.pgp>