Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d841827840b0a33b424d91aea46af33d30f5de7c.camel@ndufresne.ca>
Subject: Re: [PATCH v2 1/2] media: add RealVideo format
From:   Nicolas Dufresne <nicolas@ndufresne.ca>
To:     Ming Qian <ming.qian@nxp.com>, mchehab@kernel.org,
        hverkuil-cisco@xs4all.nl
Cc:     shawnguo@kernel.org, robh+dt@kernel.org, s.hauer@pengutronix.de,
        kernel@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com,
        xiahong.bao@nxp.com, eagle.zhou@nxp.com, tao.jiang_2@nxp.com,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Date:   Thu, 12 Jan 2023 16:29:56 -0500
In-Reply-To: <ef8212602467151a6e20ccce3e7ef7adcd7a7c91.1673514352.git.ming.qian@nxp.com>
References: <cover.1673514352.git.ming.qian@nxp.com>
         <ef8212602467151a6e20ccce3e7ef7adcd7a7c91.1673514352.git.ming.qian@nxp.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.2 (3.46.2-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

Hi Ming,

Le jeudi 12 janvier 2023 =C3=A0 17:18 +0800, Ming Qian a =C3=A9crit=C2=A0:
> 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.
> RV10 and RV20 are the H.263-based codecs.
> RV30 and RV40 are RealNetworks' proprietary H.264-based codecs.
> RV60 is RealNetworks' proprietary HEVC-based codecs.
>=20
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> ---
>  .../userspace-api/media/v4l/pixfmt-compressed.rst     | 11 +++++++++++
>  drivers/media/v4l2-core/v4l2-ioctl.c                  |  1 +
>  include/uapi/linux/videodev2.h                        |  1 +
>  3 files changed, 13 insertions(+)
>=20
> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst =
b/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
> index a0230f357680..22035d92c788 100644
> --- a/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
> +++ b/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
> @@ -237,6 +237,17 @@ Compressed Formats
>          Metadata associated with the frame to decode is required to be p=
assed
>          through the ``V4L2_CID_STATELESS_FWHT_PARAMS`` control.
>  	See the :ref:`associated Codec Control ID <codec-stateless-fwht>`.
> +    * .. _V4L2-PIX-FMT-RV:
> +
> +      - ``V4L2_PIX_FMT_RV``
> +      - 'RV00'
> +      - RealVideo, or also spelled as Real Video, is a suite of propriet=
ary
> +        video compression formats developed by RealNetworks -
> +        the specific format changes with the version.
> +        RealVideo codecs are identified by four-character codes.
> +        RV10 and RV20 are the H.263-based codecs.
> +        RV30 and RV40 are RealNetworks' proprietary H.264-based codecs.
> +        RV60 is RealNetworks' proprietary HEVC-based codecs.

Much better description. Though you haven't commented or addressed my conce=
rn of
having 4 different CODECs under the same FMT. I thinks it would make more s=
ense
to export the four of them. Then driver can pick the set they support,
regardless if it requires special handling of not at the HW level. What do =
you
think ?

Even though unlikely considering the age of the CODECs, but having a single=
 FMT
for an encoder would just increase the encoder interface complexity (could
require a control to select which actual format to encode to).
> =20
>  .. raw:: latex
> =20
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-co=
re/v4l2-ioctl.c
> index 9b5b04b8aa69..c375261d169f 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1473,6 +1473,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *f=
mt)
>  		case V4L2_PIX_FMT_FWHT:		descr =3D "FWHT"; break; /* used in vicodec *=
/
>  		case V4L2_PIX_FMT_FWHT_STATELESS:	descr =3D "FWHT Stateless"; break; /=
* used in vicodec */
>  		case V4L2_PIX_FMT_SPK:		descr =3D "Sorenson Spark"; break;
> +		case V4L2_PIX_FMT_RV:		descr =3D "RealVideo"; break;
>  		case V4L2_PIX_FMT_CPIA1:	descr =3D "GSPCA CPiA YUV"; break;
>  		case V4L2_PIX_FMT_WNVA:		descr =3D "WNVA"; break;
>  		case V4L2_PIX_FMT_SN9C10X:	descr =3D "GSPCA SN9C10X"; break;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev=
2.h
> index 262ef10cfa02..0a7ddb023d7c 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -740,6 +740,7 @@ 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') /* Sorenso=
n Spark */
> +#define V4L2_PIX_FMT_RV       v4l2_fourcc('R', 'V', '0', '0') /* RealVid=
eo */
> =20
>  /*  Vendor-specific formats   */
>  #define V4L2_PIX_FMT_CPIA1    v4l2_fourcc('C', 'P', 'I', 'A') /* cpia1 Y=
UV */