[RFC]: Deprecate VA_ENC_PACKED_HEADER_NONE and similar

[uartie]

VAAPI provides two ways for drivers to report/handle some unsupported attributes. For example, driver can report that packed headers are unsupported by either setting the value to VA_ATTRIB_NOT_SUPPORTED or VA_ENC_PACKED_HEADER_NONE. Unfortunately, VA_ENC_PACKED_HEADER_NONE evaluates to zero (0) and can produce unintended results if not handled properly in both driver and middleware (since the packed header attribute is a bitfield).
VA_ATTRIB_NOT_SUPPORTED on the other hand is not zero and makes it suitable for simple bitwise operations on bitfield attributes. To handle both cases properly, drivers/middleware are forced to do extra work that would be unnecessary otherwise (e.g. https://github.com/intel/intel-vaapi-driver/blob/0b37282fa1c302d2b0c791fdc46c69af0d16d09b/src/i965_drv_video.c#L1418). AFAICT, having attribute values such as VA_ENC_PACKED_HEADER_NONE=0 serves no purpose and only creates confusion and bugs.

Also, there are other bitfield attributes that have similar _NONE = 0 usage which should also be deprecated.

[xhaihao]

VA_ATTRIB_NOT_SUPPORTED and VA_ENC_PACKED_HEADER_NONE means have a little different meanings, VA_ATTRIB_NOT_SUPPORTED means the corresponding attribute is not supported at all however VA_ENC_PACKED_HEADER_NONE means packed header isn't required from application.

[MicheleL-Intel]

Haihao's description above helps to clarify the usage and purpose for each flag a bit more. Since the purpose is subtle, it will be helpful if the usage for each flag is clearly documented to minimize ambiguity and misinterpretation. :))

I suggest checking both flags in succession. In other words, check the flag which supersedes the other, and based on the evaluation result, make the determine whether or not to check the subordinate flag.

So for instance, assuming that VA_ATTRIB_NOT_SUPPORTED supersedes VA_ENC_PACKED_HEADER_NONE (i.e. VA_ENC_PACKED_HEADER_NONE is one of the attributes and is subordinate to VA_ATTRIB_NOT_SUPPORTED), verify whether the ambiguity can be resolved by evaluating VA_ATTRIB_NOT_SUPPORTED as a qualifier BEFORE evaluating VA_ENC_PACKED_HEADER_NONE in two successive "if" conditional statements.

[uartie]

@xhaihao, that's quite the contrary to how VA_ENC_PACKED_HEADER_NONE is documented .(

libva/va/va.h

Line 841 in 71fd9ec

/** \brief Driver does not support any packed headers mode. */

).

@MicheleL-Intel, that is exactly the problem... apps and/or drivers are forced to handle it correctly with extra logic that would otherwise be unnecessary if VA_ENC_PACKED_HEADER_NONE != 0. And given that the documentation essentially says it means the same as VA_ATTRIB_NOT_SUPPORTED, that is why apps/middleware don't usually do the right thing.

So, either it should be deprecated or the definition changed to non-zero value with better documentation. Honestly, I would actually prefer the latter.

[xhaihao]

@uartie I agree the two flags and the comments make thing messy. The corresponding attribute value should be set to VA_ATTRIB_NOT_SUPPORTED if the attribute is not applicable, other values shouldn't indicate non-supported attribute.

/**                                                                                                                                                                                            
 * Get attributes for a given profile/entrypoint pair                                                                                                                                          
 * The caller must provide an "attrib_list" with all attributes to be                                                                                                                          
 * retrieved.  Upon return, the attributes in "attrib_list" have been                                                                                                                          
 * updated with their value.  Unknown attributes or attributes that are                                                                                                                        
 * not supported for the given profile/entrypoint pair will have their                                                                                                                         
 * value set to VA_ATTRIB_NOT_SUPPORTED                                                                                                                                                        
 */
VAStatus vaGetConfigAttributes (

I prefer deprecating VA_ENC_PACKED_HEADER_NONE because redefining VA_ENC_PACKED_HEADER_NONE to a non-zero value will break binary compatibility.

[uartie]

@xhaihao ok, I agree... deprecating is better.

It probably makes sense to do an audit for other similar cases that suffer from the same semantic issues, too (e.g. VA_ENC_INTERLACED_NONE, VA_ENC_QUANTIZATION_NONE, VA_ENC_INTRA_REFRESH_NONE, etc.). Those could be deprecated at the same time.

[uartie]

@XinfengZhang do you have any input about this RFC for iHD driver?

[fhvwy]

For the return value from vaGetConfigAttributes(), sure, the zero value of VAConfigAttribEncPackedHeaders from the driver is not useful and should be VA_ATTRIB_NOT_SUPPORTED instead. On the other hand, when passed to vaCreateConfig() by the user it should still be valid - it is the user telling the driver explicitly that they are going to supply none of the packed headers which were offered.

I'm not sure that the specific macro VA_ENC_PACKED_HEADER_NONE is worth keeping - e.g. the code which deals with this in libavcodec never names it explicitly, rather a zero value will only ever be made as the bitwise and of non-intersecting sets.

[xhaihao]

If an attribute is not supported at all, does it make sense to pass this attribute to the driver ? I think it will confuse the user.

[uartie]

@fhvwy libavcodec discards all VA_ATTRIB_NOT_SUPPORTED attributes returned by vaGetConfigAttributes before sending to the driver, regardless of what the code sets ctx->va_packed_headers to. So libavcodec should not be affected by this change.

[fhvwy]

@uartie As long as we are clear that zero is still a valid value to pass for VAConfigAttribEncPackedHeaders in vaCreateConfig() regardless of the status of VA_ENC_PACKED_HEADER_NONE. (Or at least if it does become invalid that change happens after a major version bump so that old programs cannot expect API/ABI compatibility with the new version.)

[uartie]

It's up to the driver to decide if zero is a valid value or not. If the driver reports to the program/client that the attribute is unsupported, then the client should comply. Otherwise, it's a bug in the client program. Clients should not make any assumptions about the driver implementation. This has nothing to do with API/ABI compatibility.

If we deprecate/remove VA_ENC_PACKED_HEADER_NONE, then yes it's an API/ABI change.

[uartie]

@fhvwy hmmm... on the other hand, maybe you're saying that the user is not required to send packed header when they "are supported" (which would imply VA_ENC_PACKED_HEADER_NONE is implicit when driver offers up other supported options)? Would this be true for all codecs? If not, then how would driver relay this information?

[fhvwy]

Maybe there actually need to be two different fields here - the set of packed headers which are supported (that is, the user may optionally provide them but the encoder can work without them), and the set of packed headers which are required (that is, the user must provide them and the encoder will not work without them)?

(I think some of the discussion here has been confusing the two cases, as VAAPI does not currently distinguish between them. I have always been talking about the first of the two (optional headers), where signalling that you are not going to provide them (passing zero or VA_ENC_PACKED_HEADER_NONE) is meaningful. If the headers are required then obviously signalling that is not meaningful and the driver must reject it because it can't work.)

[uartie]

Yes, this is essentially what the other proposed option was intended to address (#178 (comment)). That is, change VA_ENC_PACKED_HEADER_NONE to not equal 0 so users can check if it's optional. Although, it was unclear to me at the time if there were any such cases. And it appears VP9 codec is such a case. But there were concerns about changing the value of VA_ENC_PACKED_HEADER_NONE... so additional fields, as you pointed out, would be needed instead.