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

[Gwenole Beauchesne]

Le 6 mars 09 ? 20:06, Michael Niedermayer a ?crit :

>> +    if (rds->slice_data) {
>> +        if (!rds->mapped_slice_data)
>> +            av_free(rds->slice_data);
>> +        else
>> +            av_log(NULL, AV_LOG_ERROR,  
>> "ff_vaapi_render_state_fini(): VASliceDataBuffer for surface 0x%08x  
>> was not unmapped!\n", rds->surface);
>
> all av_log() calls should generally have a non NULL context (except  
> stuff
> just for debuging or when its really inconvenient to pass a contest  
> around)

This call fits exactly the purposes you mentioned. ;-) Nowadays, with  
the way vaapi_*.c are implemented, it should not be possible to reach  
this message, unless someone forgot to call common_{start,end}_frame().

>> +    /* Render the picture */
>> +    status = vaBeginPicture(rds->display, rds->context_id, rds- 
>> >surface);
>> +
>> +    if (status != VA_STATUS_SUCCESS)
>> +        return -1;
>
> the intermediate variable "status" seems useless

Its use is to avoid multiline if()'s as they won't be particularly  
good looking otherwise...

>> +    status = vaRenderPicture(rds->display, rds->context_id,
>> +                             &rds->slice_data_buf_id, 1);
>> +
>> +    if (status != VA_STATUS_SUCCESS)
>> +        return -1;
>
> it is intended to do the very same call several times?
> if so it should be a loop ...

It was for debugging. Actually, vaRenderPicture() takes an array of  
buffer ids. So, something like:

VABufferID va_buffers[5];
unsigned int n_va_buffers = 0;

va_buffers[n_va_buffers++] = rds->pic_param_buf_id;
if (rds->iq_matrix_buf_id)
va_buffers[n_va_buffers++] = rds->iq_matrix_buf_id;
va_buffers[n_va_buffers++] = rds->slice_param_buf_id;
[...]
if (vaRenderPicture(rds->display, rds->context_id, va_buffers,  
n_va_buffers) != VA_STATUS_SUCCESS)
     return -1;

would suit you? This is also what I am doing for "another API"...

> ..]
>> +/** Append slice data to temporary buffer, or server mapped buffer  
>> */
>> +int ff_vaapi_slice_data_append(struct vaapi_render_state_private  
>> *rds,
>> +                               const uint8_t *buffer, uint32_t size)
>> +{
>> +    dprintf(NULL, "ff_vaapi_slice_data_append(): buffer %p, %d  
>> bytes\n", buffer, size);
>> +
>> +    if (rds->mapped_slice_data) {
>> +        /* XXX: fallback to temporary slice data buffers? */
>> +        assert(rds->slice_data_size + size <= rds- 
>> >slice_data_size_max);
>
> is it certain that this assert is true? if yes, please explain
> if not the code is likely exploitable

rds->slice_data_size_max, in the "mapped" case, is the buf_size arg  
from start_frame(). So, if a codec passed a non-zero buf_size, I'd  
expect cumulated slices from it would fit this mapped slice_data  
buffer. Otherwise, the codec lied.

>> +    else {
>> +        if (rds->slice_data_size + size > rds- 
>> >slice_data_size_max) {
>> +            rds->slice_data_size_max += size;
>> +            rds->slice_data = av_realloc(rds->slice_data, rds- 
>> >slice_data_size_max);
>> +            if (!rds->slice_data)
>> +                return -1;
>> +        }
>
> memleak and possibly exploitable, at least the values are left in  
> inconsistant
> state.

Could you please explain the memleak part? If rds->slice_data ==  
NULL,  the memory is gone anyway.

>> +    }
>> +    memcpy(rds->slice_data + rds->slice_data_size, buffer, size);
>> +    rds->slice_data_size += size;
>
> also whats the point in allocating a buffer and memcpy into it that is
> compared to using the original?

We can be memcpy()'ing into mapped memory from HW accelerator. For the  
other case, this would imply fragmentation into several  
vaCreateBuffer() and additional work in codec implementations to cope  
with with those different buffers. Uh, I am not even sure it's  
possible to create several slice-data buffers and/or if the driver  
really supports that. You know, there are at least 3 "native" drivers  
and 2 fake (bridges) drivers supporting VA API. This starts to be a  
lot to check...

> [...]
>> +/** A magic number identifying VA API render structures */
>> +#define FF_VAAPI_RENDER_MAGIC 0x56415049 /* 'VAPI' */
>
> whats the point of this?
> Is the code designed to mix structures of several types?
> Is the code designed to work if it is passed a wrong kind of
> struct?

XvMC/VDPAU heritage + debugging while playing with various vd_ffmpeg.c  
arrangements. In normal operation, I can drop both magic and state  
members.