Re: [PATCH v3 1/2] media: add RealVideo format RV30 and RV40

[Nicolas Dufresne]

Le vendredi 13 janvier 2023 à 12:13 +0800, Ming Qian a écrit :
> RealVideo, or also spelled as Real Video, is a suite of proprietary
> video compression formats developed by RealNetworks -
> the specific format changes with the version.
> RealVideo codecs are identified by four-character codes.
> RV30 and RV40 are RealNetworks' proprietary H.264-based codecs.
> 
> Signed-off-by: Ming Qian <ming.qian@xxxxxxx>
> ---
>  .../media/v4l/pixfmt-compressed.rst           | 21 +++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ioctl.c          |  2 ++
>  include/uapi/linux/videodev2.h                |  2 ++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst b/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
> index a0230f357680..50ac095c1f16 100644
> --- a/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
> +++ b/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
> @@ -237,6 +237,27 @@ Compressed Formats
>          Metadata associated with the frame to decode is required to be passed
>          through the ``V4L2_CID_STATELESS_FWHT_PARAMS`` control.
>  	See the :ref:`associated Codec Control ID <codec-stateless-fwht>`.
> +    * .. _V4L2-PIX-FMT-RV30:
> +
> +      - ``V4L2_PIX_FMT_RV30``
> +      - 'RV30'
> +      - RealVideo, or also spelled as Real Video, is a suite of proprietary
> +        video compression formats developed by RealNetworks -
> +        the specific format changes with the version.
> +        RealVideo codecs are identified by four-character codes.
> +        RV30 corresponds to RealVideo 8, suspected to based largely on an early draft of H.264

to *be* based on.

Just a style comment too, but the flow could be improved while fixing this typo.
Lines in this paragraph seems very uneven.

> +    * .. _V4L2-PIX-FMT-RV40:
> +
> +      - ``V4L2_PIX_FMT_RV40``
> +      - 'RV40'
> +      - RV40 represents RealVideo 9 and RealVideo 10.
> +        RealVideo 9, suspected to be based on H.264,
> +        RealVideo 10, aka RV9 EHQ, This refers to
> +        an improved encoder for the RV9 format
> +        that is fully backwards compatible with RV9 players -
> +        the format and decoder did not change,
> +        only the encoder did.
> +        As a result, it uses the same FourCC.

It feels this one could be wider ? But again, this is cosmetic at this point.
For the API changes, assuming fixed typo and improved style:

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>

>  
>  .. raw:: latex
>  
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 9b5b04b8aa69..0546b00d3fc9 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1473,6 +1473,8 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>  		case V4L2_PIX_FMT_FWHT:		descr = "FWHT"; break; /* used in vicodec */
>  		case V4L2_PIX_FMT_FWHT_STATELESS:	descr = "FWHT Stateless"; break; /* used in vicodec */
>  		case V4L2_PIX_FMT_SPK:		descr = "Sorenson Spark"; break;
> +		case V4L2_PIX_FMT_RV30:		descr = "RealVideo 8"; break;
> +		case V4L2_PIX_FMT_RV40:		descr = "RealVideo 9 & 10"; break;
>  		case V4L2_PIX_FMT_CPIA1:	descr = "GSPCA CPiA YUV"; break;
>  		case V4L2_PIX_FMT_WNVA:		descr = "WNVA"; break;
>  		case V4L2_PIX_FMT_SN9C10X:	descr = "GSPCA SN9C10X"; break;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 262ef10cfa02..b121154a6e24 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -740,6 +740,8 @@ struct v4l2_pix_format {
>  #define V4L2_PIX_FMT_H264_SLICE v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */
>  #define V4L2_PIX_FMT_HEVC_SLICE v4l2_fourcc('S', '2', '6', '5') /* HEVC parsed slices */
>  #define V4L2_PIX_FMT_SPK      v4l2_fourcc('S', 'P', 'K', '0') /* Sorenson Spark */
> +#define V4L2_PIX_FMT_RV30     v4l2_fourcc('R', 'V', '3', '0') /* RealVideo 8 */
> +#define V4L2_PIX_FMT_RV40     v4l2_fourcc('R', 'V', '4', '0') /* RealVideo 9 & 10 */
>  
>  /*  Vendor-specific formats   */
>  #define V4L2_PIX_FMT_CPIA1    v4l2_fourcc('C', 'P', 'I', 'A') /* cpia1 YUV */