Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Tomasz Figa <tfiga@chromium.org>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>,
	Hans Verkuil <HansVerkuil@cisco.com>,
	Hans Verkuil <hverkuil@xs4all.nl>,
	Linux Media Mailing List <linux-media@vger.kernel.org>,
	Helen Koike <helen.koike@collabora.com>,
	Ezequiel Garcia <ezequiel@collabora.com>,
	kernel@collabora.com, Dafna Hirschfeld <dafna3@gmail.com>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip@lists.infradead.org>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Philipp Zabel <p.zabel@pengutronix.de>
Subject: Re: [RFC v3 1/2] v4l2: add support for colorspace conversion for video capture
Date: Tue, 30 Jun 2020 20:02:08 +0200
Message-ID: <CAAFQd5AD5iUN1hU_yHXNnfuE9nShSdLwkEK2EzM2PqduAo7ycg@mail.gmail.com> (raw)
In-Reply-To: <20200630174347.GN5850@pendragon.ideasonboard.com>

On Tue, Jun 30, 2020 at 7:43 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Tomasz,
>
> On Tue, Jun 30, 2020 at 06:31:19PM +0200, Tomasz Figa wrote:
> > On Tue, Jun 30, 2020 at 6:11 PM Dafna Hirschfeld wrote:
> > > On 10.06.20 15:34, Tomasz Figa wrote:
> > >> On Fri, Jun 05, 2020 at 12:11:33PM +0200, Dafna Hirschfeld wrote:
> > >>> On 04.06.20 19:39, Tomasz Figa wrote:
> > >>>> On Thu, Apr 16, 2020 at 04:56:04PM +0200, Dafna Hirschfeld wrote:
> > >>>>> From: Philipp Zabel <p.zabel@pengutronix.de>
> > >>>>>
> > >>>>> For video capture it is the driver that reports the colorspace,
> > >>>>> Y'CbCr/HSV encoding, quantization range and transfer function
> > >>>>> used by the video, and there is no way to request something
> > >>>>> different, even though many HDTV receivers have some sort of
> > >>>>> colorspace conversion capabilities.
> > >>>>
> > >>>> Thanks for working on this! Please see my comments inline.
> > >>>>
> > >>>>> For output video this feature already exists since the application
> > >>>>> specifies this information for the video format it will send out, and
> > >>>>> the transmitter will enable any available CSC if a format conversion has
> > >>>>> to be performed in order to match the capabilities of the sink.
> > >>>>>
> > >>>>> For video capture we propose adding new pix_format flag:
> > >>>>> V4L2_PIX_FMT_FLAG_HAS_CSC. The flag is set by the application,
> > >>>>> the driver will interpret the ycbcr_enc/hsv_enc, and quantization fields
> > >>>>> as the requested colorspace information and will attempt to
> > >>>>> do the conversion it supports.
> > >>>>>
> > >>>>> Drivers set the flags
> > >>>>> V4L2_FMT_FLAG_CSC_YCBCR_ENC,
> > >>>>> V4L2_FMT_FLAG_CSC_HSV_ENC,
> > >>>>> V4L2_FMT_FLAG_CSC_HSV_QUANTIZATION,
> > >>>>
> > >>>> Do we need this level of granularity? The drivers would ignore
> > >>>> unsupported encoding/quantization settings and reset them to supported
> > >>>> ones anyway, so if one doesn't support changing given parameter, it
> > >>>> would just return back the original stream parameter.
> > >>>
> > >>> I think this granularity allows userspace to know ahead what is supported
> > >>> and what is not so it doesn't have to guess.
> > >>>
> > >>
> > >> The userspace needs to guess anyway, because there is no way to
> > >> enumerate the supported target parameters. For example, even if the
> > >> CSC_YCBCR_ENC bit is set, only only DEFAULT, 601 and BT2020 could be
> > >> supported. If the userspace wants to get the 709 encoding, it needs to
> > >> explicitly try setting it and see if the TRY_FMT/S_FMT operation accepts
> > >> the setting.
> > >
> > > yes, indeed, Hans Verkuil suggested those flags. Maybe it is indeed enough
> > > to have one flag.
> > >
> >
> > Hans, what's your thought on this?
> >
> > >>
> > >> [snip]
> > >>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > >>>>> index a376b351135f..51e009936aad 100644
> > >>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > >>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > >>>>> @@ -477,6 +477,13 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > >>>>>            case VIDIOC_SUBDEV_S_FMT: {
> > >>>>>                    struct v4l2_subdev_format *format = arg;
> > >>>>> +          if (!(format->format.flags & V4L2_MBUS_FRAMEFMT_HAS_CSC)) {
> > >>>>> +                  format->format.colorspace = V4L2_COLORSPACE_DEFAULT;
> > >>>>> +                  format->format.xfer_func = V4L2_XFER_FUNC_DEFAULT;
> > >>>>> +                  format->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > >>>>> +                  format->format.quantization = V4L2_QUANTIZATION_DEFAULT;
> > >>>>> +          }
> > >>>>
> > >>>> Wouldn't this break setting the colorspaces on the sink pads, for which
> > >>>> the flag isn't required? Actually there is some unfortunate statement in
> > >>>> the documentation related to this:
> > >>>>
> > >>>> "This information supplements the colorspace and must be set by the
> > >>>> driver for capture streams and by the application for output streams,
> > >>>> see Colorspaces."
> > >>>>
> > >>>> However, I don't think there is any notion of "capture" and "output" for
> > >>>> subdevices, right? Instead, the pad direction would have to be checked,
> > >>>> but AFAICT there is no access to this kind of information from this
> > >>>> wrapper.
> > >>>
> > >>> Actually in coming v4 there is no new code added accept of the new fields and
> > >>> macros of the API. I think there is no need to change any code.
> > >>
> > >> Agreed.
> > >>
> > >>>>> +
> > >>>>>                    memset(format->reserved, 0, sizeof(format->reserved));
> > >>>>>                    memset(format->format.reserved, 0, sizeof(format->format.reserved));
> > >>>>>                    return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
> > >>>>> diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h
> > >>>>> index 123a231001a8..89ff0ad6a631 100644
> > >>>>> --- a/include/uapi/linux/v4l2-mediabus.h
> > >>>>> +++ b/include/uapi/linux/v4l2-mediabus.h
> > >>>>> @@ -16,6 +16,8 @@
> > >>>>>    #include <linux/types.h>
> > >>>>>    #include <linux/videodev2.h>
> > >>>>> +#define V4L2_MBUS_FRAMEFMT_HAS_CSC        0x0001
> > >>>>> +
> > >>>>>    /**
> > >>>>>     * struct v4l2_mbus_framefmt - frame format on the media bus
> > >>>>>     * @width:      image width
> > >>>>> @@ -36,7 +38,8 @@ struct v4l2_mbus_framefmt {
> > >>>>>            __u16                   ycbcr_enc;
> > >>>>>            __u16                   quantization;
> > >>>>>            __u16                   xfer_func;
> > >>>>> -  __u16                   reserved[11];
> > >>>>> +  __u16                   flags;
> > >>>>
> > >>>> Are we okay with a u16 for flags?
> > >>>
> > >>> I am fine with it, though don't mind changing it if there are objections.
> > >>>
> > >>
> > >> I'd suggest making it a u32 to be a bit more future-proof.
> > >
> > > ok, I see that just changing the type to __u32 and the reserved array
> > > to 'reserved[9]' increases the struct size from 48 to 52 because of padding.
> > > There are two ways to solve it,
> > > - move the flags field to be just above 'ycbcr_enc'
> > > - change reserve to 'reserve[8]'
> > >
> > > Is moving fields order in a struct ok? If so it save us 2 bytes.
> >
> > Since the structure is a part of the stable UAPI, we can't reorder the
> > fields. Similarly, we can't change the struct size, because it's
> > embedded in the ioctl code. (Although there are ways around it, not
> > currently implemented by V4L2.) That leaves us only the second option
> > - changing reserved to [8].
>
> You can also possibly do
>
>         __u16                   ycbcr_enc;
>         __u16                   quantization;
>         __u16                   xfer_func;
>         __u16                   reserved2;
>         __u32                   flags;
>         __u16                   reserved[8];
>
> to explicitly show there's a hole.

Good point. I didn't realize that there actually was a hole. Thought
that xfer_func ended at a 32-bit boundary.

Perhaps when changing this, we could make it __u32 reserved[4]? Or
would that have some compatibility concerns?

Best regards,
Tomasz

      reply index

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-16 14:56 Dafna Hirschfeld
2020-04-16 14:56 ` [RFC v3 2/2] media: staging: rkisp1: allow quantization conversion from userspace for isp source pad Dafna Hirschfeld
2020-06-04 17:54   ` Tomasz Figa
2020-06-04 19:11     ` Dafna Hirschfeld
2020-06-10 12:08       ` Tomasz Figa
2020-06-18 17:27         ` Tomasz Figa
2020-06-18 18:26           ` Dafna Hirschfeld
2020-06-18 18:49             ` Tomasz Figa
2020-06-18 18:54               ` Dafna Hirschfeld
2020-06-18 19:25                 ` Tomasz Figa
2020-06-26  0:15                   ` Helen Koike
2020-06-28 13:10                     ` Tomasz Figa
2020-06-28 13:15                       ` Laurent Pinchart
2020-06-28 14:59                         ` Tomasz Figa
2020-06-28 19:34                           ` Dafna Hirschfeld
2020-06-29 15:50                             ` Helen Koike
2020-06-29 23:08                               ` Laurent Pinchart
2020-06-30  9:36                                 ` Dafna Hirschfeld
2020-07-01 12:52                                   ` Laurent Pinchart
2020-07-01 14:41                                     ` Dafna Hirschfeld
2020-07-02 18:43                                       ` Laurent Pinchart
2020-06-28 20:27                           ` Laurent Pinchart
2020-06-28 21:33                             ` Tomasz Figa
2020-06-28 21:37                               ` Laurent Pinchart
2020-06-29 12:08                                 ` Dafna Hirschfeld
2020-06-29 12:21                                   ` Laurent Pinchart
2020-06-29 12:29                                     ` Dafna Hirschfeld
2020-06-29 12:33                                       ` Laurent Pinchart
2020-06-26  0:01             ` Helen Koike
2020-05-07 13:00 ` [RFC v3 1/2] v4l2: add support for colorspace conversion for video capture Hans Verkuil
2020-05-25 13:56   ` Dafna Hirschfeld
2020-05-25 14:25     ` Hans Verkuil
2020-05-26 12:36       ` Dafna Hirschfeld
2020-05-26 13:47         ` Hans Verkuil
2020-06-04 17:39 ` Tomasz Figa
2020-06-05 10:11   ` Dafna Hirschfeld
2020-06-10 13:34     ` Tomasz Figa
2020-06-30 16:11       ` Dafna Hirschfeld
2020-06-30 16:31         ` Tomasz Figa
2020-06-30 17:43           ` Laurent Pinchart
2020-06-30 18:02             ` Tomasz Figa [this message]

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=CAAFQd5AD5iUN1hU_yHXNnfuE9nShSdLwkEK2EzM2PqduAo7ycg@mail.gmail.com \
    --to=tfiga@chromium.org \
    --cc=HansVerkuil@cisco.com \
    --cc=dafna.hirschfeld@collabora.com \
    --cc=dafna3@gmail.com \
    --cc=ezequiel@collabora.com \
    --cc=helen.koike@collabora.com \
    --cc=hverkuil@xs4all.nl \
    --cc=kernel@collabora.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=p.zabel@pengutronix.de \
    --cc=sakari.ailus@linux.intel.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

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git