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

Mark Thompson sw at jkqxz.net
Thu Mar 8 02:50:32 EET 2018


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 at ffmpeg.org] On Behalf Of
>>>> Mark Thompson
>>>> Sent: Tuesday, February 13, 2018 11:54 AM
>>>> To: FFmpeg development discussions and patches <ffmpeg-devel at 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 at 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


More information about the ffmpeg-devel mailing list