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

[Michael Niedermayer]

On Sat, Mar 07, 2009 at 06:43:07AM +0100, Gwenole Beauchesne wrote:
> Le 6 mars 09 ? 20:06, Michael Niedermayer a ?crit :
[...]
> >> +    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"...

yes, i like this more


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

please check for this by normal if()
i agree the codec would be buggy if it failed but it seems safer to check
this properly


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

realloc() returning NULL means the original is still there just that
it failed re allocating it


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

i dont mind memcpy() into memory from the accelerator, i do mind if this
could be memcpy into normal memory, it simply seems unneeded in that case
...

also glibc memcpy() is shit, even more so for copying ito non system memory
you should maybe look at mplayer which has some memcpy written for that.


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

please do drop them

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- 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/20090307/1367af57/attachment.pgp>