[FFmpeg-devel] [PATCH][VAAPI][2/6] Add common data structures and helpers (take 3)
Michael Niedermayer
michaelni
Fri Mar 6 20:06:42 CET 2009
On Fri, Mar 06, 2009 at 11:20:25AM +0100, Gwenole Beauchesne wrote:
> Hi,
>
> New patch attached, sync'ed to current patch queue and SVN.
>
>>> The buf/buf_size args to AVHWAccel::start_frame() are particularly useful
>>> for optimization purposes. Whenever possible, slice data chunks are
>>> directly copied to mapped HW memory. Otherwise, they are accumulated to a
>>> temporary buffer that will be copied in end_frame().
>>> - vaapi_slice_data_prepare() [usually called in ::start_frame()]
>>> - vaapi_slice_data_append() [usually called in ::decode_slice()]
>>> - vaapi_slice_data_commit() [usually called in ::end_frame()]
>>> The same happens for slice control blocks. If we can predetermine the
>>> number of slices, we can map the whole slice params buffer from HW memory
>>> space. Otherwise, they are accumulated to a temporary buffer that is also
>>> copied in end_frame().
>>> - vaapi_slice_params_prepare() [usually called in ::start_frame()]
>>> - vaapi_slice_params_next() [usually called in ::decode_slice()]
>>> - vaapi_slice_params_commit() [usually called in ::end_frame()]
>>> Other helpers include:
>>> - vaapi_common_end_frame(): common commit code to the HW accelerator
>>> - vaapi_render_picture(): doing the actual rendering
>>
>> - ff_vaapi_destroy_picture(): implements AVHWAccel::close() for
>> hwaccel_data_private destruction.
>
> Removed since hwaccel_data_private allocation/destruction is controlled by
> {alloc,free}_frame_buffer() only.
>
> Renamed ff_get_vaapi_render_state() to ff_get_vaapi_render_state_private()
> for consistency and updated the former to use AVFrame.data[3].
[...]
> +/** Initialize VA API render state */
> +int ff_vaapi_render_state_init(struct vaapi_render_state_private *rds, Picture *pic)
> +{
doxy should generally be in the header file if there is one
> + struct vaapi_render_state * const r = ff_get_vaapi_render_state(pic);
> +
> + assert(r && r->va_context);
> + rds->display = r->va_context->display;
> + rds->context_id = r->va_context->context_id;
> + rds->surface = r->surface;
> + rds->pic_param_buf_id = 0;
> + rds->iq_matrix_buf_id = 0;
> + rds->bitplane_buf_id = 0;
> + rds->slice_param_buf_id = 0;
> + rds->slice_data_buf_id = 0;
> + return 0;
> +}
> +
> +/** Destroy VA API render state buffers */
> +int ff_vaapi_render_state_fini(struct vaapi_render_state_private *rds)
> +{
> + if (rds->pic_param_buf_id) {
> + vaDestroyBuffer(rds->display, rds->pic_param_buf_id);
> + rds->pic_param_buf_id = 0;
> + }
> + if (rds->iq_matrix_buf_id) {
> + vaDestroyBuffer(rds->display, rds->iq_matrix_buf_id);
> + rds->iq_matrix_buf_id = 0;
> + }
> + if (rds->bitplane_buf_id) {
> + vaDestroyBuffer(rds->display, rds->bitplane_buf_id);
> + rds->bitplane_buf_id = 0;
> + }
> + if (rds->slice_param_buf_id) {
> + vaDestroyBuffer(rds->display, rds->slice_param_buf_id);
> + rds->slice_param_buf_id = 0;
> + }
> + if (rds->slice_data_buf_id) {
> + vaDestroyBuffer(rds->display, rds->slice_data_buf_id);
> + rds->slice_data_buf_id = 0;
> + }
VA should have a function or macro that does destruction & =NULL ...
> + 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)
[...]
> + /* 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
> +
> + status = vaRenderPicture(rds->display, rds->context_id,
> + &rds->pic_param_buf_id, 1);
> +
> + if (status != VA_STATUS_SUCCESS)
> + return -1;
same and many more below ...
> +
> + if (rds->iq_matrix_buf_id) {
> + status = vaRenderPicture(rds->display, rds->context_id,
> + &rds->iq_matrix_buf_id, 1);
> +
> + if (status != VA_STATUS_SUCCESS)
> + return -1;
> + }
> +
> + if (rds->bitplane_buf_id) {
> + status = vaRenderPicture(rds->display, rds->context_id,
> + &rds->bitplane_buf_id, 1);
> +
> + if (status != VA_STATUS_SUCCESS)
> + return -1;
> + }
> +
> + status = vaRenderPicture(rds->display, rds->context_id,
> + &rds->slice_param_buf_id, 1);
> +
> + if (status != VA_STATUS_SUCCESS)
> + return -1;
> +
> + 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 ...
[..]
> +/** 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
> + }
> + 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.
> + }
> + 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?
[...]
> + rds->slice_params = av_realloc(rds->slice_params,
> + rds->slice_count_max * rds->slice_param_size);
> + }
> + }
> + assert(rds->slice_params);
this use of assert is invalid, you cannot use assert () to check for
error conditions.
[...]
> +/** 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?
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- 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/20090306/6c2761c6/attachment.pgp>
More information about the ffmpeg-devel
mailing list