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

[Gwenole Beauchesne]

Le 19 mars 09 ? 00:01, Michael Niedermayer a ?crit :

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

Hey, I'd rather keep the code as is because removing that check would  
require another pill for me. ;-)
(mainly because of the unlikely case I exposed -- unlikely does not  
mean risk-free to happen).

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

?

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

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  
the user clean up things allocated and only used by lavc internally.  
The rest are very minor things, and IMHO it's also better to keep data  
for the decoding task in a single location. i.e. if pic_param needs to  
be there anyway, just keep his friends with him in the same struct.