Hi,
Thx for your review. I'll submit a new version of the patch. Just replying
here first to answer your questions:
On 19 October 2015 at 18:10, Emil Velikov <emil.l.veli...@gmail.com> wrote:
> On 17 October 2015 at 00:14, Julien Isorce <julien.iso...@gmail.com>
> 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.
>
oki I'll split as above.
>
> > Add handleVAProcPipelineParameterBufferType helper.
> >
> > One of the application is:
> > VASurfaceNV12 -> gstvaapipostproc -> VASurfaceRGBA
> >
> > Signed-off-by: Julien Isorce <j.iso...@samsung.com>
> > ---
> > 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,
> 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.
>
oki I'll put just 1 then.
>
> > + &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.
>
> > +
> > + 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);
> > + }
> > +
> 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.
>
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 :)
>
> > + 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;
> Cough :)
>
> > +
> > + 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)
> if (!surfaces || !surfaces[0]) ?
>
> oki, it will save some lines.
> > + 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;
> Something looks fishy here. Shouldn't one return SUCCESS or ENOTSUPP
> like error, when we have FilterNone ?
>
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.
>
> > +}
> > +
> > +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;
> Mapping ProfileNone to PROFILE_UNKNOW and vice versa (incl. all the
> uses above) seems like an abuse ?
>
Yeah I have not found a better solution. There is no
PIPE_VIDEO_PROFILE_NONE so I picked PIPE_VIDEO_PROFILE_UNKNOWN.
At least vlVaCreateConfig returns PIPE_VIDEO_PROFILE_UNKNOWN only for
VAEntrypointVideoProc.
Cheers
Julien
>
>
> Cheers
> Emil
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
