[VDPAU] [PATCH 2/2] [PATCH] Enable lossless video playback via VDPAU

[Rémi Denis-Courmont]

Le 2014-10-29 06:56, ksreenivasan at nvidia.com a écrit :
> From: Karthikeyan Sreenivasan <ksreenivasan at nvidia.com>
>
> Exposes the hardware flags via VDPAU to enable lossess video playback

Nit: "lossless"

> for x264 and JM encoded bit streams.
>
> This revision adds a new struct VdpPictureInfoH264Extended which
> contains a struct_version and the transform_bypass which can take
> the follwing values,

Nit: "following"

> 0 - No lossless
> 1 - x264 encoded streams
> 2 - JM encoded streams
>
> The transform_bypass field will be used to populate lossless fields
> when passing values to NVVA based on the type of lossless encoding
> and then passed on to hardware.
>
> Added H264_HIGH_444PP profile which was not present before.

Since all defined VDPAU video surface chroma types so far have 8-bits 
depth, I assume this only supports 8-bits... Probably that should be 
documented.

> This revision to vdpau.h would require an API change.
> ---
>  include/vdpau/vdpau.h |   29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/include/vdpau/vdpau.h b/include/vdpau/vdpau.h
> index 22497e2..4d43bb0 100644
> --- a/include/vdpau/vdpau.h
> +++ b/include/vdpau/vdpau.h
> @@ -2437,6 +2437,8 @@ typedef uint32_t VdpDecoderProfile;
>  #define VDP_DECODER_PROFILE_DIVX5_HOME_THEATER
> (VdpDecoderProfile)20
>  /** \hideinitializer */
>  #define VDP_DECODER_PROFILE_DIVX5_HD_1080P
> (VdpDecoderProfile)21
> +/** \hideinitializer */
> +#define VDP_DECODER_PROFILE_H264_HIGH_444PP
> (VdpDecoderProfile)22
>
>  /** \hideinitializer */
>  #define VDP_DECODER_LEVEL_MPEG1_NA 0
> @@ -2813,11 +2815,7 @@ typedef struct {
>      uint8_t  deblocking_filter_control_present_flag;
>      /** Copy of the H.264 bitstream field. */
>      uint8_t  redundant_pic_cnt_present_flag;
> -    /** Copy of the H.264 bitstream field. */
> -    uint8_t lossless_ipred8x8_filter_enable;
> -    /** Copy of the H.264 bitstream field. */
> -    uint8_t qpprime_y_zero_transform_bypass_flag;
> -
> +

Removing members of a public structure breaks the binary compatibility. 
I think that's a bad idea.

I'd leave the members as they are and note that they're not actually 
used (or that they must be zero, FWIW).

>      /** Copy of the H.264 bitstream field, converted to raster 
> order. */
>      uint8_t scaling_lists_4x4[6][16];
>      /** Copy of the H.264 bitstream field, converted to raster 
> order. */
> @@ -2827,6 +2825,27 @@ typedef struct {
>      VdpReferenceFrameH264 referenceFrames[16];
>  } VdpPictureInfoH264;
>
> +#define VDP_H264_EXTENDED_VERSION 0
> +
> +/**
> + * \brief Extended picture parameter information for an H.264 
> picture.
> + *
> + * Note: Contains field for lossless decode.
> + */
> +typedef struct {
> +    /**
> +     * This field must be filled with VDP_H264_EXTENDED_VERSION
> +     */
> +    uint32_t struct_version;
> +    /** Performs lossless decode
> +     * 0: no lossless decode
> +     * 1: x264 lossless decode
> +     * 2: JM lossless decode
> +     * >2: unsupported lossless decode stream
> +     */
> +    uint8_t transform_bypass;
> +} VdpPictureInfoH264Extended;
> +

It's not clear where/when that structure should be used by application. 
I think the documentation should probably explain that.

>  /**
>   * \brief Picture parameter information for a VC1 picture.
>   *

-- 
Rémi Denis-Courmont