<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">Am 20.07.2016 um 20:00 schrieb Zhang, Boyuan: </div> <blockquote cite="mid:CY4PR12MB1127DBA7EB3A95E05937F35787080@CY4PR12MB1127.namprd12.prod.outlook.com" type="cite"> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> <meta name="Generator" content="Microsoft Word 14 (filtered medium)">  <style></style> <div class="WordSection1"> >Makes sense, but I suggest that in this case we should add at least a comment why this is still disabled.<o:p></o:p> >And it would look better if we have an "#if 0" or something like this in the code which gets explicitly removed with the last patch.<o:p></o:p> <o:p> </o:p> Sure, I agree. I will submit a new patch set to add this and other minor changes.<o:p></o:p> <o:p> </o:p> >The problem with slice level encoding is that we haven't fully implemented it. E.g. the last time I checked the h264encode test it would try to add an SPS and PPS in front of the slice data returned from our VA-API driver.<o:p></o:p> >Since our VA-API driver doesn't return slice data, but rather a full blown elementary stream you end up with a complete mess which looks something like this:<o:p></o:p> >SPS (added by the application), PPS (added by the application), Slice Header, SPS (added by the driver), PPS(added by the driver), Slice Header/Slice Data.<o:p></o:p> >That might work in some if not most cases, but is certainly not complaint to the VA-API specification.<o:p></o:p> ><o:p> </o:p> >Christian.<o:p></o:p> <o:p> </o:p> I just tried to disable slice encoding support, and even Gstreamer is not working. It will give error message saying “unsupported HW profile”. <o:p></o:p> On the other hand, by exposing slice encoding, Gstreamer will still work as “Frame-in, Frame-out” mode. I didn’t see that Gstreamer will add extra headers. And by dumping the output 264 output, it seems gstreamer is using picture encoding. However, as my test shown, gstreamer will not work at all if we don’t expose slice encoding. Do you have any suggestions how we should do for this situation? </div> </blockquote> Good question. From the GStreamer source it looks like VA-API was switched from returning only slice data to a full blown elementary stream at some point. Picture encoding seems to be only used for JPEG as far as I can see. Anyway let's keep it like this for now and fix all the fallout we will run into later on. Regards, Christian. <blockquote cite="mid:CY4PR12MB1127DBA7EB3A95E05937F35787080@CY4PR12MB1127.namprd12.prod.outlook.com" type="cite"> <div class="WordSection1"> <o:p></o:p> <o:p> </o:p> Regards,<o:p></o:p> Boyuan <o:p></o:p> <o:p> </o:p> <div> <div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0cm 0cm 0cm"> From: Christian König [<a class="moz-txt-link-freetext" href="mailto:deathsimple@vodafone.de">mailto:deathsimple@vodafone.de</a>] Sent: July-20-16 4:48 AM To: Zhang, Boyuan; <a class="moz-txt-link-abbreviated" href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> Cc: <a class="moz-txt-link-abbreviated" href="mailto:adf.lists@gmail.com">adf.lists@gmail.com</a> Subject: Re: [PATCH 05/12] st/va: add encode entrypoint<o:p></o:p> </div> </div> <o:p> </o:p> <div> Am 20.07.2016 um 06:12 schrieb Zhang, Boyuan:<o:p></o:p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <div id="divtagdefaultwrapper"> >> @@ -150,7 +167,16 @@ vlVaCreateConfig(VADriverContextP ctx, VAProfile profile, VAEntrypoint entrypoin >> if (entrypoint != VAEntrypointVLD) >> return VA_STATUS_ERROR_UNSUPPORTED_ENTRYPOINT; >> >> - *config_id = p; >> + if (entrypoint == VAEntrypointEncSlice || entrypoint == VAEntrypointEncPicture) >> + config->entrypoint = PIPE_VIDEO_ENTRYPOINT_ENCODE; >> + else >> + config->entrypoint = PIPE_VIDEO_ENTRYPOINT_BITSTREAM; >Well that doesn't make much sense here. >First we return and error if the entrypoint isn't VAEntrypointVLD and >then check if it's an encoding entry point. >Additional to that I already wondered if we are really going to support >slice level as well as picture level encoding. >I think that it should only be one of the two. >Regards, >Christian.<o:p></o:p> <o:p> </o:p> Hi Christian,<o:p></o:p> <o:p> </o:p> Sorry for the confusion, The first 2 lines of codes<o:p></o:p> >> if (entrypoint != VAEntrypointVLD) >> return VA_STATUS_ERROR_UNSUPPORTED_ENTRYPOINT;<o:p></o:p> will actually be removed in the last patch where we enable the VAAPI Encode (Patch 12/12). In other word, we don't accept VAEncode entrypoint until the time we enable VAAPI Encode. Therefore, we still only accept VAEntrypointVLD at this patch.<o:p></o:p> </div> </blockquote> Makes sense, but I suggest that in this case we should add at least a comment why this is still disabled. And it would look better if we have an "#if 0" or something like this in the code which gets explicitly removed with the last patch. <o:p></o:p> <div id="divtagdefaultwrapper"> <o:p> </o:p> And we need to accept both picture level and slice level entrypoint. For some application, e.g. libva h264encode test, if we don't enable slice level encode, it will fail the call and report h264 encode is not supported. If we enable both, it will still use picture level encode. That's why I put both here.<o:p></o:p> </div> The problem with slice level encoding is that we haven't fully implemented it. E.g. the last time I checked the h264encode test it would try to add an SPS and PPS in front of the slice data returned from our VA-API driver. Since our VA-API driver doesn't return slice data, but rather a full blown elementary stream you end up with a complete mess which looks something like this: SPS (added by the application), PPS (added by the application), Slice Header, SPS (added by the driver), PPS(added by the driver), Slice Header/Slice Data. That might work in some if not most cases, but is certainly not complaint to the VA-API specification. Christian. <o:p></o:p> <div id="divtagdefaultwrapper"> <o:p> </o:p> Regards,<o:p></o:p> Boyuan <o:p></o:p> <div> <div> <div class="MsoNormal" style="text-align:center;background:white" align="center"> <hr width="98%" align="center" size="2"> </div> <div id="x_divRplyFwdMsg"> From: Christian König <a moz-do-not-send="true" href="mailto:deathsimple@vodafone.de"><deathsimple@vodafone.de></a> Sent: July 19, 2016 4:52 AM To: Zhang, Boyuan; <a moz-do-not-send="true" href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> Cc: <a moz-do-not-send="true" href="mailto:adf.lists@gmail.com">adf.lists@gmail.com</a> Subject: Re: [PATCH 05/12] st/va: add encode entrypoint <o:p></o:p> <div> <o:p></o:p> </div> </div> </div> <div> Am 19.07.2016 um 00:43 schrieb Boyuan Zhang: > VAAPI passes PIPE_VIDEO_ENTRYPOINT_ENCODE as entry point for encoding case. We will save this encode entry point in config. config_id was used as profile previously. Now, config has both profile and entrypoint field, and config_id is used to get the config object. Later on, we pass this entrypoint to context->templat.entrypoint instead of always hardcoded to PIPE_VIDEO_ENTRYPOINT_BITSTREAM for decoding case previously. > > Signed-off-by: Boyuan Zhang <a moz-do-not-send="true" href="mailto:boyuan.zhang@amd.com"><boyuan.zhang@amd.com></a> > --- > src/gallium/state_trackers/va/config.c | 69 +++++++++++++++++++++++++++--- > src/gallium/state_trackers/va/context.c | 59 ++++++++++++++----------- > src/gallium/state_trackers/va/surface.c | 14 ++++-- > src/gallium/state_trackers/va/va_private.h | 5 +++ > 4 files changed, 115 insertions(+), 32 deletions(-) > > diff --git a/src/gallium/state_trackers/va/config.c b/src/gallium/state_trackers/va/config.c > index 9ca0aa8..7ea7e24 100644 > --- a/src/gallium/state_trackers/va/config.c > +++ b/src/gallium/state_trackers/va/config.c > @@ -34,6 +34,8 @@ > > #include "va_private.h" > > +#include "util/u_handle_table.h" > + > DEBUG_GET_ONCE_BOOL_OPTION(mpeg4, "VAAPI_MPEG4_ENABLED", false) > > VAStatus > @@ -128,14 +130,29 @@ VAStatus > vlVaCreateConfig(VADriverContextP ctx, VAProfile profile, VAEntrypoint entrypoint, > VAConfigAttrib *attrib_list, int num_attribs, VAConfigID *config_id) > { > + vlVaDriver *drv; > + vlVaConfig *config; > struct pipe_screen *pscreen; > enum pipe_video_profile p; > > if (!ctx) > return VA_STATUS_ERROR_INVALID_CONTEXT; > > + drv = VL_VA_DRIVER(ctx); > + > + if (!drv) > + return VA_STATUS_ERROR_INVALID_CONTEXT; > + > + config = CALLOC(1, sizeof(vlVaConfig)); > + if (!config) > + return VA_STATUS_ERROR_ALLOCATION_FAILED; > + > if (profile == VAProfileNone && entrypoint == VAEntrypointVideoProc) { > - *config_id = PIPE_VIDEO_PROFILE_UNKNOWN; > + config->entrypoint = VAEntrypointVideoProc; > + config->profile = PIPE_VIDEO_PROFILE_UNKNOWN; > + pipe_mutex_lock(drv->mutex); > + *config_id = handle_table_add(drv->htab, config); > + pipe_mutex_unlock(drv->mutex); > return VA_STATUS_SUCCESS; > } > > @@ -150,7 +167,16 @@ vlVaCreateConfig(VADriverContextP ctx, VAProfile profile, VAEntrypoint entrypoin > if (entrypoint != VAEntrypointVLD) > return VA_STATUS_ERROR_UNSUPPORTED_ENTRYPOINT; > > - *config_id = p; > + if (entrypoint == VAEntrypointEncSlice || entrypoint == VAEntrypointEncPicture) > + config->entrypoint = PIPE_VIDEO_ENTRYPOINT_ENCODE; > + else > + config->entrypoint = PIPE_VIDEO_ENTRYPOINT_BITSTREAM; Well that doesn't make much sense here. First we return and error if the entrypoint isn't VAEntrypointVLD and then check if it's an encoding entry point. Additional to that I already wondered if we are really going to support slice level as well as picture level encoding. I think that it should only be one of the two. Regards, Christian. > + > + config->profile = p; > + > + pipe_mutex_lock(drv->mutex); > + *config_id = handle_table_add(drv->htab, config); > + pipe_mutex_unlock(drv->mutex); > > return VA_STATUS_SUCCESS; > } > @@ -158,9 +184,27 @@ vlVaCreateConfig(VADriverContextP ctx, VAProfile profile, VAEntrypoint entrypoin > VAStatus > vlVaDestroyConfig(VADriverContextP ctx, VAConfigID config_id) > { > + vlVaDriver *drv; > + vlVaConfig *config; > + > if (!ctx) > return VA_STATUS_ERROR_INVALID_CONTEXT; > > + drv = VL_VA_DRIVER(ctx); > + > + if (!drv) > + return VA_STATUS_ERROR_INVALID_CONTEXT; > + > + pipe_mutex_lock(drv->mutex); > + config = handle_table_get(drv->htab, config_id); > + > + if (!config) > + return VA_STATUS_ERROR_INVALID_CONFIG; > + > + FREE(config); > + handle_table_remove(drv->htab, config_id); > + pipe_mutex_unlock(drv->mutex); > + > return VA_STATUS_SUCCESS; > } > > @@ -168,18 +212,33 @@ VAStatus > vlVaQueryConfigAttributes(VADriverContextP ctx, VAConfigID config_id, VAProfile *profile, > VAEntrypoint *entrypoint, VAConfigAttrib *attrib_list, int *num_attribs) > { > + vlVaDriver *drv; > + vlVaConfig *config; > + > if (!ctx) > return VA_STATUS_ERROR_INVALID_CONTEXT; > > - *profile = PipeToProfile(config_id); > + drv = VL_VA_DRIVER(ctx); > + > + if (!drv) > + return VA_STATUS_ERROR_INVALID_CONTEXT; > + > + pipe_mutex_lock(drv->mutex); > + config = handle_table_get(drv->htab, config_id); > + pipe_mutex_unlock(drv->mutex); > + > + if (!config) > + return VA_STATUS_ERROR_INVALID_CONFIG; > + > + *profile = PipeToProfile(config->profile); > > - if (config_id == PIPE_VIDEO_PROFILE_UNKNOWN) { > + if (config->profile == PIPE_VIDEO_PROFILE_UNKNOWN) { > *entrypoint = VAEntrypointVideoProc; > *num_attribs = 0; > return VA_STATUS_SUCCESS; > } > > - *entrypoint = VAEntrypointVLD; > + *entrypoint = config->entrypoint; > > *num_attribs = 1; > attrib_list[0].type = VAConfigAttribRTFormat; > diff --git a/src/gallium/state_trackers/va/context.c b/src/gallium/state_trackers/va/context.c > index 402fbb2..8882cba 100644 > --- a/src/gallium/state_trackers/va/context.c > +++ b/src/gallium/state_trackers/va/context.c > @@ -195,18 +195,23 @@ vlVaCreateContext(VADriverContextP ctx, VAConfigID config_id, int picture_width, > { > vlVaDriver *drv; > vlVaContext *context; > + vlVaConfig *config; > int is_vpp; > > if (!ctx) > return VA_STATUS_ERROR_INVALID_CONTEXT; > > - is_vpp = config_id == PIPE_VIDEO_PROFILE_UNKNOWN && !picture_width && > + drv = VL_VA_DRIVER(ctx); > + pipe_mutex_lock(drv->mutex); > + config = handle_table_get(drv->htab, config_id); > + pipe_mutex_unlock(drv->mutex); > + > + is_vpp = config->profile == PIPE_VIDEO_PROFILE_UNKNOWN && !picture_width && > !picture_height && !flag && !render_targets && !num_render_targets; > > if (!(picture_width && picture_height) && !is_vpp) > return VA_STATUS_ERROR_INVALID_IMAGE_FORMAT; > > - drv = VL_VA_DRIVER(ctx); > context = CALLOC(1, sizeof(vlVaContext)); > if (!context) > return VA_STATUS_ERROR_ALLOCATION_FAILED; > @@ -218,8 +223,8 @@ vlVaCreateContext(VADriverContextP ctx, VAConfigID config_id, int picture_width, > return VA_STATUS_ERROR_INVALID_CONTEXT; > } > } else { > - context->templat.profile = config_id; > - context->templat.entrypoint = PIPE_VIDEO_ENTRYPOINT_BITSTREAM; > + context->templat.profile = config->profile; > + context->templat.entrypoint = config->entrypoint; > context->templat.chroma_format = PIPE_VIDEO_CHROMA_FORMAT_420; > context->templat.width = picture_width; > context->templat.height = picture_height; > @@ -234,16 +239,18 @@ vlVaCreateContext(VADriverContextP ctx, VAConfigID config_id, int picture_width, > > case PIPE_VIDEO_FORMAT_MPEG4_AVC: > context->templat.max_references = 0; > - 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); > - return VA_STATUS_ERROR_ALLOCATION_FAILED; > + if (config->entrypoint != PIPE_VIDEO_ENTRYPOINT_ENCODE) { > + 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); > + return VA_STATUS_ERROR_ALLOCATION_FAILED; > + } > } > break; > > @@ -267,7 +274,9 @@ vlVaCreateContext(VADriverContextP ctx, VAConfigID config_id, int picture_width, > } > } > > - context->desc.base.profile = config_id; > + context->desc.base.profile = config->profile; > + context->desc.base.entry_point = config->entrypoint; > + > pipe_mutex_lock(drv->mutex); > *context_id = handle_table_add(drv->htab, context); > pipe_mutex_unlock(drv->mutex); > @@ -293,15 +302,17 @@ vlVaDestroyContext(VADriverContextP ctx, VAContextID context_id) > } > > 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); > - } > - if (u_reduce_video_profile(context->decoder->profile) == > - PIPE_VIDEO_FORMAT_HEVC) { > - FREE(context->desc.h265.pps->sps); > - FREE(context->desc.h265.pps); > + if (context->desc.base.entry_point != PIPE_VIDEO_ENTRYPOINT_ENCODE) { > + 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 (u_reduce_video_profile(context->decoder->profile) == > + PIPE_VIDEO_FORMAT_HEVC) { > + FREE(context->desc.h265.pps->sps); > + FREE(context->desc.h265.pps); > + } > } > context->decoder->destroy(context->decoder); > } > diff --git a/src/gallium/state_trackers/va/surface.c b/src/gallium/state_trackers/va/surface.c > index 3e74353..8ce4143 100644 > --- a/src/gallium/state_trackers/va/surface.c > +++ b/src/gallium/state_trackers/va/surface.c > @@ -319,17 +319,18 @@ vlVaUnlockSurface(VADriverContextP ctx, VASurfaceID surface) > } > > VAStatus > -vlVaQuerySurfaceAttributes(VADriverContextP ctx, VAConfigID config, > +vlVaQuerySurfaceAttributes(VADriverContextP ctx, VAConfigID config_id, > VASurfaceAttrib *attrib_list, unsigned int *num_attribs) > { > vlVaDriver *drv; > + vlVaConfig *config; > VASurfaceAttrib *attribs; > struct pipe_screen *pscreen; > int i, j; > > STATIC_ASSERT(ARRAY_SIZE(vpp_surface_formats) <= VL_VA_MAX_IMAGE_FORMATS); > > - if (config == VA_INVALID_ID) > + if (config_id == VA_INVALID_ID) > return VA_STATUS_ERROR_INVALID_CONFIG; > > if (!attrib_list && !num_attribs) > @@ -348,6 +349,13 @@ vlVaQuerySurfaceAttributes(VADriverContextP ctx, VAConfigID config, > if (!drv) > return VA_STATUS_ERROR_INVALID_CONTEXT; > > + pipe_mutex_lock(drv->mutex); > + config = handle_table_get(drv->htab, config_id); > + pipe_mutex_unlock(drv->mutex); > + > + if (!config) > + return VA_STATUS_ERROR_INVALID_CONFIG; > + > pscreen = VL_VA_PSCREEN(ctx); > > if (!pscreen) > @@ -363,7 +371,7 @@ vlVaQuerySurfaceAttributes(VADriverContextP ctx, VAConfigID config, > > /* vlVaCreateConfig returns PIPE_VIDEO_PROFILE_UNKNOWN > * only for VAEntrypointVideoProc. */ > - if (config == PIPE_VIDEO_PROFILE_UNKNOWN) { > + if (config->profile == PIPE_VIDEO_PROFILE_UNKNOWN) { > for (j = 0; j < ARRAY_SIZE(vpp_surface_formats); ++j) { > attribs[i].type = VASurfaceAttribPixelFormat; > attribs[i].value.type = VAGenericValueTypeInteger; > diff --git a/src/gallium/state_trackers/va/va_private.h b/src/gallium/state_trackers/va/va_private.h > index d91de44..723983d 100644 > --- a/src/gallium/state_trackers/va/va_private.h > +++ b/src/gallium/state_trackers/va/va_private.h > @@ -244,6 +244,11 @@ typedef struct { > } vlVaContext; > > typedef struct { > + VAEntrypoint entrypoint; > + enum pipe_video_profile profile; > +} vlVaConfig; > + > +typedef struct { > VABufferType type; > unsigned int size; > unsigned int num_elements;<o:p></o:p> </div> </div> </div> <o:p> </o:p> </div> </blockquote> </body> </html>