On 07/03/18 07:07, Xiang, Haihao wrote: > On Tue, 2018-03-06 at 14:42 +0000, Mark Thompson wrote: >> On 06/03/18 06:04, Eoff, Ullysses A wrote: >>>> -----Original Message----- >>>> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf Of >>>> Mark Thompson >>>> Sent: Tuesday, February 13, 2018 11:54 AM >>>> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> >>>> Subject: Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode: Don't pass >>>> VAConfigAttribEncPackedHeaders with value set to 0 >>>> >>>> On 13/02/18 18:52, Mark Thompson wrote: >>>>> On 13/02/18 08:24, Haihao Xiang wrote: >>>>>> Recent Intel i965 driver commit strictly disallows application to set >>>>>> unsupported attribute values, VA_ENC_PACKED_HEADER_NONE (0) is not >>>>>> used >>>>>> in Intel i965 driver, so application shouldn't pass this value to the >>>>>> driver. On the other hand, VA_ENC_PACKED_HEADER_NONE (0) means the >>>>>> driver doesn't support any packed header mode, so application also >>>>>> shouldn't pass packed header to driver if a driver returns >>>>>> VA_ENC_PACKED_HEADER_NONE (0), the driver should work without >>>>>> VAConfigAttribEncPackedHeaders set for this case. >>>>>> >>>>>> In addition, VA_ATTRIB_NOT_SUPPORTED and VA_ENC_PACKED_HEADER_NONE >>>>>> make >>>>>> thing messy, we will deprecate VA_ENC_PACKED_HEADER_NONE in the >>>>>> future. See https://github.com/intel/libva/issues/178 for more >>>>>> information. >>>>>> >>>>>> This fixes broken vp9 encoder on Kably Lake with Intel I965 driver. >>>>>> >>>>>> Signed-off-by: Haihao Xiang <haihao.xi...@intel.com> >>>>>> --- >>>>>> libavcodec/vaapi_encode.c | 4 ++++ >>>>>> 1 file changed, 4 insertions(+) >>>>>> >>>>>> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c >>>>>> index e371f5761ee..1d30aabed40 100644 >>>>>> --- a/libavcodec/vaapi_encode.c >>>>>> +++ b/libavcodec/vaapi_encode.c >>>>>> @@ -1111,6 +1111,10 @@ static av_cold int >>>>>> vaapi_encode_config_attributes(AVCodecContext *avctx) >>>>>> ctx->va_packed_headers, attr[i].value); >>>>>> ctx->va_packed_headers &= attr[i].value; >>>>>> } >>>>>> + >>>>>> + if (!ctx->va_packed_headers) >>>>>> + continue; >>>>>> + >>>>>> ctx->config_attributes[ctx->nb_config_attributes++] = >>>>>> (VAConfigAttrib) { >>>>>> .type = VAConfigAttribEncPackedHeaders, >>>>>> >>>>> >>>>> This seems wrong to me: the driver has indicated that packed headers are >>>>> supported, so the user is providing this attribute to >>>> >>>> indicate to the driver that they will not use any of them. Compare the >>>> VP8 case, where the driver does not support them and says so - >>>> there we correctly don't provide the attribute (though maybe the >>>> commentary could be clearer on that). >>>> >>>> Right, I hadn't realised you had already made a change so that encoding is >>>> currently broken. I've made >>>> <https://github.com/intel/intel-vaapi-driver/pull/358> to revert the >>>> API/ABI-breaking part of the change. >>>> >>>> Thanks, >>>> >>>> - Mark >>> >>> I prefer this patch over the one for intel-vaapi-driver. >> >> Well, the driver patch should be applied anyway to fix the API/ABI break >> (existing libavcodec builds should continue to work on the new >> library/driver), but it can be reverted on the next major version bump. >> Maybe >> a warning (i965_log_info()) could be added to the patch to indicate to the >> client that the usage is deprecated in libva 2 and will be removed in libva >> 3? >> > > Ok, I will merge your driver patch for this special case (allow 0 for > VAConfigAttribEncPackedHeaders when calling vaCreateConfig()) to resolve this > issue. Could you update your patch to add some warning message?
Added in <https://github.com/intel/intel-vaapi-driver/pull/358>. That gets the VP9 encoder in FFmpeg 3.4 working again with the warning: [AVHWDeviceContext @ 0x56220f1b0340] libva: vaCreateConfig: setting the EncPackedHeaders attribute to zero to indicate that no packed headers will be used is deprecated. >>> The va.h documentation for VA_ENC_PACKED_HEADER_NONE >>> says "Driver does not support any packed headers mode". >>> Hence, it's only valid for reporting to client that packed headers >>> are "unsupported". Unfortunately, VA_ENC_PACKED_HEADER_NONE >>> is redundant/ambiguous since there is also >>> VA_ATTRIB_NOT_SUPPORTED to relay the same information. >>> This is why we want to deprecate VA_ENC_PACKED_HEADER_NONE >>> in VAAPI. >>> >>> I don't think it's correct for clients to send >>> VA_ENC_PACKED_HEADER_NONE attribute value to the driver >>> when the driver reports packed headers are "supported". It >>> goes against VA_ENC_PACKED_HEADER_NONE's documented >>> purpose. AFAIK, libavcodec is the only library that does this. It >>> is better to just omit the attribute altogether if client does not >>> want to use any of the "supported" packed headers. And this >>> patch solves that. >> >> I still disagree with this logic. The driver supplied a bitmask of allowed >> packed headers, and the client picks which of those it will send and supplies >> that back to the driver with vaCreateConfig(). If the driver believes that >> set is not sufficient then it can reject that choice, but if it is sufficient >> then the empty set should be just as much a valid setting as any other usable >> subset of the given headers. >> >> Any talk of VA_ENC_PACKED_HEADER_NONE is orthogonal - that symbol isn't used >> in libavcodec at all, and the fact that it happens to have the same value >> (zero) as an empty bitmask is unfortunate but not relevant because one is >> only >> used in the driver -> client case (vaGetConfigAttributes()) while the other >> is >> only used in the client -> driver case (vaCreateConfig()). >> > > It will be better to update the va.h documentation for the value for the > VAConfigAttribEncPackedHeaders attribute when calling vaCreateConfig(). I > prefer > not to set VAConfigAttribEncPackedHeaders if VAConfigAttribEncPackedHeaders > is > not supported by the driver or user application doesn't provide any valid > packed > headers, and it should work with the old / new drivers. > >>> In the future, it's probably worth amending VAAPI to allow for >>> drivers to relay when packed headers are required vs. optional, >>> too. >> >> That sounds like a good idea, but the existing API does need to continue to >> work at least until a new major version is made. >> > > How about to use bit16-bit30 of the value to indicate a header is optional or > not? bit31 is defined as VA_ATTRIB_NOT_SUPPORTED. While it seems highly unlikely that enough different packed headers will be created to use those bits, a new attribute would probably still be clearer. If it did go in the old field then how would a user distinguish between a header being optional and the driver being an older version without those bits set? Thanks, - Mark _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
