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? > > 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. > Thanks, > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel