Re: [PATCH v13 09/16] media: uapi: Define audio sample format fourcc type

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Mar 8, 2024 at 8:06 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>
> On 08/03/2024 12:52 pm, Shengjiu Wang wrote:
> > On Fri, Mar 8, 2024 at 3:34 PM Hans Verkuil <hverkuil@xxxxxxxxx> 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@xxxxxxx>
> >>> ---
> >>>  .../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.

I test this definition, the compiler doesn't report warning.

best regards
Shengjiu Wang

> >>
> >> Regards,
> >>
> >>         Hans
> >>
> >>>  /* priv field value to indicates that subsequent fields are valid. */
> >>>  #define V4L2_PIX_FMT_PRIV_MAGIC              0xfeedcafe
> >>>
> >>
>




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux