From: Hans Verkuil <hverkuil@xs4all.nl>
To: Shengjiu Wang <shengjiu.wang@gmail.com>
Cc: Shengjiu Wang <shengjiu.wang@nxp.com>,
sakari.ailus@iki.fi, tfiga@chromium.org,
m.szyprowski@samsung.com, mchehab@kernel.org,
linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
Xiubo.Lee@gmail.com, festevam@gmail.com, nicoleotsuka@gmail.com,
lgirdwood@gmail.com, broonie@kernel.org, perex@perex.cz,
tiwai@suse.com, alsa-devel@alsa-project.org,
linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v13 09/16] media: uapi: Define audio sample format fourcc type
Date: Fri, 8 Mar 2024 13:06:46 +0100 [thread overview]
Message-ID: <56368a0d-6ada-4ab6-8389-b4bd20d6efc4@xs4all.nl> (raw)
In-Reply-To: <CAA+D8AMJOCfp6WdqYqy7KSj=mX9o_D5U-aF6Wn=3cOnhWg7VDg@mail.gmail.com>
On 08/03/2024 12:52 pm, Shengjiu Wang wrote:
> On Fri, Mar 8, 2024 at 3:34 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> Hi Shengjiu,
>>
>> After thinking it over I think this patch can be improved:
>>
>> On 26/02/2024 9:28 am, Shengjiu Wang wrote:
>>> The audio sample format definition is from alsa,
>>> the header file is include/uapi/sound/asound.h, but
>>> don't include this header file directly, because in
>>> user space, there is another copy in alsa-lib.
>>> There will be conflict in userspace for include
>>> videodev2.h & asound.h and asoundlib.h
>>>
>>> Here still use the fourcc format.
>>>
>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
>>> ---
>>> .../userspace-api/media/v4l/pixfmt-audio.rst | 87 +++++++++++++++++++
>>> .../userspace-api/media/v4l/pixfmt.rst | 1 +
>>> drivers/media/v4l2-core/v4l2-ioctl.c | 13 +++
>>> include/uapi/linux/videodev2.h | 23 +++++
>>> 4 files changed, 124 insertions(+)
>>> create mode 100644 Documentation/userspace-api/media/v4l/pixfmt-audio.rst
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-audio.rst b/Documentation/userspace-api/media/v4l/pixfmt-audio.rst
>>> new file mode 100644
>>> index 000000000000..04b4a7fbd8f4
>>> --- /dev/null
>>> +++ b/Documentation/userspace-api/media/v4l/pixfmt-audio.rst
>>> @@ -0,0 +1,87 @@
>>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>>> +
>>> +.. _pixfmt-audio:
>>> +
>>> +*************
>>> +Audio Formats
>>> +*************
>>> +
>>> +These formats are used for :ref:`audiomem2mem` interface only.
>>
>> Here you should also document that all these fourccs start with 'AU' and are
>> reserved for mappings of the snd_pcm_format_t type.
>>
>> Also document the v4l2_fourcc_to_audfmt define and the v4l2_audfmt_to_fourcc
>> define (see also below).
>
> How about below description?
> '
> All these fourccs starting with 'AU' are reserved for mappings
All these fourccs -> All FourCCs
> of the snd_pcm_format_t type.
>
> The v4l2_audfmt_to_fourcc() is defined to convert snd_pcm_format_t
convert -> convert the
> type to fourcc. The first character is 'A', the second character
fourcc -> a FourCC
> is 'U', the third character is ten's digit of snd_pcm_format_t,
> the fourth character is unit's digit of snd_pcm_format_t.
"is 'U', and the remaining two characters is the snd_pcm_format_t
value in ASCII. Example: SNDRV_PCM_FORMAT_S16_LE (with value 2)
maps to 'AU02' and SNDRV_PCM_FORMAT_S24_3LE (with value 32) maps
to 'AU32'."
>
> The v4l2_fourcc_to_audfmt() is defined to convert these fourccs to
fourccs -> FourCCs
> snd_pcm_format_t type.
BTW, given the way snd_pcm_format_t is defined I am fairly certain
some casts are needed for the v4l2_audfmt_to_fourcc define.
Regards,
Hans
> '
> Best regards
> Shengjiu Wang
>>
>>> +
>>> +.. tabularcolumns:: |p{5.8cm}|p{1.2cm}|p{10.3cm}|
>>> +
>>> +.. cssclass:: longtable
>>> +
>>> +.. flat-table:: Audio Format
>>> + :header-rows: 1
>>> + :stub-columns: 0
>>> + :widths: 3 1 4
>>> +
>>> + * - Identifier
>>> + - Code
>>> + - Details
>>> + * .. _V4L2-AUDIO-FMT-S8:
>>> +
>>> + - ``V4L2_AUDIO_FMT_S8``
>>> + - 'S8'
>>> + - Corresponds to SNDRV_PCM_FORMAT_S8 in ALSA
>>> + * .. _V4L2-AUDIO-FMT-S16-LE:
>>> +
>>> + - ``V4L2_AUDIO_FMT_S16_LE``
>>> + - 'S16_LE'
>>> + - Corresponds to SNDRV_PCM_FORMAT_S16_LE in ALSA
>>> + * .. _V4L2-AUDIO-FMT-U16-LE:
>>> +
>>> + - ``V4L2_AUDIO_FMT_U16_LE``
>>> + - 'U16_LE'
>>> + - Corresponds to SNDRV_PCM_FORMAT_U16_LE in ALSA
>>> + * .. _V4L2-AUDIO-FMT-S24-LE:
>>> +
>>> + - ``V4L2_AUDIO_FMT_S24_LE``
>>> + - 'S24_LE'
>>> + - Corresponds to SNDRV_PCM_FORMAT_S24_LE in ALSA
>>> + * .. _V4L2-AUDIO-FMT-U24-LE:
>>> +
>>> + - ``V4L2_AUDIO_FMT_U24_LE``
>>> + - 'U24_LE'
>>> + - Corresponds to SNDRV_PCM_FORMAT_U24_LE in ALSA
>>> + * .. _V4L2-AUDIO-FMT-S32-LE:
>>> +
>>> + - ``V4L2_AUDIO_FMT_S32_LE``
>>> + - 'S32_LE'
>>> + - Corresponds to SNDRV_PCM_FORMAT_S32_LE in ALSA
>>> + * .. _V4L2-AUDIO-FMT-U32-LE:
>>> +
>>> + - ``V4L2_AUDIO_FMT_U32_LE``
>>> + - 'U32_LE'
>>> + - Corresponds to SNDRV_PCM_FORMAT_U32_LE in ALSA
>>> + * .. _V4L2-AUDIO-FMT-FLOAT-LE:
>>> +
>>> + - ``V4L2_AUDIO_FMT_FLOAT_LE``
>>> + - 'FLOAT_LE'
>>> + - Corresponds to SNDRV_PCM_FORMAT_FLOAT_LE in ALSA
>>> + * .. _V4L2-AUDIO-FMT-IEC958-SUBFRAME-LE:
>>> +
>>> + - ``V4L2_AUDIO_FMT_IEC958_SUBFRAME_LE``
>>> + - 'IEC958_SUBFRAME_LE'
>>> + - Corresponds to SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE in ALSA
>>> + * .. _V4L2-AUDIO-FMT-S24-3LE:
>>> +
>>> + - ``V4L2_AUDIO_FMT_S24_3LE``
>>> + - 'S24_3LE'
>>> + - Corresponds to SNDRV_PCM_FORMAT_S24_3LE in ALSA
>>> + * .. _V4L2-AUDIO-FMT-U24-3LE:
>>> +
>>> + - ``V4L2_AUDIO_FMT_U24_3LE``
>>> + - 'U24_3LE'
>>> + - Corresponds to SNDRV_PCM_FORMAT_U24_3LE in ALSA
>>> + * .. _V4L2-AUDIO-FMT-S20-3LE:
>>> +
>>> + - ``V4L2_AUDIO_FMT_S20_3LE``
>>> + - 'S20_3LE'
>>> + - Corresponds to SNDRV_PCM_FORMAT_S24_3LE in ALSA
>>> + * .. _V4L2-AUDIO-FMT-U20-3LE:
>>> +
>>> + - ``V4L2_AUDIO_FMT_U20_3LE``
>>> + - 'U20_3LE'
>>> + - Corresponds to SNDRV_PCM_FORMAT_U20_3LE in ALSA
>>> diff --git a/Documentation/userspace-api/media/v4l/pixfmt.rst b/Documentation/userspace-api/media/v4l/pixfmt.rst
>>> index 11dab4a90630..2eb6fdd3b43d 100644
>>> --- a/Documentation/userspace-api/media/v4l/pixfmt.rst
>>> +++ b/Documentation/userspace-api/media/v4l/pixfmt.rst
>>> @@ -36,3 +36,4 @@ see also :ref:`VIDIOC_G_FBUF <VIDIOC_G_FBUF>`.)
>>> colorspaces
>>> colorspaces-defs
>>> colorspaces-details
>>> + pixfmt-audio
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index 961abcdf7290..be229c69e991 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -1471,6 +1471,19 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>>> case V4L2_PIX_FMT_Y210: descr = "10-bit YUYV Packed"; break;
>>> case V4L2_PIX_FMT_Y212: descr = "12-bit YUYV Packed"; break;
>>> case V4L2_PIX_FMT_Y216: descr = "16-bit YUYV Packed"; break;
>>> + case V4L2_AUDIO_FMT_S8: descr = "8-bit Signed"; break;
>>> + case V4L2_AUDIO_FMT_S16_LE: descr = "16-bit Signed LE"; break;
>>> + case V4L2_AUDIO_FMT_U16_LE: descr = "16-bit Unsigned LE"; break;
>>> + case V4L2_AUDIO_FMT_S24_LE: descr = "24(32)-bit Signed LE"; break;
>>> + case V4L2_AUDIO_FMT_U24_LE: descr = "24(32)-bit Unsigned LE"; break;
>>> + case V4L2_AUDIO_FMT_S32_LE: descr = "32-bit Signed LE"; break;
>>> + case V4L2_AUDIO_FMT_U32_LE: descr = "32-bit Unsigned LE"; break;
>>> + case V4L2_AUDIO_FMT_FLOAT_LE: descr = "32-bit Float LE"; break;
>>> + case V4L2_AUDIO_FMT_IEC958_SUBFRAME_LE: descr = "32-bit IEC958 LE"; break;
>>> + case V4L2_AUDIO_FMT_S24_3LE: descr = "24(24)-bit Signed LE"; break;
>>> + case V4L2_AUDIO_FMT_U24_3LE: descr = "24(24)-bit Unsigned LE"; break;
>>> + case V4L2_AUDIO_FMT_S20_3LE: descr = "20(24)-bit Signed LE"; break;
>>> + case V4L2_AUDIO_FMT_U20_3LE: descr = "20(24)-bit Unsigned LE"; break;
>>>
>>> default:
>>> /* Compressed formats */
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 2c03d2dfadbe..673a6235a029 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -843,6 +843,29 @@ struct v4l2_pix_format {
>>> #define V4L2_META_FMT_RK_ISP1_PARAMS v4l2_fourcc('R', 'K', '1', 'P') /* Rockchip ISP1 3A Parameters */
>>> #define V4L2_META_FMT_RK_ISP1_STAT_3A v4l2_fourcc('R', 'K', '1', 'S') /* Rockchip ISP1 3A Statistics */
>>>
>>> +/*
>>> + * Audio-data formats
>>> + * All these audio formats use a fourcc starting with 'AU'
>>> + * followed by the SNDRV_PCM_FORMAT_ value from asound.h.
>>
>> Also document here that fourccs starting with 'AU' are reserved for
>> the snd_pcm_format_t to fourcc mappings.
>>
>> That to avoid that someone adds a 'AUXX' fourcc later.
>>
>>> + */
>>> +#define V4L2_AUDIO_FMT_S8 v4l2_fourcc('A', 'U', '0', '0')
>>> +#define V4L2_AUDIO_FMT_S16_LE v4l2_fourcc('A', 'U', '0', '2')
>>> +#define V4L2_AUDIO_FMT_U16_LE v4l2_fourcc('A', 'U', '0', '4')
>>> +#define V4L2_AUDIO_FMT_S24_LE v4l2_fourcc('A', 'U', '0', '6')
>>> +#define V4L2_AUDIO_FMT_U24_LE v4l2_fourcc('A', 'U', '0', '8')
>>> +#define V4L2_AUDIO_FMT_S32_LE v4l2_fourcc('A', 'U', '1', '0')
>>> +#define V4L2_AUDIO_FMT_U32_LE v4l2_fourcc('A', 'U', '1', '2')
>>> +#define V4L2_AUDIO_FMT_FLOAT_LE v4l2_fourcc('A', 'U', '1', '4')
>>> +#define V4L2_AUDIO_FMT_IEC958_SUBFRAME_LE v4l2_fourcc('A', 'U', '1', '8')
>>> +#define V4L2_AUDIO_FMT_S24_3LE v4l2_fourcc('A', 'U', '3', '2')
>>> +#define V4L2_AUDIO_FMT_U24_3LE v4l2_fourcc('A', 'U', '3', '4')
>>> +#define V4L2_AUDIO_FMT_S20_3LE v4l2_fourcc('A', 'U', '3', '6')
>>> +#define V4L2_AUDIO_FMT_U20_3LE v4l2_fourcc('A', 'U', '3', '8')
>>> +
>>> +#define v4l2_fourcc_to_audfmt(fourcc) \
>>> + (__force snd_pcm_format_t)(((((fourcc) >> 16) & 0xff) - '0') * 10 \
>>> + + ((((fourcc) >> 24) & 0xff) - '0'))
>>> +
>>
>> As I suggested in an earlier reply, add this:
>>
>> #define v4l2_audfmt_to_fourcc(audfmt) \
>> v4l2_fourcc('A', 'U', '0' + (audfmt) / 10, '0' + (audfmt) % 10)
>>
>> Even though it is not used in the drivers, since this is a public header used
>> by drivers and applications, it makes sense to provide the reverse mapping as well.
>>
>> Please test it in actual code to make sure there are no compilation warnings.
>>
>> Regards,
>>
>> Hans
>>
>>> /* priv field value to indicates that subsequent fields are valid. */
>>> #define V4L2_PIX_FMT_PRIV_MAGIC 0xfeedcafe
>>>
>>
next prev parent reply other threads:[~2024-03-08 12:06 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-26 8:28 [PATCH v13 00/16] Add audio support in v4l2 framework Shengjiu Wang
2024-02-26 8:28 ` [PATCH v13 01/16] media: v4l2-ctrls: add support for fraction_bits Shengjiu Wang
2024-02-26 8:28 ` [PATCH v13 02/16] ASoC: fsl_asrc: define functions for memory to memory usage Shengjiu Wang
2024-02-26 8:28 ` [PATCH v13 03/16] ASoC: fsl_easrc: " Shengjiu Wang
2024-02-26 8:28 ` [PATCH v13 04/16] ASoC: fsl_asrc: move fsl_asrc_common.h to include/sound Shengjiu Wang
2024-02-26 8:28 ` [PATCH v13 05/16] ASoC: fsl_asrc: register m2m platform device Shengjiu Wang
2024-02-26 8:28 ` [PATCH v13 06/16] ASoC: fsl_easrc: " Shengjiu Wang
2024-02-26 8:28 ` [PATCH v13 07/16] media: uapi: Add V4L2_CAP_AUDIO_M2M capability flag Shengjiu Wang
2024-02-26 8:28 ` [PATCH v13 08/16] media: v4l2: Add audio capture and output support Shengjiu Wang
2024-02-26 8:28 ` [PATCH v13 09/16] media: uapi: Define audio sample format fourcc type Shengjiu Wang
2024-02-26 13:55 ` Nicolas Dufresne
2024-02-27 3:44 ` Shengjiu Wang
2024-02-27 8:34 ` Hans Verkuil
2024-03-08 7:34 ` Hans Verkuil
2024-03-08 11:52 ` Shengjiu Wang
2024-03-08 12:06 ` Hans Verkuil [this message]
2024-03-08 13:52 ` Shengjiu Wang
2024-03-08 14:04 ` Hans Verkuil
2024-03-11 2:07 ` Shengjiu Wang
2024-02-26 8:28 ` [PATCH v13 10/16] media: uapi: Add V4L2_CTRL_CLASS_M2M_AUDIO Shengjiu Wang
2024-02-26 8:28 ` [PATCH v13 11/16] media: uapi: Add audio rate controls support Shengjiu Wang
2024-02-26 8:28 ` [PATCH v13 12/16] media: uapi: Declare interface types for Audio Shengjiu Wang
2024-02-26 8:28 ` [PATCH v13 13/16] media: uapi: Add an entity type for audio resampler Shengjiu Wang
2024-02-26 8:28 ` [PATCH v13 14/16] media: vivid: add fixed point test controls Shengjiu Wang
2024-02-26 8:28 ` [PATCH v13 15/16] media: imx-asrc: Add memory to memory driver Shengjiu Wang
2024-02-26 8:28 ` [PATCH v13 16/16] media: vim2m-audio: add virtual driver for audio memory to memory Shengjiu Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56368a0d-6ada-4ab6-8389-b4bd20d6efc4@xs4all.nl \
--to=hverkuil@xs4all.nl \
--cc=Xiubo.Lee@gmail.com \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=festevam@gmail.com \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=m.szyprowski@samsung.com \
--cc=mchehab@kernel.org \
--cc=nicoleotsuka@gmail.com \
--cc=perex@perex.cz \
--cc=sakari.ailus@iki.fi \
--cc=shengjiu.wang@gmail.com \
--cc=shengjiu.wang@nxp.com \
--cc=tfiga@chromium.org \
--cc=tiwai@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).