[VDPAU] Support H264 Hi10 via vdpau

[Aaron Plattner]

On 10/28/2013 10:47 AM, Stephen Warren wrote:
> Peter Frühberger wrote at Saturday, October 26, 2013 8:08 AM:
>> The attached patches bump libvdpau and add the two fields that are
>> needed to support decoding hi10p via vdpau and e.g. radeon oss uvd
>> decoder.
>>
>> Changes are for libvdpau and vdpauinfo
>>
>> I will post the mesa patches to the mesa mailing list later.
>
> It'd be best to post patches using git send-email, so that each patch is
> a separate email. Also, if you could make the patch subject lines e.g.:
>
> [vdpauinfo PATCH 2/2] hi10p: add output for new profile
>
> Rather than:
>
> [PATCH 2/2] hi10p: add output for new profile
>
> ... that would make it much easier to know which patches are for which
> application or library.
>
> Re: [PATCH 2/2] hi10p: add required fields
>
>> diff --git a/include/vdpau/vdpau.h b/include/vdpau/vdpau.h
> ...
>> -#define VDPAU_VERSION 1
>> +#define VDPAU_VERSION 2
>
> You shouldn't need to bump the overall API version; the change can be
> made in a compatible fashion.

Are you thinking of VDPAU_INTERFACE_VERSION?  The comment above 
VDPAU_VERSION says,

  * This version will increase whenever any non-documentation change is 
made to
  * vdpau.h, or related header files such as vdpau_x11.h. Such changes
  * typically involve the addition of new functions, constants, or features.
  * Such changes are expected to be completely backwards-compatible.

>> +/** \hideinitializer */
>> +#define VDP_DECODER_PROFILE_H264_HI10                   (VdpDecoderProfile)22
>
> Here, you introduce a completely new decoder profile. As such, it's
> possible for your modification to be limited so they are only relevant when
> using that new profile, and hence existing code can be left unaffected.
>
>> @@ -2821,6 +2823,12 @@ typedef struct {
>>
>>       /** See \ref VdpPictureInfoH264 for instructions regarding this field. */
>>       VdpReferenceFrameH264 referenceFrames[16];
>> +
>> +    /* The following two fields are used for Hi10p */
>> +    /** Copy of the H.264 bitstream field. */
>> +    uint8_t  bit_depth_luma_minus8;
>> +    /** Copy of the H.264 bitstream field. */
>> +    uint8_t  bit_depth_chroma_minus8;
>>   } VdpPictureInfoH264;
>
> Rather than modifying the existing structure, I would suggest creating a
> completely new structure for the new profile. You can make it "extend" the
> existing structure so as to re-use all relevant code, etc. In other words:
>
> typedef struct {
>      /** "Inherit" from basic H.264 structure */
>      struct VdpPictureInfoH264 h264;
>      /** Copy of the H.264 bitstream field. */
>      uint8_t  bit_depth_luma_minus8;
>      /** Copy of the H.264 bitstream field. */
>      uint8_t  bit_depth_chroma_minus8;
> } VdpPictureInfoH264Hi10;
>
> ... although actually looking at the new fields you're adding, won't the
> values *always* be "10" if the new profile is in use? I think you only
> need to do one of adding a new profile value, or adding new fields to the
> picture info struct, not both?

This point aside, adding a new field at the end of an existing structure 
that is only read by the driver if a new profile is in use *should* be 
safe, but I agree that it is clearer to add a new structure specifically 
for the new profile.

Please add code to dump these new fields (in whatever form the 
structures end up being written) to vdpau_trace.c.

> Re: [PATCH 1/2] hi10: bump version in configure.ac
>
>> diff --git a/configure.ac b/configure.ac
> ...
>> -AC_INIT(libvdpau, 0.7, [vdpau at lists.freedesktop.org], libvdpau)
>> +AC_INIT(libvdpau, 0.8, [vdpau at lists.freedesktop.org], libvdpau)
>
> There are two issues with this patch:
>
> 1) This patch makes a new version of the library. That's an action that
> should happen separately as and when the maintainer sees fit. It isn't
> something that each and every feature/bugfix patch should do itself.

I agree.  Please remove this chunk from the patch.  I'll take care of 
bumping the version when the time comes.

> 2) If it were legitimate to send such a patch, it should happen after any
> Changes you want included in the new release, not before it.

-- 
Aaron