[FFmpeg-devel] lavc/vaapi_encode: Don't pass VAConfigAttribEncPackedHeaders with value set to 0

Submitted by Haihao Xiang on Feb. 13, 2018, 8:24 a.m.

Details

Message ID 20180213082456.25402-1-haihao.xiang@intel.com
State New
Headers show

Commit Message

Haihao Xiang Feb. 13, 2018, 8:24 a.m.
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(+)

Comments

Mark Thompson Feb. 13, 2018, 6:52 p.m.
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
Mark Thompson Feb. 13, 2018, 7:54 p.m.
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

Patch hide | download patch | download mbox

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,