[VDPAU] Support H264 Hi10 via vdpau

[Stephen Warren]

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.

> +/** \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?

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.

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.

-- 
nvpublic