| Message ID | 20180213082456.25402-1-haihao.xiang@intel.com |
|---|---|
| State | New |
| Headers | show |
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.xiang@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). - Mark
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.xiang@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
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@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.xiang@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. 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. In the future, it's probably worth amending VAAPI to allow for drivers to relay when packed headers are required vs. optional, too. U. Artie > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On 06/03/18 06:04, Eoff, Ullysses A wrote: >> -----Original Message----- >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@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.xiang@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? > 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()). > 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. Thanks, - Mark
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-bounces@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.xiang@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
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-bounces@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.xiang@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
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,
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.xiang@intel.com> --- libavcodec/vaapi_encode.c | 4 ++++ 1 file changed, 4 insertions(+)