<div dir="ltr"><div>Hi, </div>Thx for your review. I'll submit a new version of the patch. Just replying here first to answer your questions:<div class="gmail_extra"> <div class="gmail_quote">On 19 October 2015 at 18:10, Emil Velikov <<a href="mailto:emil.l.velikov@gmail.com" target="_blank">emil.l.velikov@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 17 October 2015 at 00:14, Julien Isorce <<a href="mailto:julien.isorce@gmail.com" target="_blank">julien.isorce@gmail.com</a>> wrote: > Improve following functions to support VA_PROFILE_NONE profile (vpp): > vlVaQueryConfigProfiles > vlVaQueryConfigEntrypoints > vlVaCreateConfig > vlVaQueryConfigAttributes > > Add VADriverVTableVPP and improve following functions to support vpp: > vlVaCreateContext > vlVaDestroyContext > vlVaBeginPicture > vlVaRenderPicture > vlVaEndPicture > Please split into two patches - roughly as per above grouping or otherwise you feel is appropriate. </blockquote><div>oki I'll split as above. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div> > Add handleVAProcPipelineParameterBufferType helper. > > One of the application is: > VASurfaceNV12 -> gstvaapipostproc -> VASurfaceRGBA > > Signed-off-by: Julien Isorce <<a href="mailto:j.isorce@samsung.com" target="_blank">j.isorce@samsung.com</a>> > --- > src/gallium/state_trackers/va/config.c | 20 +++++++ > src/gallium/state_trackers/va/context.c | 94 +++++++++++++++++++----------- > src/gallium/state_trackers/va/picture.c | 89 +++++++++++++++++++++++++++- > src/gallium/state_trackers/va/surface.c | 73 +++++++++++++++++++++++ > src/gallium/state_trackers/va/va_private.h | 13 ++++- > 5 files changed, 254 insertions(+), 35 deletions(-) > > diff --git a/src/gallium/state_trackers/va/config.c b/src/gallium/state_trackers/va/config.c > index cfb0b25..bde6615 100644 > --- a/src/gallium/state_trackers/va/config.c > +++ b/src/gallium/state_trackers/va/config.c > @@ -52,6 +52,9 @@ vlVaQueryConfigProfiles(VADriverContextP ctx, VAProfile *profile_list, int *num_ > profile_list[(*num_profiles)++] = vap; > } > > + /* Support postprocessing through vl_compositor */ > + profile_list[(*num_profiles)++] = VAProfileNone; > + > return VA_STATUS_SUCCESS; > } > > @@ -67,6 +70,11 @@ vlVaQueryConfigEntrypoints(VADriverContextP ctx, VAProfile profile, > > *num_entrypoints = 0; > > + if (profile == VAProfileNone) { > + entrypoint_list[(*num_entrypoints)++] = VAEntrypointVideoProc; > + return VA_STATUS_SUCCESS; > + } > + > p = ProfileToPipe(profile); > if (p == PIPE_VIDEO_PROFILE_UNKNOWN) > return VA_STATUS_ERROR_UNSUPPORTED_PROFILE; > @@ -118,6 +126,11 @@ vlVaCreateConfig(VADriverContextP ctx, VAProfile profile, VAEntrypoint entrypoin > if (!ctx) > return VA_STATUS_ERROR_INVALID_CONTEXT; > > + if (profile == VAProfileNone && entrypoint == VAEntrypointVideoProc) { > + *config_id = PIPE_VIDEO_PROFILE_UNKNOWN; > + return VA_STATUS_SUCCESS; > + } > + > p = ProfileToPipe(profile); > if (p == PIPE_VIDEO_PROFILE_UNKNOWN) > return VA_STATUS_ERROR_UNSUPPORTED_PROFILE; > @@ -151,6 +164,13 @@ vlVaQueryConfigAttributes(VADriverContextP ctx, VAConfigID config_id, VAProfile > return VA_STATUS_ERROR_INVALID_CONTEXT; > > *profile = PipeToProfile(config_id); > + > + if (config_id == PIPE_VIDEO_PROFILE_UNKNOWN) { > + *entrypoint = VAEntrypointVideoProc; > + *num_attribs = 0; > + return VA_STATUS_SUCCESS; > + } > + > *entrypoint = VAEntrypointVLD; > > *num_attribs = 1; > diff --git a/src/gallium/state_trackers/va/context.c b/src/gallium/state_trackers/va/context.c > index 8949d42..ddc863b 100644 > --- a/src/gallium/state_trackers/va/context.c > +++ b/src/gallium/state_trackers/va/context.c > @@ -87,6 +87,14 @@ static struct VADriverVTable vtable = > &vlVaQuerySurfaceAttributes > }; > > +static struct VADriverVTableVPP vtable_vpp = > +{ > + VA_DRIVER_VTABLE_VPP_VERSION, </div></div>Please _never_ do such a thing. Afaict the define provides the VERSION currently defined in the API, rather than the one implemented here. They might align now, but things will break badly as the API gets updated. </blockquote><div> </div><div>oki I'll put just 1 then. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + &vlVaQueryVideoProcFilters, > + &vlVaQueryVideoProcFilterCaps, > + &vlVaQueryVideoProcPipelineCaps > +}; > + > PUBLIC VAStatus > VA_DRIVER_INIT_FUNC(VADriverContextP ctx) > { > @@ -122,6 +130,7 @@ VA_DRIVER_INIT_FUNC(VADriverContextP ctx) > ctx->version_major = 0; > ctx->version_minor = 1; > *ctx->vtable = vtable; > + *ctx->vtable_vpp = vtable_vpp; > ctx->max_profiles = PIPE_VIDEO_PROFILE_MPEG4_AVC_HIGH - PIPE_VIDEO_PROFILE_UNKNOWN; > ctx->max_entrypoints = 1; > ctx->max_attributes = 1; > @@ -151,11 +160,16 @@ vlVaCreateContext(VADriverContextP ctx, VAConfigID config_id, int picture_width, > struct pipe_video_codec templat = {}; > vlVaDriver *drv; > vlVaContext *context; > + int is_vpp = 0; Drop the = 0 part. > > if (!ctx) > return VA_STATUS_ERROR_INVALID_CONTEXT; > > - if (!(picture_width && picture_height)) > + is_vpp = config_id == PIPE_VIDEO_PROFILE_UNKNOWN && > + picture_width == 0 && picture_height == 0 && flag ==0 && !render_targets > + && num_render_targets == 0; Please indent - all the conditionals should start at the same column and nuke the == 0. <div><div> > + > + if (!(picture_width && picture_height) && !is_vpp) > return VA_STATUS_ERROR_INVALID_IMAGE_FORMAT; > > drv = VL_VA_DRIVER(ctx); > @@ -163,38 +177,48 @@ vlVaCreateContext(VADriverContextP ctx, VAConfigID config_id, int picture_width, > if (!context) > return VA_STATUS_ERROR_ALLOCATION_FAILED; > > - templat.profile = config_id; > - templat.entrypoint = PIPE_VIDEO_ENTRYPOINT_BITSTREAM; > - templat.chroma_format = PIPE_VIDEO_CHROMA_FORMAT_420; > - templat.width = picture_width; > - templat.height = picture_height; > - templat.max_references = num_render_targets; > - templat.expect_chunked_decode = true; > - > - if (u_reduce_video_profile(templat.profile) == > - PIPE_VIDEO_FORMAT_MPEG4_AVC) > - templat.level = u_get_h264_level(templat.width, templat.height, > - &templat.max_references); > - > - context->decoder = drv->pipe->create_video_codec(drv->pipe, &templat); > - if (!context->decoder) { > - FREE(context); > - return VA_STATUS_ERROR_ALLOCATION_FAILED; > - } > - > - if (u_reduce_video_profile(context->decoder->profile) == > - PIPE_VIDEO_FORMAT_MPEG4_AVC) { > - context->desc.h264.pps = CALLOC_STRUCT(pipe_h264_pps); > - if (!context->desc.h264.pps) { > + if (is_vpp) { > + context->decoder = NULL; > + if (!drv->compositor.upload) { > FREE(context); > - return VA_STATUS_ERROR_ALLOCATION_FAILED; > + return VA_STATUS_ERROR_INVALID_CONTEXT; > } > - context->desc.h264.pps->sps = CALLOC_STRUCT(pipe_h264_sps); > - if (!context->desc.h264.pps->sps) { > - FREE(context->desc.h264.pps); > + } else { > + templat.profile = config_id; > + templat.entrypoint = PIPE_VIDEO_ENTRYPOINT_BITSTREAM; > + templat.chroma_format = PIPE_VIDEO_CHROMA_FORMAT_420; > + templat.width = picture_width; > + templat.height = picture_height; > + templat.max_references = 2; > + templat.expect_chunked_decode = true; > + > + if (u_reduce_video_profile(templat.profile) == > + PIPE_VIDEO_FORMAT_MPEG4_AVC) { > + templat.max_references = 16; > + templat.level = u_get_h264_level(templat.width, templat.height, > + &templat.max_references); > + } > + </div></div>Why the max_references changes ? Shouldn't those be a separate patch ? > + context->decoder = drv->pipe->create_video_codec(drv->pipe, &templat); > + if (!context->decoder) { > FREE(context); > return VA_STATUS_ERROR_ALLOCATION_FAILED; > } > + > + if (u_reduce_video_profile(context->decoder->profile) == > + PIPE_VIDEO_FORMAT_MPEG4_AVC) { > + context->desc.h264.pps = CALLOC_STRUCT(pipe_h264_pps); > + if (!context->desc.h264.pps) { > + FREE(context); > + return VA_STATUS_ERROR_ALLOCATION_FAILED; > + } > + context->desc.h264.pps->sps = CALLOC_STRUCT(pipe_h264_sps); > + if (!context->desc.h264.pps->sps) { > + FREE(context->desc.h264.pps); > + FREE(context); (Mildly related) Keeping track of all these might not fair well. Most of st/va and other video st do this, yet (iirc) Christian did not see any issues with opting for goto statements. </blockquote><div>Do you want me to do something here ? Not sure to follow, it is just indenting existing code. I'll prefer not to change anything here just in case :) </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div> > + return VA_STATUS_ERROR_ALLOCATION_FAILED; > + } > + } > } > > context->desc.base.profile = config_id; > @@ -214,12 +238,16 @@ vlVaDestroyContext(VADriverContextP ctx, VAContextID context_id) > > drv = VL_VA_DRIVER(ctx); > context = handle_table_get(drv->htab, context_id); > - if (u_reduce_video_profile(context->decoder->profile) == > - PIPE_VIDEO_FORMAT_MPEG4_AVC) { > - FREE(context->desc.h264.pps->sps); > - FREE(context->desc.h264.pps); > + > + if (context->decoder) { > + if (u_reduce_video_profile(context->decoder->profile) == > + PIPE_VIDEO_FORMAT_MPEG4_AVC) { > + FREE(context->desc.h264.pps->sps); > + FREE(context->desc.h264.pps); > + } > + context->decoder->destroy(context->decoder); > } > - context->decoder->destroy(context->decoder); > + > FREE(context); > handle_table_remove(drv->htab, context_id); > > diff --git a/src/gallium/state_trackers/va/picture.c b/src/gallium/state_trackers/va/picture.c > index 9b94b39..0b42afe 100644 > --- a/src/gallium/state_trackers/va/picture.c > +++ b/src/gallium/state_trackers/va/picture.c > @@ -32,6 +32,7 @@ > #include "util/u_video.h" > > #include "vl/vl_vlc.h" > +#include "vl/vl_winsys.h" > > #include "va_private.h" > > @@ -58,6 +59,16 @@ vlVaBeginPicture(VADriverContextP ctx, VAContextID context_id, VASurfaceID rende > return VA_STATUS_ERROR_INVALID_SURFACE; > > context->target = surf->buffer; > + > + if (!context->decoder) { > + /* VPP */ > + if ((context->target->buffer_format != PIPE_FORMAT_B8G8R8A8_UNORM && > + context->target->buffer_format != PIPE_FORMAT_R8G8B8A8_UNORM) || > + context->target->interlaced) > + return VA_STATUS_ERROR_UNIMPLEMENTED; > + return VA_STATUS_SUCCESS; > + } > + > context->decoder->begin_frame(context->decoder, context->target, NULL); > > return VA_STATUS_SUCCESS; > @@ -521,11 +532,79 @@ handleVASliceDataBufferType(vlVaContext *context, vlVaBuffer *buf) > num_buffers, (const void * const*)buffers, sizes); > } > > +static VAStatus > +handleVAProcPipelineParameterBufferType(vlVaDriver *drv, vlVaContext *context, vlVaBuffer *buf) > +{ > + struct u_rect src_rect = {0}; > + struct u_rect dst_rect = {0}; > + struct u_rect *dirty_area = NULL; > + vlVaSurface *src_surface = NULL; > + VAProcPipelineParameterBuffer *pipeline_param = NULL; > + struct pipe_surface **surfaces = NULL; > + struct pipe_screen *screen = NULL; > + struct pipe_surface *psurf = NULL; </div></div>Cough :) <div><div> > + > + if (!drv || !context) > + return VA_STATUS_ERROR_INVALID_CONTEXT; > + > + if (!buf || !buf->data) > + return VA_STATUS_ERROR_INVALID_BUFFER; > + > + if (!context->target) > + return VA_STATUS_ERROR_INVALID_SURFACE; > + > + pipeline_param = buf->data; // cast > + > + src_surface = handle_table_get(drv->htab, pipeline_param->surface); > + if (!src_surface || !src_surface->buffer) > + return VA_STATUS_ERROR_INVALID_SURFACE; > + > + screen = drv->pipe->screen; > + > + if(src_surface->fence) { > + screen->fence_finish(screen, src_surface->fence, PIPE_TIMEOUT_INFINITE); > + screen->fence_reference(screen, &src_surface->fence, NULL); > + } > + > + surfaces = context->target->get_surfaces(context->target); > + > + if (!surfaces) </div></div>if (!surfaces || !surfaces[0]) ? <div><div> </div></div></blockquote><div>oki, it will save some lines. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div> > + return VA_STATUS_ERROR_INVALID_SURFACE; > + > + psurf = surfaces[0]; > + > + if (!psurf) > + return VA_STATUS_ERROR_INVALID_SURFACE; > + > + src_rect.x0 = pipeline_param->surface_region->x; > + src_rect.y0 = pipeline_param->surface_region->y; > + src_rect.x1 = pipeline_param->surface_region->x + pipeline_param->surface_region->width; > + src_rect.y1 = pipeline_param->surface_region->y + pipeline_param->surface_region->height; > + > + dst_rect.x0 = pipeline_param->output_region->x; > + dst_rect.y0 = pipeline_param->output_region->y; > + dst_rect.x1 = pipeline_param->output_region->x + pipeline_param->output_region->width; > + dst_rect.y1 = pipeline_param->output_region->y + pipeline_param->output_region->height; > + > + dirty_area = vl_screen_get_dirty_area(drv->vscreen); > + > + vl_compositor_clear_layers(&drv->cstate); > + vl_compositor_set_buffer_layer(&drv->cstate, &drv->compositor, 0, src_surface->buffer, &src_rect, NULL, VL_COMPOSITOR_WEAVE); > + vl_compositor_set_layer_dst_area(&drv->cstate, 0, &dst_rect); > + vl_compositor_render(&drv->cstate, &drv->compositor, psurf, dirty_area, true); > + > + screen->fence_reference(screen, &src_surface->fence, NULL); > + drv->pipe->flush(drv->pipe, &src_surface->fence, 0); > + > + return VA_STATUS_SUCCESS; > +} > + > VAStatus > vlVaRenderPicture(VADriverContextP ctx, VAContextID context_id, VABufferID *buffers, int num_buffers) > { > vlVaDriver *drv; > vlVaContext *context; > + VAStatus vaStatus = VA_STATUS_SUCCESS; > > unsigned i; > > @@ -561,13 +640,16 @@ vlVaRenderPicture(VADriverContextP ctx, VAContextID context_id, VABufferID *buff > case VASliceDataBufferType: > handleVASliceDataBufferType(context, buf); > break; > + case VAProcPipelineParameterBufferType: > + vaStatus = handleVAProcPipelineParameterBufferType(drv, context, buf); > + break; > > default: > break; > } > } > > - return VA_STATUS_SUCCESS; > + return vaStatus; > } > > VAStatus > @@ -587,6 +669,11 @@ vlVaEndPicture(VADriverContextP ctx, VAContextID context_id) > if (!context) > return VA_STATUS_ERROR_INVALID_CONTEXT; > > + if (!context->decoder) { > + /* VPP */ > + return VA_STATUS_SUCCESS; > + } > + > context->mpeg4.frame_num++; > context->decoder->end_frame(context->decoder, context->target, &context->desc.base); > > diff --git a/src/gallium/state_trackers/va/surface.c b/src/gallium/state_trackers/va/surface.c > index eb5b8ca..bfa4803 100644 > --- a/src/gallium/state_trackers/va/surface.c > +++ b/src/gallium/state_trackers/va/surface.c > @@ -625,3 +625,76 @@ no_res: > > return VA_STATUS_ERROR_ALLOCATION_FAILED; > } > + > +VAStatus > +vlVaQueryVideoProcFilters(VADriverContextP ctx, VAContextID context, > + VAProcFilterType *filters, unsigned int *num_filters) > +{ > + unsigned int num = 0; > + > + if (!ctx) > + return VA_STATUS_ERROR_INVALID_CONTEXT; > + > + if (!num_filters || !filters) > + return VA_STATUS_ERROR_INVALID_PARAMETER; > + > + filters[num++] = VAProcFilterNone; > + > + *num_filters = num; > + > + return VA_STATUS_SUCCESS; > +} > + > +VAStatus > +vlVaQueryVideoProcFilterCaps(VADriverContextP ctx, VAContextID context, > + VAProcFilterType type, void *filter_caps, > + unsigned int *num_filter_caps) > +{ > + if (!ctx) > + return VA_STATUS_ERROR_INVALID_CONTEXT; > + > + return VA_STATUS_ERROR_UNIMPLEMENTED; </div></div>Something looks fishy here. Shouldn't one return SUCCESS or ENOTSUPP like error, when we have FilterNone ? </blockquote><div>Your are right, I was only testing with gstreamer-vaapi and it seems to really this function only for other VPP than VAProcFilterNone. But I am going to provide a minimal impl just to handle FilterNone. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div> > +} > + > +static VAProcColorStandardType vpp_input_color_standards[VAProcColorStandardCount] = { > + VAProcColorStandardBT601 > +}; > + > +static VAProcColorStandardType vpp_output_color_standards[VAProcColorStandardCount] = { > + VAProcColorStandardBT601 > +}; > + > +VAStatus > +vlVaQueryVideoProcPipelineCaps(VADriverContextP ctx, VAContextID context, > + VABufferID *filters, unsigned int num_filters, > + VAProcPipelineCaps *pipeline_cap) > +{ > + unsigned int i = 0; > + > + if (!ctx) > + return VA_STATUS_ERROR_INVALID_CONTEXT; > + > + if (!pipeline_cap) > + return VA_STATUS_ERROR_INVALID_PARAMETER; > + > + if (num_filters && !filters) > + return VA_STATUS_ERROR_INVALID_PARAMETER; > + > + pipeline_cap->pipeline_flags = 0; > + pipeline_cap->filter_flags = 0; > + pipeline_cap->num_forward_references = 0; > + pipeline_cap->num_backward_references = 0; > + pipeline_cap->num_input_color_standards = 1; > + pipeline_cap->input_color_standards = vpp_input_color_standards; > + pipeline_cap->num_output_color_standards = 1; > + pipeline_cap->output_color_standards = vpp_output_color_standards; > + > + for (i = 0; i < num_filters; i++) { > + vlVaBuffer *buf = handle_table_get(VL_VA_DRIVER(ctx)->htab, filters[i]); > + > + if (!buf || buf->type >= VABufferTypeMax) > + return VA_STATUS_ERROR_INVALID_BUFFER; > + } > + > + return VA_STATUS_SUCCESS; > +} > diff --git a/src/gallium/state_trackers/va/va_private.h b/src/gallium/state_trackers/va/va_private.h > index 2cdd787..d3a29bc 100644 > --- a/src/gallium/state_trackers/va/va_private.h > +++ b/src/gallium/state_trackers/va/va_private.h > @@ -33,6 +33,7 @@ > > #include <va/va.h> > #include <va/va_backend.h> > +#include <va/va_backend_vpp.h> > #include <va/va_drmcommon.h> > > #include "pipe/p_video_enums.h" > @@ -148,7 +149,8 @@ PipeToProfile(enum pipe_video_profile profile) > case PIPE_VIDEO_PROFILE_MPEG4_AVC_HIGH: > return VAProfileH264High; > case PIPE_VIDEO_PROFILE_MPEG4_AVC_EXTENDED: > - return VAProfileNone; > + case PIPE_VIDEO_PROFILE_UNKNOWN: > + return VAProfileNone; > default: > assert(0); > return -1; > @@ -179,6 +181,8 @@ ProfileToPipe(VAProfile profile) > return PIPE_VIDEO_PROFILE_MPEG4_AVC_MAIN; > case VAProfileH264High: > return PIPE_VIDEO_PROFILE_MPEG4_AVC_HIGH; > + case VAProfileNone: > + return PIPE_VIDEO_PROFILE_UNKNOWN; </div></div>Mapping ProfileNone to PROFILE_UNKNOW and vice versa (incl. all the uses above) seems like an abuse ? </blockquote><div> </div><div>Yeah I have not found a better solution. There is no PIPE_VIDEO_PROFILE_NONE so I picked PIPE_VIDEO_PROFILE_UNKNOWN. </div><div>At least vlVaCreateConfig returns PIPE_VIDEO_PROFILE_UNKNOWN only for VAEntrypointVideoProc. </div><div> </div><div>Cheers </div><div>Julien </div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Cheers Emil </blockquote></div> </div></div>