[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-media
Subject: Re: [RCF 1/2] media: videodev2: Add V4L2_FMT_FLAG_ALL_FORMATS flag
From: Benjamin Gaignard <benjamin.gaignard () collabora ! com>
Date: 2024-01-31 10:06:03
Message-ID: 6a31726b-1e91-46b1-889c-4f643c759afb () collabora ! com
[Download RAW message or body]
Le 17/01/2024 à 20:41, Nicolas Dufresne a écrit :
> Le jeudi 11 janvier 2024 à 17:07 +0100, Benjamin Gaignard a écrit :
> > Add new flag to allow enumerate all pixels formats when
> > calling VIDIOC_ENUM_FMT ioctl.
> > When this flag is set drivers must ignore the configuration
> > and return the hardware supported pixel formats for the specified queue.
> > This will permit to discover which pixels formats are supported
> > without setting codec-specific information so userland can more easily
> > knows if the driver suit well to what it needs.
> > The main target are stateless decoders so update the documentation
> > about how use this flag.
> >
> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > ---
> > .../userspace-api/media/v4l/dev-stateless-decoder.rst | 3 +++
> > Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst | 4 ++++
> > Documentation/userspace-api/media/videodev2.h.rst.exceptions | 1 +
> > drivers/media/v4l2-core/v4l2-ioctl.c | 2 +-
> > include/uapi/linux/videodev2.h | 1 +
> > 5 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst \
> > b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst index \
> > 35ed05f2695e..b7b650f1a18f 100644
> > --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> > +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> > @@ -58,6 +58,9 @@ Querying capabilities
> > default values for these controls being used, and a returned set of formats
> > that may not be usable for the media the client is trying to decode.
> >
> > + * If ``V4L2_FMT_FLAG_ALL_FORMATS`` flag is set the driver must enumerate
> > + all the supported formats without taking care of codec-dependent controls.
> > +
> > 3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported
> > resolutions for a given format, passing desired pixel format in
> > > c:type:`v4l2_frmsizeenum`'s ``pixel_format``.
> > diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst \
> > b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst index \
> > 000c154b0f98..db8bc8e29a91 100644
> > --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> > +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> > @@ -227,6 +227,10 @@ the ``mbus_code`` field is handled differently:
> > The application can ask to configure the quantization of the capture
> > device when calling the :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl with
> > :ref:`V4L2_PIX_FMT_FLAG_SET_CSC <v4l2-pix-fmt-flag-set-csc>` set.
> > + * - ``V4L2_FMT_FLAG_ALL_FORMATS``
> > + - 0x0200
> > + - Set by userland application to enumerate all possible pixels formats
> > + without taking care of the current configuration.
> This is a bit ambiguous regarding if OUTPUT queue FMT is ignored or not. From
> our chat, it is ignored in this implementation. Such if I use MTK VCODEC as an
> example, using that feature would return:
I will reword it for next version, but yes the goal of this flag is to enumerate
all pixels formats without taking care of any queue configuration.
>
> - MM21
> - MT2T
> - MT2R
>
> At high level, the use case is to find an easy way to combine the per codec
> profile information and the pixel format, since userspace can only use e.g.
> 10bit capability if it knows the associated pixel formats. This implementation
> is already useful in my opinion, I'll try and draft a GStreamer change to use
> it, as I think it will better support the idea. But it has come ceavats.
>
> Notably, if you had a userland that implement MT2T (VP9/AV1/HEVC) but not MT2R
> (H264), it would not have an easy API to figure-out. It would still have to
> resort into enumerating formats for each possible codec and codec specific
> compound control configuration.
>
> An alternative is to make this enumerate "all" for the configure OUTPUT format.
> This increase the precision, while still allowing generic code to be used. In
> pseudo code that would be like:
>
> for output formats
> S_FMT(OUTPUT)
>
> for ALL capture formats
> store(format)
>
> Where right now we have do do:
>
>
> for output formats
> S_FMT(OUTPUT)
>
> S_CTRL(codec_specific_ctl_config_1)
> for capture formats
> store(format)
>
>
> S_CTRL(codec_specific_ctl_config_n)
> for capture format
> store(format)
>
> ...
>
> S_CTRL(codec_specific_ctl_config_n)
> for capture formats
> store(format)
>
> Where each config would demote a specific feature, like 10bit, 422, 444, film-
> grain (posprocessing affect output formats). The posprocessing remains a bit
> hard to figure-out in both cases though. But in practice, if I use Hantro AV1
> decoder as an example, I'd get something like:
>
> NV15_4L4
> P010
>
> Where NV15_4L4 is not available with filmgrain filter, but P010 is always
> available. For my GStreamer use case (template caps) this wouldn't be a problem,
> P010 is a well supported format and I strictly need a superset of the supported
> formats.
>
> What I'd really gain is that I don't have to do complicated enumeration logic
> per codec features.
>
> Nicolas
>
> p.s. It would be logical to update dev-stateless-decoder doc, to mention this
> enumeration option. Currently it says:
>
>
> To enumerate the set of supported raw formats, the client calls
> VIDIOC_ENUM_FMT() on the CAPTURE queue.
>
> * The driver must return only the formats supported for the format
> currently active on the OUTPUT queue.
>
> * Depending on the currently set OUTPUT format, the set of supported
> raw formats may depend on the value of some codec-dependent controls. The
> client is responsible for making sure that these controls are set before
> querying the CAPTURE queue. Failure to do so will result in the default
> values for these controls being used, and a returned set of formats that
> may not be usable for the media the client is trying to decode.
I have done it, look at the top of this patch.
>
>
> >
> > Return Value
> > ============
> > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions \
> > b/Documentation/userspace-api/media/videodev2.h.rst.exceptions index \
> > 3e58aac4ef0b..42d9075b7fc2 100644
> > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > @@ -215,6 +215,7 @@ replace define V4L2_FMT_FLAG_CSC_XFER_FUNC fmtdesc-flags
> > replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags
> > replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags
> > replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags
> > +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags
> >
> > # V4L2 timecode types
> > replace define V4L2_TC_TYPE_24FPS timecode-type
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c \
> > b/drivers/media/v4l2-core/v4l2-ioctl.c index 33076af4dfdb..22a93d074a5b 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -1544,7 +1544,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > p->mbus_code = 0;
> >
> > mbus_code = p->mbus_code;
> > - memset_after(p, 0, type);
> > + memset_after(p, 0, flags);
> In other similar places, we still clear the flags, and only keep the allowed
> bits. Maybe we should do this here too to avoid accidental flags going through ?
>
> That should maybe be under some capability flag, so that userland knows if the
> driver did implement that feature or not. If the driver didn't set that flag, we
> can then clear it so that userlands not checking that flag would at least get an
> enumeration response without it.
I will do that in next version.
Regards,
Benjamin
>
> > p->mbus_code = mbus_code;
> >
> > switch (p->type) {
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 68e7ac178cc2..82d8c8a7fb7f 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -869,6 +869,7 @@ struct v4l2_fmtdesc {
> > #define V4L2_FMT_FLAG_CSC_YCBCR_ENC 0x0080
> > #define V4L2_FMT_FLAG_CSC_HSV_ENC V4L2_FMT_FLAG_CSC_YCBCR_ENC
> > #define V4L2_FMT_FLAG_CSC_QUANTIZATION 0x0100
> > +#define V4L2_FMT_FLAG_ALL_FORMATS 0x0200
> >
> > /* Frame Size and frame rate enumeration */
> > /*
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic