[VDPAU] [PATCH] Add support for Hi444PP in VDPAU API

[José Hiram Soltren]

Hello,

On 12/10/2014 06:36 AM, Rémi Denis-Courmont wrote:
> Le 2014-12-10 02:50, Karthikeyan Sreenivasan a écrit :
>> diff --git a/include/vdpau/vdpau.h b/include/vdpau/vdpau.h
>> index 41bdf2d..d45376f 100644
>> --- a/include/vdpau/vdpau.h
>> +++ b/include/vdpau/vdpau.h
>> @@ -2460,6 +2460,9 @@ typedef uint32_t VdpDecoderProfile;
>>  #define VDP_DECODER_PROFILE_H264_PROGRESSIVE_HIGH
>> ((VdpDecoderProfile)24)
>>  /** \hideinitializer */
>>  #define VDP_DECODER_PROFILE_H264_CONSTRAINED_HIGH
>> ((VdpDecoderProfile)25)
>> +/** \hideinitializer */
>> +/** \brief Support for 8 bit depth only */
>> +#define VDP_DECODER_PROFILE_H264_HIGH_444_PREDICTIVE
>> ((VdpDecoderProfile)26)
> 
> In my understanding, the High 4:4:4 Predictive profile is:
>  - a superset of the High 4:2:2 profile, which is:
>    - a superset of the High 4:2:2 Intra profile
>  - a superset of the High 4:4:4 Intra profile, which is:
>    - a superset of the CAVLC 4:4:4 Intra profile.
> 
> So without rehashing the earlier discussion regarding Constrained Baseline,
> should we not add separate definitions for those four profiles too?

Thanks for the feedback Rémi. I too often view VDPAU as a driver implementer
and not as an application developer.

Perhaps? I would be fine with adding these on as a separate change. But it
might be good to get them now to keep the enum values in order. If I recall the
previous discussion I was in favor of adding additional profiles. A future
NVIDIA driver release will correctly allow/deny creating decoders with these
profiles based on our hardware capabilities.

If we are going this route I propose adding any of the profiles in Rec. ITU-T
H.264 (02/2014), Annex A, Section A.2 that we do not currently offer. If I'm
reading this correctly we need to add:

VDP_DECODER_PROFILE_H264_CONSTRAINED_BASELINE
VDP_DECODER_PROFILE_H264_EXTENDED
VDP_DECODER_PROFILE_H264_PROGRESSIVE_HIGH
VDP_DECODER_PROFILE_H264_CONSTRAINED_HIGH
VDP_DECODER_PROFILE_H264_HIGH_10
VDP_DECODER_PROFILE_H264_HIGH_422
VDP_DECODER_PROFILE_H264_HIGH_444_PREDICTIVE (being added here)
VDP_DECODER_PROFILE_H264_HIGH_10_INTRA
VDP_DECODER_PROFILE_H264_HIGH_422_INTRA
VDP_DECODER_PROFILE_H264_HIGH_444_INTRA
VDP_DECODER_PROFILE_H264_CAVLC_444_INTRA

Some of these need to be added in line, between other values, which will ruin
the enumeration values regardless. We can either make the enum values out of
order, or force a major API change as we alter these values. I vote for the
former as ugly as it is.

While we're at it we may as well add the additional decoder level from Rec.
ITU-T H.264 (02/2014) Table A-1:

VDP_DECODER_LEVEL_H264_5_2

Again, if we're okay with putting in out of order enum values, this can all be
a follow-on change.

>>  /** \hideinitializer */
>>  #define VDP_DECODER_LEVEL_MPEG1_NA 0
> 
>> @@ -2847,6 +2855,33 @@ typedef struct {
>>  } VdpPictureInfoH264;
>>
>>  /**
>> + * \brief Picture parameter information for an  H.264 Hi444PP picture.
>> + *
>> + * Note: VDPAU clients must use VdpPictureInfoH264Predictive to
>> describe the
>> + * attributes of a frame being decoded with
>> + * VDP_DECODER_PROFILE_H264_HIGH_444_PREDICTIVE.
>> + *
>> + * Note: software drivers may choose to honor values of
>> + * transform_bypass greater than 1 for internal use.
>> + */
>> +typedef struct {
>> +    /** \ref VdpPictureInfoH264 struct. */
>> +    VdpPictureInfoH264 pictureInfo;
>> +    /** Copy of the H.264 bitstream field.
>> +     *
>> +     *  0 - lossless disabled
>> +     *  1 - lossless enabled
>> +     */
>> +    uint8_t transform_bypass;
>> +    /** Copy of the H.264 bitstream field..
>> +     *  0 - disabled
>> +     *  1 - enabled
>> +     */
>> +    uint8_t separate_colour_plane__flag;
> 
> One underscore too many.
> 
>> +
>> +} VdpPictureInfoH264Predictive;
>> +
>> +/**
>>   * \brief Picture parameter information for a VC1 picture.
>>   *
>>   * Note: References to "copy of bitstream field" in the field descriptions
> 
> Otherwise LGTM.

Yes, me too. I await a final patch for my "Reviewed-by" line.

Thanks,
--José